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