Re: [PATCH v6 1/2] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode

[Sean Anderson <sean.anderson@seco.com>]

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