| Message ID | 20190821031602.1030-1-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] usb: xhci: avoid VBus glitch during controller reset operation | expand |
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? > > 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. 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. 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. -Mathias
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
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@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
On 19-08-23 01:59:24, Ran Wang wrote: > 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. Yes, Vbus glitch should exists whenever we do controller reset, I thought you only meet this issue during boots up. > > 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 :( > Per spec 4.19.4 Port Power: Whether an xHC implementation supports port power switches or not, it shall automatically enable VBus on all Root Hub ports after a Chip Hardware Reset or HCRST. Ran's observation confirmed it, PP is asserted once xHCI is enabled. From the code, the HCRST will be at boots up and system resume. So, it seems we can't keep PP always asserted. For xHCI, to avoid Vbus glitch totally, we may have to control Vbus without PP (eg, configure PP pin as GPIO). Peter > > Regards, > Ran > > > > > > > > > > > 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
Hi Peter, On Friday, August 23, 2019 11:34, Peter Chen wrote: > > On 19-08-23 01:59:24, Ran Wang wrote: > > 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. > > Yes, Vbus glitch should exists whenever we do controller reset, I thought you > only meet this issue during boots up. > > > > > 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 :( > > > > Per spec 4.19.4 Port Power: > > Whether an xHC implementation supports port power switches or not, it shall > automatically enable VBus on all Root Hub ports after a Chip Hardware Reset or > HCRST. > > Ran's observation confirmed it, PP is asserted once xHCI is enabled. > From the code, the HCRST will be at boots up and system resume. > So, it seems we can't keep PP always asserted. For xHCI, to avoid Vbus glitch > totally, we may have to control Vbus without PP (eg, configure PP pin as GPIO). Yes, another option is to design a better VBUS driving circuit on HW side to filter this glitch out. I have found some earlier version of LS1043ARDB board design is perfect on doing this. Anyway, for boot, my suggestion is to do it in DWC3 driver once after enabling host mode. But that solution is ugly, I have to admit. And for resume, frankly I didn’t notice this before (thanks for remind), would do further survey if can found a similar solution. Regards, Ran > > > > > > > > > > > > > > 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
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); + } + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Reset the HC"); command = readl(&xhci->op_regs->command); command |= CMD_RESET;
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. 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(-)