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?
> ---
> 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
