Hi Hans,
On 30 April 2015 at 13:38, Hans de Goede hdegoede@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@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