Re: [PATCH 0/3] usb: dwc3: ulpi: Fix UPLI registers read/write ops

[Serge Semin]

On Tue, Oct 27, 2020 at 11:13:14AM +0200, Felipe Balbi wrote:
> Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> writes:
> 
> > Our Baikal-T1 SoC is equipped with DWC USB3 IP core as a USB2.0 bus
> > controller. In general the DWC USB3 driver is working well for it except
> > the ULPI-bus part. We've found out that the DWC USB3 ULPI-bus driver detected
> > PHY with VID:PID tuple as 0x0000:0x0000, which of course wasn't true since
> > it was supposed to be 0x0424:0x0006. After a short digging inside the
> > ulpi.c code and studying the DWC USB3 documentation, it has been
> > discovered that the ULPI bus IO ops didn't work quite correct. The
> > busy-loop had stopped waiting before the actual operation was finished. We
> > found out that the problem was caused by several bugs hidden in the DWC
> > USB3 ULPI-bus IO implementation.
> >
> > First of all in accordance with the DWC USB3 databook [1] the ULPI IO
> > busy-loop is supposed to use the GUSB2PHYACCn.VStsDone flag as an
> > indication of the PHY vendor control access completion. Instead it polled
> > the GUSB2PHYACCn.VStsBsy flag, which as we discovered can be cleared a
> > bit before the VStsDone flag.
> >
> > Secondly having the simple counter-based loop in the modern kernel is
> > really a weak design of the busy-looping pattern especially seeing the
> > ULPI operations delay can be easily estimated [2], since the bus clock is
> > fixed to 60MHz.
> >
> > Finally the root cause of the denoted in the prologue problem was due to
> > the Suspend PHY DWC USB3 feature perception. The commit e0082698b689
> > ("usb: dwc3: ulpi: conditionally resume ULPI PHY") introduced the Suspend
> > USB2.0 HS/FS/LS PHY regression as the Low-power consumption mode would be
> > disable after a first attempt to read/write from the ULPI PHY control
> > registers, and still didn't fix the problem it was originally intended for
> > since the very first attempt of the ULPI PHY control registers IO would
> > need much more time than the busy-loop provided. So instead of disabling
> > the Suspend USB2.0 HS/FS/LS PHY feature we suggest to just extend the
> > busy-loop delay in case if the GUSB2PHYCFGn.SusPHY flag set to 1. By doing
> > so we'll eliminate the regression and the fix the false busy-loop timeout
> > problem.
> >
> > [1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
> >     Databook, 2.70a, December 2013, p.388
> >
> > [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
> >     October 20, 2004, pp. 30 - 36.
> >
> > Fixes: e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> > Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@xxxxxxxxxxxxxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> 

> these should go in the relevant commits instead.

Could you elaborate what do you mean by "these"? If you meant the "Fixes" tag,
then it's already there except the very first patch. Which I think could be also
tagged with one.

> 
> > Serge Semin (3):
> >   usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion
> >   usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one
> >   usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression
> 

> make sure fixes don't depend on other rework otherwise they can't be
> taken during the -rc cycle.

Logically they don't, but still the later patches won't apply cleanly without
the former ones. So I can add the "Fixes" tag to the very first patch (it would
be correct since basically it's also a fix) so all of them would be ported to
the -rc and stable kernels. Is that ok?

-Sergey

> 
> -- 
> balbi