| Message ID | 20220121181841.2331225-2-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xilinx ZynqMP USB fixes | expand |
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
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
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
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
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
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 --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) {
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(-)