Use raspberrypi-power to turn on USB power #1700
Conversation
Use the raspberrypi-power driver to turn on USB power. Signed-off-by: Noralf Trønnes <[email protected]>
The raspberrypi-power driver is now used to turn on USB power. This partly reverts commit: firmware: bcm2835: Support ARCH_BCM270x Signed-off-by: Noralf Trønnes <[email protected]>
|
Works okay for me. @pelwell okay to merge? |
|
Just as an outsider looking in, is there any value to this other then moving code from one driver to another? Putting it another way,are people that actually use the RPI going to notice any difference with a change like this? |
|
Even if it was just copying code from one place to another, putting power-related functionality in the power driver makes sense, but this is removing duplicated code - the memory footprint will be reduced. A bit. |
|
The value is reducing downstream patches which in this case is: 0c533fb That patch was made for 4.2 prior to the raspberrypi-power driver entering with 4.4. And it was made partly in effort to drop a previous driver which only task at that time was to turn on usb power. We can't completly drop that patch, because we don't want the video driver to defer probing due to the firmware driver not being probed yet. We want video as early as possible so we can see the log if something goes wrong. Another way to solve it would be to edit the driver include order in the Makefile(s). End users won't notice any difference from this particular change. |
|
Cool, I understand better now. Thanks. Perhaps the downstream patch in this case should be completely removed then. I've been using the linux-next version of the upstream for a bit and I noticed that the logging issue isn't an issue anymore. The "simple" driver is used until the vc4 driver loads and switches over. In fact, on the upstream kernel I sometimes just use the simple driver to reduce the amount of stuff I have to recompile. It actually works except for being a bit on the slow side. Of course the downstream video driver is much faster, but that's a whole story in itself... |
|
BTW, just to add one more comment. I think the upstream driver probing could be improved. I see no reason why deferred should ever happen. Except in very rare cases, drivers don't have circular dependencies. Most of the time, one driver is completely dependent on the other. So why couldn't the driver loading code walk the tree from the lowest levels first so that drivers that are depended on are always loaded first? |
|
The vc4 driver isn't production ready yet.
The linux device/driver model doesn't have a way to specify that kind of dependency. |
|
Actually, I have no idea how this is working but I have a pure upstream tree without u-boot on the machine at all and simplefb is working. I have vc4 completely removed from the build config. Perhaps I'm missing something. I'm not trying to start something here or be too argumentative. I'm just trying to understand... |
|
OK, I understand what is happening now. The firmware is munging the DT so that it loads the simple-simple-framebuffer with the correct parameters. /proc/device-tree/chosen/framebuffer@3d7fe000$ ls -l cat compatible Like I said, I don't have u-boot on the RPI at all and this is a RPI 2. |
I realize that. I'm just saying perhaps the device model could be improved. That's all. |
|
The firmware only adds the simplefb framebuffer node if the VPU-based V3D driver has been disabled, which should only happen if Eric's VC4 driver is enabled. The new bcm2835-based DTBs include the nodes that the firmware was looking for, but disabled by default, and until a patch on October 11th the firmware was ignoring the "status" property. You should update your firmware and see if simplefb is still initialised - it shouldn't be. |
|
OK, thanks for the explanation. My DT does have vc4 enabled because that is the default in upstream. I just disabled VC4 by removing the driver from the config/build image. Actually, I thought it was kind of cool that simplefb is loading. It gives me the nice bootup logs and the performance isn't nearly as bad as I expected.. I don't know why anybody would want it disabled. |
|
I think that a static initialisation order based on device class (i.e. a property of the module) is potentially inadequate given that different instances of a device may require different initialisation orders with respect to instances of devices in another class. |
Because the accelerated bcm2708_fb driver doesn't include the necessary code to take over the framebuffer created by/for simplefb. The VC4 driver has this code, and I don't think it would be much work to add it to bcm2708_fb, it just hasn't been done yet. If you're looking for a project... |
|
Perhaps that would be cool and I may well submit a PR for that. I saw in the VC4 code that it's only a few lines of code to kick out the simplefb driver. At one point on a local version I even had the bcm2708_fb loading first and the VC4 driver kicking it out once it was loaded.. |
|
simplefb is probed after bcm2708_fb so I can't see any benefit from combining them. simplefb is meant to be used in distros that bundle many big graphics drivers which they don't want in the kernel binary, so they are built as modules and hence loaded late. |
|
My suggestion was based on Eric's comments here, which made me think that simplefb could be used like earlycon, but that could be my mistake. I like the simplified information flow with simplefb:
vs:
Although these look the same length, I think the former is more logical (information clearly flows from VPU to ARM), and it has the advantage that the FB can created early and perhaps even written to by the VPU (error messages, etc.). |
|
@pelwell, that's kind of what I was thinking. The only other wrinkle I was kicking around in my head is why can't bcm2708_fb act like simplefb itself in the case where simplefb is not compiled into the image? For example:
Then if VC4 is enabled, VC4 turns on late and kicks bcm2708_fb. But this is very similar to your idea. |
|
Exactly. Provided the initial framebuffer has the same bindings and functionality as simplefb (so the firmware knows how to talk to it), I don't think it matters what the compatible string says. |
|
I agree. In fact, by acting like simplefb it would be possible to test all this without any firmware changes. |
|
BTW, I think the firmware would only be needed for changing the framebuffer mode at runtime, but I think this is rare. In fact, I don't think the driver needs the firmware at all until the mode is changed. |
|
@notro I had a report from @MilhouseVH that last night's LibreELEC build fails to boot from USB disk. LE uses the 4.8 kernel.
I've tried reproducing here, but both a raspbian nfs boot and a raspbian boot from USB both worked fine for me. Now LE has its own kernel config options: I can't see a problem there. Any ideas? |
|
Do you have a kernel log? |
|
Could this be the coherent_pool=256k issue? |
It works with this PR reverted and fails with it in place. Could this PR affect coherent allocation? |
No, you're right, that was the previous PR. |
|
@popcornmix I'm not booting with USB disk, but I have both a 2B and 3B using WD USB Pi drives for ROOT partitions with thie patch applied to rpi-4.8.y HEAD. |
|
I can't see any problems with the config either. |
|
From @MilhouseVH
In good case we see |
|
Ping @P33M |
|
You could try dwc2 to see if that works reliably: CONFIG_USB_DWC2=y |
|
Talking to @P33M, most likely reason is the USB power is being enabled after the dwc_otg driver tries to init. i.e. "new full-speed USB device" is appearing because a USB peripheral register read is returning garbage (possibly 0xdeadbeef), and subsequent lack of activity is due to a reset polling a register to wait for it to complete. So, are we sure that USB power is being applied before dwc_otg initialises? |
|
dmesg with There is a small amount of difference to the "without usb" dmesg, however I now see the message |
|
Power is turned on before dwc_otg probe is called. module_init(dwc_otg_driver_init); dwc_otg_driver_init() -> platform_driver_register():__platform_driver_register() -> driver_register() ->> platform_drv_probe(): static int platform_drv_probe(struct device *_dev)
ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
ret = drv->probe(dev);dev_pm_domain_attach() -> genpd_dev_pm_attach() -> Turn on USB power The difference from before this PR, is the time from turning on power to probe is called. This used to be 2 seconds. I found this instrumented boot log in my terminal buffer which shows when it happens: |
earlycon is a serial console initialized before any drivers are loaded: early_param("earlycon", param_setup_earlycon); Initialized at this point in time: start_kernel() -> setup_arch() -> parse_early_param() -> parse_early_options() -> do_early_param() fbdev is initialized at subsys_initcall(fbmem_init) which happens after irq and timer drivers are loaded, but before most drivers. This is our current init call order: arch_initcall(customize_machine); /* arch/arm/mach-bcm/board_bcm2835.c: bcm2835_init() */
arch_initcall(bcm2835_mbox_init);
subsys_initcall(fbmem_init);
subsys_initcall(bcm2835_dma_init);
subsys_initcall(rpi_firmware_init);
module_init(bcm2708_fb_init);This can be done about the load order, not sure which messages we can catch with this: arch_initcall(customize_machine); /* arch/arm/mach-bcm/board_bcm2835.c: bcm2835_init() */
arch_initcall(bcm2835_mbox_init);
arch_initcall_sync(bcm2835_dma_init);
arch_initcall_sync(rpi_firmware_init);
subsys_initcall(fbmem_init);
subsys_initcall_sync(bcm2708_fb_init);Available init hooks: #define core_initcall(fn) __define_initcall(fn, 1)
#define core_initcall_sync(fn) __define_initcall(fn, 1s)
#define postcore_initcall(fn) __define_initcall(fn, 2)
#define postcore_initcall_sync(fn) __define_initcall(fn, 2s)
#define arch_initcall(fn) __define_initcall(fn, 3)
#define arch_initcall_sync(fn) __define_initcall(fn, 3s)
#define subsys_initcall(fn) __define_initcall(fn, 4)
#define subsys_initcall_sync(fn) __define_initcall(fn, 4s)
#define fs_initcall(fn) __define_initcall(fn, 5)
#define fs_initcall_sync(fn) __define_initcall(fn, 5s)
#define rootfs_initcall(fn) __define_initcall(fn, rootfs)
#define device_initcall(fn) __define_initcall(fn, 6)
#define device_initcall_sync(fn) __define_initcall(fn, 6s)
#define late_initcall(fn) __define_initcall(fn, 7)
#define late_initcall_sync(fn) __define_initcall(fn, 7s)
#define __initcall(fn) device_initcall(fn)
#define module_init(x) __initcall(x);If we want to see error messages from mailbox/dma/fw then we can use simplefb and do this: subsys_initcall(fbmem_init);
subsys_initcall_sync(simplefb_init); /* changed from fs_initcall */
subsys_initcall_sync(bcm2835_dma_init);
subsys_initcall_sync(bcm2835_mbox_init);
fs_initcall(rpi_firmware_init);
module_init(bcm2708_fb_init); /* kicks out simplefb */If we make sure all drivers support deferred probing, then there's no need to change the initcall order, since simplefb will not defer and will load before the other drivers. Now it doesn't matter that bcm2708_fb defers probing, because simplefb prints the messages. Yes, this sounds like a good solution. And it will work with vc4 as well. Currently there's no way to get very early graphics console messages except for VGA and the like which has text mode. I did some work on panic message rendering in the DRM subsystem which in theory could be extended to include early boot messages (after unflatten_device_tree()). But that worked stalled due to David Herrmann being busy with BUS1 (I guess). |
|
It looks like kicking simplefb is a simple function call. This is the function that vc4 calls to kick simplefb. It would only take a few lines to have bcm2708_fb call this function as well. `int do_remove_conflicting_framebuffers(struct apertures_struct *a, }` |
|
Regarding USB power-on delay: in Videocore the sequence is
Having played around with slave-mode access before, I know that there's an additional debounce delay required before the PHY properly reports line status - this was experimentally determined to be several hundred milliseconds. I can well believe that having the 2 second interval implicit in the boot process stopped things going wrong before... |
|
In that case, I think we should add back the poweron delay that was removed in the firmware over a year ago: #1026 (comment) Most Pi's are multi cpu so the added delay won't matter now with asynchronous probing support: torvalds/linux@765230b |
|
I've tried adding a config.txt parameter to delay at the end of powering on the USB (before responding to mailbox power request), but even 15 seconds doesn't fix the LibreELEC build. A build with this patch resulted in this log which doesn't include the "set_power" message I see with raspbian kernel. |
|
And both of these are present? |
|
And this: |
|
Thanks @notro & @popcornmix, Sorry for all the aggro with our non-standard configs! |
|
I wonder if CONFIG_RASPBERRYPI_POWER should depend on or select CONFIG_PM to avoid this sort of issue in future? |
|
Yes, I would think so. Both of_genpd_add_provider_onecell() and pm_genpd_init() are empty if CONFIG_PM_GENERIC_DOMAINS is not set, so it should probably Current situation: Thanks @MilhouseVH for all your testing. |
I just keep on chipping away on the downstream patches by using what's available in mainline.
@anholt has really done much good in mainline for the Pi and it's more people contributing there now.