Hi,
On 11-11-15 19:15, Simon Glass wrote:
Hi Hans,
On 11 November 2015 at 10:02, Hans de Goede <j.w.r.dego...@gmail.com> wrote:
<snip>
Ok, I've ran a whole battery of tests on your u-boot-dm/usb-working branch.
Thanks!
There are 2 issues:
1) You need to add these change to the commit introducing usb-keyb dm
support (or one of the related commits), otherwise usb-keyb support
breaks:
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -513,6 +513,7 @@ config ARCH_SUNXI
select DM
select DM_GPIO
select DM_ETH
+ select DM_KEYBOARD
select DM_SERIAL
select DM_USB
select OF_CONTROL
The breakage is that without this usb_scan_device() returns -96
(EPFNOSUPPORT) for usb keyboards.
OK. I am a bit concerned this might affect other boards too. Still, if
we get this series applied soon there is plenty of time for testing.
I'll look a bit closer.
Ok.
2) With that branch there still is the purely cosmetical issue,
that if one plugs a usb-stick + keyb into the same hub, then swaps
their place and do "usb reset" and then "usb tree" looks like this:
USB device tree:
1 Hub (480 Mb/s, 100mA)
| USB2.0 Hub
|
+-3 Human Interface (1.5 Mb/s, 100mA)
| SINO WEALTH USB Composite Device
|
+-2 Mass Storage (480 Mb/s, 100mA)
USB Flash Disk 4C0E960F
Notice the order of devices listed: 1 - 3 - 2, which is a bit weird,
as said this is purely cosmetical.
Note you can easily fix this by only reverting the:
"dm: usb: Rename usb_find_child to usb_find_emul_child"
commit, and keep the other 2 you revert and dropping this patch ?
As I already suggested before, this is both more KISS and as you
can see it solves some actually (admittedly very minor) issues.
I'm not really buying your arguments for your more complex solution,
as shown above re-using the devices actually causes issues.
Your other argument of wanting to attach device-tree properties
I also do not find a strong argument, for one there has never been
a need to do so sofar, and if we ever need this we need a way
to specify which usb-device the properties belong to based on
the topology under the host / root-hub anyway, and match things
up when first scanning the bus. And if we can match things up
on the first scan we can also match them up on subsequent scans
and attach the same of-node again.
Anyways I'm fine with doing things your way, but I still have
a preference for the more KISS and IMHO robust solution of
simple unbinding all devices on usb-stop.
I wonder if it really is more complex. My preferred algorithm is to
remove any devices that have not been probed after a scan. Yours is to
remove and unbind any USB devices before a scan. What is the
difference?
If you unbind everything before (re)scanning you always start with
a clean slate, making things more reproducible and simpler.
As for it being more complex, your approach needs a whole new
helper function and needs to walk the list of devices twice, where
as mine is just a single extra function call + error checking.
As an added bonus my approach allows adding an ifdef to usb_find_child
turning it into a nop like this
#if defined CONFIG_SANDBOX || !defined CONFIG_DM_DEVICE_REMOVE
...
... complex code to find child
...
#else
return -ENODEV;
#endif
So besides not needing the extra helper function your solution
needs, my version also removes a bunch of extra code (from the
resulting binary) in almost all cases.
So your solution is definitely more complex, needlessly so IMHO.
I much prefer not unbinding and rebinding devices. To my mind, devices
should be bound once if possible, and most of the time it is possible.
Well in case of a re-scan we are discovering all devices from scratch,
not just looking for changed devices, simply starting with a clean
device list reflects this.
This is different from the Linux approach of combining 'bind' and
'probe'. At present sandbox cannot support 'usb reset'. I would like
sandbox to provide full support so that we can use tests to verify
functionality rather than manual testing.
Re the ordering above, I suppose I could fix that command easily
enough. This is not a common problem though. Does it really matter?
Nope, as said this is a cosmetic issue, and for the record
I'm fine with your approach of always re-using existing devices
on a re-scan. I prefer the start with a clean slate approach but
either is fine with me.
###
Talking about usb-stop, there is still one (BIG!) problem after
this patch set when building usb-support with DM_DEVICE_REMOVE
not set. This means that the controllers dma engines will not
be stopped when booting the actual OS and they will still be
accessing parts of the main memory while the actual OS is
booting, which is BAD. So I suggest adding something like
this to all host drivers which use dma in this way:
#if defined CONFIG_DM_USB && !defined CONFIG_DM_DEVICE_REMOVE
#error The EHCI driver cannot be used without CONFIG_DM_DEVICE_REMOVE otherwise
DMA stays active while booting the OS
#endif
Sounds good. I am not suggesting that we encourage people to build USB
with out DM_DEVICE_REMOVE. I just want to make sure that we don't
create unnecessary dependencies such that DM_DEVICE_REMOVE becomes
impossible to use.
We have a note in the Kconfig for that, but I agree we need to make it
more explicit. With the #if approach it makes the pitfall much more
obvious.
Ack, so we need to have patches for this for at least ohci, ehci, xhci
and I guess also uhci ? Who is going to write these patches ?
Regards,
Hans
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
