From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Tue, 30 Jun 2015 17:54:54 +0200 Subject: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop In-Reply-To: References: <1434569645-30322-1-git-send-email-hdegoede@redhat.com> <1434569645-30322-9-git-send-email-hdegoede@redhat.com> <5592917D.9040000@redhat.com> Message-ID: <5592BBCE.7020008@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 06/30/2015 04:58 PM, Simon Glass wrote: > Hi Hans, > > On 30 June 2015 at 06:54, Hans de Goede wrote: >> Hi, >> >> On 29-06-15 05:45, Simon Glass wrote: >>> >>> Hi Hans, >>> >>> On 17 June 2015 at 13:33, Hans de Goede wrote: >>>> >>>> On an usb stop instead of leaving orphan usb devices behind simply remove >>> >>> >>> On a usb_stop() >>> >>> or >>> >>> On a 'usb stop' command ? >> >> >> My intention was for both, since I was under the assumption that "usb stop" >> on the cmdline, was the only caller of usb_stop(), but a quick grep to the >> sources show that I'm wrong ... >> >>>> them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build >>>> usb_stop() when that is set. >>> >>> >>> This seems a little unfortunate. I can see the reasoning, but do you >>> think this is necessary? I suspect people chasing code size may remove >>> that option and still want to use USB properly. >> >> >> This was mostly a result of my thinking that usb_stop() is only used >> on "usb stop" at the cmdline, which I know realize is wrong. >> >> However my quick grep has learned that we do really need >> CONFIG_DM_DEVICE_REMOVE >> to properly implement usb_stop(): >> >> From common/bootm.c : >> >> #if defined(CONFIG_CMD_USB) >> /* >> * turn off USB to prevent the host controller from writing to the >> * SDRAM while Linux is booting. This could happen (at least for >> OHCI >> * controller), because the HCCA (Host Controller Communication >> Area) >> * lies within the SDRAM and the host controller writes continously >> to >> * this area (as busmaster!). The HccaFrameNumber is for example >> * updated every 1 ms within the HCCA structure in SDRAM! For more >> * details see the OpenHCI specification. >> */ >> usb_stop(); >> #endif >> >> And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove >> callback and thus do not properly stop the usb controller. >> >> So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists >> before this patch. If you want I can split out the adding of the #ifdef >> in a separate commit, spelling out why usb_stop() MUST have >> CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all >> to >> Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE? >> > > I don't think that is necessary, it feels a bit too inflexible. But > perhaps you could add a comment to the Kconfig help for > CONFIG_DM_DEVICE_REMOVE? Ok will do. > It is remove() that is needed, not unbind(). Actually I think it is > quite unfortunate to make usb_stop() call unbind. It is a waste of > time to do this just before booting the kernel - the current design > leaves all devices bound (but I hope we can remove() them at some > point). > > Instead, I wonder if we can remove the children when we probe the bus? That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge. > Also, what happens to children that are in the device tree - i.e. > static USB devices like WiFi? The device tree might have parameters > for them. Still, that might not matter as I'm not sure that case is > handled correctly today. AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus. > >> >> >>> >>>> >>>> The result of this commit is best seen in the output of "dm tree" after >>>> plugging out an usb hub with 2 devices plugges in and plugging in a keyb. >>>> instead, before this commit the output would be: >>>> >>>> usb [ + ] `-- sunxi-musb >>>> usb_hub [ ] |-- usb_hub >>>> usb_mass_st [ ] | |-- usb_mass_storage >>>> usb_dev_gen [ ] | `-- generic_bus_0_dev_3 >>>> usb_dev_gen [ + ] `-- generic_bus_0_dev_1 >>>> >>>> Notice the non active usb_hub child and its 2 non active children. The >>>> first child being non-active as in this example also causes >>>> usb_get_dev_index >>>> to return NULL when probing the first child, which results in the usb kbd >>>> code not binding to the keyboard. >>> >>> >>> Although I suspect that could be fixed. >> >> >> Right, but just removing the children is a much cleaner solution, and also >> makes the output of "dm tree" properly reflect reality. > > True, although you also have 'usb tree' for that. Another option would > be to mark devices that were found and remove the others after the > scan. That seems like needless complexity. I believe that simply removing + unbinding the children on usb_stop is the right thing to do, and it also is the KISS solution. Regards, Hans