| Message ID | 20241210-s2r-cdns-v6-4-28a17f9715a2@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci) | expand |
On 10/12/2024 19:13, Théo Lebrun wrote: > The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they > expect a reset after resume. It is also used by some to enforce a XHCI > reset on resume (see needs-reset-on-resume DT prop). > > Some wrappers are unsure beforehands if they will reset. Add a mechanism > to signal *at resume* if power has been lost. Parent devices can set > this flag, that defaults to false. > > The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the > controller. This is required as we do not know if a suspend will > trigger a reset, so the best guess is to avoid runtime PM. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/host/xhci.c | 3 ++- > drivers/usb/host/xhci.h | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg) > > spin_lock_irq(&xhci->lock); > > - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend) > + if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || > + xhci->broken_suspend || xhci->lost_power) > reinit_xhc = true; > > if (!reinit_xhc) { > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1645,6 +1645,12 @@ struct xhci_hcd { > unsigned broken_suspend:1; > /* Indicates that omitting hcd is supported if root hub has no ports */ > unsigned allow_single_roothub:1; > + /* > + * Signal from upper stacks that we lost power during system-wide > + * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning > + * it is safe for wrappers to not modify lost_power at resume. > + */ > + unsigned lost_power:1; I suppose this is private to XHCI driver and not legitimate to be accessed by another driver after HCD is instantiated? Doesn't access to xhci_hcd need to be serialized via xhci->lock? Just curious, what happens if you don't include patch 4 and 5? Is USB functionality still broken for you? Doesn't XHCI driver detect that power was lost and issue a reset in any case via the below code /* re-initialize the HC on Restore Error, or Host Controller Error */ if ((temp & (STS_SRE | STS_HCE)) && !(xhci->xhc_state & XHCI_STATE_REMOVING)) { reinit_xhc = true; if (!xhci->broken_suspend) xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp); } > /* cached extended protocol port capabilities */ > struct xhci_port_cap *port_caps; > unsigned int num_port_caps; >
On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote: > On 10/12/2024 19:13, Théo Lebrun wrote: > > The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they > > expect a reset after resume. It is also used by some to enforce a XHCI > > reset on resume (see needs-reset-on-resume DT prop). > > > > Some wrappers are unsure beforehands if they will reset. Add a mechanism > > to signal *at resume* if power has been lost. Parent devices can set > > this flag, that defaults to false. > > > > The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the > > controller. This is required as we do not know if a suspend will > > trigger a reset, so the best guess is to avoid runtime PM. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/host/xhci.c | 3 ++- > > drivers/usb/host/xhci.h | 6 ++++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg) > > > > spin_lock_irq(&xhci->lock); > > > > - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend) > > + if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || > > + xhci->broken_suspend || xhci->lost_power) > > reinit_xhc = true; > > > > if (!reinit_xhc) { > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > > index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644 > > --- a/drivers/usb/host/xhci.h > > +++ b/drivers/usb/host/xhci.h > > @@ -1645,6 +1645,12 @@ struct xhci_hcd { > > unsigned broken_suspend:1; > > /* Indicates that omitting hcd is supported if root hub has no ports */ > > unsigned allow_single_roothub:1; > > + /* > > + * Signal from upper stacks that we lost power during system-wide > > + * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning > > + * it is safe for wrappers to not modify lost_power at resume. > > + */ > > + unsigned lost_power:1; > > I suppose this is private to XHCI driver and not legitimate to be accessed > by another driver after HCD is instantiated? Yes it is private. > Doesn't access to xhci_hcd need to be serialized via xhci->lock? Good question. In theory maybe. In practice I don't see how cdns_host_resume(), called by cdns_resume(), could clash with anything else. I'll add that to be safe. > Just curious, what happens if you don't include patch 4 and 5? > Is USB functionality still broken for you? No it works fine. Patches 4+5 are only there to avoid the below warning. Logging "xHC error in resume" is a lie, so I want to avoid it. > Doesn't XHCI driver detect that power was lost and issue a reset in any case > via the below code > > /* re-initialize the HC on Restore Error, or Host Controller Error */ > if ((temp & (STS_SRE | STS_HCE)) && > !(xhci->xhc_state & XHCI_STATE_REMOVING)) { > reinit_xhc = true; > if (!xhci->broken_suspend) > xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp); > } > > > /* cached extended protocol port capabilities */ > > struct xhci_port_cap *port_caps; > > unsigned int num_port_caps; > > I'll wait for your opinion on the [PATCH v6 2/5] email thread before sending a new revision. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 13/12/2024 18:03, Théo Lebrun wrote: > On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote: >> On 10/12/2024 19:13, Théo Lebrun wrote: >>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they >>> expect a reset after resume. It is also used by some to enforce a XHCI >>> reset on resume (see needs-reset-on-resume DT prop). >>> >>> Some wrappers are unsure beforehands if they will reset. Add a mechanism >>> to signal *at resume* if power has been lost. Parent devices can set >>> this flag, that defaults to false. >>> >>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the >>> controller. This is required as we do not know if a suspend will >>> trigger a reset, so the best guess is to avoid runtime PM. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> drivers/usb/host/xhci.c | 3 ++- >>> drivers/usb/host/xhci.h | 6 ++++++ >>> 2 files changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644 >>> --- a/drivers/usb/host/xhci.c >>> +++ b/drivers/usb/host/xhci.c >>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg) >>> >>> spin_lock_irq(&xhci->lock); >>> >>> - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend) >>> + if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || >>> + xhci->broken_suspend || xhci->lost_power) >>> reinit_xhc = true; >>> >>> if (!reinit_xhc) { >>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644 >>> --- a/drivers/usb/host/xhci.h >>> +++ b/drivers/usb/host/xhci.h >>> @@ -1645,6 +1645,12 @@ struct xhci_hcd { >>> unsigned broken_suspend:1; >>> /* Indicates that omitting hcd is supported if root hub has no ports */ >>> unsigned allow_single_roothub:1; >>> + /* >>> + * Signal from upper stacks that we lost power during system-wide >>> + * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning >>> + * it is safe for wrappers to not modify lost_power at resume. >>> + */ >>> + unsigned lost_power:1; >> >> I suppose this is private to XHCI driver and not legitimate to be accessed >> by another driver after HCD is instantiated? > > Yes it is private. > >> Doesn't access to xhci_hcd need to be serialized via xhci->lock? > > Good question. In theory maybe. In practice I don't see how > cdns_host_resume(), called by cdns_resume(), could clash with anything > else. I'll add that to be safe. > >> Just curious, what happens if you don't include patch 4 and 5? >> Is USB functionality still broken for you? > > No it works fine. Patches 4+5 are only there to avoid the below warning. > Logging "xHC error in resume" is a lie, so I want to avoid it. How is it a lie? The XHCI controller did loose its save/restore state during a PM operation. As far as XHCI is concerned this is an error. no? > >> Doesn't XHCI driver detect that power was lost and issue a reset in any case >> via the below code >> >> /* re-initialize the HC on Restore Error, or Host Controller Error */ >> if ((temp & (STS_SRE | STS_HCE)) && >> !(xhci->xhc_state & XHCI_STATE_REMOVING)) { >> reinit_xhc = true; >> if (!xhci->broken_suspend) >> xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp); >> } >> >>> /* cached extended protocol port capabilities */ >>> struct xhci_port_cap *port_caps; >>> unsigned int num_port_caps; >>> > > I'll wait for your opinion on the [PATCH v6 2/5] email thread before > sending a new revision. Sorry for the delay. I'm not an expert in PM but will give my opinion there anyways. > > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > >
Hello Roger, Peter, Pawel, Greg, Mathias, On Tue Dec 17, 2024 at 10:00 PM CET, Roger Quadros wrote: > > > On 13/12/2024 18:03, Théo Lebrun wrote: > > On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote: > >> On 10/12/2024 19:13, Théo Lebrun wrote: > >>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they > >>> expect a reset after resume. It is also used by some to enforce a XHCI > >>> reset on resume (see needs-reset-on-resume DT prop). > >>> > >>> Some wrappers are unsure beforehands if they will reset. Add a mechanism > >>> to signal *at resume* if power has been lost. Parent devices can set > >>> this flag, that defaults to false. > >>> > >>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the > >>> controller. This is required as we do not know if a suspend will > >>> trigger a reset, so the best guess is to avoid runtime PM. > >>> > >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > >>> --- > >>> drivers/usb/host/xhci.c | 3 ++- > >>> drivers/usb/host/xhci.h | 6 ++++++ > >>> 2 files changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > >>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644 > >>> --- a/drivers/usb/host/xhci.c > >>> +++ b/drivers/usb/host/xhci.c > >>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg) > >>> > >>> spin_lock_irq(&xhci->lock); > >>> > >>> - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend) > >>> + if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || > >>> + xhci->broken_suspend || xhci->lost_power) > >>> reinit_xhc = true; > >>> > >>> if (!reinit_xhc) { > >>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > >>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644 > >>> --- a/drivers/usb/host/xhci.h > >>> +++ b/drivers/usb/host/xhci.h > >>> @@ -1645,6 +1645,12 @@ struct xhci_hcd { > >>> unsigned broken_suspend:1; > >>> /* Indicates that omitting hcd is supported if root hub has no ports */ > >>> unsigned allow_single_roothub:1; > >>> + /* > >>> + * Signal from upper stacks that we lost power during system-wide > >>> + * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning > >>> + * it is safe for wrappers to not modify lost_power at resume. > >>> + */ > >>> + unsigned lost_power:1; > >> > >> I suppose this is private to XHCI driver and not legitimate to be accessed > >> by another driver after HCD is instantiated? > > > > Yes it is private. > > > >> Doesn't access to xhci_hcd need to be serialized via xhci->lock? > > > > Good question. In theory maybe. In practice I don't see how > > cdns_host_resume(), called by cdns_resume(), could clash with anything > > else. I'll add that to be safe. > > > >> Just curious, what happens if you don't include patch 4 and 5? > >> Is USB functionality still broken for you? > > > > No it works fine. Patches 4+5 are only there to avoid the below warning. > > Logging "xHC error in resume" is a lie, so I want to avoid it. > > How is it a lie? > The XHCI controller did loose its save/restore state during a PM operation. > As far as XHCI is concerned this is an error. no? The `xhci->quirks & XHCI_RESET_ON_RESUME` is exactly the same thing; both the quirk and the flag we add have for purpose: 1. skipping this block if (!reinit_xhc) { retval = xhci_handshake(&xhci->op_regs->status, STS_CNR, 0, 10 * 1000 * 1000); // ... xhci_restore_registers(xhci); xhci_set_cmd_ring_deq(xhci); command = readl(&xhci->op_regs->command); command |= CMD_CRS; writel(command, &xhci->op_regs->command); if (xhci_handshake(&xhci->op_regs->status, STS_RESTORE, 0, 100 * 1000)) { // ... } } 2. avoiding this warning: xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp); I don't think the block skipped is important in resume duration (to be confirmed). But the xhci_warn() is not desired: we do not want to log warnings if we know it is expected. I'll think some more about it. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg) spin_lock_irq(&xhci->lock); - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend) + if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || + xhci->broken_suspend || xhci->lost_power) reinit_xhc = true; if (!reinit_xhc) { diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1645,6 +1645,12 @@ struct xhci_hcd { unsigned broken_suspend:1; /* Indicates that omitting hcd is supported if root hub has no ports */ unsigned allow_single_roothub:1; + /* + * Signal from upper stacks that we lost power during system-wide + * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning + * it is safe for wrappers to not modify lost_power at resume. + */ + unsigned lost_power:1; /* cached extended protocol port capabilities */ struct xhci_port_cap *port_caps; unsigned int num_port_caps;
The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they expect a reset after resume. It is also used by some to enforce a XHCI reset on resume (see needs-reset-on-resume DT prop). Some wrappers are unsure beforehands if they will reset. Add a mechanism to signal *at resume* if power has been lost. Parent devices can set this flag, that defaults to false. The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the controller. This is required as we do not know if a suspend will trigger a reset, so the best guess is to avoid runtime PM. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/host/xhci.c | 3 ++- drivers/usb/host/xhci.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)