ARCH_BCM270x: Moving more devices to DT #936
Comments
|
I now have the dwc_otg driver working (no fiq) with ARCH_BCM2835, well partly #937 |
|
Sounds good to me. |
|
How do you want it:
|
|
Not too bothered. I'll say one commit per device as it's easier to squash than split. |
|
AFAICT I now have the major 2708 drivers (mostly) working on ARCH_BCM2835! dwc_otgNo FIQ support which leads to broken low speed device support. When trying to boot directly from VC without u-boot, it breaks: bcm2708_dmaCopied to drivers/dma/bcm2708-dma.c bcm2835-mmcbcm2708_vcioCopied to drivers/mailbox/bcm2708-vcio.c bcm2835-thermalbcm2708_fbHDMI is ok. Not sure if the DMA part is tested by just starting X windows. vchiqBased on work by Lubomir Rintel. bcm2835_AUD0omxplayer crashed the kernel (8 blinks on the green led). vc_memCopied to drivers/char/broadcom/vc_mem.c Here's the diff: https://gist.github.com/notro/dbb63a021d8ea575ddb5 @popcornmix are you interested in pulling something like this? |
|
Sounds awesome! Yes, this is something I'm very interested in.
Any thoughts on why uboot is required? Is uboot doing some reset/init/power/clock setup of dwc that is not occurring with 2835 arch? Would be good to get this solved. |
Yes. I don't know why this is address bit must be set though, it might be different on the Pi2: #ifdef CONFIG_ARCH_BCM2835
#define TO_VC_PHYS(a) (0x40000000 | (a))
bcm_mailbox_write(MBOX_CHAN_FB, TO_VC_PHYS(fb->dma));
#else
bcm_mailbox_write(MBOX_CHAN_FB, fb->dma);
#endif
Yes.
Yes, I got an allocation failure in vchiq at first: g_slot_mem = dma_alloc_coherent(NULL, g_slot_mem_size + frag_mem_size,
&g_slot_phys, GFP_ATOMIC);I couldn't see why this needed to be atomic (during init/probe), so I changed it to GFP_KERNEL. It worked fine after that.
I didn't do arch/arm/mach-bcm2708/power.c, because of this statement: This driver provides a mailbox interface to power up/down the domains that notionally belong to the ARM, however currently this is not used by any of the peripheral drivers. #ifdef CONFIG_USB
#define BCM_POWER_ALWAYS_ON (BCM_POWER_USB)
#endif
static int __init bcm_power_init(void)
{
[...]
#if defined(BCM_POWER_ALWAYS_ON)
if (BCM_POWER_ALWAYS_ON) {
bcm_power_open(&always_on_handle);
bcm_power_request(always_on_handle, BCM_POWER_ALWAYS_ON);
}
#endifSo USB power seems to be the missing piece. I'll try it.
Not supported in the irq driver. @P33M made this comment:
|
#ifdef CONFIG_ARCH_BCM2835
#define TO_VC_PHYS(a) (0x40000000 | (a))
bcm_mailbox_write(MBOX_CHAN_FB, TO_VC_PHYS(fb->dma));
#else
bcm_mailbox_write(MBOX_CHAN_FB, fb->dma);
#endifLooks wrong for 2836. I suspect this offset should be: so wants to be 0xC0000000 (bypassing the GPU caches) on 2836. I think modifying the upstream bcm2835_defconfig is okay. We are modifying other upstream files, so trying to build an clean upstream kernel from this tree is not advisable. |
Then why is this not needed on ARCH_BCM2708? Isn't there some dma sync function that we can use instead of bypassing the cache? |
|
Is USB the only peripheral that the firmware doesn't power on? |
|
Read this message for details on the different cache aliases: The bcm2708_fb driver on Pi2 reports: and on Pi1 reports: so it is allocing a bus address suitable for use by GPU. We convert this back into an ARM physical address here: https://github.com/raspberrypi/linux/blob/rpi-3.18.y/drivers/video/fbdev/bcm2708_fb.c#L316 |
|
Originally I believe MMC and USB required powering from the arm side. I think the linux driver powered "on demand" which slowed down the MMC driver and meant if the GPU locked up you lost access to MMC which was unfortunate, so that got removed. Removing power control of USB and making it always on and powered by the GPU may make sense. It rules out a (very small) power saving possible from the arm side, but we're not taking advantage of that. @pelwell @P33M any objection to enabling USB power from GPU by default? |
|
I agree that the use case where we would want USB turned off is of marginal benefit - the power consumption versus connectivity tradeoff doesn't really make sense. As it removes some complexity (for now) from the ARM side code then it seems reasonable to just change the default state in firmware, but in the future we would want the state to be communicated to whatever PM/power domain driver that exists in the upstream world. |
|
No objection from me. |
|
@notro can you post you config? I suspect you are not using an untouched bcm2835_defconfig as that doesn't have CONFIG_MAILBOX enabled. |
|
Sorry I forgot that, I set the config in a script. |
|
@notro I just get |
|
There is something wrong with that diff I gave, because some of the options are not disable in the kernel: I'll have to look into this. |
|
These are the options I set/change in my script: VAR['LINUX_DEFCONFIG'] = 'bcm2835_defconfig'
config 'DYNAMIC_DEBUG', :enable
config ['CONFIG_IKCONFIG', 'CONFIG_IKCONFIG_PROC'], :enable
config 'PROC_DEVICETREE', :enable
config ['DMADEVICES', 'DMA_BCM2708'], :enable
config 'MMC_BCM2835', :enable
config 'MMC_BCM2835_DMA', :enable
config 'USB_DWCOTG', :enable
config 'THERMAL', :enable
config 'THERMAL_BCM2835', :enable
config 'MAILBOX', :enable
config 'BCM2708_MBOX', :enable
config 'FB_BCM2708', :enable
config 'FB_SIMPLE', :disable
config 'SOUND', :enable
config 'SND', :enable #:module
config 'SND_BCM2835', :enable #:module
config 'BRCM_CHAR_DRIVERS', :enable
config 'BCM2708_VCMEM_2', :enable |
|
Can you run Do you have any config.txt/cmdline.txt settings? Did you run mkknlimg --dtok on the kernel? |
|
Am I doing something wrong here: defconfig: https://gist.github.com/notro/62767303672ebba49161
No
Yes when I tested booting directly without uboot. |
Sorry, /boot/config.txt: device_tree=bcm2835-rpi-b-plus.dtb |
The defconfigs only list non-default options, so it's expected to have more options in .config than in the original defconfig. Now if a newly created defconfig has even fewer options, then that suggests the original defconfig had some invalid (or already default) options in that have been removed. |
|
How do we treat the arch modules: dma.c, vc_mem.c, vcio.c? If we don't move, the depending drivers would need different includes for ARCH_BCM2835. What we choose depends on how long until we reach ARCH_BCM2835, and the expected future changes to these drivers. If we have copies, both would need updating. So what do you think? |
|
It doesn't sound right to make copies of the drivers... sigh. dma PR with 7 commits:
|
|
I think we need to move to using platform devices to configure the existing hardcoded drivers. We should also - either at the same time or later - remove the 2708/2709 duplication. |
|
Yes, perhaps I should first make a PR that fixes the hardcoding and then do one for ARCH_BCM2835. That will make it easier on me :-) BCM270x: Add irq resources to dmaman device dma: Add bcm2708 legacy driver (plain copy of dma.c) Another solution is to merge dma.c with bcm2708-dmaengine.c: BCM270x: Add memory and irq resources to dmaengine device and DT dmaengine: bcm2708: Add depends on ARCH_BCM2835 That will give us just one device/driver for the DMA controller. |
|
Patch merging dma.c with bcm2708-dmaengine.c: https://gist.github.com/notro/656e0c483089a0bf4138 Changes to the dma.c code during merging: I'm uncertain about dmachans. The firmware only sets the module parameter and not the DT property. |
|
I guess firmware should set dmachans in DT. @pelwell? |
|
Let's leave this in as a default to aid the transition, then I'll patch it from the firmware. |
|
Should I put the audio node in |
|
Put it in |
|
The logging doesn't show any obvious errors, just an absence of data arriving. Annoyingly the logging level required to see arriving data is "trace", whereas for outbound data it is only "info". I've just pushed a patch that changes the logging level for inbound data to "info", for future cases. Fortunately the test fails quickly, so lets just up the levels one more time: |
|
The interesting thread in the log is the DISP service, in that the final message from it goes unanswered. Annotating the trace with a bit of protocol decoding gives: This show that there is no response to the update_submit_sync. VCHIQ knows nothing of the dispserve protocol, and all replies are sent in the same way. There are two likely explanations:
As a general test of VCHIQ, try running before stopping. If that passes, we can rule out case 2) by adding some background traffic: With this running, restart the test with the same logging as before and we'll see how the results differ. |
dmesg: https://gist.github.com/notro/1a234a49512f63cf25d8 I'll do vc-mem now. But I would like to boot directly from the VC bootloader without u-boot. For this I need USB power turned on. I see 2 solutions:
|
|
Interesting log. It looks as though it just ground to a halt after a few pings. While ping_test is stalled, can you capture the output from |
|
|
I was quite easy to move the power driver, so I'll do that. drivers/power was the wrong place, so I moved it to drivers/soc/bcm2835. So I'll make a PR for that, then I'll do one for vc-mem. @pelwell With those in place it's possible to boot ARC_BCM2835 directly from VC and with vc-mem in place, I think it's better that you continue with tracking down where/why this fails at your leisure. |
|
Thanks, that's appreciated. It's on my list. |
|
ARCH_BCM2835 is not able to halt, it just reboots. +static int calc_rsts(int partition)
+{
+ return PM_PASSWORD |
+ ((partition & (1 << 0)) << 0) |
+ ((partition & (1 << 1)) << 1) |
+ ((partition & (1 << 2)) << 2) |
+ ((partition & (1 << 3)) << 3) |
+ ((partition & (1 << 4)) << 4) |
+ ((partition & (1 << 5)) << 5);
+}
+
/*
* We can't really power off, but if we do the normal reset scheme, and
* indicate to bootcode.bin not to reboot, then most of the chip will be
* powered off.
*/
static void bcm2835_power_off(void)
{
u32 val;
/*
* We set the watchdog hard reset bit here to distinguish this reset
* from the normal (full) reset. bootcode.bin will not reboot after a
* hard reset.
*/
val = readl_relaxed(wdt_regs + PM_RSTS);
val &= ~PM_RSTC_WRCFG_MASK;
val |= PM_PASSWORD | PM_RSTS_HADWRH_SET;
+
+ /* partition 63 is special code for HALT the bootloader knows not to boot*/
+ val = calc_rsts(63);
+
writel_relaxed(val, wdt_regs + PM_RSTS);
/* Continue with normal reset mechanism */
bcm2835_restart(REBOOT_HARD, "");
}What's the reasoning behind the calc_rsts() formula? bcm2835_power_off() reads PM_RSTS and uses part of that value when writing back, but arch/arm/mach-bcm2708/bcm2708.c only writes. Care to elaboarate on this? |
|
There are 6 bits of "state" that are preserved over a reset in the PM_RSTS register. NOOBS writes a partition number before rebooting to support multiboot. calc_rsts is historical. Looks like (PM_PASSWORD | (partition & 0x3f)) would be equivalent. |
That would not give the same value: Is get_rsts being masked somehow, because I don't get the 0x555 value back: PM_RSTS flags Ref: #932 |
|
The ^ was corrected to an & just after I posted it yesterday. |
|
This did not work, it rebooted instead of halting: I expected to get 0x3f here, but bit 3 is zero: |
|
Sorry I misread calc_rsts. The six-bit partition is spread into bits 0, 2, 4, 6, 8, 10 of RSTS. I don't believe we want to preserve anything in there so I'd say: looks right. |
BCM270x Device Tree statusThese arch/arm/mach-bcm270x/bcm270x.c devices have not been moved to Device Tree. uart0 See #910 bcm2708_systemtimer This device does not have a matching driver. serial8250 The driver for this device is not enabled (SERIAL_8250). Only the Compute Module has access to the necessary pins if I'm not mistaken. To simplify the life for those wanting to use this, I suggest adding a DT node. kconfig: Links: bcm2708_powerman This device does not have a matching driver. bcm2835_hwmon The matching bcm2835_hwmon driver is not enabled (can't be used alongside bcm2835-thermal): |
|
The 8250/pl011 UART (uart1) can use the same pins as the mini UART (uart0), but not at the same time. If you configure GPIOs 14-15 (and 16-17 if you want RTS/CTS) to ALT5 then you will lose ttyAMA0, but gain a way to use UART1. |
|
I suspect bcm2835_hwmon can be dropped. I don't think it gives us anything over bcm2835-thermal (unless anyone knows better). |
Apparently the thermal driver can act as a hwmon device in case someone should need that: http://lxr.free-electrons.com/source/drivers/thermal/Kconfig |
|
Here's a proposal for uart1: diff --git a/arch/arm/boot/dts/bcm2708_common.dtsi b/arch/arm/boot/dts/bcm2708_common.dtsi
+ uart1: uart@7e215040 {
+ compatible = "brcm,bcm2835-aux-uart", "ns16550";
+ reg = <0x7e215040 0x40>;
+ interrupts = <1 29>;
+ clock-frequency = <500000000>;
+ reg-shift = <2>;
+ no-loopback-test;
+ status = "disabled";
+ };
+
diff --git a/arch/arm/mach-bcm2708/bcm2708.c b/arch/arm/mach-bcm2708/bcm2708.c
+ /* Turn on the mini uart if it is used */
+ np = of_find_compatible_node(NULL, NULL, "brcm,bcm2835-aux-uart");
+ if (of_device_is_available(np)) {
+ pr_info("%s: Turn on Mini UART\n", __func__);
+ writel(1, __io_address(UART1_BASE + 0x4));
+ } else {
+ bcm_register_device_dt(&bcm2708_uart1_device);
+ }
+uart1 will only be used on the Compute Module and it won't work without a patch that enables it in AUXENB. |
|
Making it work without DT sends the wrong message. James has just written a good tutorial on CM + DT, so I can't think of a good reason not to use DT. |
|
Glad to hear that, I'll make a PR. |
|
All devices have a Device Tree counterpart now and ARCH_BCM2835 is working with all drivers (except dwc_otg and lirc_rpi), so mission accomplished. |
|
Thanks @notro! |
Booting with CONFIG_DEBUG_VIRTUAL leads to following warning when passing hugepage reservation on command line: Kernel command line: hugepagesz=1g hugepages=1 hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1 noreboot HugeTLB: allocating 1 of page size 1.00 GiB failed. Only allocated 0 hugepages. ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at arch/powerpc/include/asm/io.h:948 __alloc_bootmem_huge_page+0xd4/0x284 Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 6.10.0-rc6-00396-g6b0e82791bd0-dirty #936 Hardware name: MPC8544DS e500v2 0x80210030 MPC8544 DS NIP: c1020240 LR: c10201d0 CTR: 00000000 REGS: c13fdd30 TRAP: 0700 Not tainted (6.10.0-rc6-00396-g6b0e82791bd0-dirty) MSR: 00021000 <CE,ME> CR: 44084288 XER: 20000000 GPR00: c10201d0 c13fde20 c130b560 e8000000 e8001000 00000000 00000000 c1420000 GPR08: 00000000 00028001 00000000 00000004 44084282 01066ac0 c0eb7c9c efffe149 GPR16: c0fc4228 0000005f ffffffff c0eb7d0c c0eb7cc0 c0eb7ce0 ffffffff 00000000 GPR24: c1441cec efffe153 e8001000 c14240c0 00000000 c1441d64 00000000 e8000000 NIP [c1020240] __alloc_bootmem_huge_page+0xd4/0x284 LR [c10201d0] __alloc_bootmem_huge_page+0x64/0x284 Call Trace: [c13fde20] [c10201d0] __alloc_bootmem_huge_page+0x64/0x284 (unreliable) [c13fde50] [c10207b8] hugetlb_hstate_alloc_pages+0x8c/0x3e8 [c13fdeb0] [c1021384] hugepages_setup+0x240/0x2cc [c13fdef0] [c1000574] unknown_bootoption+0xfc/0x280 [c13fdf30] [c0078904] parse_args+0x200/0x4c4 [c13fdfa0] [c1000d9c] start_kernel+0x238/0x7d0 [c13fdff0] [c0000434] set_ivor+0x12c/0x168 Code: 554aa33e 7c042840 3ce0c142 80a7427c 5109a016 50caa016 7c9a2378 7fdcf378 4180000c 7c052040 41810160 7c095040 <0fe00000> 38c00000 40800108 3c60c0eb ---[ end trace 0000000000000000 ]--- This is due to virt_addr_valid() using high_memory before it is set. high_memory is set in mem_init() using max_low_pfn, but max_low_pfn is available long before, it is set in mem_topology_setup(). So just like commit daa9ada ("powerpc/mm: Fix boot crash with FLATMEM") moved the setting of max_mapnr immediately after the call to mem_topology_setup(), the same can be done for high_memory. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Link: https://msgid.link/62b69c4baad067093f39e7e60df0fe27a86b8d2a.1723100702.git.christophe.leroy@csgroup.eu
[ Upstream commit e7e846d ] Booting with CONFIG_DEBUG_VIRTUAL leads to following warning when passing hugepage reservation on command line: Kernel command line: hugepagesz=1g hugepages=1 hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1 noreboot HugeTLB: allocating 1 of page size 1.00 GiB failed. Only allocated 0 hugepages. ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at arch/powerpc/include/asm/io.h:948 __alloc_bootmem_huge_page+0xd4/0x284 Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 6.10.0-rc6-00396-g6b0e82791bd0-dirty raspberrypi#936 Hardware name: MPC8544DS e500v2 0x80210030 MPC8544 DS NIP: c1020240 LR: c10201d0 CTR: 00000000 REGS: c13fdd30 TRAP: 0700 Not tainted (6.10.0-rc6-00396-g6b0e82791bd0-dirty) MSR: 00021000 <CE,ME> CR: 44084288 XER: 20000000 GPR00: c10201d0 c13fde20 c130b560 e8000000 e8001000 00000000 00000000 c1420000 GPR08: 00000000 00028001 00000000 00000004 44084282 01066ac0 c0eb7c9c efffe149 GPR16: c0fc4228 0000005f ffffffff c0eb7d0c c0eb7cc0 c0eb7ce0 ffffffff 00000000 GPR24: c1441cec efffe153 e8001000 c14240c0 00000000 c1441d64 00000000 e8000000 NIP [c1020240] __alloc_bootmem_huge_page+0xd4/0x284 LR [c10201d0] __alloc_bootmem_huge_page+0x64/0x284 Call Trace: [c13fde20] [c10201d0] __alloc_bootmem_huge_page+0x64/0x284 (unreliable) [c13fde50] [c10207b8] hugetlb_hstate_alloc_pages+0x8c/0x3e8 [c13fdeb0] [c1021384] hugepages_setup+0x240/0x2cc [c13fdef0] [c1000574] unknown_bootoption+0xfc/0x280 [c13fdf30] [c0078904] parse_args+0x200/0x4c4 [c13fdfa0] [c1000d9c] start_kernel+0x238/0x7d0 [c13fdff0] [c0000434] set_ivor+0x12c/0x168 Code: 554aa33e 7c042840 3ce0c142 80a7427c 5109a016 50caa016 7c9a2378 7fdcf378 4180000c 7c052040 41810160 7c095040 <0fe00000> 38c00000 40800108 3c60c0eb ---[ end trace 0000000000000000 ]--- This is due to virt_addr_valid() using high_memory before it is set. high_memory is set in mem_init() using max_low_pfn, but max_low_pfn is available long before, it is set in mem_topology_setup(). So just like commit daa9ada ("powerpc/mm: Fix boot crash with FLATMEM") moved the setting of max_mapnr immediately after the call to mem_topology_setup(), the same can be done for high_memory. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Link: https://msgid.link/62b69c4baad067093f39e7e60df0fe27a86b8d2a.1723100702.git.christophe.leroy@csgroup.eu Signed-off-by: Sasha Levin <sashal@kernel.org>
This is a list of the devices in arch/arm/mach-bcm2709/bcm2709.c that isn't loaded through Device Tree:
This driver allocates DMA channels. The bcm2835-dma driver will take care of this on ARCH_BCM2835.
Mailbox driver. Eric Anholt has worked on upstreaming this functionality, not sure of the current status.
I could not find the driver that uses this.
This device can move to DT (driver depends on mailbox and dma). Probably still a while until the vc4 graphics driver is ready.
Device can move to DT. This usb driver will probably stay with us for some time.
This device should be possible to move to DT, but I don't have a CM to test with.
I could not find the driver that uses this. Maybe arch/arm/mach-bcm2708/power.c was meant to use it?
Devices can move to DT.
The device can move to DT (driver depends on mailbox). Driver is not enabled in the default config.
The device can move to DT (driver depends on mailbox).
PR BCM270x_DT: Configure UART using DT #910 (BCM270x_DT: Configure UART using DT)
I have made a patch for the following devices: bcm2708_fb_device, bcm2708_usb_device, bcm2708_alsa_devices[0-7], bcm2835_thermal_device
Patch: https://gist.github.com/notro/882b88430626269d0572
(the patch is for Pi2 against rpi-4.0.y)
Audio: Should we use 2708 in the compatibility string to avoid a possible clash with a future mainline driver?
Are these in use: bcm2708_systemtimer_device, bcm2708_powerman_device ?
I can make a PR if this is useful. Comments are welcome.
The text was updated successfully, but these errors were encountered: