| Message ID | 1649704614-31518-6-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 Tue, Apr 12, 2022 at 12:46:53AM +0530, Sandeep Maheswaram wrote: > Keep the power domain always on during runtime suspend or if the > controller supports wakeup in order to retain controller status > and to support wakeup from devices. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 9804a19..9747be6 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -17,6 +17,7 @@ > #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> > @@ -724,6 +725,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > struct resource *res, *parent_res = NULL; > int ret, i; > bool ignore_pipe_clk; > + struct generic_pm_domain *genpd; nit: I'm not really a fan of gazillions of whitespaces between the type and the variable name, but if they are kept all variable names above should move a tab to the right. In any case the style in this file isn't consistent, so an alternative would be to just get rid of the alignment completely. > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > @@ -732,6 +734,8 @@ 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) { > @@ -839,7 +843,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (ret) > goto interconnect_exit; > > - device_init_wakeup(&pdev->dev, 1); > + genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON; > + > + if (device_may_wakeup(&qcom->dwc3->dev)) > + genpd->flags |= GENPD_FLAG_ALWAYS_ON; In v12 only GENPD_FLAG_ALWAYS_ON was set, not GENPD_FLAG_RPM_ALWAYS_ON. I asked a few times for a change log, in this instance it would be useful to explain why GENPD_FLAG_RPM_ALWAYS_ON is now set. > + > + device_init_wakeup(&pdev->dev, device_may_wakeup(&qcom->dwc3->dev)); device_may_wakeup() isn't an expensive operation, but it's still not nice to call it twice in three lines of code. Instead you could do this: if (device_may_wakeup(&qcom->dwc3->dev)) { genpd->flags |= GENPD_FLAG_ALWAYS_ON; device_init_wakeup(&pdev->dev, true); }
Hi, On 4/12/2022 2:43 AM, Matthias Kaehlcke wrote: > On Tue, Apr 12, 2022 at 12:46:53AM +0530, Sandeep Maheswaram wrote: >> Keep the power domain always on during runtime suspend or if the >> controller supports wakeup in order to retain controller status >> and to support wakeup from devices. >> >> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 9804a19..9747be6 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -17,6 +17,7 @@ >> #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> >> @@ -724,6 +725,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) >> struct resource *res, *parent_res = NULL; >> int ret, i; >> bool ignore_pipe_clk; >> + struct generic_pm_domain *genpd; > nit: I'm not really a fan of gazillions of whitespaces between the type > and the variable name, but if they are kept all variable names above > should move a tab to the right. In any case the style in this file isn't > consistent, so an alternative would be to just get rid of the alignment > completely. Okay. Will do it in next version. > >> qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); >> if (!qcom) >> @@ -732,6 +734,8 @@ 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) { >> @@ -839,7 +843,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) >> if (ret) >> goto interconnect_exit; >> >> - device_init_wakeup(&pdev->dev, 1); >> + genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON; >> + >> + if (device_may_wakeup(&qcom->dwc3->dev)) >> + genpd->flags |= GENPD_FLAG_ALWAYS_ON; > In v12 only GENPD_FLAG_ALWAYS_ON was set, not GENPD_FLAG_RPM_ALWAYS_ON. > I asked a few times for a change log, in this instance it would be > useful to explain why GENPD_FLAG_RPM_ALWAYS_ON is now set. Okay. Will add it in next version. > >> + >> + device_init_wakeup(&pdev->dev, device_may_wakeup(&qcom->dwc3->dev)); > device_may_wakeup() isn't an expensive operation, but it's still not nice to > call it twice in three lines of code. Instead you could do this: > > if (device_may_wakeup(&qcom->dwc3->dev)) { > genpd->flags |= GENPD_FLAG_ALWAYS_ON; > device_init_wakeup(&pdev->dev, true); > } Okay. Will do it in next version.
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 9804a19..9747be6 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -17,6 +17,7 @@ #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> @@ -724,6 +725,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) struct resource *res, *parent_res = NULL; int ret, i; bool ignore_pipe_clk; + struct generic_pm_domain *genpd; qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); if (!qcom) @@ -732,6 +734,8 @@ 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) { @@ -839,7 +843,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ret) goto interconnect_exit; - device_init_wakeup(&pdev->dev, 1); + genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON; + + if (device_may_wakeup(&qcom->dwc3->dev)) + genpd->flags |= GENPD_FLAG_ALWAYS_ON; + + device_init_wakeup(&pdev->dev, device_may_wakeup(&qcom->dwc3->dev)); qcom->is_suspended = false; pm_runtime_set_active(dev); pm_runtime_enable(dev);
Keep the power domain always on during runtime suspend or if the controller supports wakeup in order to retain controller status and to support wakeup from devices. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)