Re: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation

[Peter Chen <peter.chen@nxp.com>]

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?

> 
> > 
> > Reported-by: Ran Wang <ran.wang_1@nxp.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >   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