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