[coreboot] USB changes

[Peter Stuge]

Hi again Leandro,

Leandro Dorileo wrote:
> 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.

Right, but my point is that the different HCIs may not have the same
registers so just changing the names of the register acccess
functions may not be very useful?


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

Ok, but what about the endpoint address?

Is the array index guaranteed to always be the same as the endpoint
address sent on the bus?

Or is the endpoint address for each endpoint stored in the struct?

Note there can be two endpoints with the same address. (IN and OUT)

My point is that the default pipe is the endpoint with address 0, so
only if that is actually guaranteed by endpoint[0] then it's ok to
use endpoint[0]. Otherwise I guess there should be a function to look
up the default endpoint for the device.


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

Hm, I don't know.

SET_FEATURE can be sent to either device (bmRequestType=0), interface
or endpoint. What is endp? Is it explicitly documented to be the
index of the endpoints array - or is it the endpoint address as
described in the USB spec?

Also, possibly the function should accept an intf argument. The same
issue with USB stack array index vs. actual number in hardware
applies to interfaces. Interfaces have a number in hardware, and
that's what I think applications calling libpayload should be using.

I realize this discussion is a little out of scope for your project,
because it is about libpayload USB in general, but I still think it
should be cleared up - I think libpayload USB will be more popular in
the future thanks to added HCI support.


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

Again, I'm not sure.

GET_STATUS can also be sent to device, interface or endpoint. The
function should probably handle that somehow.


> >> @@ -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?
> 
> From the declaration you commented above. :)

Right! Thanks! ;)


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

Ok.


> in fact it should be used in every call commented previously.

Well, see above..


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

Actually it isn't. CLEAR_FEATURE can be sent to device, interface or
endpoint just like SET_FEATURE and GET_STATUS.


> Thanks a lot for reviewing.

No problem.


> Maybe the whole API should be changed to match devices with more
> than one control endpoint, but this is a first change.

Right. I think that's a good idea.


//Peter