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

[Hans de Goede]

Hi,

On 01-05-15 00:04, Simon Glass wrote:
> 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.

TBH I'm not sure if it is the best decision (in hindsight) we may hit
similar issues in other cases. Anyways this is how things work now,
changing this is going to be a lot of work, so lets just roll with
it.

>
>>
>> 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.

I've done a lot of USB work (including redirection of USB devices
into virtual machines for qemu) and I've seen stranger devices :)

But I must admit that I cannot actually name an example, it is just that
I've a feeling that this may become a problem.

>
> 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.

We already have a struct usb_device before a struct udevice, just
because the struct usb_device is hiding on the stack rather then
sitting in the heap does not mean it is not there.

>
>>
>> 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'.

We do not just need the usb_device to send usb requests before we
have a udevice (which a struct urb would fix) we also need it to
store the descriptors read before we've a udevice.

Also see my reply to your review of patch 1/8.

Regards,

Hans