[PATCH v6 0/2] Xilinx ZynqMP USB fixes

[PATCH v6 0/2] Xilinx ZynqMP USB fixes

[Robert Hancock]

Some fixes related to the Xilinx ZynqMP DWC3 wrapper driver to allow
ZynqMP USB to work properly when the hardware is configured in USB
2.0-only mode.

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


Robert Hancock (2):
  usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode
  usb: dwc3: xilinx: Fix error handling when getting USB3 PHY

 drivers/usb/dwc3/dwc3-xilinx.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Robert Hancock]

It appears that the PIPE clock should not be selected when only USB 2.0
is being used in the design and no USB 3.0 reference clock is used. Fix
to set the correct value depending on whether a USB3 PHY is present.

Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index 9cc3ad701a29..dd6218d05159 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	/* Set PIPE Power Present signal in FPD Power Present Register*/
 	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs + XLNX_USB_FPD_POWER_PRSNT);
 
-	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
-	writel(PIPE_CLK_SELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
+	/*
+	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if a USB3
+	 * PHY is in use, deselect otherwise
+	 */
+	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
+	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
 
 	ret = reset_control_deassert(crst);
 	if (ret < 0) {
-- 
2.31.1

RE: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Manish Narani]

Hi Robert,

Thanks for the patch! Please see my comments below inline!

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Friday, January 21, 2022 11:49 PM
> 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; Robert Hancock <robert.hancock@calian.com>
> Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0
> mode
> 
> It appears that the PIPE clock should not be selected when only USB 2.0
> is being used in the design and no USB 3.0 reference clock is used. Fix
> to set the correct value depending on whether a USB3 PHY is present.
> 
> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index 9cc3ad701a29..dd6218d05159 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> *priv_data)
>  	/* Set PIPE Power Present signal in FPD Power Present Register*/
>  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
> XLNX_USB_FPD_POWER_PRSNT);
> 
> -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> -	writel(PIPE_CLK_SELECT, priv_data->regs +
> XLNX_USB_FPD_PIPE_CLK);
> +	/*
> +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if a USB3
> +	 * PHY is in use, deselect otherwise
> +	 */
> +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
> +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);

When USB3.0 is enabled in the design, FSBL will set this bit to PIPE_CLK_SELECT
And it's state will be persistent till Linux stage. When this driver finds the usb3-phy property
In the device tree, it will again set this bit.
But in case if the usb3-phy is not present in the device tree and design has
USB3.0 enabled, then this will clear this bit and ultimately it will fail.

It will be better to skip touching that bit in case the device tree does not have the usb3-phy property.
This will skip the whole sequence of PHY initialization (reset assert/deassert are done in order to help initialize PHY).
Something like below should work.
---
        int                     ret;
        u32                     reg;
 
-       usb3_phy = devm_phy_get(dev, "usb3-phy");
-       if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
-               ret = -EPROBE_DEFER;
+       usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
+       if (IS_ERR(usb3_phy)) {
+               ret = PTR_ERR(usb3_phy);
+               dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
                goto err;
-       } else if (IS_ERR(usb3_phy)) {
-               usb3_phy = NULL;
        }
 
+       if (!usb3_phy)
+               goto skip_usb3_phy;
+
        crst = devm_reset_control_get_exclusive(dev, "usb_crst");
        if (IS_ERR(crst)) {
                ret = PTR_ERR(crst);
@@ -188,6 +190,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
                goto err;
        }
 
+skip_usb3_phy:
        /*
         * This routes the USB DMA traffic to go through FPD path instead
         * of reaching DDR directly. This traffic routing is needed to
---

Thanks,
Manish

Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Robert Hancock]

On Mon, 2022-01-24 at 06:55 +0000, Manish Narani wrote:
> Hi Robert,
> 
> Thanks for the patch! Please see my comments below inline!
> 
> > -----Original Message-----
> > From: Robert Hancock <robert.hancock@calian.com>
> > Sent: Friday, January 21, 2022 11:49 PM
> > 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; Robert Hancock <robert.hancock@calian.com>
> > Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
> > USB2.0
> > mode
> > 
> > It appears that the PIPE clock should not be selected when only USB 2.0
> > is being used in the design and no USB 3.0 reference clock is used. Fix
> > to set the correct value depending on whether a USB3 PHY is present.
> > 
> > Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
> > xilinx.c
> > index 9cc3ad701a29..dd6218d05159 100644
> > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > *priv_data)
> >  	/* Set PIPE Power Present signal in FPD Power Present Register*/
> >  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
> > XLNX_USB_FPD_POWER_PRSNT);
> > 
> > -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> > -	writel(PIPE_CLK_SELECT, priv_data->regs +
> > XLNX_USB_FPD_PIPE_CLK);
> > +	/*
> > +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if a USB3
> > +	 * PHY is in use, deselect otherwise
> > +	 */
> > +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
> > +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
> 
> When USB3.0 is enabled in the design, FSBL will set this bit to
> PIPE_CLK_SELECT
> And it's state will be persistent till Linux stage. When this driver finds
> the usb3-phy property
> In the device tree, it will again set this bit.
> But in case if the usb3-phy is not present in the device tree and design has
> USB3.0 enabled, then this will clear this bit and ultimately it will fail.
> 
> It will be better to skip touching that bit in case the device tree does not
> have the usb3-phy property.
> This will skip the whole sequence of PHY initialization (reset
> assert/deassert are done in order to help initialize PHY).
> Something like below should work.

So the original patch was tested against hardware that only had USB 2.0 support
and seemed to work fine. However, we've since found an issue with some other
hardware supporting USB 3.0 where either it doesn't detect devices at all, or
they get detected but then seem to drop off the bus very quickly, and we get
this repeatedly:

[   99.858607] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?

The same problem is reproducible on the Xilinx ZCU102 board with the same
kernel build, where the USB works fine with the Xilinx kernel and a Petalinux
2021.2 pre-built ZCU102 image, so it's no hardware issue.

I've been trying to isolate any relevant differences between the Xilinx kernel
and mainline in this respect but haven't had much success. One difference in
this particular dwc3-xilinx code is that the Xilinx kernel has code to reset
the ULPI PHY which is not in the mainline version yet. However, adding that in
doesn't seem to fix the problem.

Have you (or anyone else on the CC list) done any testing of USB 3.0 devices
and USB 3.0 capable hardware on mainline with ZynqMP to know if this is a more
general issue?

> ---
>         int                     ret;
>         u32                     reg;
>  
> -       usb3_phy = devm_phy_get(dev, "usb3-phy");
> -       if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
> -               ret = -EPROBE_DEFER;
> +       usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
> +       if (IS_ERR(usb3_phy)) {
> +               ret = PTR_ERR(usb3_phy);
> +               dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
>                 goto err;
> -       } else if (IS_ERR(usb3_phy)) {
> -               usb3_phy = NULL;
>         }
>  
> +       if (!usb3_phy)
> +               goto skip_usb3_phy;
> +
>         crst = devm_reset_control_get_exclusive(dev, "usb_crst");
>         if (IS_ERR(crst)) {
>                 ret = PTR_ERR(crst);
> @@ -188,6 +190,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> *priv_data)
>                 goto err;
>         }
>  
> +skip_usb3_phy:
>         /*
>          * This routes the USB DMA traffic to go through FPD path instead
>          * of reaching DDR directly. This traffic routing is needed to
> ---
> 
> Thanks,
> Manish

RE: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Manish Narani]

Hi Robert,

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Tuesday, January 25, 2022 12:31 AM
> To: Manish Narani <MNARANI@xilinx.com>; linux-usb@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; sean.anderson@seco.com;
> gregkh@linuxfoundation.org; balbi@kernel.org
> Subject: Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
> USB2.0 mode
> 
> On Mon, 2022-01-24 at 06:55 +0000, Manish Narani wrote:
> > Hi Robert,
> >
> > Thanks for the patch! Please see my comments below inline!
> >
> > > -----Original Message-----
> > > From: Robert Hancock <robert.hancock@calian.com>
> > > Sent: Friday, January 21, 2022 11:49 PM
> > > 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; Robert Hancock
> <robert.hancock@calian.com>
> > > Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
> > > USB2.0
> > > mode
> > >
> > > It appears that the PIPE clock should not be selected when only USB 2.0
> > > is being used in the design and no USB 3.0 reference clock is used. Fix
> > > to set the correct value depending on whether a USB3 PHY is present.
> > >
> > > Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > ---
> > >  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
> > > xilinx.c
> > > index 9cc3ad701a29..dd6218d05159 100644
> > > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > > @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct
> dwc3_xlnx
> > > *priv_data)
> > >  	/* Set PIPE Power Present signal in FPD Power Present Register*/
> > >  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
> > > XLNX_USB_FPD_POWER_PRSNT);
> > >
> > > -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> > > -	writel(PIPE_CLK_SELECT, priv_data->regs +
> > > XLNX_USB_FPD_PIPE_CLK);
> > > +	/*
> > > +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if a USB3
> > > +	 * PHY is in use, deselect otherwise
> > > +	 */
> > > +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
> > > +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
> >
> > When USB3.0 is enabled in the design, FSBL will set this bit to
> > PIPE_CLK_SELECT
> > And it's state will be persistent till Linux stage. When this driver finds
> > the usb3-phy property
> > In the device tree, it will again set this bit.
> > But in case if the usb3-phy is not present in the device tree and design has
> > USB3.0 enabled, then this will clear this bit and ultimately it will fail.
> >
> > It will be better to skip touching that bit in case the device tree does not
> > have the usb3-phy property.
> > This will skip the whole sequence of PHY initialization (reset
> > assert/deassert are done in order to help initialize PHY).
> > Something like below should work.
> 
> So the original patch was tested against hardware that only had USB 2.0
> support
> and seemed to work fine. However, we've since found an issue with some
> other
> hardware supporting USB 3.0 where either it doesn't detect devices at all, or
> they get detected but then seem to drop off the bus very quickly, and we get
> this repeatedly:
> 
> [   99.858607] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
> 
> The same problem is reproducible on the Xilinx ZCU102 board with the same
> kernel build, where the USB works fine with the Xilinx kernel and a Petalinux
> 2021.2 pre-built ZCU102 image, so it's no hardware issue.
> 
> I've been trying to isolate any relevant differences between the Xilinx kernel
> and mainline in this respect but haven't had much success. One difference in
> this particular dwc3-xilinx code is that the Xilinx kernel has code to reset
> the ULPI PHY which is not in the mainline version yet. However, adding that
> in
> doesn't seem to fix the problem.
> 
> Have you (or anyone else on the CC list) done any testing of USB 3.0 devices
> and USB 3.0 capable hardware on mainline with ZynqMP to know if this is a
> more
> general issue?

Yes, The USB fixes are work in progress and will be sent to mainline in near future.
This fix is one of them. Your patch is solving the problem in case of USB 2.0 
But not the USB 3.0 entirely. There is this corner case which I mentioned, breaks
USB 3.0 functionality with your patch.

Thanks,
Manish

> 
> > ---
> >         int                     ret;
> >         u32                     reg;
> >
> > -       usb3_phy = devm_phy_get(dev, "usb3-phy");
> > -       if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
> > -               ret = -EPROBE_DEFER;
> > +       usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
> > +       if (IS_ERR(usb3_phy)) {
> > +               ret = PTR_ERR(usb3_phy);
> > +               dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
> >                 goto err;
> > -       } else if (IS_ERR(usb3_phy)) {
> > -               usb3_phy = NULL;
> >         }
> >
> > +       if (!usb3_phy)
> > +               goto skip_usb3_phy;
> > +
> >         crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> >         if (IS_ERR(crst)) {
> >                 ret = PTR_ERR(crst);
> > @@ -188,6 +190,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > *priv_data)
> >                 goto err;
> >         }
> >
> > +skip_usb3_phy:
> >         /*
> >          * This routes the USB DMA traffic to go through FPD path instead
> >          * of reaching DDR directly. This traffic routing is needed to
> > ---
> >
> > Thanks,
> > Manish

Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Robert Hancock]

On Tue, 2022-01-25 at 05:36 +0000, Manish Narani wrote:
> Hi Robert,
> 
> > -----Original Message-----
> > From: Robert Hancock <robert.hancock@calian.com>
> > Sent: Tuesday, January 25, 2022 12:31 AM
> > To: Manish Narani <MNARANI@xilinx.com>; linux-usb@vger.kernel.org
> > Cc: Michal Simek <michals@xilinx.com>; sean.anderson@seco.com;
> > gregkh@linuxfoundation.org; balbi@kernel.org
> > Subject: Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
> > USB2.0 mode
> > 
> > On Mon, 2022-01-24 at 06:55 +0000, Manish Narani wrote:
> > > Hi Robert,
> > > 
> > > Thanks for the patch! Please see my comments below inline!
> > > 
> > > > -----Original Message-----
> > > > From: Robert Hancock <robert.hancock@calian.com>
> > > > Sent: Friday, January 21, 2022 11:49 PM
> > > > 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; Robert Hancock
> > <robert.hancock@calian.com>
> > > > Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
> > > > USB2.0
> > > > mode
> > > > 
> > > > It appears that the PIPE clock should not be selected when only USB 2.0
> > > > is being used in the design and no USB 3.0 reference clock is used. Fix
> > > > to set the correct value depending on whether a USB3 PHY is present.
> > > > 
> > > > Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > > ---
> > > >  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
> > > > xilinx.c
> > > > index 9cc3ad701a29..dd6218d05159 100644
> > > > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > > > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > > > @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct
> > dwc3_xlnx
> > > > *priv_data)
> > > >  	/* Set PIPE Power Present signal in FPD Power Present
> > > > Register*/
> > > >  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
> > > > XLNX_USB_FPD_POWER_PRSNT);
> > > > 
> > > > -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> > > > -	writel(PIPE_CLK_SELECT, priv_data->regs +
> > > > XLNX_USB_FPD_PIPE_CLK);
> > > > +	/*
> > > > +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if
> > > > a USB3
> > > > +	 * PHY is in use, deselect otherwise
> > > > +	 */
> > > > +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
> > > > +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
> > > 
> > > When USB3.0 is enabled in the design, FSBL will set this bit to
> > > PIPE_CLK_SELECT
> > > And it's state will be persistent till Linux stage. When this driver
> > > finds
> > > the usb3-phy property
> > > In the device tree, it will again set this bit.
> > > But in case if the usb3-phy is not present in the device tree and design
> > > has
> > > USB3.0 enabled, then this will clear this bit and ultimately it will
> > > fail.
> > > 
> > > It will be better to skip touching that bit in case the device tree does
> > > not
> > > have the usb3-phy property.
> > > This will skip the whole sequence of PHY initialization (reset
> > > assert/deassert are done in order to help initialize PHY).
> > > Something like below should work.
> > 
> > So the original patch was tested against hardware that only had USB 2.0
> > support
> > and seemed to work fine. However, we've since found an issue with some
> > other
> > hardware supporting USB 3.0 where either it doesn't detect devices at all,
> > or
> > they get detected but then seem to drop off the bus very quickly, and we
> > get
> > this repeatedly:
> > 
> > [   99.858607] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
> > 
> > The same problem is reproducible on the Xilinx ZCU102 board with the same
> > kernel build, where the USB works fine with the Xilinx kernel and a
> > Petalinux
> > 2021.2 pre-built ZCU102 image, so it's no hardware issue.
> > 
> > I've been trying to isolate any relevant differences between the Xilinx
> > kernel
> > and mainline in this respect but haven't had much success. One difference
> > in
> > this particular dwc3-xilinx code is that the Xilinx kernel has code to
> > reset
> > the ULPI PHY which is not in the mainline version yet. However, adding that
> > in
> > doesn't seem to fix the problem.
> > 
> > Have you (or anyone else on the CC list) done any testing of USB 3.0
> > devices
> > and USB 3.0 capable hardware on mainline with ZynqMP to know if this is a
> > more
> > general issue?
> 
> Yes, The USB fixes are work in progress and will be sent to mainline in near
> future.
> This fix is one of them. Your patch is solving the problem in case of USB
> 2.0 
> But not the USB 3.0 entirely. There is this corner case which I mentioned,
> breaks
> USB 3.0 functionality with your patch.

You mentioned the case where the design is using USB 3.0 but the usb3-phy is
missing from the device tree - with this patch it would switch the clock source
into the USB 2 configuration and might break things. I agree it's probably
better to just skip the whole reset process and keep the FSBL clock selection
if it's not needed in the USB 2 case.

We still have another case where USB 3.0 appears to be broken on a setup where
the usb3-phy is specified, and where the Xilinx/Petalinux images work on the
same hardware. I haven't yet isolated whether it is related to the kernel or
might be caused by the different PL logic or PS configuration settings somehow.

> 
> Thanks,
> Manish
> 
> > > ---
> > >         int                     ret;
> > >         u32                     reg;
> > > 
> > > -       usb3_phy = devm_phy_get(dev, "usb3-phy");
> > > -       if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
> > > -               ret = -EPROBE_DEFER;
> > > +       usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
> > > +       if (IS_ERR(usb3_phy)) {
> > > +               ret = PTR_ERR(usb3_phy);
> > > +               dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
> > >                 goto err;
> > > -       } else if (IS_ERR(usb3_phy)) {
> > > -               usb3_phy = NULL;
> > >         }
> > > 
> > > +       if (!usb3_phy)
> > > +               goto skip_usb3_phy;
> > > +
> > >         crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> > >         if (IS_ERR(crst)) {
> > >                 ret = PTR_ERR(crst);
> > > @@ -188,6 +190,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > > *priv_data)
> > >                 goto err;
> > >         }
> > > 
> > > +skip_usb3_phy:
> > >         /*
> > >          * This routes the USB DMA traffic to go through FPD path instead
> > >          * of reaching DDR directly. This traffic routing is needed to
> > > ---
> > > 
> > > Thanks,
> > > Manish
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Sean Anderson]

On 1/25/22 11:19 AM, Robert Hancock wrote:
> On Tue, 2022-01-25 at 05:36 +0000, Manish Narani wrote:
>> Hi Robert,
>> 
>> > -----Original Message-----
>> > From: Robert Hancock <robert.hancock@calian.com>
>> > Sent: Tuesday, January 25, 2022 12:31 AM
>> > To: Manish Narani <MNARANI@xilinx.com>; linux-usb@vger.kernel.org
>> > Cc: Michal Simek <michals@xilinx.com>; sean.anderson@seco.com;
>> > gregkh@linuxfoundation.org; balbi@kernel.org
>> > Subject: Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
>> > USB2.0 mode
>> > 
>> > On Mon, 2022-01-24 at 06:55 +0000, Manish Narani wrote:
>> > > Hi Robert,
>> > > 
>> > > Thanks for the patch! Please see my comments below inline!
>> > > 
>> > > > -----Original Message-----
>> > > > From: Robert Hancock <robert.hancock@calian.com>
>> > > > Sent: Friday, January 21, 2022 11:49 PM
>> > > > 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; Robert Hancock
>> > <robert.hancock@calian.com>
>> > > > Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for
>> > > > USB2.0
>> > > > mode
>> > > > 
>> > > > It appears that the PIPE clock should not be selected when only USB 2.0
>> > > > is being used in the design and no USB 3.0 reference clock is used. Fix
>> > > > to set the correct value depending on whether a USB3 PHY is present.
>> > > > 
>> > > > Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
>> > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
>> > > > ---
>> > > >  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
>> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
>> > > > xilinx.c
>> > > > index 9cc3ad701a29..dd6218d05159 100644
>> > > > --- a/drivers/usb/dwc3/dwc3-xilinx.c
>> > > > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
>> > > > @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct
>> > dwc3_xlnx
>> > > > *priv_data)
>> > > >  	/* Set PIPE Power Present signal in FPD Power Present
>> > > > Register*/
>> > > >  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
>> > > > XLNX_USB_FPD_POWER_PRSNT);
>> > > > 
>> > > > -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
>> > > > -	writel(PIPE_CLK_SELECT, priv_data->regs +
>> > > > XLNX_USB_FPD_PIPE_CLK);
>> > > > +	/*
>> > > > +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if
>> > > > a USB3
>> > > > +	 * PHY is in use, deselect otherwise
>> > > > +	 */
>> > > > +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
>> > > > +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
>> > > 
>> > > When USB3.0 is enabled in the design, FSBL will set this bit to
>> > > PIPE_CLK_SELECT
>> > > And it's state will be persistent till Linux stage. When this driver
>> > > finds
>> > > the usb3-phy property
>> > > In the device tree, it will again set this bit.
>> > > But in case if the usb3-phy is not present in the device tree and design
>> > > has
>> > > USB3.0 enabled, then this will clear this bit and ultimately it will
>> > > fail.
>> > > 
>> > > It will be better to skip touching that bit in case the device tree does
>> > > not
>> > > have the usb3-phy property.
>> > > This will skip the whole sequence of PHY initialization (reset
>> > > assert/deassert are done in order to help initialize PHY).
>> > > Something like below should work.
>> > 
>> > So the original patch was tested against hardware that only had USB 2.0
>> > support
>> > and seemed to work fine. However, we've since found an issue with some
>> > other
>> > hardware supporting USB 3.0 where either it doesn't detect devices at all,
>> > or
>> > they get detected but then seem to drop off the bus very quickly, and we
>> > get
>> > this repeatedly:
>> > 
>> > [   99.858607] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
>> > 
>> > The same problem is reproducible on the Xilinx ZCU102 board with the same
>> > kernel build, where the USB works fine with the Xilinx kernel and a
>> > Petalinux
>> > 2021.2 pre-built ZCU102 image, so it's no hardware issue.
>> > 
>> > I've been trying to isolate any relevant differences between the Xilinx
>> > kernel
>> > and mainline in this respect but haven't had much success. One difference
>> > in
>> > this particular dwc3-xilinx code is that the Xilinx kernel has code to
>> > reset
>> > the ULPI PHY which is not in the mainline version yet. However, adding that
>> > in
>> > doesn't seem to fix the problem.
>> > 
>> > Have you (or anyone else on the CC list) done any testing of USB 3.0
>> > devices
>> > and USB 3.0 capable hardware on mainline with ZynqMP to know if this is a
>> > more
>> > general issue?
>> 
>> Yes, The USB fixes are work in progress and will be sent to mainline in near
>> future.
>> This fix is one of them. Your patch is solving the problem in case of USB
>> 2.0 
>> But not the USB 3.0 entirely. There is this corner case which I mentioned,
>> breaks
>> USB 3.0 functionality with your patch.
> 
> You mentioned the case where the design is using USB 3.0 but the usb3-phy is
> missing from the device tree

Do we even need to support that?

Either phy (with an appropriate phy-names) or usb3-phy should be present for
USB3. Mainline has had GTR support since 5.8(?) or so.

--Sean

Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Robert Hancock]

On Tue, 2022-01-25 at 10:19 -0600, Robert Hancock wrote:
> On Tue, 2022-01-25 at 05:36 +0000, Manish Narani wrote:
> > Hi Robert,
> > 
> > > -----Original Message-----
> > > From: Robert Hancock <robert.hancock@calian.com>
> > > Sent: Tuesday, January 25, 2022 12:31 AM
> > > To: Manish Narani <MNARANI@xilinx.com>; linux-usb@vger.kernel.org
> > > Cc: Michal Simek <michals@xilinx.com>; sean.anderson@seco.com;
> > > gregkh@linuxfoundation.org; balbi@kernel.org
> > > Subject: Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection
> > > for
> > > USB2.0 mode
> > > 
> > > On Mon, 2022-01-24 at 06:55 +0000, Manish Narani wrote:
> > > > Hi Robert,
> > > > 
> > > > Thanks for the patch! Please see my comments below inline!
> > > > 
> > > > > -----Original Message-----
> > > > > From: Robert Hancock <robert.hancock@calian.com>
> > > > > Sent: Friday, January 21, 2022 11:49 PM
> > > > > 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; Robert Hancock
> > > <robert.hancock@calian.com>
> > > > > Subject: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection
> > > > > for
> > > > > USB2.0
> > > > > mode
> > > > > 
> > > > > It appears that the PIPE clock should not be selected when only USB
> > > > > 2.0
> > > > > is being used in the design and no USB 3.0 reference clock is used.
> > > > > Fix
> > > > > to set the correct value depending on whether a USB3 PHY is present.
> > > > > 
> > > > > Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > > > ---
> > > > >  drivers/usb/dwc3/dwc3-xilinx.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-
> > > > > xilinx.c
> > > > > index 9cc3ad701a29..dd6218d05159 100644
> > > > > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > > > > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > > > > @@ -167,8 +167,12 @@ static int dwc3_xlnx_init_zynqmp(struct
> > > dwc3_xlnx
> > > > > *priv_data)
> > > > >  	/* Set PIPE Power Present signal in FPD Power Present
> > > > > Register*/
> > > > >  	writel(FPD_POWER_PRSNT_OPTION, priv_data->regs +
> > > > > XLNX_USB_FPD_POWER_PRSNT);
> > > > > 
> > > > > -	/* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> > > > > -	writel(PIPE_CLK_SELECT, priv_data->regs +
> > > > > XLNX_USB_FPD_PIPE_CLK);
> > > > > +	/*
> > > > > +	 * Set the PIPE Clock Select bit in FPD PIPE Clock register if
> > > > > a USB3
> > > > > +	 * PHY is in use, deselect otherwise
> > > > > +	 */
> > > > > +	writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT,
> > > > > +	       priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
> > > > 
> > > > When USB3.0 is enabled in the design, FSBL will set this bit to
> > > > PIPE_CLK_SELECT
> > > > And it's state will be persistent till Linux stage. When this driver
> > > > finds
> > > > the usb3-phy property
> > > > In the device tree, it will again set this bit.
> > > > But in case if the usb3-phy is not present in the device tree and
> > > > design
> > > > has
> > > > USB3.0 enabled, then this will clear this bit and ultimately it will
> > > > fail.
> > > > 
> > > > It will be better to skip touching that bit in case the device tree
> > > > does
> > > > not
> > > > have the usb3-phy property.
> > > > This will skip the whole sequence of PHY initialization (reset
> > > > assert/deassert are done in order to help initialize PHY).
> > > > Something like below should work.
> > > 
> > > So the original patch was tested against hardware that only had USB 2.0
> > > support
> > > and seemed to work fine. However, we've since found an issue with some
> > > other
> > > hardware supporting USB 3.0 where either it doesn't detect devices at
> > > all,
> > > or
> > > they get detected but then seem to drop off the bus very quickly, and we
> > > get
> > > this repeatedly:
> > > 
> > > [   99.858607] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
> > > 
> > > The same problem is reproducible on the Xilinx ZCU102 board with the same
> > > kernel build, where the USB works fine with the Xilinx kernel and a
> > > Petalinux
> > > 2021.2 pre-built ZCU102 image, so it's no hardware issue.
> > > 
> > > I've been trying to isolate any relevant differences between the Xilinx
> > > kernel
> > > and mainline in this respect but haven't had much success. One difference
> > > in
> > > this particular dwc3-xilinx code is that the Xilinx kernel has code to
> > > reset
> > > the ULPI PHY which is not in the mainline version yet. However, adding
> > > that
> > > in
> > > doesn't seem to fix the problem.
> > > 
> > > Have you (or anyone else on the CC list) done any testing of USB 3.0
> > > devices
> > > and USB 3.0 capable hardware on mainline with ZynqMP to know if this is a
> > > more
> > > general issue?
> > 
> > Yes, The USB fixes are work in progress and will be sent to mainline in
> > near
> > future.
> > This fix is one of them. Your patch is solving the problem in case of USB
> > 2.0 
> > But not the USB 3.0 entirely. There is this corner case which I mentioned,
> > breaks
> > USB 3.0 functionality with your patch.
> 
> You mentioned the case where the design is using USB 3.0 but the usb3-phy is
> missing from the device tree - with this patch it would switch the clock
> source
> into the USB 2 configuration and might break things. I agree it's probably
> better to just skip the whole reset process and keep the FSBL clock selection
> if it's not needed in the USB 2 case.
> 
> We still have another case where USB 3.0 appears to be broken on a setup
> where
> the usb3-phy is specified, and where the Xilinx/Petalinux images work on the
> same hardware. I haven't yet isolated whether it is related to the kernel or
> might be caused by the different PL logic or PS configuration settings
> somehow.

Just to follow up, it appears this problem was due to a bug in the phy-zynqmp
driver corrupting the PS-GTR settings for other lanes when SGMII mode was
enabled on any lane. I'll be submitting a separate patch for this shortly. I'll
also update this patch set to skip the resets and USB3-only register settings
when no USB3 PHY is specified.

> 
> > Thanks,
> > Manish
> > 
> > > > ---
> > > >         int                     ret;
> > > >         u32                     reg;
> > > > 
> > > > -       usb3_phy = devm_phy_get(dev, "usb3-phy");
> > > > -       if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
> > > > -               ret = -EPROBE_DEFER;
> > > > +       usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
> > > > +       if (IS_ERR(usb3_phy)) {
> > > > +               ret = PTR_ERR(usb3_phy);
> > > > +               dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
> > > >                 goto err;
> > > > -       } else if (IS_ERR(usb3_phy)) {
> > > > -               usb3_phy = NULL;
> > > >         }
> > > > 
> > > > +       if (!usb3_phy)
> > > > +               goto skip_usb3_phy;
> > > > +
> > > >         crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> > > >         if (IS_ERR(crst)) {
> > > >                 ret = PTR_ERR(crst);
> > > > @@ -188,6 +190,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
> > > > *priv_data)
> > > >                 goto err;
> > > >         }
> > > > 
> > > > +skip_usb3_phy:
> > > >         /*
> > > >          * This routes the USB DMA traffic to go through FPD path
> > > > instead
> > > >          * of reaching DDR directly. This traffic routing is needed to
> > > > ---
> > > > 
> > > > Thanks,
> > > > Manish
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

[PATCH v6 2/2] usb: dwc3: xilinx: Fix error handling when getting USB3 PHY

[Robert Hancock]

The code that looked up the USB3 PHY was ignoring all errors other than
EPROBE_DEFER in an attempt to handle the PHY not being present. Fix and
simplify the code by using devm_phy_optional_get and dev_err_probe so
that a missing PHY is not treated as an error and unexpected errors
are handled properly.

Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/usb/dwc3/dwc3-xilinx.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index dd6218d05159..63490c261108 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -102,12 +102,11 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	int			ret;
 	u32			reg;
 
-	usb3_phy = devm_phy_get(dev, "usb3-phy");
-	if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) {
-		ret = -EPROBE_DEFER;
+	usb3_phy = devm_phy_optional_get(dev, "usb3-phy");
+	if (IS_ERR(usb3_phy)) {
+		ret = PTR_ERR(usb3_phy);
+		dev_err_probe(dev, ret, "failed to get USB3 PHY\n");
 		goto err;
-	} else if (IS_ERR(usb3_phy)) {
-		usb3_phy = NULL;
 	}
 
 	crst = devm_reset_control_get_exclusive(dev, "usb_crst");
-- 
2.31.1

Re: [PATCH v6 0/2] Xilinx ZynqMP USB fixes

[Sean Anderson]

Hi Robert,

On 1/21/22 1:18 PM, Robert Hancock wrote:
> Some fixes related to the Xilinx ZynqMP DWC3 wrapper driver to allow
> ZynqMP USB to work properly when the hardware is configured in USB
> 2.0-only mode.
> 
> 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
> 
> 
> Robert Hancock (2):
>    usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode
>    usb: dwc3: xilinx: Fix error handling when getting USB3 PHY
> 
>   drivers/usb/dwc3/dwc3-xilinx.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 

Looks like this got my usb working, thanks!

For ZynqMP,

Tested-by: Sean Anderson <sean.anderson@seco.com>