| Message ID | 1635753224-23975-3-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 |
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 <quic_c_sanm@quicinc.com> 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 <briannorris@chromium.org> > --- > 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 <quic_c_sanm@quicinc.com> > > 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 <briannorris@chromium.org> > > > --- > > 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/1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com/
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/1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com/ 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 <quic_c_sanm@quicinc.com> > --- > 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
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)
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 <quic_c_sanm@quicinc.com> --- drivers/usb/dwc3/core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)