2022-04-12 07:09:51

by Sandeep Maheswaram

[permalink] [raw]
Subject: [PATCH v13 5/6] usb: dwc3: qcom: Keep power domain on to retain controller status

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 <[email protected]>
---
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;

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);
--
2.7.4


2022-04-12 22:26:26

by Sandeep Maheswaram

[permalink] [raw]
Subject: Re: [PATCH v13 5/6] usb: dwc3: qcom: Keep power domain on to retain controller status

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 <[email protected]>
>> ---
>> 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.

2022-04-13 00:11:55

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v13 5/6] usb: dwc3: qcom: Keep power domain on to retain controller status

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 <[email protected]>
> ---
> 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);
}