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

Marek Vasut
2012-03-01T13:13:04Z

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