Avoiding phy powerdown when wakeup capable devices are connected
by checking wakeup property of xhci plat device.
Phy should be on to wake up the device from suspend using wakeup capable
devices such as keyboard and mouse.
Signed-off-by: Sandeep Maheswaram <[email protected]>
---
drivers/usb/dwc3/core.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 643239d..a6ad0ed 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1787,7 +1787,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
dwc3_core_exit(dwc);
break;
case DWC3_GCTL_PRTCAP_HOST:
- if (!PMSG_IS_AUTO(msg)) {
+ if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
dwc3_core_exit(dwc);
break;
}
@@ -1848,13 +1848,16 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
spin_unlock_irqrestore(&dwc->lock, flags);
break;
case DWC3_GCTL_PRTCAP_HOST:
- if (!PMSG_IS_AUTO(msg)) {
+ if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
ret = dwc3_core_init_for_resume(dwc);
if (ret)
return ret;
dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
break;
+ } else {
+ dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
}
+
/* Restore GUSB2PHYCFG bits that were modified in suspend */
reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
if (dwc->dis_u2_susphy_quirk)
--
2.7.4
On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking wakeup property of xhci plat device.
> Phy should be on to wake up the device from suspend using wakeup capable
> devices such as keyboard and mouse.
>
> Signed-off-by: Sandeep Maheswaram <[email protected]>
Because you've now factored in a device_may_wakeup() (which evaluates
'false' for me), this no longer breaks my Rockchip RK3399 systems
(previous versions did). So from that angle:
Tested-by: Brian Norris <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d..a6ad0ed 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1787,7 +1787,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> - if (!PMSG_IS_AUTO(msg)) {
> + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
I still find it odd to use device_may_wakeup(), since that's something
controlled by user space (sysfs) and doesn't fully factor in hardware
support. But that's what xhci-plat.c is doing, so I guess I see why
you're imitating it...
...still, feels wrong to me. But so does a lot of how dwc3 works.
Brian
> dwc3_core_exit(dwc);
> break;
> }
> @@ -1848,13 +1848,16 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> spin_unlock_irqrestore(&dwc->lock, flags);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> - if (!PMSG_IS_AUTO(msg)) {
> + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> ret = dwc3_core_init_for_resume(dwc);
> if (ret)
> return ret;
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> break;
> + } else {
> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> }
> +
> /* Restore GUSB2PHYCFG bits that were modified in suspend */
> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> if (dwc->dis_u2_susphy_quirk)
> --
> 2.7.4
>
On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote:
> On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> > Avoiding phy powerdown when wakeup capable devices are connected
> > by checking wakeup property of xhci plat device.
> > Phy should be on to wake up the device from suspend using wakeup capable
> > devices such as keyboard and mouse.
> >
> > Signed-off-by: Sandeep Maheswaram <[email protected]>
>
> Because you've now factored in a device_may_wakeup() (which evaluates
> 'false' for me), this no longer breaks my Rockchip RK3399 systems
> (previous versions did). So from that angle:
>
> Tested-by: Brian Norris <[email protected]>
>
> > ---
> > drivers/usb/dwc3/core.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 643239d..a6ad0ed 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1787,7 +1787,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > dwc3_core_exit(dwc);
> > break;
> > case DWC3_GCTL_PRTCAP_HOST:
> > - if (!PMSG_IS_AUTO(msg)) {
> > + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
>
> I still find it odd to use device_may_wakeup(), since that's something
> controlled by user space (sysfs) and doesn't fully factor in hardware
> support. But that's what xhci-plat.c is doing, so I guess I see why
> you're imitating it...
> ...still, feels wrong to me. But so does a lot of how dwc3 works.
device_may_wakeup() actually factors in hardware support, at least if the
driver does the right thing (TM). The (current) implementation is:
static inline bool device_may_wakeup(struct device *dev)
{
return dev->power.can_wakeup && !!dev->power.wakeup;
}
'.can_wakeup' should describe the hardware capability to wake up and the
other flag whether wakeup is enabled (which can be altered by userspace).
What this series currently does with the .can_wakeup flag is still wrong
though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3
controller" [1] dynamically sets the flag with a value that depends on what
is connected to the bus, so it doesn't specify any longer whether the
hardware supports wakeup or not.
[1] https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/
On Tue, Nov 16, 2021 at 05:14:14PM -0800, Matthias Kaehlcke wrote:
> On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote:
> > On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> > > + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> >
> > I still find it odd to use device_may_wakeup(), since that's something
> > controlled by user space (sysfs) and doesn't fully factor in hardware
> > support. But that's what xhci-plat.c is doing, so I guess I see why
> > you're imitating it...
> > ...still, feels wrong to me. But so does a lot of how dwc3 works.
>
> device_may_wakeup() actually factors in hardware support, at least if the
> driver does the right thing (TM).
Well in theory, maybe. But the latter half of the sentence is the key :)
In particular, xhci-plat does the Wrong Thing before this series:
device_set_wakeup_capable(&pdev->dev, true);
i.e., it doesn't factor any "capability" in at all; it just assumes it.
And per your thoughts below, it's still Wrong after this series.
> The (current) implementation is:
>
> static inline bool device_may_wakeup(struct device *dev)
> {
> return dev->power.can_wakeup && !!dev->power.wakeup;
> }
>
> '.can_wakeup' should describe the hardware capability to wake up and the
> other flag whether wakeup is enabled (which can be altered by userspace).
>
> What this series currently does with the .can_wakeup flag is still wrong
> though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3
> controller" [1] dynamically sets the flag with a value that depends on what
> is connected to the bus, so it doesn't specify any longer whether the
> hardware supports wakeup or not.
>
> [1] https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/
I'm not sure either your patch nor Sandeep's patch really get at the
heart of my problem here, which is that neither dwc3 nor xhci-plat are
trying to reflect wakeup capability of the host controller at all. (And
if the host controller doesn't suppor wakeup, it doesn't really matter
what any of its children think.) It seems that
drivers/usb/dwc3/dwc3-imx8mp.c is the only one that actually sets the
correct wakeup flag at the level that we _really_ know what's up -- the
platform/"glue" driver.
Maybe we need to do a little of both: teach the glue drivers (e.g.,
dwc3-qcom.c) to reflect their wakeup capability properly, and then look
at *that* capability (as well as any children, recursively, but only if
the glue driver supports it) when trying to make wakeup decisions.
It still feels wrong that there are 3 separate "can wakeup"
determinations for the host controller though: 1 dwc3-{glue}, 1
dwc3(core), and 1 xhci-plat. But maybe we have to hold our noses on that
one.
Brian
Hi Sandeep,
On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking wakeup property of xhci plat device.
> Phy should be on to wake up the device from suspend using wakeup capable
> devices such as keyboard and mouse.
>
> Signed-off-by: Sandeep Maheswaram <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d..a6ad0ed 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1787,7 +1787,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> - if (!PMSG_IS_AUTO(msg)) {
> + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> dwc3_core_exit(dwc);
> break;
> }
> @@ -1848,13 +1848,16 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> spin_unlock_irqrestore(&dwc->lock, flags);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> - if (!PMSG_IS_AUTO(msg)) {
> + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> ret = dwc3_core_init_for_resume(dwc);
> if (ret)
> return ret;
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> break;
> + } else {
> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> }
> +
During runtime resume, we enter else block and call dwc3_set_prtcap() which
is not done before. Is that intentional?
Thanks,
Pavan