[RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Implement workaround for Octeon Known Issue Id 29206:
| The USB high speed logic contains a PLL that must lock during
| initialization for correct operation. In very rare circumstances,
| it is possible for the PLL to fail to start correctly.
| Workaround
| After initialization, check the USB PLL lock register via the
| UPHY CR interface. If the PLL is not running, power it down and
| back up and restart the initialization.

PLL initialization code taken from Cavium's vendor bootloader:
u-boot/drivers/usb/host/xhci-octeon.c:octeon3_usb_clocks_start

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 NOTE:
 This patch fixes initialization issue found on some CN7020 based boards.
 Without this patch, controller sometimes fails to detect devices connected.
 Original code comes from Cavium released u-boot monster patch, which seems
 to suffer from mistakes made while resolving merge conflicts when upgrading
 to newer u-boot.
 Testing revealed that only single reinit is needed to properly lock PLL,
 this agrees with comment in Cavium's u-boot code, which is claiming the
 same. However, same as in u-boot code, reinit is attempted three times.
 (in could be done using while loop instead of goto, just let me know
 which way do you prefer)
 SoCs suffering from this problem would fail to initialize PHY about
 several tens times of thousand boots. This patch always restored
 functional state.

 drivers/usb/dwc3/dwc3-octeon.c | 285 ++++++++++++++++++++++++++++++++-
 1 file changed, 284 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index d45d62c72b2d..497c18e19c73 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -122,8 +122,32 @@
 #define USBDRD_UCTL_INTSTAT			0x30
 #define USBDRD_UCTL_PORT_CFG_HS(port)		(0x40 + (0x20 * port))
 #define USBDRD_UCTL_PORT_CFG_SS(port)		(0x48 + (0x20 * port))
+
+/*
+ * UCTL Port Debug Configuration Registers
+ */
 #define USBDRD_UCTL_PORT_CR_DBG_CFG(port)	(0x50 + (0x20 * port))
+/* Rising edge triggers a register write operation of the captured
+ * address with the captured data
+ */
+# define USBDRD_UCTL_PORT_CR_DBG_CFG_WRITE	BIT_ULL(0)
+/* Rising edge triggers a register read operation of the capture address */
+# define USBDRD_UCTL_PORT_CR_DBG_CFG_READ	BIT_ULL(1)
+/* Rising edge triggers the [DATA_IN] field to be captured as the write data */
+# define USBDRD_UCTL_PORT_CR_DBG_CFG_CAP_DATA	BIT_ULL(2)
+/* Rising edge triggers the [DATA_IN] field to be captured as the address */
+# define USBDRD_UCTL_PORT_CR_DBG_CFG_CAP_ADDR	BIT_ULL(3)
+/* Address or data to be written to the CR interface */
+# define USBDRD_UCTL_PORT_CR_DBG_CFG_DATA_IN	GENMASK_ULL(47, 32)
+
+/*
+ * UCTL Port Debug Status Registers
+ */
 #define USBDRD_UCTL_PORT_CR_DBG_STATUS(port)	(0x58 + (0x20 * port))
+/* Acknowledge that the CAP_ADDR, CAP_DATA, READ, WRITE commands have completed */
+# define USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK	BIT_ULL(0)
+/* Last data read from the CR interface */
+# define USBDRD_UCTL_PORT_CR_DBG_STATUS_DATA_OUT GENMASK_ULL(47, 32)
 
 /*
  * UCTL Configuration Register
@@ -410,6 +434,252 @@ static void dwc3_octeon_phy_reset(struct dwc3_octeon *octeon)
 	dwc3_octeon_writeq(uctl_ctl_reg, val);
 }
 
+/* Internal indirect register that reports if the phy PLL has lock.
+ * This will be 1 if lock, 0 if no lock.
+ */
+#define DWC3_INT_IND_PLL_LOCK_REG			0x200b
+
+/* Internal indirect UPHY register that controls the power to the UPHY PLL. */
+#define DWC3_INT_IND_UPHY_PLL_PU			0x2012
+/* Write enable bit for DWC3_INT_IND_PLL_POWER_CTL */
+# define DWC3_INT_IND_UPHY_PLL_PU_WE			BIT(5)
+/* Power enable bit for DWC3_INT_IND_PLL_POWER_CTL */
+# define DWC3_INT_IND_UPHY_PLL_PU_POWER_EN		BIT(2)
+
+/* Internal indirect UPHY PLL register */
+#define DWC3_INT_IND_UPHY_PLL_RESET			0x201C
+/* Write enable bit */
+# define DWC3_INT_IND_UPHY_PLL_RESET_WE			BIT(4)
+/* VCO reset bit */
+# define DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST		BIT(0)
+
+static int dwc3_octeon_indirect_read(struct dwc3_octeon *octeon, u32 addr)
+{
+	int ret;
+	u64 val;
+	unsigned long timeout;
+	struct device *dev = octeon->dev;
+	void __iomem *cfg_reg = octeon->base + USBDRD_UCTL_PORT_CR_DBG_CFG(0);
+	void __iomem *status_reg = octeon->base + USBDRD_UCTL_PORT_CR_DBG_STATUS(0);
+
+	/* 1 */
+	val = FIELD_PREP(USBDRD_UCTL_PORT_CR_DBG_CFG_DATA_IN, addr);
+	dwc3_octeon_writeq(cfg_reg, val);
+	/* 2 */
+	val |= USBDRD_UCTL_PORT_CR_DBG_CFG_CAP_ADDR;
+	dwc3_octeon_writeq(cfg_reg, val);
+	/* 3 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (!(dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK)) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "set read address timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 4 */
+	dwc3_octeon_writeq(cfg_reg, 0);
+	/* 5 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "read ack address clear timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 6 */
+	dwc3_octeon_writeq(cfg_reg, USBDRD_UCTL_PORT_CR_DBG_CFG_READ);
+	/* 7 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (!((val = dwc3_octeon_readq(status_reg)) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK)) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "read data timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 8 */
+	ret = FIELD_GET(USBDRD_UCTL_PORT_CR_DBG_STATUS_DATA_OUT, val);
+	/* 9 */
+	dwc3_octeon_writeq(cfg_reg, 0);
+	/* 10 */
+	while (dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "read ack data clear timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+
+	return ret;
+}
+
+static int dwc3_octeon_indirect_write(struct dwc3_octeon *octeon, u32 addr, u16 value)
+{
+	u64 val;
+	unsigned long timeout;
+	struct device *dev = octeon->dev;
+	void __iomem *cfg_reg = octeon->base + USBDRD_UCTL_PORT_CR_DBG_CFG(0);
+	void __iomem *status_reg = octeon->base + USBDRD_UCTL_PORT_CR_DBG_STATUS(0);
+
+	/* 1 */
+	val = FIELD_PREP(USBDRD_UCTL_PORT_CR_DBG_CFG_DATA_IN, addr);
+	dwc3_octeon_writeq(cfg_reg, val);
+	/* 2 */
+	val |= USBDRD_UCTL_PORT_CR_DBG_CFG_CAP_ADDR;
+	dwc3_octeon_writeq(cfg_reg, val);
+	/* 3 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (!(dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK)) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "set write address timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 4 */
+	dwc3_octeon_writeq(cfg_reg, 0);
+	/* 5 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "write ack address clear timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 6 */
+	val = FIELD_PREP(USBDRD_UCTL_PORT_CR_DBG_CFG_DATA_IN, value);
+	dwc3_octeon_writeq(cfg_reg, FIELD_PREP(USBDRD_UCTL_PORT_CR_DBG_CFG_DATA_IN, value));
+	/* 7 */
+	val |= USBDRD_UCTL_PORT_CR_DBG_CFG_CAP_DATA;
+	dwc3_octeon_writeq(cfg_reg, val);
+	/* 8 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (!(dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK)) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "write set data timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 9 */
+	dwc3_octeon_writeq(cfg_reg, 0);
+	/* 10 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "write ack clear timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 11 */
+	dwc3_octeon_writeq(cfg_reg, USBDRD_UCTL_PORT_CR_DBG_CFG_WRITE);
+	/* 12 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (!(dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK)) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "write data timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+	/* 13 */
+	dwc3_octeon_writeq(cfg_reg, 0);
+	/* 14 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (dwc3_octeon_readq(status_reg) & USBDRD_UCTL_PORT_CR_DBG_STATUS_ACK) {
+                if (time_after(jiffies, timeout)) {
+			dev_warn(dev, "write ack clear timeout (%x)\n", addr);
+                        return -ETIMEDOUT;
+		}
+                cpu_relax();
+        }
+
+	return 0;
+}
+
+static int dwc3_octeon_pll_locked(struct dwc3_octeon *octeon)
+{
+	int ret = dwc3_octeon_indirect_read(octeon, DWC3_INT_IND_PLL_LOCK_REG);
+
+	if (ret < 0)
+		return ret;
+	return ret & 1;
+}
+
+/**
+ * Performs a full reset of the UPHY PLL. Note that this is normally done
+ * internally by a state machine when the UPHY is brought out of reset but this
+ * version gives far more time for things to settle before continuing.
+ */
+static int dwc3_uphy_pll_reset(struct dwc3_octeon *octeon)
+{
+	u16 ctrl, pwr;
+
+	/* 1. Turn on write enable so we can assert reset to the PLL VCO */
+	ctrl = dwc3_octeon_indirect_read(octeon, DWC3_INT_IND_UPHY_PLL_RESET);
+	ctrl |= DWC3_INT_IND_UPHY_PLL_RESET_WE;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+
+	/* 2. Turn on write enable for PLL power control */
+	pwr = dwc3_octeon_indirect_read(octeon, DWC3_INT_IND_UPHY_PLL_PU);
+	pwr |= DWC3_INT_IND_UPHY_PLL_PU_WE;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+
+	/* 3. Assert VCO reset */
+	ctrl |= DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+
+	/* 4. Power off the PLL */
+	pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+	usleep_range(1000, 2000);
+
+	/* 5. Power on the PLL while VCO is held in reset */
+	pwr |= DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+
+	/* Wait for things to stabilize before taking VCO out of reset */
+	usleep_range(1000, 2000);
+
+	/* 6. Take the VCO out of reset */
+	ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+	usleep_range(1000, 2000);
+
+	/* 7. Put the VCO back in reset */
+	ctrl |= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+
+	/* 8. Power off the PLL */
+	pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+	usleep_range(1000, 2000);
+
+	/* 9. Power on the PLL while VCO is held in reset */
+	pwr |= DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+
+	/* 10. Take the VCO out of reset */
+	ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+
+	/* 11. Turn off write enables */
+	pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_WE;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
+
+	ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_WE;
+	dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
+
+	usleep_range(1000, 2000);
+
+	/* Return if we have lock or not */
+	return dwc3_octeon_pll_locked(octeon);
+}
+
 static int dwc3_octeon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -418,7 +688,7 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	const char *hs_clock_type, *ss_clock_type;
 	int ref_clk_sel, ref_clk_fsel, mpll_mul;
 	int power_active_low, power_gpio;
-	int err, len;
+	int err, len, tries = 0;
 	u32 clock_rate;
 
 	if (of_property_read_u32(node, "refclk-frequency", &clock_rate)) {
@@ -503,6 +773,7 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	if (IS_ERR(octeon->base))
 		return PTR_ERR(octeon->base);
 
+retry:
 	err = dwc3_octeon_setup(octeon, ref_clk_sel, ref_clk_fsel, mpll_mul,
 				power_gpio, power_active_low);
 	if (err)
@@ -511,6 +782,18 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	dwc3_octeon_set_endian_mode(octeon);
 	dwc3_octeon_phy_reset(octeon);
 
+	usleep_range(50, 100);
+	if (dwc3_octeon_pll_locked(octeon) == 0) {
+		dev_warn(dev, "PLL unlocked, reseting (%d of 3)\n", ++tries);
+		err = dwc3_uphy_pll_reset(octeon);
+		if (err < 0)
+			return err;
+		if (tries < 3)
+			goto retry;
+		dev_err(dev, "PLL lock failed\n");
+		return -EIO;
+	}
+
 	platform_set_drvdata(pdev, octeon);
 
 	return of_platform_populate(node, NULL, NULL, dev);
-- 
2.39.2

Re: [RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[Thinh Nguyen]

On Thu, Oct 12, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Implement workaround for Octeon Known Issue Id 29206:
> | The USB high speed logic contains a PLL that must lock during
> | initialization for correct operation. In very rare circumstances,
> | it is possible for the PLL to fail to start correctly.
> | Workaround
> | After initialization, check the USB PLL lock register via the
> | UPHY CR interface. If the PLL is not running, power it down and
> | back up and restart the initialization.

Minor nit: Can we replace "|" with tabs. I think it's easier to read.

> 
> PLL initialization code taken from Cavium's vendor bootloader:
> u-boot/drivers/usb/host/xhci-octeon.c:octeon3_usb_clocks_start
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  NOTE:
>  This patch fixes initialization issue found on some CN7020 based boards.
>  Without this patch, controller sometimes fails to detect devices connected.
>  Original code comes from Cavium released u-boot monster patch, which seems
>  to suffer from mistakes made while resolving merge conflicts when upgrading
>  to newer u-boot.
>  Testing revealed that only single reinit is needed to properly lock PLL,
>  this agrees with comment in Cavium's u-boot code, which is claiming the
>  same. However, same as in u-boot code, reinit is attempted three times.
>  (in could be done using while loop instead of goto, just let me know
>  which way do you prefer)
>  SoCs suffering from this problem would fail to initialize PHY about
>  several tens times of thousand boots. This patch always restored
>  functional state.

What kernel version did you use? Perhaps it has the same issue due to
the commit e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was
configured as host-only")

Did you test this against Greg's usb-linus branch with the fix for the
above?

Thanks,
Thinh

Re: [RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[Ladislav Michl]

On Thu, Oct 12, 2023 at 10:23:30PM +0000, Thinh Nguyen wrote:
> On Thu, Oct 12, 2023, Ladislav Michl wrote:
> > From: Ladislav Michl <ladis@linux-mips.org>
> > 
> > Implement workaround for Octeon Known Issue Id 29206:
> > | The USB high speed logic contains a PLL that must lock during
> > | initialization for correct operation. In very rare circumstances,
> > | it is possible for the PLL to fail to start correctly.
> > | Workaround
> > | After initialization, check the USB PLL lock register via the
> > | UPHY CR interface. If the PLL is not running, power it down and
> > | back up and restart the initialization.
> 
> Minor nit: Can we replace "|" with tabs. I think it's easier to read.

Ok.
 
> > PLL initialization code taken from Cavium's vendor bootloader:
> > u-boot/drivers/usb/host/xhci-octeon.c:octeon3_usb_clocks_start
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  NOTE:
> >  This patch fixes initialization issue found on some CN7020 based boards.
> >  Without this patch, controller sometimes fails to detect devices connected.
> >  Original code comes from Cavium released u-boot monster patch, which seems
> >  to suffer from mistakes made while resolving merge conflicts when upgrading
> >  to newer u-boot.
> >  Testing revealed that only single reinit is needed to properly lock PLL,
> >  this agrees with comment in Cavium's u-boot code, which is claiming the
> >  same. However, same as in u-boot code, reinit is attempted three times.
> >  (in could be done using while loop instead of goto, just let me know
> >  which way do you prefer)
> >  SoCs suffering from this problem would fail to initialize PHY about
> >  several tens times of thousand boots. This patch always restored
> >  functional state.
> 
> What kernel version did you use? Perhaps it has the same issue due to
> the commit e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was
> configured as host-only")

Production devices are running 6.1.y, patch was developed and tested against
6.1.38, then ported to 6.6-rc5. The purpose of this RFC was to figure out
how to handle situation when informations are comming from NDA document
and code is based on the one coming from unpublished vendor tree.

> Did you test this against Greg's usb-linus branch with the fix for the
> above?

Do you mean "usb: dwc3: Soft reset phy on probe for host" patch? I had
noticed it, however this patch was not tested with that fix.

Cavium's u-boot is also resetting PHYs similar way it is done there,
but I ommited this change as testing revealed it is not needed.

Please see dwc3_uphy_pll_reset function and comment above it.
Without hardware documentation I can only guess whenever it makes sense.

Regards,
	ladis

Re: [RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[kernel test robot]

Hi Ladislav,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.6-rc6 next-20231016]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ladislav-Michl/usb-dwc3-dwc3-octeon-Fix-USB-PHY-High-Speed-PLL-Initialization/20231017-112612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/ZShGmL4mph91Ncib%40lenoch
patch subject: [RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231017/202310171355.p5enLEbv-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/202310171355.p5enLEbv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310171355.p5enLEbv-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/usb/dwc3/dwc3-octeon.c:614: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Performs a full reset of the UPHY PLL. Note that this is normally done


vim +614 drivers/usb/dwc3/dwc3-octeon.c

   612	
   613	/**
 > 614	 * Performs a full reset of the UPHY PLL. Note that this is normally done
   615	 * internally by a state machine when the UPHY is brought out of reset but this
   616	 * version gives far more time for things to settle before continuing.
   617	 */
   618	static int dwc3_uphy_pll_reset(struct dwc3_octeon *octeon)
   619	{
   620		u16 ctrl, pwr;
   621	
   622		/* 1. Turn on write enable so we can assert reset to the PLL VCO */
   623		ctrl = dwc3_octeon_indirect_read(octeon, DWC3_INT_IND_UPHY_PLL_RESET);
   624		ctrl |= DWC3_INT_IND_UPHY_PLL_RESET_WE;
   625		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   626	
   627		/* 2. Turn on write enable for PLL power control */
   628		pwr = dwc3_octeon_indirect_read(octeon, DWC3_INT_IND_UPHY_PLL_PU);
   629		pwr |= DWC3_INT_IND_UPHY_PLL_PU_WE;
   630		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   631	
   632		/* 3. Assert VCO reset */
   633		ctrl |= DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
   634		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   635	
   636		/* 4. Power off the PLL */
   637		pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
   638		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   639		usleep_range(1000, 2000);
   640	
   641		/* 5. Power on the PLL while VCO is held in reset */
   642		pwr |= DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
   643		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   644	
   645		/* Wait for things to stabilize before taking VCO out of reset */
   646		usleep_range(1000, 2000);
   647	
   648		/* 6. Take the VCO out of reset */
   649		ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
   650		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   651		usleep_range(1000, 2000);
   652	
   653		/* 7. Put the VCO back in reset */
   654		ctrl |= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
   655		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   656	
   657		/* 8. Power off the PLL */
   658		pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
   659		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   660		usleep_range(1000, 2000);
   661	
   662		/* 9. Power on the PLL while VCO is held in reset */
   663		pwr |= DWC3_INT_IND_UPHY_PLL_PU_POWER_EN;
   664		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   665	
   666		/* 10. Take the VCO out of reset */
   667		ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_VCO_RST;
   668		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   669	
   670		/* 11. Turn off write enables */
   671		pwr &= ~DWC3_INT_IND_UPHY_PLL_PU_WE;
   672		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_PU, pwr);
   673	
   674		ctrl &= ~DWC3_INT_IND_UPHY_PLL_RESET_WE;
   675		dwc3_octeon_indirect_write(octeon, DWC3_INT_IND_UPHY_PLL_RESET, ctrl);
   676	
   677		usleep_range(1000, 2000);
   678	
   679		/* Return if we have lock or not */
   680		return dwc3_octeon_pll_locked(octeon);
   681	}
   682	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC] usb: dwc3: dwc3-octeon: Fix USB PHY High-Speed PLL Initialization

[Thinh Nguyen]

Sorry for the delay response.

On Fri, Oct 13, 2023, Ladislav Michl wrote:
> On Thu, Oct 12, 2023 at 10:23:30PM +0000, Thinh Nguyen wrote:
> > On Thu, Oct 12, 2023, Ladislav Michl wrote:
> > > From: Ladislav Michl <ladis@linux-mips.org>
> > > 
> > > Implement workaround for Octeon Known Issue Id 29206:
> > > | The USB high speed logic contains a PLL that must lock during
> > > | initialization for correct operation. In very rare circumstances,
> > > | it is possible for the PLL to fail to start correctly.
> > > | Workaround
> > > | After initialization, check the USB PLL lock register via the
> > > | UPHY CR interface. If the PLL is not running, power it down and
> > > | back up and restart the initialization.
> > 
> > Minor nit: Can we replace "|" with tabs. I think it's easier to read.
> 
> Ok.
>  
> > > PLL initialization code taken from Cavium's vendor bootloader:
> > > u-boot/drivers/usb/host/xhci-octeon.c:octeon3_usb_clocks_start
> > > 
> > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > > ---
> > >  NOTE:
> > >  This patch fixes initialization issue found on some CN7020 based boards.
> > >  Without this patch, controller sometimes fails to detect devices connected.
> > >  Original code comes from Cavium released u-boot monster patch, which seems
> > >  to suffer from mistakes made while resolving merge conflicts when upgrading
> > >  to newer u-boot.
> > >  Testing revealed that only single reinit is needed to properly lock PLL,
> > >  this agrees with comment in Cavium's u-boot code, which is claiming the
> > >  same. However, same as in u-boot code, reinit is attempted three times.
> > >  (in could be done using while loop instead of goto, just let me know
> > >  which way do you prefer)
> > >  SoCs suffering from this problem would fail to initialize PHY about
> > >  several tens times of thousand boots. This patch always restored
> > >  functional state.
> > 
> > What kernel version did you use? Perhaps it has the same issue due to
> > the commit e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was
> > configured as host-only")
> 
> Production devices are running 6.1.y, patch was developed and tested against
> 6.1.38, then ported to 6.6-rc5. The purpose of this RFC was to figure out
> how to handle situation when informations are comming from NDA document
> and code is based on the one coming from unpublished vendor tree.
> 
> > Did you test this against Greg's usb-linus branch with the fix for the
> > above?
> 
> Do you mean "usb: dwc3: Soft reset phy on probe for host" patch? I had
> noticed it, however this patch was not tested with that fix.

Ok.

> 
> Cavium's u-boot is also resetting PHYs similar way it is done there,
> but I ommited this change as testing revealed it is not needed.
> 
> Please see dwc3_uphy_pll_reset function and comment above it.
> Without hardware documentation I can only guess whenever it makes sense.
> 

I have much less knowledge of this platform than you. As long as you
document your findings and how you came to the logic for the patch as
you did. It should be fine.

I notice that there are debug comments, and this looks like a
work-in-progress. When it's ready, please clean it up and remove the RFC
so we can merge it.

Thanks,
Thinh