Sorry for the delay response.
On Fri, Oct 13, 2023, Ladislav Michl wrote:
> On Thu, Oct 12, 2023 at 10:23:30PM +0000, Thinh Nguyen wrote:
> > On Thu, Oct 12, 2023, Ladislav Michl wrote:
> > > From: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
> > >
> > > Implement workaround for Octeon Known Issue Id 29206:
> > > | The USB high speed logic contains a PLL that must lock during
> > > | initialization for correct operation. In very rare circumstances,
> > > | it is possible for the PLL to fail to start correctly.
> > > | Workaround
> > > | After initialization, check the USB PLL lock register via the
> > > | UPHY CR interface. If the PLL is not running, power it down and
> > > | back up and restart the initialization.
> >
> > Minor nit: Can we replace "|" with tabs. I think it's easier to read.
>
> Ok.
>
> > > PLL initialization code taken from Cavium's vendor bootloader:
> > > u-boot/drivers/usb/host/xhci-octeon.c:octeon3_usb_clocks_start
> > >
> > > Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
> > > ---
> > > NOTE:
> > > This patch fixes initialization issue found on some CN7020 based boards.
> > > Without this patch, controller sometimes fails to detect devices connected.
> > > Original code comes from Cavium released u-boot monster patch, which seems
> > > to suffer from mistakes made while resolving merge conflicts when upgrading
> > > to newer u-boot.
> > > Testing revealed that only single reinit is needed to properly lock PLL,
> > > this agrees with comment in Cavium's u-boot code, which is claiming the
> > > same. However, same as in u-boot code, reinit is attempted three times.
> > > (in could be done using while loop instead of goto, just let me know
> > > which way do you prefer)
> > > SoCs suffering from this problem would fail to initialize PHY about
> > > several tens times of thousand boots. This patch always restored
> > > functional state.
> >
> > What kernel version did you use? Perhaps it has the same issue due to
> > the commit e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was
> > configured as host-only")
>
> Production devices are running 6.1.y, patch was developed and tested against
> 6.1.38, then ported to 6.6-rc5. The purpose of this RFC was to figure out
> how to handle situation when informations are comming from NDA document
> and code is based on the one coming from unpublished vendor tree.
>
> > Did you test this against Greg's usb-linus branch with the fix for the
> > above?
>
> Do you mean "usb: dwc3: Soft reset phy on probe for host" patch? I had
> noticed it, however this patch was not tested with that fix.
Ok.
>
> Cavium's u-boot is also resetting PHYs similar way it is done there,
> but I ommited this change as testing revealed it is not needed.
>
> Please see dwc3_uphy_pll_reset function and comment above it.
> Without hardware documentation I can only guess whenever it makes sense.
>
I have much less knowledge of this platform than you. As long as you
document your findings and how you came to the logic for the patch as
you did. It should be fine.
I notice that there are debug comments, and this looks like a
work-in-progress. When it's ready, please clean it up and remove the RFC
so we can merge it.
Thanks,
Thinh
