[coreboot] USB changes

[Leandro Dorileo]

Hi Peter


On Tue, Jul 21, 2009 at 5:24 PM, Peter Stuge<peter at stuge.se> wrote:
> Hi Leandro,
>
> Leandro Dorileo wrote:
>> -     uhci_reg_write16 (controller, USBCMD, 4);
>> +     hci_reg_write16 (controller, USBCMD, 4);
>
> This change may not be a good idea.
>
> Just changing the function names is not enough to abstract the code
> for different HCIs. I would prefer if the function names remain until
> a commit which actually covers one of OHCI and EHCI.





The hci_reg_* functions use the reg_base in hci_t structure to
determine which port to write to/read from, this structure is used
both for OHCI, UHCI and will be for EHCI, so doesn`t matter if the
controller is UHCI or OHCI there will always be a reg_base, with that
I thought I could move it to usb.h and reuse it.

The changes you pointed are just in the caller, UHCI in the case.






>
>
>> Subject: [PATCH 3/5] usb: API change, control receive endpoint_t
>>
>> Changed the usb API where the control function first parameter now
>> is a pointer of endpoint_t instead of a pointer of usbdevice_t.
>
> Changing the API like this is a good thing.
>
>
>> The previous implementation assumed the first endpoint(index 0) as
>> control, which is not true, we can have devices with more than a
>> single control line.
>
> What do you mean by index 0 here? Is it the index in an array in the
> USB stack? Is it the endpoint number?





Sorry, I forgot the mention. usbdev structure has an array of its endpoints.





>
>
>> Subject: [PATCH 4/5] uhci: control adaptations
>>
>> Chaging the implementation of uhci_control function to match the api
>> changes done in the previous patch.
>
> These two changes should be in the same commit, otherwise the code is
> broken in between.





Ok.





>
>
>> Subject: [PATCH 5/5] control users: change the callers of ->control
>
> This also belongs in the same commit as 3 and 4.





Ok.





>
>
>> This patch introduces changes in the usb main program and msc driver
>> as well. It basically passes an endpoint_t instead of a usbdevice_t
>> to control function.
>>
>> We are still assuming the first endpoint to be the control one. We
>> may need to change the functions in usb.c with a depper adaptation
>> to accommodate drivers for devices with more than a single control
>> endpoint but for now endpoint[0] should work.
>
> How is this array populated?



In the set_address, this function is called when a device is
"attached", the function usb_attach_device calls set_address to
configure the new detected device. The array is populated based on
interface_descriptor_t which reports the bNumEndpoints(number of
endpoints) but before anything it does configure an endpoint[0].type =
CONTROL.





>
> The default pipe always accepts control transfers, but is it
> automatically populated to index 0 in the endpoints array? Note that
> the default pipe does not usually show up in any descriptor.




Answered above.




>
>
> There are some issues in the following code:
>
>> @@ -109,7 +109,7 @@ set_feature (usbdev_t *dev, int endp, int feature, int rtype)
>> +     dev->controller->control (&dev->endpoints[endp], OUT, sizeof (dr), &dr, 0, 0);
>
> This is good. (Or is it? Is endp specified in the API to be the
> index, and not the endpoint number?)




Yes sorry, maybe wrong. It should be [0].




>
>
>> @@ -123,7 +123,7 @@ get_status (usbdev_t *dev, int intf, int rtype, int len, void *data)
>> +     dev->controller->control (&dev->endpoints[intf], IN, sizeof (dr), &dr, len, data);
>
> Here an interface number is suddenly used as index in the endpoints
> array. Please explain how that can be correct?




Same here.




>
>
>> @@ -134,6 +134,7 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
>> +    endpoint_t *ep = &dev->endpoints[langID];
>
> Here langID is used as index in the endpoints array. That also seems
> like it will be a problem.




Yes, same as above.




>
>
>> @@ -141,7 +142,7 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
>> +     if (dev->controller->control (ep, IN, sizeof (dr), &dr, 8, buf)) {
>
> Where does this ep come from?