HI Peter, Mathias,
Sorry for the late review.
On Friday, August 23, 2019 09:02, Peter Chen wrote:
>
> On 19-08-22 14:08:26, Mathias Nyman wrote:
> > On 21.8.2019 6.18, Peter Chen wrote:
> > > According to xHCI 1.1 CH4.19.4 Port Power:
> > > While Chip Hardware Reset or HCRST is asserted,
> > > the value of PP is undefined. If the xHC supports
> > > power switches (PPC = ‘1’) then VBus may be deasserted
> > > during this time. PP (and VBus) shall be enabled immediately
> > > upon exiting the reset condition.
> > >
> > > The VBus glitch may cause some USB devices work abnormal, we observe
> > > it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB platforms.
> To
> > > avoid this Vbus glitch, we could set PP as 0 before HCRST, and the
> > > PP will back to 1 after HCRST according to spec.
> >
> > Is this glitch causing issues already at the first xHC reset when we
> > are loading the xhci driver, or only later resets, like xHC reset at resume?
>
> The first time, Ran would you please confirm?
My understand is this will happened whenever PP is set to 0, such as xHCI reset.
So I think it might also be observed during resume if xHC do reset.
However, according to my previous testing, it might be too late to
do this port power off in xhci_reset(), actually the VBUS will assert once DWC3 driver
set IP to host mode (before doing xHC reset). So the glitch still can be observed on the scope;
I have more issue description in another patch discussion about this, please see
lore.kernel.org/patchwork/patch/1032425/
Quoted from it:
Actually I have done experiment like what you suggested (in xhci-plat.c), but the timing
seems too late--making VBUS waveform look like a square wave as below:
Here DWC3 enable host mode, VBUS on
|
+5V /---------\ 40ms /---------------------------....
0V ________/ 90ms \______/
| |
| Here do xhci reset, VBUS back to +5V again
Here set all PORTSC[PP] to 0 in xhci_gen_setup()
So I am afraid the solution might have to be added in DWC3 core driver where just after host mode enabling code if want fix this :(
Regards,
Ran
> >
> > >
> > > Reported-by: Ran Wang <ran.wang_1@xxxxxxx>
> > > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> > > ---
> > > drivers/usb/host/xhci.c | 15 ++++++++++++++-
> > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
> > > 6b34a573c3d9..f5a7b5d63031 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
> > > {
> > > u32 command;
> > > u32 state;
> > > - int ret;
> > > + int ret, i;
> > > + u32 portsc;
> > > state = readl(&xhci->op_regs->status); @@ -181,6 +182,18 @@ int
> > > xhci_reset(struct xhci_hcd *xhci)
> > > return 0;
> > > }
> > > + /*
> > > + * Keep PORTSC.PP as 0 before HCRST to eliminate
> > > + * Vbus glitch, see CH 4.19.4.
> > > + */
> > > + for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> > > + __le32 __iomem *port_addr = &xhci->op_regs-
> >port_status_base +
> > > + NUM_PORT_REGS * i;
> > > + portsc = readl(port_addr);
> > > + portsc &= ~PORT_POWER;
> > > + writel(portsc, port_addr);
> >
> > Not all bits read from PORTSC should be written back, you might need
> > portsc = xhci_port_state_to_neutral(portsc) here.
>
> Will change.
>
> >
> > Normally I'd recommend using the xhci_set_port_power() helper instead,
> > but if this is is needed at driver load time then port arrays may not be set up
> yet.
> > xhci_set_port_power() would take care of possible ACPI method calls, and add
> some debugging.
> >
>
> It is needed at load time, so I did not use port array.
>
> > Not sure if this will impact some other platforms, maybe it would be
> > better to move this to a separate function, and call it before xhci_reset() if a
> quirk is set.
> >
>
> It follows spec, not at quirk. We talked with Synopsis engineer
> (case: 8001235479), they confirmed this behaviour follows spec.
> Besides, I tried at both dwc3 and cadence3 xHCI platforms, the PORT_POWER
> will be set again after controller set.
>
> What's potential issue you consider?
>
> Thanks,
> Peter
