i2c: designware-platdrv: Some improvments related to PM
Subsystems (3)
cover i2c: designware-platdrv: Some improvments related to PM
Here's a couple changes for the i2c-designware driver. Most of them a related to the support for runtime PM and system PM, but there's also a few that improves some error handling.
I have tested these on Hisilicon Linaro 96-board (hi6220). I used a couple local changes to enable the power-key to act as a wakeup in system PM suspend state. If anyone are interested about those as well, I am happy to share them.
Ulf Hansson (10):
i2c: designware-platdrv: Return error in ->probe() when clk ungate
fails
i2c: designware-platdrv: Gate clk in error path in ->probe()
i2c: designware-platdrv: Unconditionally enable runtime PM
i2c: designware-platdrv: Disable autosuspend in error path in
->probe()
i2c: designware-platdrv: Fix clk gating in ->remove()
i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
i2c: designware-platdrv: Re-init the HW when resuming
i2c: designware-platdrv: Check return value from clk_prepare_enable()
i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
i2c: designware-platdrv: Rework system PM support
drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++--------------- 1 file changed, 50 insertions(+), 56 deletions(-)
Signature · 1 line
1.9.1
PATCH 1/10 i2c: designware-platdrv: Return error in ->probe() when clk ungate fails
The error code received from clk_prepare_enable() becomes ignored when i2c_dw_plat_prepare_clk() is called in ->probe().
Fix this by invoking clk_prepare_enable() in ->probe() instead of i2c_dw_plat_prepare_clk(), as it allows the error code to be properly propagated.
A side-effect from this change, makes the i2c_dw_plat_prepare_clk() used only when CONFIG_PM is set. Avoid the compiler warning by moving the function within the corresponding #ifdef.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +17 -13
@@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev) }#endif -static int i2c_dw_plat_prepare_clk (struct dw_i2c_dev *i_dev,bool prepare) -{ -if (IS_ERR (i_dev->clk)) -return PTR_ERR (i_dev->clk); - -if (prepare) -return clk_prepare_enable (i_dev->clk); - -clk_disable_unprepare (i_dev->clk); -return 0 ; -} -static int dw_i2c_plat_probe (struct platform_device *pdev) {struct dw_i2c_platform_data *pdata =dev_get_platdata (&pdev->dev);
@@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)DW_IC_CON_RESTART_EN |DW_IC_CON_SPEED_FAST ; dev->clk =devm_clk_get (&pdev->dev,NULL ); -if (!i2c_dw_plat_prepare_clk (dev, true)) { +if (!IS_ERR (dev->clk)) { + r =clk_prepare_enable (dev->clk); +if (r) +return r; + dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;if (!dev->sda_hold_time && ht)
@@ -302,6 +294,18 @@ static void dw_i2c_plat_complete(struct device *dev)#endif #ifdef CONFIG_PM +static int i2c_dw_plat_prepare_clk (struct dw_i2c_dev *i_dev,bool prepare) +{ +if (IS_ERR (i_dev->clk)) +return PTR_ERR (i_dev->clk); + +if (prepare) +return clk_prepare_enable (i_dev->clk); + +clk_disable_unprepare (i_dev->clk); +return 0 ; +} +static int dw_i2c_plat_suspend (struct device *dev) {struct platform_device *pdev =to_platform_device (dev);
Signature · 1 line
1.9.1
PATCH 1/10 i2c: designware-platdrv: Return error in ->probe() when clk ungate fails
Quoted 40 lines · Ulf Hansson
The error code received from
clk_prepare_enable()becomes ignored wheni2c_dw_plat_prepare_clk()is called in ->probe().Fix this by invoking
clk_prepare_enable()in ->probe() instead ofi2c_dw_plat_prepare_clk(), as it allows the error code to be properly propagated.A side-effect from this change, makes the
i2c_dw_plat_prepare_clk()used only whenCONFIG_PMis set. Avoid the compiler warning by moving the function within the corresponding #ifdef.Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-12@@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev) }#endif -static int i2c_dw_plat_prepare_clk (struct dw_i2c_dev *i_dev,bool prepare) -{ -if (IS_ERR (i_dev->clk)) -return PTR_ERR (i_dev->clk); - -if (prepare) -return clk_prepare_enable (i_dev->clk); - -clk_disable_unprepare (i_dev->clk); -return 0 ; -} -static int dw_i2c_plat_probe (struct platform_device *pdev) {struct dw_i2c_platform_data *pdata =dev_get_platdata (&pdev->dev);
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 213
Quoted 7 lines
drivers/i2c/busses/i2c-designware-platdrv.c +3
-1@@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)DW_IC_CON_RESTART_EN |DW_IC_CON_SPEED_FAST ; dev->clk =devm_clk_get (&pdev->dev,NULL ); -if (!i2c_dw_plat_prepare_clk (dev, true)) { +if (!IS_ERR (dev->clk)) { + r =clk_prepare_enable (dev->clk); +if (r)
Looks ok to me. My only worry was commit b33af11de236 ("i2c: designware:
Do not require clock when SSCN and FFCN are provided") but I think it's the IS_ERR(dev->clk) that captures that case not the clk_prepare_enable().
Suravee: Am I right IS_ERR(dev->clk) is enough to handle your case?
Signature · 1 line
Jarkko
PATCH 2/10 i2c: designware-platdrv: Gate clk in error path in ->probe()
In reply to Ulf HanssonThe call to i2c_dw_probe() may fail in ->probe() in which case the clock remains ungated. Fix the error path by gating the clock before the error code is returned.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +8 -2
@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)ACPI_COMPANION_SET (&adap->dev,ACPI_COMPANION (&pdev->dev)); 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 {
@@ -245,8 +246,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) } r =i2c_dw_probe (dev); -if (r && !dev->pm_runtime_disabled) -pm_runtime_disable (&pdev->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_put (&pdev->dev);return r; }
Signature · 1 line
1.9.1
Andy Shevchenko
reviewerPATCH 2/10 i2c: designware-platdrv: Gate clk in error path in ->probe()
Quoted 17 lines · Ulf Hansson
The call to
i2c_dw_probe()may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.Signed-off-by Ulf Hansson <[email protected]>---
drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--1 file changed, 8 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct · line 238
Quoted 4 lines
drivers/i2c/busses/i2c-designware-platdrv.c +0
-0@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node;
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 255
Quoted 3 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2
-0+pm_runtime_get_noresume (&pdev->dev); +pm_runtime_put (&pdev->dev);
I don't see an explanation of these add-ons.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH 2/10 i2c: designware-platdrv: Gate clk in error path in ->probe()
Quoted 24 lines · Andy Shevchenko
Quoted 21 lines · Ulf Hansson
The call to
i2c_dw_probe()may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node;
Quoted 5 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 255
Quoted 3 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2
-0+pm_runtime_get_noresume (&pdev->dev); +pm_runtime_put (&pdev->dev);I don't see an explanation of these add-ons.
As we explicitly do clk_disable_unprepare() in the error path, we must prevent the ->runtime_suspend() callback to be called during this period.
This is a common pattern used by many drivers deploying runtime PM support, which is probably why I left out the explanation.
I can update the change log with this information, if you like.
Kind regards
Uffe
PATCH 2/10 i2c: designware-platdrv: Gate clk in error path in ->probe()
Quoted 30 lines · Ulf Hansson
Quoted 28 lines · Andy Shevchenko
Quoted 26 lines · Ulf Hansson
The call to
i2c_dw_probe()may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +2
-0@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node; +pm_runtime_get_noresume (&pdev->dev); +pm_runtime_put (&pdev->dev);
Quoted 9 lines
Quoted 1 line
I don't see an explanation of these add-ons.
As we explicitly do
clk_disable_unprepare()in the error path, we must prevent the ->runtime_suspend() callback to be called during this period.This is a common pattern used by many drivers deploying runtime PM support, which is probably why I left out the explanation.
What would cause here runtime suspend during probe? Also pairing pm_runtime_get_noresume() with pm_runtime_put() instead of pm_runtime_put_noidle() looks suspicious to me.
Signature · 1 line
Jarkko
PATCH 2/10 i2c: designware-platdrv: Gate clk in error path in ->probe()
Quoted 39 lines · Jarkko Nikula
Quoted 38 lines · Ulf Hansson
Quoted 33 lines · Andy Shevchenko
Quoted 18 lines · Ulf Hansson
The call to
i2c_dw_probe()may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct
Quoted 10 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2
-0@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node; +pm_runtime_get_noresume (&pdev->dev); +pm_runtime_put (&pdev->dev);I don't see an explanation of these add-ons.
Quoted 8 lines
Quoted 7 lines
As we explicitly do
clk_disable_unprepare()in the error path, we must prevent the ->runtime_suspend() callback to be called during this period.This is a common pattern used by many drivers deploying runtime PM support, which is probably why I left out the explanation.
What would cause here runtime suspend during probe? Also pairing
In practice this might not happen, although in theory it could...
The struct device is accessible by others than the driver (userspace through sysfs for example). So, if "someone" would invoke pm_runtime_suspend() (or a similar runtime PM API) for the device, that would cause the ->runtime_suspend() callback to be invoked within this period.
I suggest we just make sure to protect from this to happen.
Quoted 3 lines
pm_runtime_get_noresume()withpm_runtime_put()instead ofpm_runtime_put_noidle()looks suspicious to me.
It's not a problem. There's two cases here:
*) Runtime PM is enabled.
In this case the pm_runtime_put() will decrease the runtime PM usage count and likely cause the device to be runtime suspended. Earlier, the similar would happen as the driver core do a pm_request_idle() when the ->probe() has completed.
**) Runtime PM is disabled.
In this case, the pm_runtime_put() has the same effect as a pm_runtime_put_noidle(). It simply decrease the runtime PM usage count, that's it.
Following this reasoning, one could decide to replace the pm_runtime_put() with pm_runtime_put_noidle(), the behaviour will be very similar.
If you would like to change to pm_runtime_put_noidle(), that's perfectly fine by me.
Kind regards
Uffe
PATCH 3/10 i2c: designware-platdrv: Unconditionally enable runtime PM
In reply to Ulf HanssonIn 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()).
--- drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +13 -12
@@ -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 ; }
Signature · 1 line
1.9.1
PATCH 3/10 i2c: designware-platdrv: Unconditionally enable runtime PM
Quoted 5 lines · Ulf Hansson
In cases when the designware specific flag "
pm_runtime_disable" is set, ->probe() callspm_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.
Quoted 24 lines
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 callingpm_runtime_forbid()intopm_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, bypm_runtime_get_noresume()).Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0@@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) adap->dev.of_node = pdev->dev.of_node;
...
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_remove(struct platform_device *pdev) · line 271
Quoted 9 lines
drivers/i2c/busses/i2c-designware-platdrv.c +4
-0+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 :-(
Signature · 1 line
Jarkko
PATCH 3/10 i2c: designware-platdrv: Unconditionally enable runtime PM
Quoted 9 lines · Jarkko Nikula
Quoted 6 lines · Ulf Hansson
In cases when the designware specific flag "
pm_runtime_disable" is set, ->probe() callspm_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?
Quoted 40 lines
Quoted 28 lines
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 callingpm_runtime_forbid()intopm_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, bypm_runtime_get_noresume()).Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0@@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) adap->dev.of_node = pdev->dev.of_node;...
Quoted 10 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2
-0+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);
Quoted 7 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_remove(struct platform_device · line 272
Quoted 2 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2
-0+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 fromacpi_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?
Quoted 4 lines
Must be something to do with
drivers/acpi/acpi_lpss.cand notifier calls fromdrivers/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.
Quoted 3 lines
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
PATCH 4/10 i2c: designware-platdrv: Disable autosuspend in error path in ->probe()
In reply to Ulf HanssonDisable runtime PM autosuspend in the error path in ->probe() to make sure it's disabled at the next probe attempt.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 1 + 1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-designware-platdrv.c +1 -0
@@ -249,6 +249,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)if (!IS_ERR (dev->clk))clk_disable_unprepare (dev->clk);pm_runtime_disable (&pdev->dev); +pm_runtime_dont_use_autosuspend (&pdev->dev);if (dev->pm_runtime_disabled)pm_runtime_put_noidle (&pdev->dev); }
Signature · 1 line
1.9.1
PATCH 5/10 i2c: designware-platdrv: Fix clk gating in ->remove()
In reply to Ulf HanssonThe current approach to gate the clock in ->remove() relies on CONFIG_PM to be set, as it's expected that the call to pm_runtime_put_sync() triggers the ->runtime_suspend() callback to be invoked.
Even in the case when CONFIG_PM is set, userspace may prevent runtime PM suspend via sysfs (pm_runtime_forbid()) for the device.
Fix the behaviour by converting from using pm_runtime_put_sync() into directly using the clk API to manage clock gating. Let's also update the runtime PM status for the device as to reflect the state of the HW.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +8 -3
@@ -268,11 +268,16 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)i2c_dw_disable (dev); +if (!IS_ERR (dev->clk)) +clk_disable_unprepare (dev->clk); + +pm_runtime_disable (&pdev->dev); +pm_runtime_dont_use_autosuspend (&pdev->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); -pm_runtime_disable (&pdev->dev); + +pm_runtime_put_noidle (&pdev->dev); +pm_runtime_set_suspended (&pdev->dev);return 0 ; }
Signature · 1 line
1.9.1
PATCH 6/10 i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
In reply to Ulf HanssonTo avoid the device to be immediately runtime suspended when ->probe() returns, update the last busy mark to use the delay from autosuspend.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 1 + 1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-designware-platdrv.c +1 -0
@@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)if (dev->pm_runtime_disabled)pm_runtime_put_noidle (&pdev->dev); } +pm_runtime_mark_last_busy (&pdev->dev);pm_runtime_put (&pdev->dev);return r;
Signature · 1 line
1.9.1
PATCH 6/10 i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
Quoted 13 lines · Ulf Hansson
To avoid the device to be immediately runtime suspended when ->probe() returns, update the last busy mark to use the delay from autosuspend.
Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 1 + 1 file changed, 1 insertion(+)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 258
Quoted 6 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1
-0@@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)if (dev->pm_runtime_disabled)pm_runtime_put_noidle (&pdev->dev); } +pm_runtime_mark_last_busy (&pdev->dev);pm_runtime_put (&pdev->dev);
I was thinking this too earlier shortly while doing cd998ded5c12 ("i2c:
designware: Prevent runtime suspend during adapter registration") but decided to keep suspending immediately after probe is better in cases when there are no slaves connected to a bus or if they start communicating after host auto-suspend timeout has already fired.
But I don't have strong opinion against this.
Signature · 1 line
Jarkko
PATCH 7/10 i2c: designware-platdrv: Re-init the HW when resuming
In reply to Ulf HanssonIn cases when the designware specific flag "pm_runtime_disable" is set, the HW becomes disabled during system PM suspend. Let's make sure it becomes operational when resuming by re-initialize it again.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +1 -3
@@ -337,9 +337,7 @@ static int dw_i2c_plat_resume(struct device *dev)struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev);i2c_dw_plat_prepare_clk (i_dev, true); - -if (!i_dev->pm_runtime_disabled) -i2c_dw_init (i_dev); +i2c_dw_init (i_dev);return 0 ; }
Signature · 1 line
1.9.1
PATCH 8/10 i2c: designware-platdrv: Check return value from clk_prepare_enable()
In reply to Ulf HanssonAs clk_prepare_enable() can fail, let's not ignore the error in the ->runtime_resume() callback, but instead propagate it.
This change decreases the callers of i2c_dw_plat_prepare_clk() down to one, which seems like a justification of removing it. Instead let's call clk_disable_unprepare() directly when needed.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +10 -15
@@ -308,25 +308,15 @@ static void dw_i2c_plat_complete(struct device *dev)#endif #ifdef CONFIG_PM -static int i2c_dw_plat_prepare_clk (struct dw_i2c_dev *i_dev,bool prepare) -{ -if (IS_ERR (i_dev->clk)) -return PTR_ERR (i_dev->clk); - -if (prepare) -return clk_prepare_enable (i_dev->clk); - -clk_disable_unprepare (i_dev->clk); -return 0 ; -} -static int dw_i2c_plat_suspend (struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev);i2c_dw_disable (i_dev); -i2c_dw_plat_prepare_clk (i_dev, false); + +if (!IS_ERR (i_dev->clk)) +clk_disable_unprepare (i_dev->clk);return 0 ; }
@@ -335,10 +325,15 @@ static int dw_i2c_plat_resume(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev); +int ret; -i2c_dw_plat_prepare_clk (i_dev, true); -i2c_dw_init (i_dev); +if (!IS_ERR (i_dev->clk)) { + ret =clk_prepare_enable (i_dev->clk); +if (ret) +return ret; + } +i2c_dw_init (i_dev);return 0 ; }
Signature · 1 line
1.9.1
PATCH 9/10 i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
In reply to Ulf Hansson--- drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +2 -4
@@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)#ifdef CONFIG_PM static int dw_i2c_plat_suspend (struct device *dev) { -struct platform_device *pdev =to_platform_device (dev); -struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev); +struct dw_i2c_dev *i_dev =dev_get_drvdata (dev);i2c_dw_disable (i_dev);
@@ -323,8 +322,7 @@ static int dw_i2c_plat_suspend(struct device *dev)static int dw_i2c_plat_resume (struct device *dev) { -struct platform_device *pdev =to_platform_device (dev); -struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev); +struct dw_i2c_dev *i_dev =dev_get_drvdata (dev);int ret;if (!IS_ERR (i_dev->clk)) {
Signature · 1 line
1.9.1
PATCH 9/10 i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
Quoted 10 lines · Ulf Hansson
Signed-off-by Ulf Hansson <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0
drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_plat_complete(struct device *dev) · line 314
Quoted 7 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1
-2@@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)#ifdef CONFIG_PM static int dw_i2c_plat_suspend (struct device *dev) { -struct platform_device *pdev =to_platform_device (dev); -struct dw_i2c_dev *i_dev =platform_get_drvdata (pdev); +struct dw_i2c_dev *i_dev =dev_get_drvdata (dev);
PATCH 10/10 i2c: designware-platdrv: Rework system PM support
In reply to Ulf HanssonThe current code that deploys the system PM support relies on the "direct_complete" feature supported by the PM core. The goal is to avoid performing unnecessary operations during the system PM sequence.
Unfortunate in this case there are some drawbacks with this solution as explained below.
1)
In case of the ->prepare() callback find the device runtime resumed it returns 0. The PM core will then run the regular set of the system PM callbacks for the device, to allow it to be suspended.
Under these circumstances the device also becomes unconditionally resumed during the system PM resume sequence. It would be better to postpone the resume operations to be managed by runtime PM and thus only when actually needed.
2)
It's good practice to keep the device's runtime PM status in sync with the the current state of the HW. In the same scenario as described in 1), the runtime PM status are RPM_ACTIVE the period in-between when the ->suspend() and ->resume() callbacks are invoked. This is wrong because the device has actually been put into a low power state.
To address the limitation in 2) and to simplify the system PM code, let's convert to deploy the so called runtime PM centric approach.
This is done by assigning the system PM ->suspend|resume() callbacks to the pm_runtime_force_suspend|resume() helper functions. In this way, the ->prepare() and ->complete() callbacks can also be removed.
Currently pm_runtime_force_resume() is also unconditionally resuming the device, which is due to legacy reasons when CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP co-existed. Changing that behaviour is a reasonable improvement to make and it would also resolve the limitation in 1).
--- drivers/i2c/busses/i2c-designware-platdrv.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +7 -22
@@ -291,24 +291,8 @@ static const struct of_device_id dw_i2c_of_match[] = {MODULE_DEVICE_TABLE (of, dw_i2c_of_match);#endif -#ifdef CONFIG_PM_SLEEP -static int dw_i2c_plat_prepare (struct device *dev) -{ -return pm_runtime_suspended (dev); -} - -static void dw_i2c_plat_complete (struct device *dev) -{ -if (dev->power.direct_complete) -pm_request_resume (dev); -} -#else -#define dw_i2c_plat_prepare NULL -#define dw_i2c_plat_complete NULL -#endif -#ifdef CONFIG_PM -static int dw_i2c_plat_suspend (struct device *dev) +static int dw_i2c_plat_runtime_suspend (struct device *dev) {struct dw_i2c_dev *i_dev =dev_get_drvdata (dev);
@@ -320,7 +304,7 @@ static int dw_i2c_plat_suspend(struct device *dev)return 0 ; } -static int dw_i2c_plat_resume (struct device *dev) +static int dw_i2c_plat_runtime_resume (struct device *dev) {struct dw_i2c_dev *i_dev =dev_get_drvdata (dev);int ret;
@@ -336,10 +320,11 @@ static int dw_i2c_plat_resume(struct device *dev) }static const struct dev_pm_ops dw_i2c_dev_pm_ops = { - .prepare = dw_i2c_plat_prepare, - .complete = dw_i2c_plat_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) -SET_RUNTIME_PM_OPS (dw_i2c_plat_suspend, dw_i2c_plat_resume,NULL ) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) +SET_RUNTIME_PM_OPS (dw_i2c_plat_runtime_suspend, + dw_i2c_plat_runtime_resume, +NULL ) };#define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
Signature · 1 line
1.9.1
Andy Shevchenko
reviewerPATCH 10/10 i2c: designware-platdrv: Rework system PM support
Quoted 19 lines · Ulf Hansson
The current code that deploys the system PM support relies on the "
direct_complete" feature supported by the PM core. The goal is to avoid
performing unnecessary operations during the system PM sequence.static const struct
dev_pm_opsdw_i2c_dev_pm_ops= {drivers/i2c/busses/i2c-designware-platdrv.c +0
-3- .prepare = dw_i2c_plat_prepare, - .complete = dw_i2c_plat_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
dw_i2c_plat_resume)(quoted diff) +0
-1- SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume,NULL)
drivers/i2c/busses/i2c-designware-platdrv.c +5
-0+SET_SYSTEM_SLEEP_PM_OPS (pm_runtime_force_suspend, + pm_runtime_force_resume) +SET_RUNTIME_PM_OPS (dw_i2c_plat_runtime_suspend, + dw_i2c_plat_runtime_resume, +NULL )
Quoted 1 line
};
UNIVERSAL_PM_OPS ?
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH 10/10 i2c: designware-platdrv: Rework system PM support
Quoted 22 lines · Andy Shevchenko
Quoted 14 lines · Ulf Hansson
The current code that deploys the system PM support relies on the "
direct_complete" feature supported by the PM core. The goal is to avoid
performing unnecessary operations during the system PM sequence.static const struct
dev_pm_opsdw_i2c_dev_pm_ops= {drivers/i2c/busses/i2c-designware-platdrv.c +0
-3- .prepare = dw_i2c_plat_prepare, - .complete = dw_i2c_plat_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
dw_i2c_plat_resume)
- SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend,dw_i2c_plat_resume,
NULL)drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_resume(struct device *dev) · line 328
Quoted 6 lines
drivers/i2c/busses/i2c-designware-platdrv.c +5
-0+SET_SYSTEM_SLEEP_PM_OPS (pm_runtime_force_suspend, + pm_runtime_force_resume) +SET_RUNTIME_PM_OPS (dw_i2c_plat_runtime_suspend, + dw_i2c_plat_runtime_resume, + NULL) };
Quoted 1 line
UNIVERSAL_PM_OPS?
The UNIVERSAL_DEV_PM_OPS assigns the same callbacks for system PM as runtime PM, that doesn't work here.
Kind regards
Uffe
Andy Shevchenko
reviewercover i2c: designware-platdrv: Some improvments related to PM
In reply to Ulf HanssonQuoted 6 lines · Ulf Hansson
Here's a couple changes for the i2c-designware driver. Most of them a related to
the support for runtime PM and system PM, but there's also a few that improves
some error handling.
Quoted 6 lines
I have tested these on Hisilicon Linaro 96-board (hi6220). I used a couple local
changes to enable the power-key to act as a wakeup in system PM suspend state.
If anyone are interested about those as well, I am happy to share them.
I know Jarkko spent a lot to understand PM flow in this driver.
My overall feelings after brief reading of the series you fixed a particular problem with your device or flow, which might have broken the half of current users. So, I wouldn't take this without Tested-by tags of (almost) all active stakeholders.
Quoted 21 lines
Ulf Hansson (10):
i2c: designware-platdrv: Return error in ->probe() when clk ungate fails
i2c: designware-platdrv: Gate clk in error path in ->probe()
i2c: designware-platdrv: Unconditionally enable runtime PM
i2c: designware-platdrv: Disable autosuspend in error path in ->probe()
i2c: designware-platdrv: Fix clk gating in ->remove()
i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
i2c: designware-platdrv: Re-init the HW when resuming
i2c: designware-platdrv: Check return value fromclk_prepare_enable()
i2c: designware-platdrv: Simplify code by usingdev_get_drvdata()
i2c: designware-platdrv: Rework system PM supportdrivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++----
-----------1 file changed, 50 insertions(+), 56 deletions(-)
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
cover i2c: designware-platdrv: Some improvments related to PM
+ Rafael, Kevin, Tomeu
Quoted 14 lines · Andy Shevchenko
Quoted 6 lines · Ulf Hansson
Here's a couple changes for the i2c-designware driver. Most of them a related to
the support for runtime PM and system PM, but there's also a few that improves
some error handling.Quoted 6 lines
I have tested these on Hisilicon Linaro 96-board (hi6220). I used a couple local
changes to enable the power-key to act as a wakeup in system PM suspend state.
If anyone are interested about those as well, I am happy to share them.
Quoted 6 lines
I know Jarkko spent a lot to understand PM flow in this driver.
My overall feelings after brief reading of the series you fixed a particular problem with your device or flow, which might have broken the half of current users. So, I wouldn't take this without Tested-by tags of (almost) all active stakeholders.
That makes sense to me. Should I resend to include some more people?
Let me also elaborate a bit more of the reason behind this series.
I was using a 4.4 kernel on my Hikey board and testing system PM suspend/resume, when I found this driver to be broken. Not only broken for the Hikey board but for *all* users of this driver!
Without going into too much details, the problem was because of how the "direct_complete" feature was being used and supported by the PM core. In the case when the driver's ->prepare() callback found the device runtime suspended and thus returning '1' to allow the PM core to try "direct_complete" for the device, it caused the driver's ->suspend() callback to be invoked twice in a row. This causes a clk reference count imbalance issue and messing up system PM suspend/resume.
Therefore I started piece by piece improving the error handling, system PM code and runtime PM code to narrow down the problem. This series solves the problem on a 4.4 based kernel, however I also realized that the issue became resolved already in 4.5 kernel because of the following commit:
commit aa8e54b559479d0cb7eb632ba443b8cacd20cd4b
Author: Tomeu Vizoso <[email protected]> Date: Thu Jan 7 16:46:14 2016 +0100
PM / sleep: Go direct_complete if driver has no callbacks
If a suitable prepare callback cannot be found for a given device and its driver has no PM callbacks at all, assume that it can go direct to complete when the system goes to sleep.
The reason for this is that there's lots of devices in a system that do no PM at all and there's no reason for them to prevent their ancestors to do direct_complete if they can support it.
I believe this commit solved the problem more as of a co-incidence, than actually intended to be a fix for a regression. Perhaps I should send this patch to "stable" as well.
Anyway, I still think the series I have posted here make some nice improvement to the driver.
Kind regards
Uffe
Quoted 27 lines
Quoted 18 lines
Ulf Hansson (10):
i2c: designware-platdrv: Return error in ->probe() when clk ungate
fails
i2c: designware-platdrv: Gate clk in error path in ->probe()
i2c: designware-platdrv: Unconditionally enable runtime PM
i2c: designware-platdrv: Disable autosuspend in error path in->probe()i2c: designware-platdrv: Fix clk gating in ->remove()
i2c: designware-platdrv: Update runtime PM last busy mark in
->probe()
i2c: designware-platdrv: Re-init the HW when resuming
i2c: designware-platdrv: Check return value fromclk_prepare_enable()
i2c: designware-platdrv: Simplify code by usingdev_get_drvdata()
i2c: designware-platdrv: Rework system PM supportQuoted 4 lines
drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++---- ----------- 1 file changed, 50 insertions(+), 56 deletions(-)--
Andy Shevchenko <[email protected]> Intel Finland Oy