[coreboot] USB changes

[Peter Stuge]

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.


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


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


> Subject: [PATCH 5/5] control users: change the callers of ->control

This also belongs in the same commit as 3 and 4.


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

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.


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?)


> @@ -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?


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


> @@ -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?


> @@ -165,7 +166,7 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
> +	    control (ep, IN, sizeof (dr), &dr, size, result)) {

Same one..


> @@ -183,7 +184,7 @@ set_configuration (usbdev_t *dev)
> +	dev->controller->control (&dev->endpoints[0], OUT, sizeof (dr), &dr, 0, 0);

Is index 0 in endpoints guaranteed to always be the default endpoint?


> @@ -201,7 +202,7 @@ clear_stall (endpoint_t *ep)
> +	dev->controller->control (ep, OUT, sizeof (dr), &dr, 0, 0);

Good.


> @@ -246,7 +247,7 @@ set_address (hci_t *controller, int lowspeed)
> +	if (dev->controller->control (&dev->endpoints[0], OUT, sizeof (dr), &dr, 0, 0)) {

Again with index 0. And it happens a few more times.


//Peter