On Tue, 2022-01-25 at 05:36 +0000, Manish Narani wrote:
> Hi Robert,
>
> > -----Original Message-----
> > From: Robert Hancock <robert.hancock@xxxxxxxxxx>
> > Sent: Tuesday, January 25, 2022 12:31 AM
> > To: Manish Narani <MNARANI@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> > Cc: Michal Simek <michals@xxxxxxxxxx>; sean.anderson@xxxxxxxx;
> > gregkh@xxxxxxxxxxxxxxxxxxx; balbi@xxxxxxxxxx
> > 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@xxxxxxxxxx>
> > > > Sent: Friday, January 21, 2022 11:49 PM
> > > > To: linux-usb@xxxxxxxxxxxxxxx
> > > > Cc: balbi@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Michal Simek
> > > > <michals@xxxxxxxxxx>; Manish Narani <MNARANI@xxxxxxxxxx>;
> > > > sean.anderson@xxxxxxxx; Robert Hancock
> > <robert.hancock@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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
