| Message ID | 20240726-s2r-cdns-v5-9-8664bfb032ac@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci) | expand |
On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value. > > 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. > > Reset the xhci->lost_power value each time in xhci_resume(), making it > safe for devices to only set lost_power on some resumes. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/host/xhci.c | 8 +++++++- > drivers/usb/host/xhci.h | 6 ++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 0a8cf6c17f82..2c9b32d339f9 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend) Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently? XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume. xhci->lost_power is used by devices that don't have consistent behavior. > reinit_xhc = true; > > + /* Reset to default value, parent devices might correct it at next resume. */ > + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); > + then you don't need to do this. > if (!reinit_xhc) { > /* > * Some controllers might lose power during suspend, so wait > @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) > if (get_quirks) > get_quirks(dev, xhci); > > + /* Default value, that can be corrected at resume. */ > + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); > + nor this. > /* In xhci controllers which follow xhci 1.0 spec gives a spurious > * success event after a short transfer. This quirk will ignore such > * spurious event. > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index ebd0afd59a60..ec7c6061363f 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1640,6 +1640,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; >
On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote: > On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value. > > > > 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. > > > > Reset the xhci->lost_power value each time in xhci_resume(), making it > > safe for devices to only set lost_power on some resumes. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/host/xhci.c | 8 +++++++- > > drivers/usb/host/xhci.h | 6 ++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 0a8cf6c17f82..2c9b32d339f9 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend) > > Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently? > > XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume. > > xhci->lost_power is used by devices that don't have consistent behavior. The goal is to avoid almost-duplicate functionality. I feel like: XHCI_RESET_ON_RESUME is the default value of xhci->lost_power, which might be modified at resume. Is a more straight forward solution than: Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was lost at resume. First must be statically known, second can be updated during runtime. If second is used, first one must NOT be set. Indeed, the first solution brings two additional lines of code as you commented below. I'd argue the easier-to-wrap-your-head-around logic is more important. Tell me if you are convinced the second approach is better. > > > > reinit_xhc = true; > > > > + /* Reset to default value, parent devices might correct it at next resume. */ > > + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); > > + > > then you don't need to do this. To be honest, I added this line out of rigor. We could remove it and say that any device that modifies xhci->lost_power once at resume must set it at each later resume. The above line felt like a small safety net to avoid logic mistakes. > > > if (!reinit_xhc) { > > /* > > * Some controllers might lose power during suspend, so wait > > @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) > > if (get_quirks) > > get_quirks(dev, xhci); > > > > + /* Default value, that can be corrected at resume. */ > > + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); > > + > > nor this. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello Théo, On 10/09/2024 16:50, Théo Lebrun wrote: > On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote: >> On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value. >>> >>> 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. >>> >>> Reset the xhci->lost_power value each time in xhci_resume(), making it >>> safe for devices to only set lost_power on some resumes. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> drivers/usb/host/xhci.c | 8 +++++++- >>> drivers/usb/host/xhci.h | 6 ++++++ >>> 2 files changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >>> index 0a8cf6c17f82..2c9b32d339f9 100644 >>> --- a/drivers/usb/host/xhci.c >>> +++ b/drivers/usb/host/xhci.c >>> @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend) >> >> Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently? >> >> XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume. >> >> xhci->lost_power is used by devices that don't have consistent behavior. > > The goal is to avoid almost-duplicate functionality. I feel like: > > XHCI_RESET_ON_RESUME is the default value of xhci->lost_power, > which might be modified at resume. > > Is a more straight forward solution than: > > Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was > lost at resume. First must be statically known, second can be > updated during runtime. If second is used, first one must NOT be > set. > > Indeed, the first solution brings two additional lines of code as you > commented below. I'd argue the easier-to-wrap-your-head-around logic is > more important. > > Tell me if you are convinced the second approach is better. > I would still vote to keep logic tied to separate flags. so XHCI_RESET_ON_RESUME to always resume on RESET xhci->lost_power, reset based on runtime checks. Which implies that platforms using xhci->lost_power should not set XHCI_RESET_ON_RESUME. But XHCI maintainers should give their opinion on this. >> >> >>> reinit_xhc = true; >>> >>> + /* Reset to default value, parent devices might correct it at next resume. */ >>> + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); >>> + >> >> then you don't need to do this. > > To be honest, I added this line out of rigor. We could remove it and say > that any device that modifies xhci->lost_power once at resume must set > it at each later resume. > > The above line felt like a small safety net to avoid logic mistakes. > >> >>> if (!reinit_xhc) { >>> /* >>> * Some controllers might lose power during suspend, so wait >>> @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) >>> if (get_quirks) >>> get_quirks(dev, xhci); >>> >>> + /* Default value, that can be corrected at resume. */ >>> + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); >>> + >> >> nor this. > > Regards, > > -- > 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 0a8cf6c17f82..2c9b32d339f9 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend) reinit_xhc = true; + /* Reset to default value, parent devices might correct it at next resume. */ + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); + if (!reinit_xhc) { /* * Some controllers might lose power during suspend, so wait @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) if (get_quirks) get_quirks(dev, xhci); + /* Default value, that can be corrected at resume. */ + xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME); + /* In xhci controllers which follow xhci 1.0 spec gives a spurious * success event after a short transfer. This quirk will ignore such * spurious event. diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index ebd0afd59a60..ec7c6061363f 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1640,6 +1640,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 the XHCI_RESET_ON_RESUME value. 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. Reset the xhci->lost_power value each time in xhci_resume(), making it safe for devices to only set lost_power on some resumes. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/host/xhci.c | 8 +++++++- drivers/usb/host/xhci.h | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-)