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

[Marek Vasut]

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!

Best regards,
Marek Vasut