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

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

[Serge Semin]

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@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

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(-)

-- 
2.27.0

[PATCH 1/3] usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion

[Serge Semin]

In accordance with [1] the DWC_usb3 core sets the GUSB2PHYACCn.VStsDone
bit when the PHY vendor control access is done and clears it when the
application initiates a new transaction. The doc doesn't say anything
about the GUSB2PHYACCn.VStsBsy flag serving for the same purpose. Moreover
we've discovered that the VStsBsy flag can be cleared before the VStsDone
bit. So using the former as a signal of the PHY control registers
completion might be dangerous. Let's have the VStsDone flag utilized
instead then.

[1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
    Databook, 2.70a, December 2013, p.388

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/usb/dwc3/core.h | 1 +
 drivers/usb/dwc3/ulpi.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2f04b3e42bf1..8d5e5bba1bc2 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -284,6 +284,7 @@
 
 /* Global USB2 PHY Vendor Control Register */
 #define DWC3_GUSB2PHYACC_NEWREGREQ	BIT(25)
+#define DWC3_GUSB2PHYACC_DONE		BIT(24)
 #define DWC3_GUSB2PHYACC_BUSY		BIT(23)
 #define DWC3_GUSB2PHYACC_WRITE		BIT(22)
 #define DWC3_GUSB2PHYACC_ADDR(n)	(n << 16)
diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index e6e6176386a4..20f5d9aba317 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -24,7 +24,7 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
 
 	while (count--) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
-		if (!(reg & DWC3_GUSB2PHYACC_BUSY))
+		if (reg & DWC3_GUSB2PHYACC_DONE)
 			return 0;
 		cpu_relax();
 	}
-- 
2.27.0

[PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one

[Serge Semin]

Originally the procedure of the ULPI transaction finish detection has been
developed as a simple busy-loop with just decrementing counter and no
delays. It's wrong since on different systems the loop will take a
different time to complete. So if the system bus and CPU are fast enough
to overtake the ULPI bus and the companion PHY reaction, then we'll get to
take a false timeout error. Fix this by converting the busy-loop procedure
to take the standard bus speed, address value and the registers access
mode into account for the busy-loop delay calculation.

Here is the way the fix works. It's known that the ULPI bus is clocked
with 60MHz signal. In accordance with [1] the ULPI bus protocol is created
so to spend 5 and 6 clock periods for immediate register write and read
operations respectively, and 6 and 7 clock periods - for the extended
register writes and reads. Based on that we can easily pre-calculate the
time which will be needed for the controller to perform a requested IO
operation. Note we'll still preserve the attempts counter in case if the
DWC USB3 controller has got some internals delays.

[1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
    October 20, 2004, pp. 30 - 36.

Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/usb/dwc3/ulpi.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index 20f5d9aba317..0dbc826355a5 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -7,6 +7,8 @@
  * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  */
 
+#include <linux/delay.h>
+#include <linux/time64.h>
 #include <linux/ulpi/regs.h>
 
 #include "core.h"
@@ -17,12 +19,22 @@
 		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
 		DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
 
-static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
+#define DWC3_ULPI_BASE_DELAY	DIV_ROUND_UP(NSEC_PER_SEC, 60000000L)
+
+static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
 {
+	unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
 	unsigned count = 1000;
 	u32 reg;
 
+	if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
+		ns += DWC3_ULPI_BASE_DELAY;
+
+	if (read)
+		ns += DWC3_ULPI_BASE_DELAY;
+
 	while (count--) {
+		ndelay(ns);
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
 		if (reg & DWC3_GUSB2PHYACC_DONE)
 			return 0;
@@ -47,7 +59,7 @@ static int dwc3_ulpi_read(struct device *dev, u8 addr)
 	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
 
-	ret = dwc3_ulpi_busyloop(dwc);
+	ret = dwc3_ulpi_busyloop(dwc, addr, true);
 	if (ret)
 		return ret;
 
@@ -71,7 +83,7 @@ static int dwc3_ulpi_write(struct device *dev, u8 addr, u8 val)
 	reg |= DWC3_GUSB2PHYACC_WRITE | val;
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
 
-	return dwc3_ulpi_busyloop(dwc);
+	return dwc3_ulpi_busyloop(dwc, addr, false);
 }
 
 static const struct ulpi_ops dwc3_ulpi_ops = {
-- 
2.27.0

[PATCH 3/3] usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression

[Serge Semin]

First of all the commit e0082698b689 ("usb: dwc3: ulpi: conditionally
resume ULPI PHY") introduced the Suspend USB2.0 HS/FS/LS PHY regression,
as by design of the fix any attempt to read/write from/to the PHY control
registers will completely disable the PHY suspension, which consequently
will increase the USB bus power consumption. Secondly the fix won't work
well for the very first attempt of the ULPI PHY control registers IO,
because after disabling the USB2.0 PHY suspension functionality it will
still take some time for the bus to resume from the sleep state if one has
been reached before it. So the very first PHY register read/write
operation will take more time than the busy-loop provides and the IO
timeout error might be returned anyway.

Here we suggest to fix the denoted problems in the following way. First of
all let's not disable the Suspend USB2.0 HS/FS/LS PHY functionality so to
make the controller and the USB2.0 bus more power efficient. Secondly
instead of that we'll extend the PHY IO op wait procedure with 1 - 1.2 ms
sleep if the PHY suspension is enabled (1ms should be enough as by LPM
specification it is at most how long it takes for the USB2.0 bus to resume
from L1 (Sleep) state). Finally in case if the USB2.0 PHY suspension
functionality has been disabled on the DWC USB3 controller setup procedure
we'll compensate the USB bus resume process latency by extending the
busy-loop attempts counter.

Fixes: e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/usb/dwc3/ulpi.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index 0dbc826355a5..f23f4c9a557e 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -24,7 +24,7 @@
 static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
 {
 	unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
-	unsigned count = 1000;
+	unsigned int count = 10000;
 	u32 reg;
 
 	if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
@@ -33,6 +33,10 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
 	if (read)
 		ns += DWC3_ULPI_BASE_DELAY;
 
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+	if (reg & DWC3_GUSB2PHYCFG_SUSPHY)
+		usleep_range(1000, 1200);
+
 	while (count--) {
 		ndelay(ns);
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
@@ -50,12 +54,6 @@ static int dwc3_ulpi_read(struct device *dev, u8 addr)
 	u32 reg;
 	int ret;
 
-	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-	if (reg & DWC3_GUSB2PHYCFG_SUSPHY) {
-		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
-	}
-
 	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
 
@@ -73,12 +71,6 @@ static int dwc3_ulpi_write(struct device *dev, u8 addr, u8 val)
 	struct dwc3 *dwc = dev_get_drvdata(dev);
 	u32 reg;
 
-	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-	if (reg & DWC3_GUSB2PHYCFG_SUSPHY) {
-		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
-	}
-
 	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
 	reg |= DWC3_GUSB2PHYACC_WRITE | val;
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
-- 
2.27.0

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@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> 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@linux.intel.com>

thanks,

-- 
heikki

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

Serge Semin <Sergey.Semin@baikalelectronics.ru> 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@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

these should go in the relevant commits instead.

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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 1/3] usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]


Hi,

Serge Semin <Sergey.Semin@baikalelectronics.ru> writes:

> In accordance with [1] the DWC_usb3 core sets the GUSB2PHYACCn.VStsDone
> bit when the PHY vendor control access is done and clears it when the
> application initiates a new transaction. The doc doesn't say anything
> about the GUSB2PHYACCn.VStsBsy flag serving for the same purpose. Moreover
> we've discovered that the VStsBsy flag can be cleared before the VStsDone
> bit. So using the former as a signal of the PHY control registers
> completion might be dangerous. Let's have the VStsDone flag utilized
> instead then.
>
> [1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
>     Databook, 2.70a, December 2013, p.388
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/usb/dwc3/core.h | 1 +
>  drivers/usb/dwc3/ulpi.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f04b3e42bf1..8d5e5bba1bc2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -284,6 +284,7 @@
>  
>  /* Global USB2 PHY Vendor Control Register */
>  #define DWC3_GUSB2PHYACC_NEWREGREQ	BIT(25)
> +#define DWC3_GUSB2PHYACC_DONE		BIT(24)
>  #define DWC3_GUSB2PHYACC_BUSY		BIT(23)
>  #define DWC3_GUSB2PHYACC_WRITE		BIT(22)
>  #define DWC3_GUSB2PHYACC_ADDR(n)	(n << 16)
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index e6e6176386a4..20f5d9aba317 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -24,7 +24,7 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
>  
>  	while (count--) {
>  		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> -		if (!(reg & DWC3_GUSB2PHYACC_BUSY))
> +		if (reg & DWC3_GUSB2PHYACC_DONE)

are you sure this works in all supported versions of the core?

John, could you confirm this for us?

thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]


Hi,

Serge Semin <Sergey.Semin@baikalelectronics.ru> writes:

> Originally the procedure of the ULPI transaction finish detection has been
> developed as a simple busy-loop with just decrementing counter and no
> delays. It's wrong since on different systems the loop will take a
> different time to complete. So if the system bus and CPU are fast enough
> to overtake the ULPI bus and the companion PHY reaction, then we'll get to
> take a false timeout error. Fix this by converting the busy-loop procedure
> to take the standard bus speed, address value and the registers access
> mode into account for the busy-loop delay calculation.
>
> Here is the way the fix works. It's known that the ULPI bus is clocked
> with 60MHz signal. In accordance with [1] the ULPI bus protocol is created
> so to spend 5 and 6 clock periods for immediate register write and read
> operations respectively, and 6 and 7 clock periods - for the extended
> register writes and reads. Based on that we can easily pre-calculate the
> time which will be needed for the controller to perform a requested IO
> operation. Note we'll still preserve the attempts counter in case if the
> DWC USB3 controller has got some internals delays.
>
> [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
>     October 20, 2004, pp. 30 - 36.
>
> Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/usb/dwc3/ulpi.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index 20f5d9aba317..0dbc826355a5 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -7,6 +7,8 @@
>   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>   */
>  
> +#include <linux/delay.h>
> +#include <linux/time64.h>
>  #include <linux/ulpi/regs.h>
>  
>  #include "core.h"
> @@ -17,12 +19,22 @@
>  		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
>  		DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
>  
> -static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> +#define DWC3_ULPI_BASE_DELAY	DIV_ROUND_UP(NSEC_PER_SEC, 60000000L)
> +
> +static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
>  {
> +	unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
>  	unsigned count = 1000;
>  	u32 reg;
>  
> +	if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
> +		ns += DWC3_ULPI_BASE_DELAY;
> +
> +	if (read)
> +		ns += DWC3_ULPI_BASE_DELAY;
> +
>  	while (count--) {
> +		ndelay(ns);

could we allow for a sleep here instead of a delay? Also, I wonder if
you need to make this so complex or should we just take the larger
access time of 7 clock cycles.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

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@baikalelectronics.ru> 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@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > Cc: linux-usb@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> 

> 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

Re: [PATCH 1/3] usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion

[Serge Semin]

On Tue, Oct 27, 2020 at 11:15:24AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Serge Semin <Sergey.Semin@baikalelectronics.ru> writes:
> 
> > In accordance with [1] the DWC_usb3 core sets the GUSB2PHYACCn.VStsDone
> > bit when the PHY vendor control access is done and clears it when the
> > application initiates a new transaction. The doc doesn't say anything
> > about the GUSB2PHYACCn.VStsBsy flag serving for the same purpose. Moreover
> > we've discovered that the VStsBsy flag can be cleared before the VStsDone
> > bit. So using the former as a signal of the PHY control registers
> > completion might be dangerous. Let's have the VStsDone flag utilized
> > instead then.
> >
> > [1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
> >     Databook, 2.70a, December 2013, p.388
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/usb/dwc3/core.h | 1 +
> >  drivers/usb/dwc3/ulpi.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 2f04b3e42bf1..8d5e5bba1bc2 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -284,6 +284,7 @@
> >  
> >  /* Global USB2 PHY Vendor Control Register */
> >  #define DWC3_GUSB2PHYACC_NEWREGREQ	BIT(25)
> > +#define DWC3_GUSB2PHYACC_DONE		BIT(24)
> >  #define DWC3_GUSB2PHYACC_BUSY		BIT(23)
> >  #define DWC3_GUSB2PHYACC_WRITE		BIT(22)
> >  #define DWC3_GUSB2PHYACC_ADDR(n)	(n << 16)
> > diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> > index e6e6176386a4..20f5d9aba317 100644
> > --- a/drivers/usb/dwc3/ulpi.c
> > +++ b/drivers/usb/dwc3/ulpi.c
> > @@ -24,7 +24,7 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> >  
> >  	while (count--) {
> >  		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> > -		if (!(reg & DWC3_GUSB2PHYACC_BUSY))
> > +		if (reg & DWC3_GUSB2PHYACC_DONE)
> 

> are you sure this works in all supported versions of the core?

I can't be sure about that since I've got only the 2.70a version of the
core. But as I said in the patch log it was a bit incorrect to use the
BUSY flag here in the first place. So if there is no IP core peculiarity
here which had been workarounded by polling the BUSY-flag, then I'd stick
with the DONE-flag in the busy-loop. In the former case I'd suggest to add
a useful comment why the BUSY-flag is required to be polled though...

-Sergey

> 
> John, could you confirm this for us?
> 
> thanks
> 
> -- 
> balbi

Re: [PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one

[Serge Semin]

On Tue, Oct 27, 2020 at 11:18:51AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Serge Semin <Sergey.Semin@baikalelectronics.ru> writes:
> 
> > Originally the procedure of the ULPI transaction finish detection has been
> > developed as a simple busy-loop with just decrementing counter and no
> > delays. It's wrong since on different systems the loop will take a
> > different time to complete. So if the system bus and CPU are fast enough
> > to overtake the ULPI bus and the companion PHY reaction, then we'll get to
> > take a false timeout error. Fix this by converting the busy-loop procedure
> > to take the standard bus speed, address value and the registers access
> > mode into account for the busy-loop delay calculation.
> >
> > Here is the way the fix works. It's known that the ULPI bus is clocked
> > with 60MHz signal. In accordance with [1] the ULPI bus protocol is created
> > so to spend 5 and 6 clock periods for immediate register write and read
> > operations respectively, and 6 and 7 clock periods - for the extended
> > register writes and reads. Based on that we can easily pre-calculate the
> > time which will be needed for the controller to perform a requested IO
> > operation. Note we'll still preserve the attempts counter in case if the
> > DWC USB3 controller has got some internals delays.
> >
> > [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
> >     October 20, 2004, pp. 30 - 36.
> >
> > Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/usb/dwc3/ulpi.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> > index 20f5d9aba317..0dbc826355a5 100644
> > --- a/drivers/usb/dwc3/ulpi.c
> > +++ b/drivers/usb/dwc3/ulpi.c
> > @@ -7,6 +7,8 @@
> >   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >   */
> >  
> > +#include <linux/delay.h>
> > +#include <linux/time64.h>
> >  #include <linux/ulpi/regs.h>
> >  
> >  #include "core.h"
> > @@ -17,12 +19,22 @@
> >  		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
> >  		DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
> >  
> > -static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> > +#define DWC3_ULPI_BASE_DELAY	DIV_ROUND_UP(NSEC_PER_SEC, 60000000L)
> > +
> > +static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
> >  {
> > +	unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
> >  	unsigned count = 1000;
> >  	u32 reg;
> >  
> > +	if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
> > +		ns += DWC3_ULPI_BASE_DELAY;
> > +
> > +	if (read)
> > +		ns += DWC3_ULPI_BASE_DELAY;
> > +
> >  	while (count--) {
> > +		ndelay(ns);
> 

> could we allow for a sleep here instead of a delay?

The kernel ULPI-bus API isn't clear about that. I also couldn't find an example
of using the ULPI-bus accessors in the atomic context or being implemented with
the sleeping methods. So there is no certain answer to your question. Anyway I
added an ms-sleep in the later patch to fix the suspend-regression problem. I
thought it was reasonable since I couldn't find an example of using the
accessors in the atomic context.

Regarding this patch. I wouldn't suggest to replace the ndelay with sleeping
here, since 5-7 ref clock ticks is enough to finish the transaction for the vast
majority of the cases. It's just 80 - 115 ns, which can't be reached by the
sleeping procedures.

> Also, I wonder if
> you need to make this so complex or should we just take the larger
> access time of 7 clock cycles.

I wouldn't say it's complex. Here I've implemented a simple calculation of the
time needed to finish the ULPI-bus commands in accordance with the number of
ticks they normally require. Regarding the while-looping alas we can't get rid
of it here for the reason I've described in the patch 3 of the series.

-Sergey

> 
> -- 
> balbi