[PATCH v8 0/3] Xilinx ZynqMP USB fixes

[PATCH v8 0/3] Xilinx ZynqMP USB fixes

[Robert Hancock]

Some fixes related to the Xilinx ZynqMP DWC3 wrapper driver.

Changes since v7:
-Drop patches already merged. Add a patch to fix a bug found in one
of those previous patches.
-Fixed error handling for reset GPIO

Changes since v6:
-skip USB core resets and register settings which are not necessary
when USB3 PHY is not specified
-added patches to implement ULPI PHY reset in driver

Changes since v5:
-code formatting fixes, no functional change

Changes since v4:
-dropped DWC3 core patches as they are superseded by Sean Anderson's
patchset "usb: dwc3: Calculate REFCLKPER et. al. from reference clock",
ZynqMP-specific patches unchanged

Changes since v3:
-fixed DT schema dt-doc-validate error

Changes since v2:
-additional kerneldoc fixes

Changes since v1:
-added DT binding documentation for new attribute
-kerneldoc formatting and reworded comments

*** BLURB HERE ***

Robert Hancock (3):
  usb: dwc3: xilinx: fix uninitialized return value
  dt-bindings: usb: dwc3-xilinx: Added reset-gpios
  usb: dwc3: xilinx: Add ULPI PHY reset handling

 .../devicetree/bindings/usb/dwc3-xilinx.yaml  |  4 ++++
 drivers/usb/dwc3/dwc3-xilinx.c                | 21 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH v8 1/3] usb: dwc3: xilinx: fix uninitialized return value

[Robert Hancock]

A previous patch to skip part of the initialization when a USB3 PHY was
not present could result in the return value being uninitialized in that
case, causing spurious probe failures. Initialize ret to 0 to avoid this.

Fixes: 9678f3361afc ("usb: dwc3: xilinx: Skip resets and USB3 register settings for USB2.0 mode")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/usb/dwc3/dwc3-xilinx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index e14ac15e24c3..a6f3a9b38789 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -99,7 +99,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	struct device		*dev = priv_data->dev;
 	struct reset_control	*crst, *hibrst, *apbrst;
 	struct phy		*usb3_phy;
-	int			ret;
+	int			ret = 0;
 	u32			reg;
 
 	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
-- 
2.31.1

[PATCH v8 2/3] dt-bindings: usb: dwc3-xilinx: Added reset-gpios

[Robert Hancock]

Update DT binding to reflect new reset-gpios property.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml b/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
index f77c16e203d5..823ce731e95f 100644
--- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
+++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
@@ -59,6 +59,10 @@ properties:
       - const: usb_hibrst
       - const: usb_apbrst
 
+  reset-gpios:
+    description: Optional GPIO connected to ULPI PHY reset line.
+    maxItems: 1
+
   phys:
     minItems: 1
     maxItems: 2
-- 
2.31.1

[PATCH v8 3/3] usb: dwc3: xilinx: Add ULPI PHY reset handling

[Robert Hancock]

Hook up an optional GPIO-based reset for the connected USB ULPI PHY
device. This is typically already done by the first-stage boot loader,
however it can be more robust to ensure this reset is done prior to
loading the driver in Linux.

Based on a patch "usb: dwc3: xilinx: Add gpio-reset support" in the
Xilinx kernel tree by Piyush Mehta <piyush.mehta@xilinx.com>.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/usb/dwc3/dwc3-xilinx.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index a6f3a9b38789..1ee6011ada44 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/of_platform.h>
@@ -101,6 +102,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	struct phy		*usb3_phy;
 	int			ret = 0;
 	u32			reg;
+	struct gpio_desc        *reset_gpio;
 
 	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
 	if (IS_ERR(usb3_phy)) {
@@ -110,6 +112,14 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 		goto err;
 	}
 
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio)) {
+		ret = PTR_ERR(reset_gpio);
+		dev_err_probe(dev, ret,
+			      "Failed to get reset gpio\n");
+		goto err;
+	}
+
 	/*
 	 * The following core resets are not required unless a USB3 PHY
 	 * is used, and the subsequent register settings are not required
@@ -201,6 +211,15 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	}
 
 skip_usb3_phy:
+	/* ulpi reset via gpio-modepin or gpio-framework driver */
+	if (reset_gpio) {
+		/* Toggle ulpi to reset the phy. */
+		gpiod_set_value(reset_gpio, 0);
+		usleep_range(5000, 10000); /* delay */
+		gpiod_set_value(reset_gpio, 1);
+		usleep_range(5000, 10000); /* delay */
+	}
+
 	/*
 	 * This routes the USB DMA traffic to go through FPD path instead
 	 * of reaching DDR directly. This traffic routing is needed to
-- 
2.31.1

Re: [PATCH v8 3/3] usb: dwc3: xilinx: Add ULPI PHY reset handling

[Sean Anderson]

Hi Rob,

On 1/26/22 6:40 PM, Robert Hancock wrote:
> Hook up an optional GPIO-based reset for the connected USB ULPI PHY
> device. This is typically already done by the first-stage boot loader,
> however it can be more robust to ensure this reset is done prior to
> loading the driver in Linux.
> 
> Based on a patch "usb: dwc3: xilinx: Add gpio-reset support" in the
> Xilinx kernel tree by Piyush Mehta <piyush.mehta@xilinx.com>.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index a6f3a9b38789..1ee6011ada44 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of_platform.h>
> @@ -101,6 +102,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	struct phy		*usb3_phy;
>  	int			ret = 0;
>  	u32			reg;
> +	struct gpio_desc        *reset_gpio;
>  
>  	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
>  	if (IS_ERR(usb3_phy)) {
> @@ -110,6 +112,14 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  		goto err;
>  	}
>  
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio)) {
> +		ret = PTR_ERR(reset_gpio);
> +		dev_err_probe(dev, ret,
> +			      "Failed to get reset gpio\n");
> +		goto err;
> +	}
> +
>  	/*
>  	 * The following core resets are not required unless a USB3 PHY
>  	 * is used, and the subsequent register settings are not required
> @@ -201,6 +211,15 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	}
>  
>  skip_usb3_phy:
> +	/* ulpi reset via gpio-modepin or gpio-framework driver */
> +	if (reset_gpio) {
> +		/* Toggle ulpi to reset the phy. */
> +		gpiod_set_value(reset_gpio, 0);
> +		usleep_range(5000, 10000); /* delay */
> +		gpiod_set_value(reset_gpio, 1);
> +		usleep_range(5000, 10000); /* delay */
> +	}
> +
>  	/*
>  	 * This routes the USB DMA traffic to go through FPD path instead
>  	 * of reaching DDR directly. This traffic routing is needed to
> 

Do we need to have this in dwc3? Why not just use the usb-nop-xceiv driver (aka usb_phy_generic)?

--Sean

Re: [PATCH v8 3/3] usb: dwc3: xilinx: Add ULPI PHY reset handling

[Robert Hancock]

On Thu, 2022-01-27 at 15:10 -0500, Sean Anderson wrote:
> Hi Rob,
> 
> On 1/26/22 6:40 PM, Robert Hancock wrote:
> > Hook up an optional GPIO-based reset for the connected USB ULPI PHY
> > device. This is typically already done by the first-stage boot loader,
> > however it can be more robust to ensure this reset is done prior to
> > loading the driver in Linux.
> > 
> > Based on a patch "usb: dwc3: xilinx: Add gpio-reset support" in the
> > Xilinx kernel tree by Piyush Mehta <piyush.mehta@xilinx.com>.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >  drivers/usb/dwc3/dwc3-xilinx.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
> > xilinx.c
> > index a6f3a9b38789..1ee6011ada44 100644
> > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/clk.h>
> >  #include <linux/of.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/of_platform.h>
> > @@ -101,6 +102,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > *priv_data)
> >  	struct phy		*usb3_phy;
> >  	int			ret = 0;
> >  	u32			reg;
> > +	struct gpio_desc        *reset_gpio;
> >  
> >  	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
> >  	if (IS_ERR(usb3_phy)) {
> > @@ -110,6 +112,14 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > *priv_data)
> >  		goto err;
> >  	}
> >  
> > +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpio)) {
> > +		ret = PTR_ERR(reset_gpio);
> > +		dev_err_probe(dev, ret,
> > +			      "Failed to get reset gpio\n");
> > +		goto err;
> > +	}
> > +
> >  	/*
> >  	 * The following core resets are not required unless a USB3 PHY
> >  	 * is used, and the subsequent register settings are not required
> > @@ -201,6 +211,15 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > *priv_data)
> >  	}
> >  
> >  skip_usb3_phy:
> > +	/* ulpi reset via gpio-modepin or gpio-framework driver */
> > +	if (reset_gpio) {
> > +		/* Toggle ulpi to reset the phy. */
> > +		gpiod_set_value(reset_gpio, 0);
> > +		usleep_range(5000, 10000); /* delay */
> > +		gpiod_set_value(reset_gpio, 1);
> > +		usleep_range(5000, 10000); /* delay */
> > +	}
> > +
> >  	/*
> >  	 * This routes the USB DMA traffic to go through FPD path instead
> >  	 * of reaching DDR directly. This traffic routing is needed to
> > 
> 
> Do we need to have this in dwc3? Why not just use the usb-nop-xceiv driver
> (aka usb_phy_generic)?

I hadn't noticed that option. Just tried it out and it seems like it does
what's needed. So patches 2 and 3 can be dispensed with. Patch 1 is still
needed however, I may resubmit that as a standalone patch..

> 
> --Sean
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH v8 2/3] dt-bindings: usb: dwc3-xilinx: Added reset-gpios

[Rob Herring]

On Wed, Jan 26, 2022 at 05:40:16PM -0600, Robert Hancock wrote:
> Update DT binding to reflect new reset-gpios property.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml b/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
> index f77c16e203d5..823ce731e95f 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml
> @@ -59,6 +59,10 @@ properties:
>        - const: usb_hibrst
>        - const: usb_apbrst
>  
> +  reset-gpios:
> +    description: Optional GPIO connected to ULPI PHY reset line.
> +    maxItems: 1

Reset for the PHY belongs in the PHY node.

> +
>    phys:
>      minItems: 1
>      maxItems: 2
> -- 
> 2.31.1
> 
> 

RE: [PATCH v8 3/3] usb: dwc3: xilinx: Add ULPI PHY reset handling

[Piyush Mehta]

Hello Robert,

Thanks for patch.
All the Xilinx USB resets are ACTIVE_LOW reset.

Regards,
Piyush Mehta
> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Thursday, January 27, 2022 5:10 AM
> To: linux-usb@vger.kernel.org
> Cc: balbi@kernel.org; gregkh@linuxfoundation.org; Michal Simek
> <michals@xilinx.com>; Manish Narani <MNARANI@xilinx.com>;
> sean.anderson@seco.com; robh+dt@kernel.org; devicetree@vger.kernel.org;
> Piyush Mehta <piyushm@xilinx.com>; Robert Hancock
> <robert.hancock@calian.com>
> Subject: [PATCH v8 3/3] usb: dwc3: xilinx: Add ULPI PHY reset handling
> 
> Hook up an optional GPIO-based reset for the connected USB ULPI PHY device.
> This is typically already done by the first-stage boot loader, however it can be
> more robust to ensure this reset is done prior to loading the driver in Linux.
> 
> Based on a patch "usb: dwc3: xilinx: Add gpio-reset support" in the Xilinx kernel
> tree by Piyush Mehta <piyush.mehta@xilinx.com>.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index a6f3a9b38789..1ee6011ada44 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of_platform.h>
> @@ -101,6 +102,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> *priv_data)
>  	struct phy		*usb3_phy;
>  	int			ret = 0;
>  	u32			reg;
> +	struct gpio_desc        *reset_gpio;
> 
>  	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
>  	if (IS_ERR(usb3_phy)) {
> @@ -110,6 +112,14 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> *priv_data)
>  		goto err;
>  	}
> 
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);

reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
Note: Xilinx Support active low resets

> +	if (IS_ERR(reset_gpio)) {
> +		ret = PTR_ERR(reset_gpio);
> +		dev_err_probe(dev, ret,
> +			      "Failed to get reset gpio\n");
> +		goto err;
> +	}
> +
>  	/*
>  	 * The following core resets are not required unless a USB3 PHY
>  	 * is used, and the subsequent register settings are not required @@ -
> 201,6 +211,15 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> *priv_data)
>  	}
> 
>  skip_usb3_phy:
> +	/* ulpi reset via gpio-modepin or gpio-framework driver */
> +	if (reset_gpio) {
> +		/* Toggle ulpi to reset the phy. */
> +		gpiod_set_value(reset_gpio, 0);
> +		usleep_range(5000, 10000); /* delay */
> +		gpiod_set_value(reset_gpio, 1);
> +		usleep_range(5000, 10000); /* delay */
> +	}
> +
>  	/*
>  	 * This routes the USB DMA traffic to go through FPD path instead
>  	 * of reaching DDR directly. This traffic routing is needed to
> --
> 2.31.1