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

Marek Vasut
2013-09-20T22:01:54Z

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@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,