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

[Troy Kisky]

On 9/20/2013 3:01 PM, Marek Vasut wrote:
> 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,
>
I suspect omap1510 will work with a value of 64 for the sizes, since it 
and mpc8xx
were added by Bryan O'Donoghue - 29 May 2006

I've added him to the CC.
Bryan, can you test changing

include/usb/omap1510_udc.h
#define UDC_INT_PACKET_SIZE     16
#define UDC_BULK_PACKET_SIZE 16
--------
or on a very old u-boot
include/usbdcore_omap1510.h
#define UDC_INT_PKTSIZE        16
#define UDC_BULK_PKTSIZE 16


from 16 to 64? Or maybe you remember the reason 16 was needed?
Also, do your remember anything about the MPC885 family and why
endpoint 0 could be 16 bytes, but bulk endpoints are limited to 8?

Thanks, I'd like my comments to be accurate...

Troy