2020-10-11 17:56:43

by Serge Semin

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

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 <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: [email protected]
Cc: [email protected]

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


2020-10-11 17:57:18

by Serge Semin

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

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 <[email protected]>
---
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 <[email protected]>
*/

+#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

2020-10-19 14:01:00

by Heikki Krogerus

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

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 <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Pavel Parkhomenko <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
>
> 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 <[email protected]>

thanks,

--
heikki

2020-10-27 14:25:42

by Felipe Balbi

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


Hi,

Serge Semin <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> */
>
> +#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


Attachments:
signature.asc (873.00 B)

2020-10-28 06:09:48

by Felipe Balbi

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

Serge Semin <[email protected]> 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 <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Pavel Parkhomenko <[email protected]>
> Cc: [email protected]
> Cc: [email protected]

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


Attachments:
signature.asc (873.00 B)

2020-10-28 21:23:59

by Serge Semin

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

On Tue, Oct 27, 2020 at 11:13:14AM +0200, Felipe Balbi wrote:
> Serge Semin <[email protected]> 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 <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Pavel Parkhomenko <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
>

> 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


2020-10-28 21:38:31

by Serge Semin

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

On Tue, Oct 27, 2020 at 11:18:51AM +0200, Felipe Balbi wrote:
>
> Hi,
>
> Serge Semin <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > */
> >
> > +#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