[U-Boot] [PATCH 0/8] usb: driver-model fixes and dm support sunxi-ehci.c

[Simon Glass]

Hi Hans,

On 30 April 2015 at 13:38, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 30-04-15 16:48, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 30 April 2015 at 08:35, Hans de Goede <hdegoede at redhat.com> wrote:
>>>
>>> Hi Simon and Marek,
>>>
>>> This series completes my work on converting the sunxi usb/ehci code to
>>> the driver model. With this series everything works as it did before,
>>> and all the issues I've been seeing when switching to the driver model
>>> are resolved.
>>>
>>> Please review / merge. I think it would be best to merge this through
>>> the dm tree.
>>
>>
>> Great to see this, thanks for all the work and fixes.
>>
>> I am not too keen on the passing through of struct usb_device, in fact
>> I went to some lengths to avoid that.
>>
>> I'll read through the patches again, but I wonder if we can avoid
>> heading in that direction? Conceptually in driver model the device
>> does not exist until we find out what it is and can locate a device
>> driver.
>
>
> Right, the problem is that AFAICT the way the driver-model works is
> that the driver needs to be known at device creation time, this
> probably is a result of the auto alloc mem on behalf of the device
> driver feature.

It's really a design decision.

>
> But for usb this is problematic as we need to read descriptors and
> whats not before we can find the right driver, and then we do not
> want to re-read those descriptors and the driver needs access to
> them too.
>
> What you've been doing sofar is in essence passing through
> struct usb_device from usb_scan_device to the pre_child_probe
> method, with the difference that you've been doing it field by
> field. My patch which just adds the maxpacket size to the fields
> you're passing (via platdata) also works, but requires re-reading
> all the descriptors which is slow(ish) and may upset some devices,
> this can be avoided by stuffing more of usb_device into the
> plat_data passed from sb_scan_device to the pre_child_probe, but
> I decided it would be easier to just pass all of the usb_device

It might take a few extra milliseconds (I have not timed it) but given
the >1000ms it seems to take to start up USB I don't think it matters.
It would be a strange device that refuses to let you read the
descriptor twice.

While it is of course easier to pass all of usb_device, I am not keen.
It is there not clear which bits are passed through and which are not.
If you like we could put the relevant bits in a struct.

>
> Another slightly cleaner (IMHO) option then copying over the entire
> usb_device would be to dynamically alloc usb_device in usb_scan_device,
> and keep using the same usb_device all the time, so remove the on
> stack copy, and do not auto-alloc the per child data for the uclass
> as it is already allocated manually in usb_scan_device.

But then we have a struct usb_device before a struct udevice. That
just cements what I think is wrong with the code.

>
> This effectively gives us the kernel model of allocating the
> generic usb_device bits before looking for a driver.

I'd be happier with that approach if we actually could split the
generic bits into some sort of 'struct urb' or similar. It represents
a destination for a request, and the request itself, not the device. I
think it is unfortunate that U-Boot conflates the device and the
request. About all we need to send a request to a device is its pipe
word and packet sizes. The device is a U-Boot concept - we can after
all fire requests all over the USB bus without a 'device'.

Regards,
Simon