[U-Boot] About the way to fix platform specific issue in source file xhci.c (U-Boot)

[Bin Meng]

Hi Ran,

On Thu, Nov 2, 2017 at 10:29 AM, Ran Wang <ran.wang_1 at nxp.com> wrote:
> Hi Bin,
>
>> -----Original Message-----
>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
>> Sent: Wednesday, November 01, 2017 6:08 PM
>> To: Marek Vasut <marex at denx.de>
>> Cc: Ran Wang <ran.wang_1 at nxp.com>; Marek Vasut
>> <marek.vasut at gmail.com>; open list <u-boot at lists.denx.de>
>> Subject: Re: [U-Boot] About the way to fix platform specific issue in source
>> file xhci.c (U-Boot)
>>
>> Hi Ran,
>>
>> On Wed, Nov 1, 2017 at 4:19 PM, Marek Vasut <marex at denx.de> wrote:
>> > On 11/01/2017 07:22 AM, Ran Wang wrote:
>> >> Hi Marek,
>> >>
>> >>> -----Original Message-----
>> >>> From: Ran Wang
>> >>> Sent: Wednesday, November 01, 2017 9:05 AM
>> >>> To: Marek Vasut <marex at denx.de>; Marek Vasut
>> <marek.vasut at gmail.com>
>> >>> Cc: open list <u-boot at lists.denx.de>
>> >>> Subject: RE: [U-Boot] About the way to fix platform specific issue
>> >>> in source file xhci.c (U-Boot)
>> >>>
>> >>> Hi Marek,
>> >>>> -----Original Message-----
>> >>>> From: Marek Vasut [mailto:marex at denx.de]
>> >>>> Sent: Tuesday, October 31, 2017 6:24 PM
>> >>>> To: Ran Wang <ran.wang_1 at nxp.com>; Marek Vasut
>> >>> <marek.vasut at gmail.com>
>> >>>> Cc: open list <u-boot at lists.denx.de>; Bin Meng <bmeng.cn at gmail.com>
>> >>>> Subject: Re: [U-Boot] About the way to fix platform specific issue
>> >>>> in source file xhci.c (U-Boot)
>> >>>>
>> >>>> On 10/31/2017 10:43 AM, Ran Wang wrote:
>> >>>>> Hi
>> >>>>>> -----Original Message-----
>> >>>>>> From: Marek Vasut [mailto:marek.vasut at gmail.com]
>> >>>>>> Sent: Tuesday, October 31, 2017 5:31 PM
>> >>>>>> To: Ran Wang <ran.wang_1 at nxp.com>; Marek Vasut
>> <marex at denx.de>
>> >>>>>> Cc: open list <u-boot at lists.denx.de>
>> >>>>>> Subject: Re: [U-Boot] About the way to fix platform specific
>> >>>>>> issue in source file xhci.c (U-Boot)
>> >>>>>>
>> >>>>>> On 10/31/2017 10:15 AM, Ran Wang wrote:
>> >>>>>>> Hi Marek,
>> >>>>>>
>> >>>>>> Hi!
>> >>>>>>
>> >>>>>>>> -----Original Message-----
>> >>>>>>>> From: Marek Vasut [mailto:marex at denx.de]
>> >>>>>>>> Sent: Monday, October 30, 2017 6:55 PM
>> >>>>>>>> To: Ran Wang <ran.wang_1 at nxp.com>
>> >>>>>>>> Cc: bmeng.cn at gmail.com
>> >>>>>>>> Subject: Re: About the way to fix platform specific issue in
>> >>>>>>>> source file xhci.c
>> >>>>>>>> (U-Boot)
>> >>>>>>>>
>> >>>>>>>> On 10/30/2017 09:39 AM, Ran Wang wrote:
>> >>>>>>>>> Hi Vasut,
>> >>>>>>>>>    For git://git.denx.de/u-boot-usb.git, I work out a patch to
>> >>>>>>>>> fix USB issue
>> >>>>>>>> which will happen on SoC LS2080A only (it's using DWC3).
>> >>>>>>>>>    Per my understanding, we should not use platform define in
>> >>>>>>>>> xhci.c to
>> >>>>>>>> control its effect. However, I am not sure how to do it that
>> >>>>>>>> can be accepted by upstream, so send you this mail for
>> >>>>>>>> suggestion before I post the patch to patchwork. Thank you in
>> advance.
>> >>>>>>>>
>> >>>>>>>> This should be fixed in common code, not in drivers.
>> >>>>>>>
>> >>>>>>> Did you mean it should be fixed in common/usb*.c rather than
>> >>>>>> drivers/usb/*?
>> >>>>>>
>> >>>>>> Yes
>> >>>>>>
>> >>>>>>> If yes, is it acceptable that I use 'if
>> >>>>>>> defined(CONFIG_ARCH_LS2080A)' in
>> >>>>>> common/usb.c?
>> >>>>>>
>> >>>>>> No
>> >>>>>>
>> >>>>>>> If answer is no, how should I do? I cannot find an example and
>> >>>>>>> not sure it's OK to related rule.
>> >>>>>>
>> >>>>>> What is the problem exactly ?
>> >>>>>> I recall there were reports of shitty USB sticks failing, but
>> >>>>>> without further details, it's hard to tell if this is the same problem.
>> >>>>>>
>> >>>>> We observed some USB2.0 drives (Transcend 8GB, 4GB, Samtec) fail
>> >>>>> to be enumerated by U-Boot, and if we try to add some time
>> >>>>> interval between control transfers (not in bulk transfers), issue get
>> resolved.
>> >>>>
>> >>>> Try disabling cache support, does it still happen ?
>> >>>> I had such an issue with DWC2 controller and it failed due to
>> >>>> incorrect cache handling.
>> >>>>
>> >>> Could you pls tell me where to disable it on DWC3? May sure I do the
>> >>> right change.
>> >>
>> >> I try to remove function dcache_enable() calling, failure still
>> >> happen. And I think cache issue might not be able to bypass if I only add
>> delay in control message sends.
>> >>
>> >> More debugging shows that xHC will generate TRB event with complete
>> >> code of TX_ERR for Set
>>
>> Do you mean xHC "Address Device" command?
>
> Yes
>>
>> >> Address command TRB execution. Then U-Boot pop "USB device not
>> >> accepting new address (error=80000000)".
>> >>
>>
>> What is the completion codes for the TX_ERR you were seeing?
>
> It's 0x04: USB Transaction Error
>>

OK, I see.

>> >> Anyway, now I work out a patch fix in common/usb.c as below, is it
>> acceptable?
>> >> diff --git a/common/usb.c b/common/usb.c index
>> 0904259757..26f2e89ba3
>> >> 100644
>> >> --- a/common/usb.c
>> >> +++ b/common/usb.c
>> >> @@ -223,6 +223,8 @@ int usb_control_msg(struct usb_device *dev,
>> unsigned int pipe,
>> >>               return -EINVAL;
>> >>       }
>> >>
>> >> +     mdelay(10);
>> >> +

So even if we need add a workaround, I suggest it should be added to
xHCI driver where it issues the "Address Device" command, not here.

>>
>> This to me is a workaround. The root cause is still under water.
>
> Yes, for now I have to do the remote debug for ENV limitation and don't have
> USB protocol analyzer to catch the trace log. Actually I occasionally found this workaround
> When opening verbose debug log of USB.
>
> So my plan is to submit this simple workaround firstly to cover more shitty
> devices and then submit another one in the future when I found the root
> cause and confirm it can be fixed by SW.
>

Yes, can you please check why xHC reports transaction error? I suspect
your USB device respond with a STALL.

Regards,
Bin