[U-Boot] [PATCH V4 06/17] usb: udc: add udc.h include file

[Marek Vasut]

Dear Troy Kisky,

> On 9/20/2013 2:20 PM, Marek Vasut wrote:
> > Dear Troy Kisky,
> > 
> >> On 9/20/2013 12:53 PM, Marek Vasut wrote:
> >>> Dear Troy Kisky,
> >>> 
> >>>> On 9/20/2013 11:52 AM, Marek Vasut wrote:
> >>>>> Dear Troy Kisky,
> >>>>> 
> >>>>>> On 9/20/2013 3:55 AM, Marek Vasut wrote:
> >>>>>>> Dear Troy Kisky,
> >>>>>>> 
> >>>>>>>> Move common definitions to udc.h
> >>>>>>>> This allows musb_udc.h to be removed as well.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Troy Kisky <troy.kisky at boundarydevices.com>
> >>>>>>>> 
> >>>>>>>> ---
> >>>>>>>> v4: updated commit message
> >>>>>>>> removed ifdef UDC_BULK_HS_PACKET_SIZE since 512
> >>>>>>>> is the only legal value, it shouldn't be overridden.
> >>>>>>> 
> >>>>>>> [...]
> >>>>>>> 
> >>>>>>>>      #endif
> >>>>>>>> 
> >>>>>>>> diff --git a/include/usb/udc.h b/include/usb/udc.h
> >>>>>>>> new file mode 100644
> >>>>>>>> index 0000000..3bcbbbc
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/include/usb/udc.h
> >>>>>>>> @@ -0,0 +1,61 @@
> >>>>>>>> +/*
> >>>>>>>> + * SPDX-License-Identifier:	GPL-2.0+
> >>>>>>>> + */
> >>>>>>>> +#ifndef USB_UDC_H
> >>>>>>>> +#define USB_UDC_H
> >>>>>>>> +
> >>>>>>>> +#ifndef EP0_MAX_PACKET_SIZE
> >>>>>>>> +#define EP0_MAX_PACKET_SIZE     64
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +#ifndef EP_MAX_PACKET_SIZE
> >>>>>>>> +#define EP_MAX_PACKET_SIZE	64
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +#ifndef UDC_OUT_PACKET_SIZE
> >>>>>>>> +#define UDC_OUT_PACKET_SIZE     EP_MAX_PACKET_SIZE
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +#ifndef UDC_IN_PACKET_SIZE
> >>>>>>>> +#define UDC_IN_PACKET_SIZE      EP_MAX_PACKET_SIZE
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +#ifndef UDC_INT_PACKET_SIZE
> >>>>>>>> +#define UDC_INT_PACKET_SIZE     EP_MAX_PACKET_SIZE
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +#ifndef UDC_BULK_PACKET_SIZE
> >>>>>>>> +#define UDC_BULK_PACKET_SIZE    EP_MAX_PACKET_SIZE
> >>>>>>>> +#endif
> >>>>>>> 
> >>>>>>> Do you expect these values to change on per-controller basis? Or
> >>>>>>> why do you have these ifndefs here ?
> >>>>>> 
> >>>>>> I don't know why they change but
> >>>>>> 
> >>>>>> include/usb/mpc8xx_udc.h:#define UDC_BULK_PACKET_SIZE
> >>>>>> EP_MIN_PACKET_SIZE    /* 8 */
> >>>>>> include/usb/omap1510_udc.h:#define UDC_BULK_PACKET_SIZE 16
> >>>>>> 
> >>>>>> include/usb/mpc8xx_udc.h:#define UDC_INT_PACKET_SIZE
> >>>>>> UDC_IN_PACKET_SIZE   /* 8 */
> >>>>>> include/usb/omap1510_udc.h:#define UDC_INT_PACKET_SIZE  16
> >>>>>> 
> >>>>>> include/usb/mpc8xx_udc.h:#define UDC_OUT_PACKET_SIZE
> >>>>>> EP_MIN_PACKET_SIZE    /* */
> >>>>> 
> >>>>> Are you sure this is not OHCI ?
> >>>>> 
> >>>>> Best regards,
> >>>>> Marek Vasut
> >>>> 
> >>>> I don't know.
> >>>> I don't understand the relevance of the question. Can you explain the
> >>>> issue a little more
> >>>> for me.
> >>> 
> >>> OMAP1510 has only OHCI controller in it, dunno about MPC8xx, but that
> >>> seems to be the case as well. Therefore, in OHCI case, the max packet
> >>> is 16 and in ehci it's 64 . Check the specs ;-)
> >>> 
> >>> Best regards,
> >>> Marek Vasut
> >> 
> >> Ok, I checked the spec for the OMAP1510, and found "Table 14–23.
> >> Endpoint n Size Values"
> >> lists maximum packet sizes of 8, 16, 32, or 64 bytes for Non-isochronous
> >> endpoints, and 8, 16, 32, 64, 128, 256, 512 for Isochronous endpoints
> >> 
> >> So, I still don't understand the limit of 16, but that isn't required.
> >> 
> >> 
> >> So, are you saying I should edit omap1510_udc.h and add
> >> 
> >> #define EP_MAX_PACKET_SIZE 16
> >> 
> >> 
> >> and remove
> >> #ifndef UDC_BULK_PACKET_SIZE
> >> 
> >> in udc.h and do
> >> 
> >> #define UDC_BULK_PACKET_SIZE    EP_MAX_PACKET_SIZE
> >> 
> >> unconditionally?
> > 
> > I'd say you should check if the controller is OHCI or EHCI , check what
> > kind of endpoint it is and based on that , configure the max packet
> > size. Or is this really controller specific ? What do the OHCI and EHCI
> > specs say ?
> > 
> > Best regards,
> > Marek Vasut
> 
> The omap1510 does have an OHCI host controller, I don't know how that
> relates to the device
> controller so I think a check would just be confusing.

I see the difference now. For EHCI UDC, the max packet sizes are as they are. 
For OMAP/MPC8xx, these are non-standard arbitrary values, it's just that the 
name is the same.

This is pretty ugly and confusing, ick. I'd say you #ifdef this stuff like this:
#ifdef OMAP1510 ... 
#elif MPC8xx
#elif ....
#endif

And put a comment around it that the old UDCs simply follow no standard.

Best regards,