Hi Peter
On Tue, Jul 21, 2009 at 5:24 PM, Peter Stuge<[email protected]> 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 > > -- > coreboot mailing list: [email protected] > http://www.coreboot.org/mailman/listinfo/coreboot > 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... -- (°= Leandro Dorileo //\ [email protected] - http://www.dorilex.net V_/ Software is a matter of freedom. -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
