USB changes

[Leandro Dorileo]

Hi Patrick

I have done the changes we discussed previously. Attached follows the
patches. The 2 first patches are just few changes in the reg
functions(*read/write*) so we can use the same functions in both OHCI
and UHCI.

The last patches are related to the changes I proposed to control
function. I`m copying here the commit log message:

"Changed the usb API where the control function first parameter now
is a pointer of endpoint_t instead of a pointer of usbdevice_t.

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

Since MSC device has always a single control endpoint I kept assuming
that, and the changes to the drivers do exactly that, takes the first
endpoint and passes it to control.

I would like to keep those changes upstream already so it can be
easily maintained. Please review and comment.

PS: I`m cc`ing the coreboot mailing list, this way we can have more reviewers.

Thanks in advance....

[Leandro Dorileo]

On Tue, Jul 21, 2009 at 4:29 PM, Leandro Dorileo wrote:
> Hi Patrick
>
> I have done the changes we discussed previously. Attached follows the
> patches. The 2 first patches are just few changes in the reg
> functions(*read/write*) so we can use the same functions in both OHCI
> and UHCI.
>
> The last patches are related to the changes I proposed to control
> function. I`m copying here the commit log message:
>
> "Changed the usb API where the control function first parameter now
> is a pointer of endpoint_t instead of a pointer of usbdevice_t.
>
> 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."
>
> Since MSC device has always a single control endpoint I kept assuming
> that, and the changes to the drivers do exactly that, takes the first
> endpoint and passes it to control.
>
> I would like to keep those changes upstream already so it can be
> easily maintained. Please review and comment.
>
> PS: I`m cc`ing the coreboot mailing list, this way we can have more reviewers.
>
> Thanks in advance....

[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

[Leandro Dorileo]

Hi Peter

On Tue, Jul 21, 2009 at 5:24 PM, Peter Stuge 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?
> From the declaration you commented above. :)
> > @@ -165,7 +166,7 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
> > control (ep, IN, sizeof (dr), &dr, size, result)) {
>
> Same one..

Same as above.
> > @@ -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, in fact it should be used in every call commented previously.
> > @@ -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.

Ok, it should always be 0.
> //Peter

Thanks a lot for reviewing. Maybe the whole API should be changed to
match devices with more than one control endpoint, but this is a first
change. Please tell me if I`m missing something about the uhci_reg_*
functions.

Regards...

[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

[Leandro Dorileo]

Hi Peter

On Tue, Jul 21, 2009 at 6:35 PM, Peter Stuge wrote:
> 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

Yes, sure and they don`t have the same registers.
> so just changing the names of the register acccess
> functions may not be very useful?

The reg_base is a u32 field which`s the base address for control
registers, it`s differently set for different Host Controller. For
example, on UHCI we do:

controller->reg_base = pci_read_config32 (controller->bus_address, 0x20) & ~1;

but for OHCI we do:

controller->reg_base = pci_read_config32 (controller->bus_address, 0x10)

UHCI for example calls hci_reg_write32 with FLBASEADD reg offset(which
is not present on OHCI). Lets look at hci_reg_write32 function:

void
hci_reg_write32 (hci_t *ctrl, int reg, u32 value)
{
    outl (value, ctrl->reg_base + reg);
}

Like I told you it`ll use the control base address to take to the
specific reg address, that may differ from Host Controller to Host
Controller. On OHCI code for example we do:

hci_reg_write32(controller, OHCI_REG_HCCA, \
            (u32) virt_to_phys (OHCI_INST (controller)->hcca));

In the above snippet we use OHCI_REG_HCCA that`s the offset for HCCA
register, we don`t have this field on UHCI and the reg_base is set
with the proper control registers base address.

I understand it works exactly the same way to any Host Controller and
both drivers can use the same function(but configuring properly the
base control address and passing the correct offset to the wanted
register). The fact of having reg_base to determine the base address
of control registers makes me think I can use the save function for
both Host Controllers, they work exactly the same way.
> > > > 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.

Hum, I think I got your point here. Is there anything that guarantee
us the first endpoint will always be control one with address 0 and
direction.

Well, I couldn`t find some information about it on usb specification -
I`m almost sure it`s defined on class device specification. The MSC
specification states "Every USB device also defines a Control endpoint
(Endpoint 0). This is the default endpoint ..." but it may be
different to other classes.

Since we only support MSC and HID this should work. I think we`ll need
a small redesign addressing these issues whenever we get to support
other devices and surely a function to look up the default endpoint
will be needed(in the case the other devices can not behave like MSC
and HID regarding control endpoints).
> > > > @@ -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?

it`s the endpoint address as described in the USB spec. I think your
point here is the endpoint address, if so the previous comments might
explain.
> 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.

It shouldn`t be using intf since all we need to control function is
the control endpoint. With the endpoint_t we can find the interface
but it`s not needed cause it`s already calling the control function
defined to a known interface(if I understand your point here).
> 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.

Sure, and I can take the job I`ll be here around even after my project
has finished. ;-)
We can redesign everything needed to match other devices needs.
> > > > @@ -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.

Well, we discussed the array index vs. endpoint address. :D
> GET_STATUS can also be sent to device, interface or endpoint. The
> function should probably handle that somehow.

As far as I know we handle get_status differently for endpoint and interface.
> > > > @@ -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..

Well, we discussed the array index vs. endpoint address. :D
> > > > @@ -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

Well, we really will need to review many things in the current usb
stack but I need to support E/OHCI for now. We`ll have time to change
things when we need support more devices. Again, thank you for your
time reviewing the changes.

Regards....

[Patrick Georgi]

Peter, thank you for your review!

Peter Stuge schrieb:
> > 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 old API was based on a misunderstanding of the spec on my part, and
the little detail that so far the stack doesn't have drivers for devices
with more than one control pipe. So yes, this is a very good thing! :-)
(and necessary because OHCI does things in different ways than UHCI in
that area)
> > @@ -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?)

There's no correlation between endpoints[x] and endpoint number x in the
code. Simply because there can be two endpoints x(in) and x(out).
The only one that's guaranteed is endpoints[0] which points to endpoint
0 - but that one is a special case as direction doesn't matter for it
(according to the spec).
> > @@ -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?

drivers/usb/usb.c(set_address) initializes endpoints[0] to the default
endpoint (type control, endpoint 0, proper maxPacketSize).

Given that this is the endpoint used to do basic initialization, I doubt
any device vendor is stupid enough to move the default endpoint to
somewhere else after that.
And if there is such a device, the special device driver for that device
could still change endpoints[0] easily.

Regards,
Patrick