| Message ID | 20220802151404.1797-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote: > On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > It should also not be used to > > work around Linux driver implementation issues such as how to coordinate > > the glue and core dwc3 drivers. > > > > For the Qualcomm dwc3 controllers, it is the glue device that manages > > the wakeup interrupts, which may or may not be able to wake the system > > up from system suspend. > > While the reasoning to add this may have been for QCom, having this > property for other users makes sense. On some platforms, 'snps,dwc3' > is the only node (i.e. there's no wrapper node). So I don't think this > should be reverted. Fair enough. Let's keep it in the core child node then where we can still retrieve from the glue parent directly. (I assume you're not suggesting also adding 'wakeup-source' to the qcom glue node, which is where the actual wakeup interrupts live.) The glue and core parts needs to work in concert even if the current implementation tends to make that harder than it should be. Johan
On Tue, Aug 02, 2022 at 11:30:34PM +0530, Krishna Kurapati PSSNV wrote: > > On 8/2/2022 8:43 PM, Johan Hovold wrote: > > This reverts commit d9be8d5c5b032e5383ff5c404ff4155e9c705429. > > > > Generic power-domain flags must be set before the power-domain is > > initialised and must specifically not be modified by drivers for devices > > that happen to be in the domain. > > > > To make sure that USB power-domains are left enabled during system > > suspend when a device in the domain is in the wakeup path, the > > GENPD_FLAG_ACTIVE_WAKEUP flag should instead be set for the domain > > unconditionally when it is registered. > In case we need the genpd framework to set the GENPD_FLAG_ACTIVE_WAKEUP > flag for a particular domain that will be used by a device (which is > wakeup capable) and hasn't been probed yet , can you help me understand if > there any dt-flags we can add to convey the same the genpd framework > so that it will set that flag during domain registration ? This can't be expressed in DT (currently), so what you need is something like the below: diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c index 7ff64d4d5920..4ff855269467 100644 --- a/drivers/clk/qcom/gcc-sc7280.c +++ b/drivers/clk/qcom/gcc-sc7280.c @@ -3125,6 +3125,7 @@ static struct gdsc gcc_usb30_prim_gdsc = { .gdscr = 0xf004, .pd = { .name = "gcc_usb30_prim_gdsc", + .flags = GENPD_FLAG_ACTIVE_WAKEUP, }, .pwrsts = PWRSTS_OFF_ON, .flags = VOTABLE, @@ -3134,6 +3135,7 @@ static struct gdsc gcc_usb30_sec_gdsc = { .gdscr = 0x9e004, .pd = { .name = "gcc_usb30_sec_gdsc", + .flags = GENPD_FLAG_ACTIVE_WAKEUP, }, .pwrsts = PWRSTS_OFF_ON, .flags = VOTABLE, to make sure that genpd keeps these domains powered during system suspend if they are used by devices that are in the wakeup path. Johan
On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote: > A device must enable wakeups during runtime suspend regardless of > whether it is capable and allowed to wake the system up from system > suspend. > > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend. In how far is runtime PM actually supported/used by this driver? The device is set 'active' in _probe(), and there are no other pm_runtime_* calls, except in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from 'active' into 'suspended'?
On Wed, Aug 03, 2022 at 09:31:06AM +0200, Johan Hovold wrote: > On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote: > > On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > It should also not be used to > > > work around Linux driver implementation issues such as how to coordinate > > > the glue and core dwc3 drivers. > > > > > > For the Qualcomm dwc3 controllers, it is the glue device that manages > > > the wakeup interrupts, which may or may not be able to wake the system > > > up from system suspend. > > > > While the reasoning to add this may have been for QCom, having this > > property for other users makes sense. On some platforms, 'snps,dwc3' > > is the only node (i.e. there's no wrapper node). So I don't think this > > should be reverted. > > Fair enough. Let's keep it in the core child node then where we can > still retrieve from the glue parent directly. > > (I assume you're not suggesting also adding 'wakeup-source' to the qcom > glue node, which is where the actual wakeup interrupts live.) 'wakeup-source' belongs wherever the interrupt that causes wakeup is defined. Rob
This series fixes some of the fallout after the recently merged series that added wakeup support to the Qualcomm dwc3 driver: https://lore.kernel.org/all/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/ The first patch fixes a long standing PHY power sequencing issue in dwc3 core. The second patch reverts a power-domain hack that was added by the above series. There are other ways to implement this which doesn't violate the genpd interface, and if for some reason those are not sufficient, the genpd implementation needs to be extended, not hacked around. The third patch fixes a couple of NULL-pointer dereferences when suspending controllers in peripheral or OTG mode due to a hack that was added to suspend path. Unfortunately, it seems the hack needs to stay for now if we want functioning suspend on some Qualcomm platforms. The fourth patch fixes another issue in the Qualcomm dwc3 implementation that has been added a while back and which breaks runtime PM. The remaining patches moves the wakeup-source property over from the core node to the glue node in the binding and instead propagates the wakeup capability to the former during probe. Note that this incidentally also avoids adding probe-deferral hacks to the driver as was recently proposed to deal with another problem with the current implementation: https://lore.kernel.org/all/1657891312-21748-1-git-send-email-quic_kriskura@quicinc.com/ With this series I have functioning USB system suspend and wakeup as well as somewhat functioning runtime PM in host mode on sc8280xp. Note that the PHYs apparently do not need to be shutdown for wakeup on this platform. Some issues remain such as that the controller needs to be suspended to handle remote wakeup during runtime PM (the wakeup interrupts probably needs to be enabled whenever there's a wakeup-capable device connected to the bus) and that root hub connect/disconnect events cannot selectively be disabled. And of course, the suspend speed hack needs to be replaced at some point but that likely requires some more heavy lifting in the dwc3 implementation. Johan Johan Hovold (8): usb: dwc3: fix PHY disable sequence Revert "usb: dwc3: qcom: Keep power domain on to retain controller status" usb: dwc3: qcom: fix broken non-host-mode suspend usb: dwc3: qcom: fix runtime PM wakeup Revert "dt-bindings: usb: dwc3: Add wakeup-source property support" dt-bindings: usb: qcom,dwc3: add wakeup-source property usb: dwc3: qcom: fix wakeup implementation usb: dwc3: qcom: clean up suspend callbacks .../devicetree/bindings/usb/qcom,dwc3.yaml | 2 + .../devicetree/bindings/usb/snps,dwc3.yaml | 5 -- drivers/usb/dwc3/core.c | 24 +++--- drivers/usb/dwc3/dwc3-qcom.c | 77 ++++++++++--------- 4 files changed, 55 insertions(+), 53 deletions(-)