[coreboot] USB changes

[Leandro Dorileo]

Hi Peter

On Tue, Jul 21, 2009 at 6:35 PM, Peter Stuge<peter at stuge.se> 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
>
> --
> coreboot mailing list: coreboot at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot



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

-- 
(°=   Leandro Dorileo
//\    ldorileo at gmail.com   -   http://www.dorilex.net
V_/  Software is a matter of freedom.