[03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

Message ID 1465916848-8207-4-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson June 14, 2016, 3:07 p.m.
In cases when the designware specific flag "pm_runtime_disable" is set,
->probe() calls pm_runtime_forbid() for the device, but without enabling
runtime PM. This increases the runtime PM usage count for the device, but
as runtime PM is disabled it's pointless.

Let's instead convert to unconditionally enable runtime PM, which has the
effect of making a parent device aware of its child.

To also maintain the old behaviour when the "pm_runtime_disable" flag is
set, which prevents userspace to enable runtime PM suspend via sysfs,
switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
during ->probe().

While changing this, let's also also correct the error path in ->probe()
and fix ->remove(), as to decrease the runtime PM usage count when it has
been in increased by pm_runtime_forbid() (after this change, by
pm_runtime_get_noresume()).

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Jarkko Nikula June 15, 2016, 12:47 p.m. | #1
On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> In cases when the designware specific flag "pm_runtime_disable" is set,
> ->probe() calls pm_runtime_forbid() for the device, but without enabling
> runtime PM. This increases the runtime PM usage count for the device, but
> as runtime PM is disabled it's pointless.
>
As far as I remember reason for that was to prevent suspending the 
device when unloading the driver.

> Let's instead convert to unconditionally enable runtime PM, which has the
> effect of making a parent device aware of its child.
>
> To also maintain the old behaviour when the "pm_runtime_disable" flag is
> set, which prevents userspace to enable runtime PM suspend via sysfs,
> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
> during ->probe().
>
> While changing this, let's also also correct the error path in ->probe()
> and fix ->remove(), as to decrease the runtime PM usage count when it has
> been in increased by pm_runtime_forbid() (after this change, by
> pm_runtime_get_noresume()).
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 19174e7..94ff953 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  	adap->dev.of_node = pdev->dev.of_node;
...
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_get_noresume(&pdev->dev);

> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
>
>  	i2c_dw_disable(dev);
>
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_put_noidle(&pdev->dev);

That will trigger power down after returning from dw_i2c_plat_remove() 
by looking at debug print from acpi_device_set_power() but now I don't 
find what call chain is actually causing it.

Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls 
from drivers/base/dd.c. Also I was wondering why RPM usage counts don't 
increase monotonically over repeated module load/unload cycle.

I had a few paper notes about these when I last time looked and debugged 
PM in i2c-designware but throw away them :-(
Ulf Hansson June 20, 2016, 5:15 a.m. | #2
On 15 June 2016 at 14:47, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/14/2016 06:07 PM, Ulf Hansson wrote:
>>
>> In cases when the designware specific flag "pm_runtime_disable" is set,
>> ->probe() calls pm_runtime_forbid() for the device, but without enabling
>> runtime PM. This increases the runtime PM usage count for the device, but
>> as runtime PM is disabled it's pointless.
>>
> As far as I remember reason for that was to prevent suspending the device
> when unloading the driver.

Hmm. To me that's a weird way of implementing this.

Moreover, as i2c_dw_disable() indeed is called in ->remove() how will
the pm_runtime_forbid() help to avoid that from happen?

>
>> Let's instead convert to unconditionally enable runtime PM, which has the
>> effect of making a parent device aware of its child.
>>
>> To also maintain the old behaviour when the "pm_runtime_disable" flag is
>> set, which prevents userspace to enable runtime PM suspend via sysfs,
>> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
>> during ->probe().
>>
>> While changing this, let's also also correct the error path in ->probe()
>> and fix ->remove(), as to decrease the runtime PM usage count when it has
>> been in increased by pm_runtime_forbid() (after this change, by
>> pm_runtime_get_noresume()).
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 25
>> +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 19174e7..94ff953 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device
>> *pdev)
>>         adap->dev.of_node = pdev->dev.of_node;
>
> ...
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_get_noresume(&pdev->dev);
>
>
>> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device
>> *pdev)
>>
>>         i2c_dw_disable(dev);
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_put_noidle(&pdev->dev);
>
>
> That will trigger power down after returning from dw_i2c_plat_remove() by
> looking at debug print from acpi_device_set_power() but now I don't find
> what call chain is actually causing it.

Oh, I didn't realize that by power down you meant the acpi power down.

Can you elaborate on why power down needs to be prevented?

Is there a problem to power on the device again in the next ->probe() attempt?

>
> Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls
> from drivers/base/dd.c. Also I was wondering why RPM usage counts don't
> increase monotonically over repeated module load/unload cycle.

$subject patch changes this, as I didn't think that was really the
wanted behaviour.

Perhaps acpi doesn't power down the device in case the runtime PM
usage count > 0. I will have look and see what I can dig out.

>
> I had a few paper notes about these when I last time looked and debugged PM
> in i2c-designware but throw away them :-(

Okay, I see.

Seems like we definitely need to do some more testing on acpi enabled platforms.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 19174e7..94ff953 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -236,21 +236,21 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap->dev.of_node = pdev->dev.of_node;
 
 	pm_runtime_get_noresume(&pdev->dev);
-	if (dev->pm_runtime_disabled) {
-		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	if (dev->pm_runtime_disabled)
+		pm_runtime_get_noresume(&pdev->dev);
 
 	r = i2c_dw_probe(dev);
 	if (r) {
 		if (!IS_ERR(dev->clk))
 			clk_disable_unprepare(dev->clk);
-		if (!dev->pm_runtime_disabled)
-			pm_runtime_disable(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		if (dev->pm_runtime_disabled)
+			pm_runtime_put_noidle(&pdev->dev);
 	}
 	pm_runtime_put(&pdev->dev);
 
@@ -267,10 +267,11 @@  static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_dw_disable(dev);
 
+	if (dev->pm_runtime_disabled)
+		pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }