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

[Heikki Krogerus]

On Sun, Oct 11, 2020 at 01:23:48AM +0300, Serge Semin wrote:
> 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
> 
> 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
> 
>  drivers/usb/dwc3/core.h |  1 +
>  drivers/usb/dwc3/ulpi.c | 38 +++++++++++++++++++++-----------------
>  2 files changed, 22 insertions(+), 17 deletions(-)

FWIW, for the whole series:

Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

thanks,

-- 
heikki