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;
/* cached extended protocol port capabilities */
struct xhci_port_cap *port_caps;
unsigned int num_port_caps;
--
2.47.1
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;
>
--
cheers,
-roger
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
>
>
--
cheers,
-roger
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
On Wed Dec 18, 2024 at 6:49 PM CET, Théo Lebrun wrote:
> 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.
About this series, there were two discussions:
- The desire to avoid putting the hardware init sequence of cdns3-ti
inside runtime_resume() as we don't do runtime PM.
*That is fine and will be fixed for the next revision.*
See [PATCH V6 2/5]: https://lore.kernel.org/lkml/8a1ed4be-c41c-46b6-ae25-33a6035b8c8d@kernel.org/
- [PATCH V6 4/5] and [PATCH V6 5/5] are dedicated to avoiding a warning
at XHCI resume on J7200:
xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-4-28a17f9715a2@bootlin.com/
https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-5-28a17f9715a2@bootlin.com/
Roger Quadros asked if we should not instead keep it, as there is
indeed a reinit of the xHC block. I don't think we do: we don't want
a warning at resume; IMO it would imply the reinit was unexpected.
Proof is there is already a platform with a ->broken_suspend boolean
that disables the warning even though there is a reinit. It doesn't
log a warning as the reinit was expected.
So we currently have:
- xhci->broken_suspend: set at probe & implies the resume sequence
won't work.
- xhci->quirks & XHCI_RESET_ON_RESUME: set at probe & implies the
controller reset during suspend.
IIUC xhci->broken_suspend is NOT equivalent to "the controller reset
during suspend". Else we wouldn't have both the broken_suspend flag
and the XHCI_RESET_ON_RESUME quirk.
In our case we want exactly the same thing as the
XHCI_RESET_ON_RESUME quirk but updated at resume depending on what
the wrapper driver detects.
We could either:
1. Update xhci->quirks at resume from upper layers.
2. Introduce a xhci->lost_power flag. It would be strictly equivalent
to the XHCI_RESET_ON_RESUME quirk BUT updated at resume from
upper layers.
@Mathias Nyman: what is your thought on the matter? Option (2) was
the one taken in this series. Is there another option I am missing?
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 8.1.2025 12.59, Théo Lebrun wrote:
> On Wed Dec 18, 2024 at 6:49 PM CET, Théo Lebrun wrote:
>> 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.
>
> About this series, there were two discussions:
>
> - The desire to avoid putting the hardware init sequence of cdns3-ti
> inside runtime_resume() as we don't do runtime PM.
> *That is fine and will be fixed for the next revision.*
> See [PATCH V6 2/5]: https://lore.kernel.org/lkml/8a1ed4be-c41c-46b6-ae25-33a6035b8c8d@kernel.org/
>
> - [PATCH V6 4/5] and [PATCH V6 5/5] are dedicated to avoiding a warning
> at XHCI resume on J7200:
>
> xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
>
Adding a new quirk or private xhci_cd meme
> https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-4-28a17f9715a2@bootlin.com/
> https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-5-28a17f9715a2@bootlin.com/
>
> Roger Quadros asked if we should not instead keep it, as there is
> indeed a reinit of the xHC block. I don't think we do: we don't want
> a warning at resume; IMO it would imply the reinit was unexpected.
>
> Proof is there is already a platform with a ->broken_suspend boolean
> that disables the warning even though there is a reinit. It doesn't
> log a warning as the reinit was expected.
>
> So we currently have:
> - xhci->broken_suspend: set at probe & implies the resume sequence
> won't work.
> - xhci->quirks & XHCI_RESET_ON_RESUME: set at probe & implies the
> controller reset during suspend.
>
> IIUC xhci->broken_suspend is NOT equivalent to "the controller reset
> during suspend". Else we wouldn't have both the broken_suspend flag
> and the XHCI_RESET_ON_RESUME quirk.
>
> In our case we want exactly the same thing as the
> XHCI_RESET_ON_RESUME quirk but updated at resume depending on what
> the wrapper driver detects.
>
> We could either:
> 1. Update xhci->quirks at resume from upper layers.
> 2. Introduce a xhci->lost_power flag. It would be strictly equivalent
> to the XHCI_RESET_ON_RESUME quirk BUT updated at resume from
> upper layers.
>
> @Mathias Nyman: what is your thought on the matter? Option (2) was
> the one taken in this series. Is there another option I am missing?
This would be a fourth way the upper layers inform xhci_resume() that the
xHC host should be reset instead of restoring the registers.
option 1 creates the first dynamic xhci->quirk flag,
option 2 adds a xhci->lost_power flag that is touched outside of xhci driver.
Neither seem like a good idea just to get rid of a dev_warn() message.
Maybe its time to split xhci_resume() into xhci_reset_resume()
and xhci_restore_resume(), and let those upper layers decide what they need.
Doesn't cdns driver already have a xhci_plat_priv resume_quirk() function
called during xhci_plat_resume(), before xhci_resume()?
Can that be used?
Thanks
Mathias
Hello Mathias,
On Wed Jan 8, 2025 at 7:43 PM CET, Mathias Nyman wrote:
> This would be a fourth way the upper layers inform xhci_resume() that the
> xHC host should be reset instead of restoring the registers.
>
> option 1 creates the first dynamic xhci->quirk flag,
> option 2 adds a xhci->lost_power flag that is touched outside of xhci driver.
>
> Neither seem like a good idea just to get rid of a dev_warn() message.
>
> Maybe its time to split xhci_resume() into xhci_reset_resume()
> and xhci_restore_resume(), and let those upper layers decide what they need.
>
> Doesn't cdns driver already have a xhci_plat_priv resume_quirk() function
> called during xhci_plat_resume(), before xhci_resume()?
> Can that be used?
I agree with your feeling: another solution is needed. Discussing the
topic gave some new ideas and I have a new solution that feels much
more appropriate.
### Opinion on splitting xhci_resume() into two implementations
About splitting xhci_resume() into two different implementations that is
picked by above layer: I am not convinced.
What would go into xhci_reset_resume() and xhci_restore_resume()? There
isn't a clear cut inbetween the reset procedure and the normal restore
procedure, as we might do a reset if the normal restore procedure
fails (with some logging that reset was unexpected).
We would probably end up with many small functions called by either,
which would blur the overall step sequence.
### Proposal
Instead, I propose we keep xhci_resume() as a single function but change
its signature from the current:
int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);
To this:
int xhci_resume(struct xhci_hcd *xhci, bool power_lost,
bool is_auto_resume);
The key insight is that xhci_resume() extracts two information out of
the message:
- whether we are after hibernation: msg.event == PM_EVENT_RESTORE,
- whether this is an auto resume: msg.event == PM_EVENT_AUTO_RESUME.
First bulletpoint is somewhat wrong: driver wants to know if the device
did lose power, it doesn't care about hibernation per se. It just
happened to be that hibernation was the only case of power loss.
Knowing that, we can refactor to ask upper layers the right questions:
(1) "did we lose power?" and, (2) "is this an auto resume?".
Then, each caller is responsible for handing those booleans. If they
themselves receive pm_message_t messages (eg xhci-pci), then they do
the simple conversion:
bool power_lost = msg.event == PM_EVENT_RESTORE;
bool is_auto_resume = msg.event == PM_EVENT_AUTO_RESUME;
Others can do more more or less computation to pick a power_lost value.
### About xhci-plat power_lost value
In the case of xhci-plat, I think it will be:
- A new power_lost field in `struct xhci_plat_priv`.
- That gets set inside the cdns_role_driver::resume() callback of
drivers/usb/cdns3/host.c.
- Then xhci_plat_resume_common() computes power_lost being:
power_lost = is_restore || priv->power_lost;
### About xhci_plat_priv::resume_quirk()
It isn't really useful to use. drivers/usb/cdns3/host.c can know whether
power was lost through its USB role resume() callback. From there to
the resume_quirk(), the boolean must be stored somewhere. That
somewhere can only be... inside xhci_plat_priv. So it is simpler if
xhci-plat retrieves the boolean directly.
xhci_plat_priv::resume_quirk() can change the power_lost value if it
wants to, that is fine. But in our case, the info isn't available from
there.
###
I am unsure if the above explanation is clear, so I'll be sending my
current work-in-progress series as an answer to this. The goal being
that you can give me your opinion on the proposal: ACK and we continue
in this direction, NACK and we keep digging.
I'll wait for the merge window to end before sending a proper revision.
Thanks!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Compatible "marvell,armada3700-xhci" match data uses the
struct xhci_plat_priv::init_quirk() function pointer to add
XHCI_RESET_ON_RESUME as quirk on XHCI.
Instead, use the struct xhci_plat_priv::quirks field.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/host/xhci-mvebu.c | 10 ----------
drivers/usb/host/xhci-mvebu.h | 6 ------
drivers/usb/host/xhci-plat.c | 2 +-
3 files changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/usb/host/xhci-mvebu.c b/drivers/usb/host/xhci-mvebu.c
index 87f1597a0e5a..257e4d79971f 100644
--- a/drivers/usb/host/xhci-mvebu.c
+++ b/drivers/usb/host/xhci-mvebu.c
@@ -73,13 +73,3 @@ int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
return 0;
}
-
-int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
-{
- struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-
- /* Without reset on resume, the HC won't work at all */
- xhci->quirks |= XHCI_RESET_ON_RESUME;
-
- return 0;
-}
diff --git a/drivers/usb/host/xhci-mvebu.h b/drivers/usb/host/xhci-mvebu.h
index 3be021793cc8..9d26e22c4842 100644
--- a/drivers/usb/host/xhci-mvebu.h
+++ b/drivers/usb/host/xhci-mvebu.h
@@ -12,16 +12,10 @@ struct usb_hcd;
#if IS_ENABLED(CONFIG_USB_XHCI_MVEBU)
int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd);
-int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd);
#else
static inline int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
{
return 0;
}
-
-static inline int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
-{
- return 0;
-}
#endif
#endif /* __LINUX_XHCI_MVEBU_H */
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ecaa75718e59..49cc24e3ce23 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -106,7 +106,7 @@ static const struct xhci_plat_priv xhci_plat_marvell_armada = {
};
static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = {
- .init_quirk = xhci_mvebu_a3700_init_quirk,
+ .quirks = XHCI_RESET_ON_RESUME,
};
static const struct xhci_plat_priv xhci_plat_brcm = {
--
2.48.1
Unify naming convention: use `is_auto_runtime` in xhci-tegra, to be in
phase with (future) drivers/usb/host/xhci.c.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/host/xhci-tegra.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 76f228e7443c..94dd32f1a0da 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -2161,11 +2161,11 @@ static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra)
}
}
-static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime)
+static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
{
struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd);
struct device *dev = tegra->dev;
- bool wakeup = runtime ? true : device_may_wakeup(dev);
+ bool wakeup = is_auto_resume ? true : device_may_wakeup(dev);
unsigned int i;
int err;
u32 usbcmd;
@@ -2231,11 +2231,11 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime)
return err;
}
-static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime)
+static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
{
struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd);
struct device *dev = tegra->dev;
- bool wakeup = runtime ? true : device_may_wakeup(dev);
+ bool wakeup = is_auto_resume ? true : device_may_wakeup(dev);
unsigned int i;
u32 usbcmd;
int err;
@@ -2286,7 +2286,7 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime)
if (wakeup)
tegra_xhci_disable_phy_sleepwalk(tegra);
- err = xhci_resume(xhci, runtime ? PMSG_AUTO_RESUME : PMSG_RESUME);
+ err = xhci_resume(xhci, is_auto_resume ? PMSG_AUTO_RESUME : PMSG_RESUME);
if (err < 0) {
dev_err(tegra->dev, "failed to resume XHCI: %d\n", err);
goto disable_phy;
--
2.48.1
The device probe function mixes management code and hardware
initialisation code. Extract the latter into an explicitly named
cdns_ti_reset_and_init_hw() function to clarify intent. It also will
allow easier transition to using runtime PM for triggering HW init.
Reviewed-by: Roger Quadros <rogerq@kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/cdns3/cdns3-ti.c | 82 +++++++++++++++++++-----------------
1 file changed, 44 insertions(+), 38 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index cfabc12ee0e3..2863249665c2 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,7 @@ struct cdns_ti {
unsigned vbus_divider:1;
struct clk *usb2_refclk;
struct clk *lpm_clk;
+ int usb2_refclk_rate_code;
};
static const int cdns_ti_rate_table[] = { /* in KHZ */
@@ -98,15 +99,50 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
{},
};
+static void cdns_ti_reset_and_init_hw(struct cdns_ti *data)
+{
+ u32 reg;
+
+ /* assert RESET */
+ reg = cdns_ti_readl(data, USBSS_W1);
+ reg &= ~USBSS_W1_PWRUP_RST;
+ cdns_ti_writel(data, USBSS_W1, reg);
+
+ /* set static config */
+ reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+ reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
+ reg |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
+
+ reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
+ if (data->vbus_divider)
+ reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
+
+ cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
+ reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+
+ /* set USB2_ONLY mode if requested */
+ reg = cdns_ti_readl(data, USBSS_W1);
+ if (data->usb2_only)
+ reg |= USBSS_W1_USB2_ONLY;
+
+ /* set default modestrap */
+ reg |= USBSS_W1_MODESTRAP_SEL;
+ reg &= ~USBSS_W1_MODESTRAP_MASK;
+ reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
+ cdns_ti_writel(data, USBSS_W1, reg);
+
+ /* de-assert RESET */
+ reg |= USBSS_W1_PWRUP_RST;
+ cdns_ti_writel(data, USBSS_W1, reg);
+}
+
static int cdns_ti_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *node = pdev->dev.of_node;
struct cdns_ti *data;
- int error;
- u32 reg;
- int rate_code, i;
unsigned long rate;
+ int error, i;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -146,7 +182,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
return -EINVAL;
}
- rate_code = i;
+ data->usb2_refclk_rate_code = i;
+ data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
+ data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+
+ cdns_ti_reset_and_init_hw(data);
pm_runtime_enable(dev);
error = pm_runtime_get_sync(dev);
@@ -155,40 +195,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
goto err;
}
- /* assert RESET */
- reg = cdns_ti_readl(data, USBSS_W1);
- reg &= ~USBSS_W1_PWRUP_RST;
- cdns_ti_writel(data, USBSS_W1, reg);
-
- /* set static config */
- reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
- reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
- reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
-
- reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
- data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
- if (data->vbus_divider)
- reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
-
- cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
- reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-
- /* set USB2_ONLY mode if requested */
- reg = cdns_ti_readl(data, USBSS_W1);
- data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
- if (data->usb2_only)
- reg |= USBSS_W1_USB2_ONLY;
-
- /* set default modestrap */
- reg |= USBSS_W1_MODESTRAP_SEL;
- reg &= ~USBSS_W1_MODESTRAP_MASK;
- reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
- cdns_ti_writel(data, USBSS_W1, reg);
-
- /* de-assert RESET */
- reg |= USBSS_W1_PWRUP_RST;
- cdns_ti_writel(data, USBSS_W1, reg);
-
error = of_platform_populate(node, NULL, cdns_ti_auxdata, dev);
if (error) {
dev_err(dev, "failed to create children: %d\n", error);
--
2.48.1
At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
if an hardware reset occurred. If it did, run the hardware init sequence.
This callback will be called at system-wide resume. Previously, if a
reset occurred during suspend, we would crash. The wrapper config had
not been written, leading to invalid register accesses inside cdns3.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 2863249665c2..225993f7bdb6 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -186,6 +186,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+ /*
+ * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
+ * detects it as uninitialised. We want to enforce a reset at probe,
+ * and so do it manually here. This means the first runtime_resume()
+ * will be a no-op.
+ */
cdns_ti_reset_and_init_hw(data);
pm_runtime_enable(dev);
@@ -230,6 +236,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
platform_set_drvdata(pdev, NULL);
}
+static int cdns_ti_runtime_resume(struct device *dev)
+{
+ const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
+ struct cdns_ti *data = dev_get_drvdata(dev);
+ u32 w1;
+
+ w1 = cdns_ti_readl(data, USBSS_W1);
+ if ((w1 & mask) != mask)
+ cdns_ti_reset_and_init_hw(data);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+ RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+ SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
static const struct of_device_id cdns_ti_of_match[] = {
{ .compatible = "ti,j721e-usb", },
{ .compatible = "ti,am64-usb", },
@@ -243,6 +267,7 @@ static struct platform_driver cdns_ti_driver = {
.driver = {
.name = "cdns3-ti",
.of_match_table = cdns_ti_of_match,
+ .pm = pm_ptr(&cdns_ti_pm_ops),
},
};
--
2.48.1
The cdns_role_driver->resume() callback takes a second boolean argument
named `hibernated` in its implementations. This is mistaken; the only
potential caller is:
int cdns_resume(struct cdns *cdns)
{
/* ... */
if (cdns->roles[cdns->role]->resume)
cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
return 0;
}
The argument can be true in cases outside of return from hibernation.
Reflect the true meaning by renaming both arguments to `lost_power`.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/cdns3/cdns3-gadget.c | 4 ++--
drivers/usb/cdns3/cdnsp-gadget.c | 2 +-
drivers/usb/cdns3/core.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index fd1beb10bba7..694aa1457739 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -3468,7 +3468,7 @@ __must_hold(&cdns->lock)
return 0;
}
-static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdns3_gadget_resume(struct cdns *cdns, bool lost_power)
{
struct cdns3_device *priv_dev = cdns->gadget_dev;
@@ -3476,7 +3476,7 @@ static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
return 0;
cdns3_gadget_config(priv_dev);
- if (hibernated)
+ if (lost_power)
writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf);
return 0;
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index 4a3f0f958256..7d05180442fb 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -1973,7 +1973,7 @@ static int cdnsp_gadget_suspend(struct cdns *cdns, bool do_wakeup)
return 0;
}
-static int cdnsp_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdnsp_gadget_resume(struct cdns *cdns, bool lost_power)
{
struct cdnsp_device *pdev = cdns->gadget_dev;
enum usb_device_speed max_speed;
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 57d47348dc19..921cccf1ca9d 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -30,7 +30,7 @@ struct cdns_role_driver {
int (*start)(struct cdns *cdns);
void (*stop)(struct cdns *cdns);
int (*suspend)(struct cdns *cdns, bool do_wakeup);
- int (*resume)(struct cdns *cdns, bool hibernated);
+ int (*resume)(struct cdns *cdns, bool lost_power);
const char *name;
#define CDNS_ROLE_STATE_INACTIVE 0
#define CDNS_ROLE_STATE_ACTIVE 1
--
2.48.1
cdns_power_is_lost() does a register read.
Call it only once rather than twice.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/cdns3/core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 465e9267b49c..799987c88960 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -524,11 +524,12 @@ EXPORT_SYMBOL_GPL(cdns_suspend);
int cdns_resume(struct cdns *cdns)
{
+ bool power_lost = cdns_power_is_lost(cdns);
enum usb_role real_role;
bool role_changed = false;
int ret = 0;
- if (cdns_power_is_lost(cdns)) {
+ if (power_lost) {
if (cdns->role_sw) {
cdns->role = cdns_role_get(cdns->role_sw);
} else {
@@ -553,7 +554,7 @@ int cdns_resume(struct cdns *cdns)
}
if (cdns->roles[cdns->role]->resume)
- cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
+ cdns->roles[cdns->role]->resume(cdns, power_lost);
return 0;
}
--
2.48.1
Previous signature was:
int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);
Internally, it extracted two information out of the message:
- whether we are after hibernation: msg.event == PM_EVENT_RESTORE,
- whether this is an auto resume: msg.event == PM_EVENT_AUTO_RESUME.
First bulletpoint is somewhat wrong: driver wants to know if the device
did lose power, it doesn't care about hibernation per se. Knowing that,
refactor to ask upper layers the right questions: (1) "did we lose
power?" and, (2) "is this an auto resume?". Change the signature to:
int xhci_resume(struct xhci_hcd *xhci, bool power_lost,
bool is_auto_resume);
The goal is to allow some upper layers (cdns3-plat) to tell us when
power was lost after system-wise suspend.
Note that lost_power is ORed at the start of xhci_resume() to
xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend. It is
simpler to keep those checks inside of xhci_resume() instead of doing
them at each caller of xhci_resume().
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/host/xhci-histb.c | 2 +-
drivers/usb/host/xhci-pci.c | 8 +++++---
drivers/usb/host/xhci-plat.c | 10 +++++-----
drivers/usb/host/xhci-tegra.c | 2 +-
drivers/usb/host/xhci.c | 19 ++++++++-----------
drivers/usb/host/xhci.h | 2 +-
6 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c
index f9a4a4b0eb57..dde2ae0e9a0f 100644
--- a/drivers/usb/host/xhci-histb.c
+++ b/drivers/usb/host/xhci-histb.c
@@ -355,7 +355,7 @@ static int __maybe_unused xhci_histb_resume(struct device *dev)
if (!device_may_wakeup(dev))
xhci_histb_host_enable(histb);
- return xhci_resume(xhci, PMSG_RESUME);
+ return xhci_resume(xhci, false, false);
}
static const struct dev_pm_ops xhci_histb_pm_ops = {
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index cb07cee9ed0c..6362a8d9a74d 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -807,8 +807,10 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
{
- struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+ bool power_lost = msg.event == PM_EVENT_RESTORE;
+ bool is_auto_resume = msg.event == PM_EVENT_AUTO_RESUME;
reset_control_reset(xhci->reset);
@@ -839,7 +841,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
xhci_pme_quirk(hcd);
- return xhci_resume(xhci, msg);
+ return xhci_resume(xhci, power_lost, is_auto_resume);
}
static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 49cc24e3ce23..831af518a6ec 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -476,7 +476,7 @@ static int xhci_plat_suspend(struct device *dev)
return 0;
}
-static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
+static int xhci_plat_resume_common(struct device *dev, bool power_lost)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
@@ -498,7 +498,7 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
if (ret)
goto disable_clks;
- ret = xhci_resume(xhci, pmsg);
+ ret = xhci_resume(xhci, power_lost, false);
if (ret)
goto disable_clks;
@@ -519,12 +519,12 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
static int xhci_plat_resume(struct device *dev)
{
- return xhci_plat_resume_common(dev, PMSG_RESUME);
+ return xhci_plat_resume_common(dev, false);
}
static int xhci_plat_restore(struct device *dev)
{
- return xhci_plat_resume_common(dev, PMSG_RESTORE);
+ return xhci_plat_resume_common(dev, true);
}
static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
@@ -545,7 +545,7 @@ static int __maybe_unused xhci_plat_runtime_resume(struct device *dev)
struct usb_hcd *hcd = dev_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- return xhci_resume(xhci, PMSG_AUTO_RESUME);
+ return xhci_resume(xhci, false, true);
}
const struct dev_pm_ops xhci_plat_pm_ops = {
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 94dd32f1a0da..74a4a2719e4f 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -2286,7 +2286,7 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
if (wakeup)
tegra_xhci_disable_phy_sleepwalk(tegra);
- err = xhci_resume(xhci, is_auto_resume ? PMSG_AUTO_RESUME : PMSG_RESUME);
+ err = xhci_resume(xhci, false, is_auto_resume);
if (err < 0) {
dev_err(tegra->dev, "failed to resume XHCI: %d\n", err);
goto disable_phy;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 899c0effb5d3..ccdc74b24d3f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1001,16 +1001,14 @@ EXPORT_SYMBOL_GPL(xhci_suspend);
* This is called when the machine transition from S3/S4 mode.
*
*/
-int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
+int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
{
- bool hibernated = (msg.event == PM_EVENT_RESTORE);
u32 command, temp = 0;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
int retval = 0;
bool comp_timer_running = false;
bool pending_portevent = false;
bool suspended_usb3_devs = false;
- bool reinit_xhc = false;
if (!hcd->state)
return 0;
@@ -1029,10 +1027,10 @@ 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)
- reinit_xhc = true;
+ if (xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
+ power_lost = true;
- if (!reinit_xhc) {
+ if (!power_lost) {
/*
* Some controllers might lose power during suspend, so wait
* for controller not ready bit to clear, just as in xHC init.
@@ -1072,12 +1070,12 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
/* 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)
+ if (!power_lost)
xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
+ power_lost = true;
}
- if (reinit_xhc) {
+ if (power_lost) {
if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
!(xhci_all_ports_seen_u0(xhci))) {
del_timer_sync(&xhci->comp_mode_recovery_timer);
@@ -1175,8 +1173,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
pending_portevent = xhci_pending_portevent(xhci);
- if (suspended_usb3_devs && !pending_portevent &&
- msg.event == PM_EVENT_AUTO_RESUME) {
+ if (suspended_usb3_devs && !pending_portevent && is_auto_resume) {
msleep(120);
pending_portevent = xhci_pending_portevent(xhci);
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f0fb696d5619..4f045a864ac1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1859,7 +1859,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
int xhci_ext_cap_init(struct xhci_hcd *xhci);
int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
-int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);
+int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume);
irqreturn_t xhci_irq(struct usb_hcd *hcd);
irqreturn_t xhci_msi_irq(int irq, void *hcd);
--
2.48.1
Now that xhci_resume() exposes a power_lost boolean argument, expose
that to all xhci-plat implementations. They are free to set it from
wherever they want:
- Their own resume() callback.
- The xhci_plat_priv::resume_quirk() callback.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/host/xhci-plat.c | 3 ++-
drivers/usb/host/xhci-plat.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 831af518a6ec..8b18494ccc41 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -479,6 +479,7 @@ static int xhci_plat_suspend(struct device *dev)
static int xhci_plat_resume_common(struct device *dev, bool power_lost)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
+ struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int ret;
@@ -498,7 +499,7 @@ static int xhci_plat_resume_common(struct device *dev, bool power_lost)
if (ret)
goto disable_clks;
- ret = xhci_resume(xhci, power_lost, false);
+ ret = xhci_resume(xhci, power_lost || priv->power_lost, false);
if (ret)
goto disable_clks;
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 6475130eac4b..fe4f95e690fa 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -15,6 +15,7 @@ struct usb_hcd;
struct xhci_plat_priv {
const char *firmware_name;
unsigned long long quirks;
+ bool power_lost;
void (*plat_start)(struct usb_hcd *);
int (*init_quirk)(struct usb_hcd *);
int (*suspend_quirk)(struct usb_hcd *);
--
2.48.1
cdns3-plat can know if power was lost across system-wide suspend.
Forward that information:
- Grab the lost_power bool from cdns_role_driver::resume(). Store it
into the power_lost field in struct xhci_plat_priv.
- xhci-plat will call xhci_resume() with that value (ORed to whether we
are in a hibernation restore).
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/usb/cdns3/host.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 7ba760ee62e3..f0df114c2b53 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -138,6 +138,16 @@ static void cdns_host_exit(struct cdns *cdns)
cdns_drd_host_off(cdns);
}
+static int cdns_host_resume(struct cdns *cdns, bool power_lost)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
+ struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+ priv->power_lost = power_lost;
+
+ return 0;
+}
+
int cdns_host_init(struct cdns *cdns)
{
struct cdns_role_driver *rdrv;
@@ -148,6 +158,7 @@ int cdns_host_init(struct cdns *cdns)
rdrv->start = __cdns_host_init;
rdrv->stop = cdns_host_exit;
+ rdrv->resume = cdns_host_resume;
rdrv->state = CDNS_ROLE_STATE_INACTIVE;
rdrv->name = "host";
--
2.48.1
© 2016 - 2025 Red Hat, Inc.