ARCH_BCM270X: Drop ATAGS support

[notro]

This PR makes Device Tree mandatory for booting the ARCH_BCM2708 kernel.

We have used Device Tree since we switched to 3.18 in January and it's time to remove ATAGS support.

This will cut down on the amount of patches that we have to keep around and apply on each new kernel release. It is also a necessary step on the way to get one kernel image that can boot both Pi1 and Pi2 (instead of having kernel.img and kernel7.img).

A /boot/config.txt that disables Device Tree will not boot anymore:

device_tree=

There will be no visible sign that the kernel didn't receive a Device Tree since it won't know which drivers to load and hence can't display an error (a custom kernel with EARLY_PRINTK enabled would print an error on the serial console).

[popcornmix]

Great I'll give this a test. Been looking forward to this day. (Even more so when 2709 gets the same treatment).

[popcornmix]

You could cherry-pick #1163 and then drop bcm2708-i2s.

[popcornmix]

67 files changed with 356 additions and 5,887 deletions.

Awesome! Still boots and seems to work.

[notro]

Been looking forward to this day

Me too. ARCH_BCM2835 is now just around the corner.

You could cherry-pick #1163 and then drop bcm2708-i2s.

Will do.

[notro]

Version 2

Removed bcm2708-i2s driver. bcm2835-i2s patches were already applied to rpi-4.3.y.

Removed the 4M init_dma_coherent_pool_size so we're back to the default 256k.

Added reboot_part parameter.
I haven't tested it with NOOBS, but this leads to poweroff:

echo 63 | sudo tee /sys/module/bcm2708/parameters/reboot_part
sudo reboot

Any other value (1,5) leads to a regular reboot. Does the bootloader boot partiton 0 if the asked for partition isn't bootable?

Added aux enable node so we can enable uart1 from board_bcm2835.

[popcornmix]

If a partition doesn't exist it falls back to partition zero.

[notro]

I discovered that the kernel was hung with no message on the console 30 min after last round of testing.
Checking back now after one hour and it's hung again.
I will add back init_dma_coherent_pool_size() tomorrow and see if that helps.

[notro]

I rebuilt the kernel from scratch and haven't been able to reproduce the problem.

[notro]

I'm not sure what I should do with reboot_part.
I have added sysfs symlinks module/{bcm2708,bcm2709} -> module/bcm2835_wdt, but just realised that the bcm2709 symlink won't work because I can't turn bcm2709 into multiplatform yet.
Is it possible to let NOOBS use /sys/module/bcm2835_wdt/parameters/reboot_part directly?
That would remove the need for those symlinks.

[popcornmix]

If necessary NOOBS can use a new mechanism when choosing next partition to boot from. The kernel version and NOOBS version tend to be tied together (updating NOOBS kernel is not generally supported).

There will be other users of reboot_part. We use it in production tests to signal results from a previous boot. No doubt others are using it for multiboot purposes. But if it needs to change, then it needs to change. It's not too hard to test for both possible paths.

Ping @lurch @ghollingworth that a future kernel update may require NOOBS changes.

[lurch]

Thanks for the heads up. NOOBS v1.4.x is still using a 3.18 kernel ( https://github.com/raspberrypi/noobs/blob/master/buildroot/.config#L309 ) without DT enabled.

If/when it becomes necessary to change the reboot_part logic though, it'll be easy to modify: https://github.com/raspberrypi/noobs/blob/master/recovery/util.cpp#L143

[pelwell]

On the subject of updating NOOBS, are you aware of @procount's efforts to port NOOBS to 4.1? You can read the saga of the odyssey here.

[lurch]

@pelwell Yup, we've been communicating via email :-) It's just a shame that I'm so busy at the moment that I haven't been able to give him more help.

[procount]

An odyssean saga? - definitely! But I'm just a small fish standing on the "shoulders of giants", grateful for any tidbits thrown my way.

[popcornmix]

@notro v4.3 is out, so this needs a rebase.

[notro]

The Pi was dead again today, no error messages. It had just been sitting idle.
I'll start bisecting.

[popcornmix]

I've noticed that without this PR the USB hardware correctly accesses memory through the 0x4 alias bus address. After this PR it accesses memory incorrectly through the 0x0 alias.

The GPU debugger catches bad accesses:

0x0000_0001  L2_ALIAS_EX         L2 bad address exception                 
   [    0] 0x1        ALIASEX             Address exception                        occurred
0x8000_00c0  L2_ALIAS_EX_ID      L2 bad address ID                        
   [ 7- 0] 0xc0       AXIID               Axi ID of bad access                     USB_ID
   [   31] 0x1        WRITE               Read (0) or Write (1)                    Write
0x0f85_4d30  L2_ALIAS_EX_ADDR    L2 bad address                           
   [31- 0] 0xf854d30  ADDR                Address of the bad access                0xf854d30

Not sure if that is the cause of your instability, but it is something suspicious.

[notro]

The 'Convert to multiplatform' patch changes the address logic, so I guess it's the culprit.
If so it's a bit strange that this didn't surface when I did my earlier bcm2835 work. Maybe I didn't have long enough run times.
Can you checkout the commit before the multiplatform one and try that with the debugger?

[popcornmix]

The difference between 0x0 and 0x4 alias is quite minor (whether it goes through GPU's L1 cache). Most peripherals like USB can't see the L1 cache, so may get the same data in either case (although we are in undefined behaviour territory). Each access does generate an exception on the GPU, so at the very least kernel accesses will cripple the gpu performance.

If the same error applies to Pi2 (0x0 alias instead of 0xC alias), then it is likely to be far more obviously broken as peripherals can see the GPU's L2 cache (but don't want to).

I'll try to do the test. Note: with v4.3 there are a few build errors with platform include files. I'm hacking it by adding extra header includes, but I suspect there is a better solution.

[popcornmix]

I can confirm that the GPU exceptions start with the multiplatform commit.

[notro]

Can you give me an example statement from the usb driver which triggers this exception?

[notro]

I've spotted something that could be the source of the problem.
If DWC_DMA_ALLOC() is the way dma buffers are allocated, then it doesn't get the required device translation (DT property dma-ranges).
dma_ctx is NULL, but it should be the device that requests the buffer. Both vchiq and bcm2708_fb had the same issue, but for those it was easy to spot since they didn't work at all.

drivers/usb/host/dwc_common_port/dwc_os.h

#define DWC_DMA_ALLOC(_size_,_dma_) __DWC_DMA_ALLOC(NULL, _size_, _dma_)

drivers/usb/host/dwc_common_port/dwc_common_linux.c

void *__DWC_DMA_ALLOC(void *dma_ctx, uint32_t size, dwc_dma_t *dma_addr)
{
#ifdef xxCOSIM /* Only works for 32-bit cosim */
    void *buf = dma_alloc_coherent(dma_ctx, (size_t)size, dma_addr, GFP_KERNEL);
#else
    void *buf = dma_alloc_coherent(dma_ctx, (size_t)size, dma_addr, GFP_KERNEL | GFP_DMA32);
#endif
    if (!buf) {
        return NULL;
    }

    memset(buf, 0, (size_t)size);
    return buf;
}

[notro]

Version 3

Removed bcm2835_wdt sysfs symlinks. NOOBS have to use the new path when it switches to kernel version >4.1: /sys/module/bcm2835_wdt/parameters/reboot_part

Pass in the device when allocating dma memory in dwc_otg to get the correct bus address.

@popcornmix Please test with the debugger to verify that the exceptions are indeed gone.

[HiassofT]

Did some tests, this is already looking really good, great job!

Only a few minor things:

Currently I have to patch mkknlimg so the kernel doesn't get flagged as 2835, otherwise it won't boot. Manually specifying the device_tree file in config.txt also works, but that's a PITA as I swap between B and B+ quite often during testing. Some idea how to improve 2835 detection in mkknlimg?

I also get a warning in dmesg every time an I2S audio device is closed. Looks like dma_free_coherent now doesn't like to be called with IRQs disabled:

[   72.211019] ------------[ cut here ]------------
[   72.211101] WARNING: CPU: 0 PID: 0 at include/asm-generic/dma-mapping-common.h:274 bcm2835_dma_desc_free+0xc0/0xd0()
[   72.211118] Modules linked in: snd_soc_pcm512x_i2c snd_soc_pcm512x regmap_i2c snd_soc_iqaudio_dac snd_soc_bcm2835_i2s regmap_mmio i2c_bcm2708 snd_soc_core snd_compress bcm2835_gpiomem snd_pcm_dmaengine snd_pcm snd_timer snd uio_pdrv_genirq uio fuse
[   72.211212] CPU: 0 PID: 0 Comm: swapper Not tainted 4.3.0+ #1
[   72.211226] Hardware name: BCM2708
[   72.211289] [<c001704c>] (unwind_backtrace) from [<c0013d88>] (show_stack+0x20/0x24)
[   72.211338] [<c0013d88>] (show_stack) from [<c032605c>] (dump_stack+0x20/0x28)
[   72.211374] [<c032605c>] (dump_stack) from [<c00267bc>] (warn_slowpath_common+0x8c/0xc4)
[   72.211403] [<c00267bc>] (warn_slowpath_common) from [<c00268b0>] (warn_slowpath_null+0x2c/0x34)
[   72.211434] [<c00268b0>] (warn_slowpath_null) from [<c036d3e4>] (bcm2835_dma_desc_free+0xc0/0xd0)
[   72.211466] [<c036d3e4>] (bcm2835_dma_desc_free) from [<c036d544>] (bcm2835_dma_terminate_all+0xc0/0x2b8)
[   72.211519] [<c036d544>] (bcm2835_dma_terminate_all) from [<bf07f0f8>] (snd_dmaengine_pcm_trigger+0x70/0x180 [snd_pcm_dmaengine])
[   72.211777] [<bf07f0f8>] (snd_dmaengine_pcm_trigger [snd_pcm_dmaengine]) from [<bf0a1d8c>] (soc_pcm_trigger+0xb4/0x134 [snd_soc_core])
[   72.212040] [<bf0a1d8c>] (soc_pcm_trigger [snd_soc_core]) from [<bf060648>] (snd_pcm_do_stop+0x64/0x68 [snd_pcm])
[   72.212170] [<bf060648>] (snd_pcm_do_stop [snd_pcm]) from [<bf0603d4>] (snd_pcm_action_single+0x48/0x88 [snd_pcm])
[   72.212296] [<bf0603d4>] (snd_pcm_action_single [snd_pcm]) from [<bf063670>] (snd_pcm_drain_done+0x24/0x2c [snd_pcm])
[   72.212428] [<bf063670>] (snd_pcm_drain_done [snd_pcm]) from [<bf066cdc>] (snd_pcm_update_state+0xe0/0x110 [snd_pcm])
[   72.212571] [<bf066cdc>] (snd_pcm_update_state [snd_pcm]) from [<bf066ec4>] (snd_pcm_update_hw_ptr0+0x1b8/0x3ac [snd_pcm])
[   72.212706] [<bf066ec4>] (snd_pcm_update_hw_ptr0 [snd_pcm]) from [<bf067164>] (snd_pcm_period_elapsed+0xac/0xd4 [snd_pcm])
[   72.212803] [<bf067164>] (snd_pcm_period_elapsed [snd_pcm]) from [<bf07f3fc>] (dmaengine_pcm_dma_complete+0x54/0x58 [snd_pcm_dmaengine])
[   72.212847] [<bf07f3fc>] (dmaengine_pcm_dma_complete [snd_pcm_dmaengine]) from [<c036c5d0>] (vchan_complete+0xdc/0x140)
[   72.212887] [<c036c5d0>] (vchan_complete) from [<c002ac24>] (tasklet_action+0x88/0xe0)
[   72.212916] [<c002ac24>] (tasklet_action) from [<c002a0ac>] (__do_softirq+0x108/0x3a8)
[   72.212942] [<c002a0ac>] (__do_softirq) from [<c002a6e0>] (irq_exit+0xcc/0x120)
[   72.212968] [<c002a6e0>] (irq_exit) from [<c00648c4>] (__handle_domain_irq+0x60/0xb4)
[   72.213004] [<c00648c4>] (__handle_domain_irq) from [<c0010928>] (handle_IRQ+0x2c/0x30)
[   72.213032] [<c0010928>] (handle_IRQ) from [<c0009488>] (bcm2835_handle_irq+0x3c/0x58)
[   72.213064] [<c0009488>] (bcm2835_handle_irq) from [<c05f4144>] (__irq_svc+0x44/0x7c)
[   72.213080] Exception stack(0xc08a3f08 to 0xc08a3f50)
[   72.213102] 3f00:                   00000000 00000000 c08a3f68 c08a5028 00000002 c08a2000
[   72.213128] 3f20: c08a4158 00000000 c0909e84 c09093c7 c09093c7 c08a3f64 c08a3f58 c08a3f58
[   72.213145] 3f40: c00109e4 c00109e8 60000013 ffffffff
[   72.213174] [<c05f4144>] (__irq_svc) from [<c00109e8>] (arch_cpu_idle+0x30/0x4c)
[   72.213206] [<c00109e8>] (arch_cpu_idle) from [<c00569b0>] (default_idle_call+0x34/0x48)
[   72.213233] [<c00569b0>] (default_idle_call) from [<c0056b70>] (cpu_startup_entry+0x1ac/0x200)
[   72.213276] [<c0056b70>] (cpu_startup_entry) from [<c05eecd4>] (rest_init+0x80/0x98)
[   72.213320] [<c05eecd4>] (rest_init) from [<c0842ce4>] (start_kernel+0x390/0x408)
[   72.213339] ---[ end trace aa5f118f8db1fe91 ]---

Ah, and could you please add commit 0bbc1e2 (limit cyclic lite transfers to 32k)? Haven't found the time yet to dig further into that..

[notro]

Thanks for giving this a spin.

Looks like dma_free_coherent now doesn't like to be called with IRQs disabled

Documentation/DMA-API.txt

void
dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
         dma_addr_t dma_handle)

Free a region of consistent memory you previously allocated.  dev,
size and dma_handle must all be the same as those passed into
dma_alloc_coherent().  cpu_addr must be the virtual address returned by
the dma_alloc_coherent().

Note that unlike their sibling allocation calls, these routines
may only be called with IRQs enabled.

It seems that the documentation has been realised in code with this 4.3 commit: torvalds/linux@6894258

How to solve this? I don't know.

Ah, and could you please add commit 0bbc1e2

Will do.

[HiassofT]

> Ah, and could you please add commit 0bbc1e2 Will do.

Thanks! BTW: you can drop the dreq/slave_id commit, the DMA channels are now properly set up in the DT and this is no longer needed.

[pelwell]

If the image support bcm283x and bcm270x then I think the firmware should load a bcm270x DTB by default. There are a few ways to achieve this:

1. If the image support both bcm283x and bcm270x, don't report bcm283x.
2. Have a command-line parameter to suppress bcm283x reporting.
3. Create a bcm270x tag, set it as appropriate, and put the DTB selection logic in the firmware.

Any thoughts?

[notro]

2. Have a command-line parameter to suppress bcm283x reporting.

This is currently possible with the --dtok parameter. It turns of auto detection.

3. Create a bcm270x tag, set it as appropriate, and put the DTB selection logic in the firmware.

I like this one. The tag decides the default dtb.
If both tags are present, the bootloader can check to see which dtb's are available and use that one. If both are present, use 270x.

[popcornmix]

Latest version of this PR is not provoking GPU asserts.
Are you still seeing crashes when sitting idle?

[notro]

I have fiq support with 2836: irqchip: irq-bcm2835: Add 2836 FIQ support
https://github.com/notro/linux/commits/bcm2836

[popcornmix]

I tried Phil's upstream interrupt controller patches and your fiq patch:
http://paste.ubuntu.com/13940188/

but dwc_otg fails with fiq enabled. It does work with fiq disabled.
Have you had upstream interrupt controller and fiq working with dwc_otg?

[notro]

Yes I have 2836 running now with fiq enabled (a bit unreliable network transfer though).
You seem to be missing the syscon DT node which the irq driver needs:

    syscon@40000000 {
           compatible = "brcm,bcm2836-arm-local", "syscon";
           reg = <0x40000000 0x100>;
    };

I use Eric's timer patch and it enables syscon (I have removed the conditional here):

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index edbe6fe..3d76e13 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -115,6 +115,7 @@ config ARCH_BCM2835
    select ARM_TIMER_SP804
    select CLKSRC_OF
    select FIQ
+   select MFD_SYSCON
    select PINCTRL
    select PINCTRL_BCM2835
    help

Looking at your patch I can see why I have problems with mmc dma on 2836.

[notro]

And I can see that @pelwell discovered the reason for the spurious interrupts, that's good!

[notro]

I see that bcm2709_defconfig has CONFIG_CACHE_L2X0 disabled. Why is that? ARCH_BCM2709 does select MIGHT_HAVE_CACHE_L2X0 in arch/arm/Kconfig
bcm2835_defconfig also selects MIGHT_HAVE_CACHE_L2X0 through ARCH_MULTI_V6_V7, so it is enabled.
I'm wondering what to do in a combined Pi1/Pi2 kernel image and I'm having stability issues in my 2836 kernel on Pi2.

[popcornmix]

Pretty sure we don't want CONFIG_CACHE_L2X0. That is an outer L2 cache (external to the arm cores). We have an inner L2 cache (integrated with the cores).

[notro]

Is it ok to enable CONFIG_NEON on Pi1?

diff -u arch/arm/configs/bcmrpi_defconfig arch/arm/configs/bcm2709_defconfig

+# CONFIG_CACHE_L2X0 is not set
+CONFIG_SMP=y
+CONFIG_HAVE_ARM_ARCH_TIMER=y
+CONFIG_VMSPLIT_2G=y
+CONFIG_NEON=y
+CONFIG_KERNEL_MODE_NEON=y
-CONFIG_LATENCYTOP=y
-CONFIG_CRYPTO_CRYPTD=m
-CONFIG_CRYPTO_SHA512=m
-CONFIG_CRYPTO_SHA1_ARM=m
-CONFIG_CRYPTO_AES_ARM=m
+CONFIG_CRYPTO_SHA1_ARM_NEON=m
+CONFIG_CRYPTO_AES_ARM_BS=m

[popcornmix]

There is no Neon support on pi1 so it's probably not good to enable it (unless it gets disabled in some way).

[popcornmix]

@notro have you had fiq + upstream interrupt controller working with bcm2709_defconfig platform?

[notro]

Yes, this is what I used: https://github.com/notro/linux/commits/irq2836-43
Looking at the patches I see that I adjust the irq numbering like @pelwell did on 2708.
If you're not trying to get the irq drivers to co-exist, try adding select SPARSE_IRQ instead of using arch_dynirq_lower_bound().

[notro]

Yes, SPARSE_IRQ is what I used in the drop atags patchset: 9b4b8da

[popcornmix]

I want to drop downstream interrupt controller in 4.4 (which has drop atags).
https://github.com/raspberrypi/linux/tree/rpi-4.4.y_irq has Phil's upstream irq patch plus your FIQ patch. Can you see anything missing?

[notro]

I guess you have this error in the kernel log: Failed to get local register map. FIQ is disabled for cpus > 1
Because you seem to missing the syscon node: https://github.com/notro/linux/blob/ceac641527b8eec687a81c0d44d5bb29f0e15693/arch/arm/boot/dts/bcm2709.dtsi#L45

I see that @anholt doesn't use a syscon node in his 2836 v2 patch like he did in his dev branch that I looked at.
irq-bcm2836 claims the reg address we need, so I'm not sure what the best solution is here.

[popcornmix]

Yes, I get that error in kernel log.

[notro]

It should work if you add that node, but I'm not sure if this is the approved way of doing it.

[popcornmix]

I'll give it a try. For now I'll settle for working. Approved can come later.
Do I need: https://github.com/notro/linux/commit/ceac641527b8eec687a81c0d44d5bb29f0e15693#diff-68483e7094a0b0a646c8462503f8262eR21

[notro]

If all the cpu's come up, I guess you're ok, but there's some smp code in irq-bcm2836 and I don't know how that plays with the current 2709 smp code:

static void
bcm2836_arm_irqchip_smp_init(void)
{
#ifdef CONFIG_SMP
    /* Unmask IPIs to the boot CPU. */
    bcm2836_arm_irqchip_cpu_notify(&bcm2836_arm_irqchip_cpu_notifier,
                       CPU_STARTING,
                       (void *)smp_processor_id());
    register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);

    set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
#endif
}

Maybe it just does the same operation twice, I haven't studied it.

[notro]

Maybe it just does the same operation twice, I haven't studied it.

I mean: maybe irq2836 and 2709 combined will so some smp operations twice.

[popcornmix]

Adding 0d3f1f5 doesn't affect the "Failed to get local register map. FIQ is disabled for cpus > 1" error.
I feel I need something else to use that node. Do I need CONFIG_MFD_SYSCON? Do I need something like: https://github.com/notro/linux/commit/ceac641527b8eec687a81c0d44d5bb29f0e15693#diff-68483e7094a0b0a646c8462503f8262eR104 ?

[popcornmix]

CONFIG_MFD_SYSCON does seem to make the fiq work. Just need to test for stability.

[popcornmix]

Upstream interrupt controller was in last night's milhouse OpenELEC build without complaints.
rpi-4.4.y tree is now -rc6 and using upstream interrupt controller and seems good.
Thanks @notro and @pelwell