| Message ID | 20220802151404.1797-3-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
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. Hi Johan, Thanks for the review and suggestion. 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 ? Regards, Krishna, > > Note that this also avoids keeping power-domains on during suspend when > wakeup has not been enabled (e.g. through sysfs). > > For the runtime PM case, making sure that the PHYs are not suspended and > that they are in the same domain as the controller prevents the domain > from being suspended. If there are cases where this is not possible or > desirable, the genpd implementation may need to be extended. > > Fixes: d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller status") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++--------------------- > 1 file changed, 7 insertions(+), 21 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index c5e482f53e9d..be2e3dd36440 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -17,7 +17,6 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > -#include <linux/pm_domain.h> > #include <linux/usb/of.h> > #include <linux/reset.h> > #include <linux/iopoll.h> > @@ -757,13 +756,12 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) > > static int dwc3_qcom_probe(struct platform_device *pdev) > { > - struct device_node *np = pdev->dev.of_node; > - struct device *dev = &pdev->dev; > - struct dwc3_qcom *qcom; > - struct resource *res, *parent_res = NULL; > - int ret, i; > - bool ignore_pipe_clk; > - struct generic_pm_domain *genpd; > + struct device_node *np = pdev->dev.of_node; > + struct device *dev = &pdev->dev; > + struct dwc3_qcom *qcom; > + struct resource *res, *parent_res = NULL; > + int ret, i; > + bool ignore_pipe_clk; > > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > @@ -772,8 +770,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, qcom); > qcom->dev = &pdev->dev; > > - genpd = pd_to_genpd(qcom->dev->pm_domain); > - > if (has_acpi_companion(dev)) { > qcom->acpi_pdata = acpi_device_get_match_data(dev); > if (!qcom->acpi_pdata) { > @@ -881,17 +877,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (ret) > goto interconnect_exit; > > - if (device_can_wakeup(&qcom->dwc3->dev)) { > - /* > - * Setting GENPD_FLAG_ALWAYS_ON flag takes care of keeping > - * genpd on in both runtime suspend and system suspend cases. > - */ > - genpd->flags |= GENPD_FLAG_ALWAYS_ON; > - device_init_wakeup(&pdev->dev, true); > - } else { > - genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON; > - } > - > + device_init_wakeup(&pdev->dev, 1); > qcom->is_suspended = false; > pm_runtime_set_active(dev); > pm_runtime_enable(dev);
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
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index c5e482f53e9d..be2e3dd36440 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -17,7 +17,6 @@ #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> -#include <linux/pm_domain.h> #include <linux/usb/of.h> #include <linux/reset.h> #include <linux/iopoll.h> @@ -757,13 +756,12 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) static int dwc3_qcom_probe(struct platform_device *pdev) { - struct device_node *np = pdev->dev.of_node; - struct device *dev = &pdev->dev; - struct dwc3_qcom *qcom; - struct resource *res, *parent_res = NULL; - int ret, i; - bool ignore_pipe_clk; - struct generic_pm_domain *genpd; + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + struct dwc3_qcom *qcom; + struct resource *res, *parent_res = NULL; + int ret, i; + bool ignore_pipe_clk; qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); if (!qcom) @@ -772,8 +770,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) platform_set_drvdata(pdev, qcom); qcom->dev = &pdev->dev; - genpd = pd_to_genpd(qcom->dev->pm_domain); - if (has_acpi_companion(dev)) { qcom->acpi_pdata = acpi_device_get_match_data(dev); if (!qcom->acpi_pdata) { @@ -881,17 +877,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ret) goto interconnect_exit; - if (device_can_wakeup(&qcom->dwc3->dev)) { - /* - * Setting GENPD_FLAG_ALWAYS_ON flag takes care of keeping - * genpd on in both runtime suspend and system suspend cases. - */ - genpd->flags |= GENPD_FLAG_ALWAYS_ON; - device_init_wakeup(&pdev->dev, true); - } else { - genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON; - } - + device_init_wakeup(&pdev->dev, 1); qcom->is_suspended = false; pm_runtime_set_active(dev); pm_runtime_enable(dev);
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. Note that this also avoids keeping power-domains on during suspend when wakeup has not been enabled (e.g. through sysfs). For the runtime PM case, making sure that the PHYs are not suspended and that they are in the same domain as the controller prevents the domain from being suspended. If there are cases where this is not possible or desirable, the genpd implementation may need to be extended. Fixes: d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller status") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-)