Hi,
On 06/30/2015 04:58 PM, Simon Glass wrote:
Hi Hans,
On 30 June 2015 at 06:54, Hans de Goede <hdego...@redhat.com> wrote:
Hi,
On 29-06-15 05:45, Simon Glass wrote:
Hi Hans,
On 17 June 2015 at 13:33, Hans de Goede <hdego...@redhat.com> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
