| Message ID | 1431693099-2292-1-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 15, 2015 at 08:31:39PM +0800, Jisheng Zhang wrote: > Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds > runtime pm support using the same ops for system sleep and runtime pm. > When suspend to ram, the i2c host may have been runtime suspended, thus > i2c_dw_disable() hangs. It hangs because it has already been powered off, right? > This patch fixes this issue by separating ops for system sleep pm and > runtime pm, and in the system suspend/resume path, runtime pm apis are > used to ensure the device is at correct state. I can see that this fixes the issue with the platform driver (as the platform bus core doesn't power on the device automatically as opposed to other buses, like PCI). However, I'm thinking that can we do better here. Instead of powering the device on again, can't we leave it in low power state? Recently added 'dev->power.direct_complete' may be used to achieve that, I think.
Dear Mika, On Mon, 18 May 2015 11:28:23 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Fri, May 15, 2015 at 08:31:39PM +0800, Jisheng Zhang wrote: > > Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds > > runtime pm support using the same ops for system sleep and runtime pm. > > When suspend to ram, the i2c host may have been runtime suspended, thus > > i2c_dw_disable() hangs. > > It hangs because it has already been powered off, right? Either be powered off or clock gated or even both. > > > This patch fixes this issue by separating ops for system sleep pm and > > runtime pm, and in the system suspend/resume path, runtime pm apis are > > used to ensure the device is at correct state. > > I can see that this fixes the issue with the platform driver (as the > platform bus core doesn't power on the device automatically as opposed > to other buses, like PCI). However, I'm thinking that can we do better > here. > > Instead of powering the device on again, can't we leave it in low power > state? Recently added 'dev->power.direct_complete' may be used to > achieve that, I think. how to handle runtime suspended via just being clock gated? Currently the only solution is using the runtime pm apis to ensure the device is in a working state during s2ram. What's your opinion? Thanks, Jisheng
On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote: > > I can see that this fixes the issue with the platform driver (as the > > platform bus core doesn't power on the device automatically as opposed > > to other buses, like PCI). However, I'm thinking that can we do better > > here. > > > > Instead of powering the device on again, can't we leave it in low power > > state? Recently added 'dev->power.direct_complete' may be used to > > achieve that, I think. > > how to handle runtime suspended via just being clock gated? As far as I can tell driver's suspend hook does the clock gating so why would you need to handle it differently? Once the device is runtime suspended, it is both clock and power gated depending on the platform. > Currently the only solution is using the runtime pm apis to ensure the > device is in a working state during s2ram. What's your opinion? Well, it sounds a bit silly to resume the device just because you want to call i2c_dw_disable() for it before suspending again ;-) At least, it would be nice to check if we can keep it runtime suspended with the help of 'dev->power.direct_complete'. If not possible, then I suppose your patch is sufficient.
Dear Mika, On Tue, 19 May 2015 16:15:16 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote: > > > I can see that this fixes the issue with the platform driver (as the > > > platform bus core doesn't power on the device automatically as opposed > > > to other buses, like PCI). However, I'm thinking that can we do better > > > here. > > > > > > Instead of powering the device on again, can't we leave it in low power > > > state? Recently added 'dev->power.direct_complete' may be used to > > > achieve that, I think. > > > > how to handle runtime suspended via just being clock gated? > > As far as I can tell driver's suspend hook does the clock gating so why > would you need to handle it differently? Once the device is runtime > suspended, it is both clock and power gated depending on the platform. Sorry for confusion. Considering one platform which doesn't support power off the i2c host but it can disable the host's clock. So in such platform, when the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will hang when s2ram. Except using the runtime pm API to ensure the host is in a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' doesn't help such case. > > > Currently the only solution is using the runtime pm apis to ensure the > > device is in a working state during s2ram. What's your opinion? > > Well, it sounds a bit silly to resume the device just because you want > to call i2c_dw_disable() for it before suspending again ;-) > Agree. But it's the only solution I know so far. > At least, it would be nice to check if we can keep it runtime suspended > with the help of 'dev->power.direct_complete'. If not possible, then I > suppose your patch is sufficient. Thanks, Jisheng
Dear Mika, On Wed, 20 May 2015 19:34:22 +0800 Jisheng Zhang <jszhang@marvell.com> wrote: > Dear Mika, > > On Tue, 19 May 2015 16:15:16 +0300 > Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote: > > > > I can see that this fixes the issue with the platform driver (as the > > > > platform bus core doesn't power on the device automatically as opposed > > > > to other buses, like PCI). However, I'm thinking that can we do better > > > > here. > > > > > > > > Instead of powering the device on again, can't we leave it in low power > > > > state? Recently added 'dev->power.direct_complete' may be used to > > > > achieve that, I think. > > > > > > how to handle runtime suspended via just being clock gated? > > > > As far as I can tell driver's suspend hook does the clock gating so why > > would you need to handle it differently? Once the device is runtime > > suspended, it is both clock and power gated depending on the platform. > > Sorry for confusion. Considering one platform which doesn't support power off > the i2c host but it can disable the host's clock. So in such platform, when > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will > hang when s2ram. Except using the runtime pm API to ensure the host is in > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' > doesn't help such case. I misunderstood the direct_complete flag usage. I think it can help such case will investigate and provide new patch if necessary. Thanks a lot
On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote: > Sorry for confusion. Considering one platform which doesn't support power off > the i2c host but it can disable the host's clock. So in such platform, when > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will > hang when s2ram. Right. This happens also when the platform powers off the device. > Except using the runtime pm API to ensure the host is in > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' > doesn't help such case. What I had in mind is something like below: static int i2c_dw_prepare(struct device *dev) { return pm_runtime_suspended(dev); } static void i2c_dw_complete(struct device *dev) { if (dev->power.direct_complete) pm_request_resume(dev); } In other words it checks if the device is already runtime suspended and prevents ->suspend() etc. from being called. If that does not work (I didn't try as this problem does not exist on ACPI platforms because we have PM domain handling things) then we do not have much of a choice than do what you suggest and runtime resume the device on ->suspend()
Dear Mika, On Wed, 20 May 2015 15:15:06 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote: > > Sorry for confusion. Considering one platform which doesn't support power off > > the i2c host but it can disable the host's clock. So in such platform, when > > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will > > hang when s2ram. > > Right. This happens also when the platform powers off the device. > > > Except using the runtime pm API to ensure the host is in > > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' > > doesn't help such case. > > What I had in mind is something like below: > > static int i2c_dw_prepare(struct device *dev) > { > return pm_runtime_suspended(dev); > } > > static void i2c_dw_complete(struct device *dev) > { > if (dev->power.direct_complete) > pm_request_resume(dev); > } > > In other words it checks if the device is already runtime suspended and > prevents ->suspend() etc. from being called. What amazing! I wrote the same code as yours after sending out the last email. > > If that does not work (I didn't try as this problem does not exist on It works! How to submit the patch? Do you mind if I cook the patch and add you signed-off? Thanks a lot, Jisheng
On Wed, 20 May 2015 20:34:30 +0800 Jisheng Zhang <jszhang@marvell.com> wrote: > Dear Mika, > > On Wed, 20 May 2015 15:15:06 +0300 > Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote: > > > Sorry for confusion. Considering one platform which doesn't support power off > > > the i2c host but it can disable the host's clock. So in such platform, when > > > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will > > > hang when s2ram. > > > > Right. This happens also when the platform powers off the device. > > > > > Except using the runtime pm API to ensure the host is in > > > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' > > > doesn't help such case. > > > > What I had in mind is something like below: > > > > static int i2c_dw_prepare(struct device *dev) > > { > > return pm_runtime_suspended(dev); > > } > > > > static void i2c_dw_complete(struct device *dev) > > { > > if (dev->power.direct_complete) > > pm_request_resume(dev); > > } > > > > In other words it checks if the device is already runtime suspended and > > prevents ->suspend() etc. from being called. > > What amazing! I wrote the same code as yours after sending out the last email. > > > > > If that does not work (I didn't try as this problem does not exist on > > It works! How to submit the patch? Do you mind if I cook the patch and add > you signed-off? > PS: If you cook the patch instead, feel free to add my acked-by and tested-by Thanks a lot for the direct_complete idea, Jisheng
On Wed, May 20, 2015 at 08:34:30PM +0800, Jisheng Zhang wrote: > Dear Mika, > > On Wed, 20 May 2015 15:15:06 +0300 > Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote: > > > Sorry for confusion. Considering one platform which doesn't support power off > > > the i2c host but it can disable the host's clock. So in such platform, when > > > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will > > > hang when s2ram. > > > > Right. This happens also when the platform powers off the device. > > > > > Except using the runtime pm API to ensure the host is in > > > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete' > > > doesn't help such case. > > > > What I had in mind is something like below: > > > > static int i2c_dw_prepare(struct device *dev) > > { > > return pm_runtime_suspended(dev); > > } > > > > static void i2c_dw_complete(struct device *dev) > > { > > if (dev->power.direct_complete) > > pm_request_resume(dev); > > } > > > > In other words it checks if the device is already runtime suspended and > > prevents ->suspend() etc. from being called. > > What amazing! I wrote the same code as yours after sending out the last email. Cool :) > > > > If that does not work (I didn't try as this problem does not exist on > > It works! How to submit the patch? Do you mind if I cook the patch and add > you signed-off? That's fine and you don't need to add my SoB there. I'll test the patch here on some our platforms to ensure it does not break anything and if it turns out working you can have my Tested-by then.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 0a80e4a..d306397 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -298,14 +298,16 @@ static const struct of_device_id dw_i2c_of_match[] = { MODULE_DEVICE_TABLE(of, dw_i2c_of_match); #endif -#ifdef CONFIG_PM +#ifdef CONFIG_PM_SLEEP static int dw_i2c_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + pm_runtime_get_sync(dev); i2c_dw_disable(i_dev); - clk_disable_unprepare(i_dev->clk); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); return 0; } @@ -315,6 +317,32 @@ static int dw_i2c_resume(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + pm_runtime_get_sync(dev); + i2c_dw_init(i_dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return 0; +} +#endif + +#ifdef CONFIG_PM +static int dw_i2c_runtime_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); + clk_disable_unprepare(i_dev->clk); + + return 0; +} + +static int dw_i2c_runtime_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + clk_prepare_enable(i_dev->clk); if (!i_dev->pm_runtime_disabled) @@ -324,8 +352,18 @@ static int dw_i2c_resume(struct device *dev) } #endif -static UNIVERSAL_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, - dw_i2c_resume, NULL); +#ifdef CONFIG_PM +static const struct dev_pm_ops dw_i2c_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume) + SET_RUNTIME_PM_OPS(dw_i2c_runtime_suspend, + dw_i2c_runtime_resume, NULL) +}; + +#define DW_I2C_DEV_PM_OPS (&dw_i2c_dev_pm_ops) + +#else +#define DW_I2C_DEV_PM_OPS NULL +#endif /* work with hotplug and coldplug */ MODULE_ALIAS("platform:i2c_designware"); @@ -337,7 +375,7 @@ static struct platform_driver dw_i2c_driver = { .name = "i2c_designware", .of_match_table = of_match_ptr(dw_i2c_of_match), .acpi_match_table = ACPI_PTR(dw_i2c_acpi_match), - .pm = &dw_i2c_dev_pm_ops, + .pm = DW_I2C_DEV_PM_OPS, }, };
Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds runtime pm support using the same ops for system sleep and runtime pm. When suspend to ram, the i2c host may have been runtime suspended, thus i2c_dw_disable() hangs. This patch fixes this issue by separating ops for system sleep pm and runtime pm, and in the system suspend/resume path, runtime pm apis are used to ensure the device is at correct state. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- drivers/i2c/busses/i2c-designware-platdrv.c | 48 ++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 5 deletions(-)