Hi Peter
On Tue, Jul 21, 2009 at 6:35 PM, Peter Stugepeter@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@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....