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

[Ran Wang]

Hi Bin,

> -----Original Message-----
> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> Sent: Thursday, November 02, 2017 4:37 PM
> To: Ran Wang <ran.wang_1 at nxp.com>
> Cc: Marek Vasut <marex at denx.de>; 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 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.
> 
Well, this issue still happen if I only added delay before issue "Address Device" command once.
It can only be fixed when I add delay to most control transfer on root-hub enumeration.
My original patch was to add to file xHCI driver, but Marek suggest me move to common/.

> >>
> >> 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.
So far I don't have the equipment to catch transactions on USB bus for the root cause.
Do you know are there the SW way to confirm STALL happen?

Best regards
Ran
> 
> Regards,
> Bin