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@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.
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
