diff mbox series

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

Message ID 20220121181841.2331225-2-robert.hancock@calian.com
State Superseded
Headers show
Series Xilinx ZynqMP USB fixes | expand

Commit Message

Robert Hancock Jan. 21, 2022, 6:18 p.m. UTC
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(-)

Comments

Manish Narani Jan. 24, 2022, 6:55 a.m. UTC | #1
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
Robert Hancock Jan. 24, 2022, 7 p.m. UTC | #2
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
Robert Hancock Jan. 25, 2022, 4:19 p.m. UTC | #3
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
Sean Anderson Jan. 25, 2022, 4:26 p.m. UTC | #4
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
Robert Hancock Jan. 25, 2022, 11:22 p.m. UTC | #5
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
diff mbox series

Patch

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