[U-Boot] [PATCH v3 2/2] usb: Add CONFIG to fetch string descriptor

[Marek Vasut]

Dear Puneet Saxena,

> Hi Marek,
> 
> On Monday 05 March 2012 06:18 PM, Marek Vasut wrote:
> > Dear Puneet Saxena,
> > 
> >>> Hi Marek,
> >>> 
> >>> On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
> >>>> Hi!
> >>>> 
> >>>>> Hi Marek,
> >>>>> 
> >>>>> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
> >>>>>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor
> >>>>>>> length and then pass this length to fetch string descriptor.
> >>>>>>> 
> >>>>>>> Signed-off-by: Puneet Saxena<puneets at nvidia.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>> Changes for V2:
> >>>>>>>       - Change existing config by "CONFIG_USB_STRING_FETCH"
> >>>>>>> 
> >>>>>>> Changes for V3:
> >>>>>>>        - Removed extra new line
> >>>>>>>        - Explained "CONFIG_USB_STRING_FETCH" in top level README
> >>>>>>>     
> >>>>>>>     README                          |    4 ++++
> >>>>>>>     common/usb.c                    |    4 ++++
> >>>>>>>     include/configs/tegra2-common.h |    2 ++
> >>>>>>>     3 files changed, 10 insertions(+), 0 deletions(-)
> >>>>>>> 
> >>>>>>> diff --git a/README b/README
> >>>>>>> index 7adf7c7..c045a37 100644
> >>>>>>> --- a/README
> >>>>>>> +++ b/README
> >>>>>>> 
> >>>>>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
> >>>>>>>     		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> >>>>>>>     		txfilltuning field in the EHCI controller on reset.
> >>>>>>> 
> >>>>>>> +		CONFIG_USB_STRING_FETCH
> >>>>>>> +		Enables settings to USB core to handle string issues
> >> 
> >> which
> >> 
> >>>>>>> +		few devices can not handle.
> >>>>>>> +
> >>>>>>> 
> >>>>>>>     - USB Device:
> >>>>>>>     		Define the below if you wish to use the USB console.
> >>>>>>>     		Once firmware is rebuilt from a serial console issue the
> >>>>>>> 
> >>>>>>> diff --git a/common/usb.c b/common/usb.c
> >>>>>>> index 191bc5b..a73cb60 100644
> >>>>>>> --- a/common/usb.c
> >>>>>>> +++ b/common/usb.c
> >>>>>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device
> >>>>>>> *dev, unsigned int langid, {
> >>>>>>> 
> >>>>>>>     	int rc;
> >>>>>>> 
> >>>>>>> +#ifdef CONFIG_USB_STRING_FETCH
> >>>>>> 
> >>>>>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
> >>>>>> then?
> >>>>>> 
> >>>>>> Anyway, how come some devices can't handle it? What happens then?
> >>>>>> What devices are those (exact type etc)?
> >>>>>> 
> >>>>>> I believe the bug is deeper and adding extra config options can be
> >>>>>> avoided, what do you think?
> >>>>>> 
> >>>>>> Thanks!
> >>>>>> 
> >>>>>> M
> >>>>> 
> >>>>> It does not avoid string fetch.
> >>>> 
> >>>> Well it does certainly not call usb_get_string()
> >>> 
> >>> Precisely, it avoids fetching string desc of 255 bytes. so better name
> >>> could be
> >>> "CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.
> >>> 
> >>>>> I checked with few mass storage devices that they does not handle
> >>>>> string descriptor request correctly and so we get
> >>>>> start/stop Cache alignment error.
> >>>> 
> >>>> Cache alignment error ? Wow, how's that actually related to SUB string
> >>>> fetching? Maybe we should manually realign the result then? I still
> >>>> don't really understand what you're seeing, sorry ... can you please
> >>>> elaborate?
> >>> 
> >>> The particular mass storage device is -
> >>> 
> >>> Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
> >>> 
> >>> Type: Removable Hard Disk
> >>> 
> >>> Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
> >>> 
> >>> When code tries to read Manufacturing Id..prod id...etc..it passes
> >>> cache aligned buffer "tbuf" in
> >>> "usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
> >>> passes as default 255 bytes to fetch string
> >>> descriptor.
> >>> The code in "ehci_submit_async() " invalidates *partially* the passed
> >>> buffer though we pass aligned buffer and "STD_ASS"
> >>> is received. Though it should invalidate only the cached line which is
> >>> equal(~32)  to string desc length.
> >> 
> >> Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should
> >> be fixed to correctly handle caches then?
> >> 
> >>> If we give delay of 1 ms after handshake() the cache alignment warning
> >>> does not spew...but delay of 1 ms is not acceptable.
> >>> So I enabled the code to fetch first string desc length and then fetch
> >>> string desc fetch, in this way the issue is resolved.
> >>> It makes sense also to fetch string desc length and then actual string
> >>> desc....
> >> 
> >> I see, I  understand now just about everything but the ehci problem, see
> >> above. Your explanation was very helpful.
> >> 
> >> Let's work this out together!
> >> 
> >> Cheers!
> >> 
> >> M
> > 
> > Is this issue fixed or is this patch still needed? Thanks in advance!
> 
> This issue is not fixed and still need a patch to fix root cause,
> explained above.
> Note that this issue is observed even though we pass aligned address and
> expects something from the device.

I see, I finally understand the issue. Can I expect a patch from you for this 
issue then?

Thanks in advance!