| Message ID | 1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
Hi Sandeep, On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote: > During suspend check if any wakeup capable devices are connected to the > controller (directly or through hubs), and set the wakeup capable property > for xhci plat device. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > drivers/usb/host/xhci-plat.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index c1edcc9..7ab272b 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev) > return 0; > } > > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev) > +{ > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > + device_set_wakeup_capable(dev, true); > + else > + device_set_wakeup_capable(dev, false); IIUC wakeup capability is typically a static property that reflects the actual hardware (or firmware) support for wakeup. In that sense it doesn't seem a good idea to change it dynamically at suspend time, depending on what is connected to the bus. I understand though that the odd split of the dwc3 driver makes it hard to do things properly ... Earlier in this discussion Felipe suggested to add a function like device_children_wakeup_capable(), to avoid having to call usb_wakeup_enabled_descendants() from the dwc3 drivers. Below is an initial implementation for device_children_wakeup_capable(), could you try if calling it from dwc3_suspend/resume_common() and dwc3_qcom_suspend() would work instead of relying on the wakeup capability? Thanks Matthias From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke <mka@chromium.org> Date: Mon, 1 Nov 2021 11:37:19 -0700 Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable() Add device_children_wakeup_capable() which checks whether the device itself or one if its descendants is wakeup capable. Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99 Suggested-by: Felipe Balbi <balbi@kernel.org> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/base/power/wakeup.c | 17 +++++++++++++++++ include/linux/pm_wakeup.h | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 9c0932603642..2aee7fa8230f 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -483,6 +483,23 @@ int device_set_wakeup_enable(struct device *dev, bool enable) } EXPORT_SYMBOL_GPL(device_set_wakeup_enable); +static int __device_children_wakeup_capable(struct device *dev, void *dummy) +{ + return device_may_wakeup(dev) || + device_for_each_child(dev, NULL, __device_children_wakeup_capable); +} + +/** + * device_children_wakeup_capable - Check whether a device or one of its descendants is + * wakeup capable. + * @dev: Device to handle. + */ +bool device_children_wakeup_capable(struct device *dev) +{ + return __device_children_wakeup_capable(dev, NULL); +} +EXPORT_SYMBOL_GPL(device_children_wakeup_capable); + /** * wakeup_source_not_registered - validate the given wakeup source. * @ws: Wakeup source to be validated. diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 661efa029c96..a0ca42aac6d6 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -97,6 +97,7 @@ extern int device_wakeup_disable(struct device *dev); extern void device_set_wakeup_capable(struct device *dev, bool capable); extern int device_init_wakeup(struct device *dev, bool val); extern int device_set_wakeup_enable(struct device *dev, bool enable); +extern bool device_children_wakeup_capable(struct device *dev); extern void __pm_stay_awake(struct wakeup_source *ws); extern void pm_stay_awake(struct device *dev); extern void __pm_relax(struct wakeup_source *ws); @@ -167,6 +168,11 @@ static inline bool device_may_wakeup(struct device *dev) static inline void device_set_wakeup_path(struct device *dev) {} +static inline bool device_children_wakeup_capable(struct device *dev) +{ + return false; +} + static inline void __pm_stay_awake(struct wakeup_source *ws) {} static inline void pm_stay_awake(struct device *dev) {}
On Mon 01 Nov 11:59 PDT 2021, Matthias Kaehlcke wrote: > Hi Sandeep, > > On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote: > > During suspend check if any wakeup capable devices are connected to the > > controller (directly or through hubs), and set the wakeup capable property > > for xhci plat device. > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > --- > > drivers/usb/host/xhci-plat.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index c1edcc9..7ab272b 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev) > > return 0; > > } > > > > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev) > > +{ > > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > > + device_set_wakeup_capable(dev, true); > > + else > > + device_set_wakeup_capable(dev, false); > > IIUC wakeup capability is typically a static property that reflects the > actual hardware (or firmware) support for wakeup. In that sense it doesn't > seem a good idea to change it dynamically at suspend time, depending on > what is connected to the bus. I understand though that the odd split > of the dwc3 driver makes it hard to do things properly ... > > Earlier in this discussion Felipe suggested to add a function like > device_children_wakeup_capable(), to avoid having to call > usb_wakeup_enabled_descendants() from the dwc3 drivers. > > Below is an initial implementation for device_children_wakeup_capable(), > could you try if calling it from dwc3_suspend/resume_common() and > dwc3_qcom_suspend() would work instead of relying on the wakeup > capability? > > Thanks > > Matthias > > From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001 > From: Matthias Kaehlcke <mka@chromium.org> > Date: Mon, 1 Nov 2021 11:37:19 -0700 > Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable() > > Add device_children_wakeup_capable() which checks whether the device itself > or one if its descendants is wakeup capable. > > Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99 > Suggested-by: Felipe Balbi <balbi@kernel.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Looks neat and useful. Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> (Without the Change-Id of course...) Regards, Bjorn > --- > drivers/base/power/wakeup.c | 17 +++++++++++++++++ > include/linux/pm_wakeup.h | 6 ++++++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index 9c0932603642..2aee7fa8230f 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -483,6 +483,23 @@ int device_set_wakeup_enable(struct device *dev, bool enable) > } > EXPORT_SYMBOL_GPL(device_set_wakeup_enable); > > +static int __device_children_wakeup_capable(struct device *dev, void *dummy) > +{ > + return device_may_wakeup(dev) || > + device_for_each_child(dev, NULL, __device_children_wakeup_capable); > +} > + > +/** > + * device_children_wakeup_capable - Check whether a device or one of its descendants is > + * wakeup capable. > + * @dev: Device to handle. > + */ > +bool device_children_wakeup_capable(struct device *dev) > +{ > + return __device_children_wakeup_capable(dev, NULL); > +} > +EXPORT_SYMBOL_GPL(device_children_wakeup_capable); > + > /** > * wakeup_source_not_registered - validate the given wakeup source. > * @ws: Wakeup source to be validated. > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 661efa029c96..a0ca42aac6d6 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -97,6 +97,7 @@ extern int device_wakeup_disable(struct device *dev); > extern void device_set_wakeup_capable(struct device *dev, bool capable); > extern int device_init_wakeup(struct device *dev, bool val); > extern int device_set_wakeup_enable(struct device *dev, bool enable); > +extern bool device_children_wakeup_capable(struct device *dev); > extern void __pm_stay_awake(struct wakeup_source *ws); > extern void pm_stay_awake(struct device *dev); > extern void __pm_relax(struct wakeup_source *ws); > @@ -167,6 +168,11 @@ static inline bool device_may_wakeup(struct device *dev) > > static inline void device_set_wakeup_path(struct device *dev) {} > > +static inline bool device_children_wakeup_capable(struct device *dev) > +{ > + return false; > +} > + > static inline void __pm_stay_awake(struct wakeup_source *ws) {} > > static inline void pm_stay_awake(struct device *dev) {}
On Mon, Nov 01, 2021 at 01:50:31PM -0700, Bjorn Andersson wrote: > On Mon 01 Nov 11:59 PDT 2021, Matthias Kaehlcke wrote: > > > Hi Sandeep, > > > > On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote: > > > During suspend check if any wakeup capable devices are connected to the > > > controller (directly or through hubs), and set the wakeup capable property > > > for xhci plat device. > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > --- > > > drivers/usb/host/xhci-plat.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > index c1edcc9..7ab272b 100644 > > > --- a/drivers/usb/host/xhci-plat.c > > > +++ b/drivers/usb/host/xhci-plat.c > > > @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev) > > > return 0; > > > } > > > > > > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev) > > > +{ > > > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > > > + device_set_wakeup_capable(dev, true); > > > + else > > > + device_set_wakeup_capable(dev, false); > > > > IIUC wakeup capability is typically a static property that reflects the > > actual hardware (or firmware) support for wakeup. In that sense it doesn't > > seem a good idea to change it dynamically at suspend time, depending on > > what is connected to the bus. I understand though that the odd split > > of the dwc3 driver makes it hard to do things properly ... > > > > Earlier in this discussion Felipe suggested to add a function like > > device_children_wakeup_capable(), to avoid having to call > > usb_wakeup_enabled_descendants() from the dwc3 drivers. > > > > Below is an initial implementation for device_children_wakeup_capable(), > > could you try if calling it from dwc3_suspend/resume_common() and > > dwc3_qcom_suspend() would work instead of relying on the wakeup > > capability? > > > > Thanks > > > > Matthias > > > > From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001 > > From: Matthias Kaehlcke <mka@chromium.org> > > Date: Mon, 1 Nov 2021 11:37:19 -0700 > > Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable() > > > > Add device_children_wakeup_capable() which checks whether the device itself > > or one if its descendants is wakeup capable. > > > > Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99 > > Suggested-by: Felipe Balbi <balbi@kernel.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > Looks neat and useful. > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Thanks! > (Without the Change-Id of course...) Sure, I usually use patman to send patches upstream, which filters the Change-Id if present, forgot to remove it when copying and pasting the patch manually.
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index c1edcc9..7ab272b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev) return 0; } +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev) +{ + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) + device_set_wakeup_capable(dev, true); + else + device_set_wakeup_capable(dev, false); +} + static int __maybe_unused xhci_plat_suspend(struct device *dev) { struct usb_hcd *hcd = dev_get_drvdata(dev); @@ -440,6 +448,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) ret = xhci_priv_suspend_quirk(hcd); if (ret) return ret; + + if (of_device_is_compatible(dev->parent->of_node, "snps,dwc3")) + xhci_dwc3_suspend_quirk(hcd, dev); + /* * xhci_suspend() needs `do_wakeup` to know whether host is allowed * to do wakeup during suspend.
During suspend check if any wakeup capable devices are connected to the controller (directly or through hubs), and set the wakeup capable property for xhci plat device. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/usb/host/xhci-plat.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)