| Message ID | 1415802748-30530-6-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > Add system suspend/resume capabilities to the pl330 driver so the amba > bus clock could be also unprepared to conserve energy. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index c3bd3584f261..fd71777fc565 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > return 0; > } > > +/* > + * Runtime PM callbacks are provided by amba/bus.c driver. > + * > + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba > + * bus driver will only disable/enable the clock in runtime PM callbacks. > + */ > +static int __maybe_unused pl330_suspend(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + > + if (!pm_runtime_status_suspended(dev)) { No, this wont work. Consider this scenario: pm_runtime_status_suspended() returns "true", which means you consider the device to be runtime PM suspended, thus you will unprepare the clock below. Later, a call to pm_runtime_get_sync() is invoked from "somewhere" and before this driver's system PM resume callback has been invoked. That will trigger the runtime PM resume callback to be invoked, causing clock unbalance issues. You need a pm_runtime_disable() prior checking the runtime PM status using pm_runtime_status_suspended(). That will prevent this from happen. I guess you may see where this is leading. I think pm_runtime_force_suspend|resume() is well suited for this situation. > + /* amba did not disable the clock */ > + amba_pclk_disable(pcdev); > + } > + amba_pclk_unprepare(pcdev); > + > + return 0; > +} > + > +static int __maybe_unused pl330_resume(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + int ret; > + > + ret = amba_pclk_prepare(pcdev); > + if (ret) > + return ret; > + > + if (!pm_runtime_status_suspended(dev)) > + ret = amba_pclk_enable(pcdev); > + > + return ret; > +} > + > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); > + > static int > pl330_probe(struct amba_device *adev, const struct amba_id *id) > { > @@ -2853,6 +2889,7 @@ static struct amba_driver pl330_driver = { > .drv = { > .owner = THIS_MODULE, > .name = "dma-pl330", > + .pm = &pl330_pm, > }, > .id_table = pl330_ids, > .probe = pl330_probe, > -- > 1.9.1 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ?ro, 2014-11-12 at 16:28 +0100, Ulf Hansson wrote: > On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > Add system suspend/resume capabilities to the pl330 driver so the amba > > bus clock could be also unprepared to conserve energy. > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index c3bd3584f261..fd71777fc565 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > > return 0; > > } > > > > +/* > > + * Runtime PM callbacks are provided by amba/bus.c driver. > > + * > > + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba > > + * bus driver will only disable/enable the clock in runtime PM callbacks. > > + */ > > +static int __maybe_unused pl330_suspend(struct device *dev) > > +{ > > + struct amba_device *pcdev = to_amba_device(dev); > > + > > + if (!pm_runtime_status_suspended(dev)) { > > No, this wont work. Consider this scenario: > > pm_runtime_status_suspended() returns "true", which means you consider > the device to be runtime PM suspended, thus you will unprepare the > clock below. > > Later, a call to pm_runtime_get_sync() is invoked from "somewhere" and > before this driver's system PM resume callback has been invoked. That > will trigger the runtime PM resume callback to be invoked, causing > clock unbalance issues. If during system resume the device runtime resume is called BEFORE normal resume of device, then you're right: clock will be unbalanced. But such case does not make sense. pm_runtime_get_sync() is called when someone wants to use the dmaengine/pl330... but the pl330 driver is still suspended in that time! If runtime resume is called AFTER normal resume of device - then it is fine with my code. > You need a pm_runtime_disable() prior checking the runtime PM status > using pm_runtime_status_suspended(). That will prevent this from > happen. > > I guess you may see where this is leading. I think > pm_runtime_force_suspend|resume() is well suited for this situation. I've been there... :) -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote: > Add system suspend/resume capabilities to the pl330 driver so the amba > bus clock could be also unprepared to conserve energy. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index c3bd3584f261..fd71777fc565 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > return 0; > } > > +/* > + * Runtime PM callbacks are provided by amba/bus.c driver. > + * > + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba > + * bus driver will only disable/enable the clock in runtime PM callbacks. > + */ > +static int __maybe_unused pl330_suspend(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + > + if (!pm_runtime_status_suspended(dev)) { It generally isn't safe to call that in .suspend(), because the status may still change in theory. On the other hand, if you do that in .suspend_late(), you'll be safe from that. > + /* amba did not disable the clock */ > + amba_pclk_disable(pcdev); > + } > + amba_pclk_unprepare(pcdev); > + > + return 0; > +} > + > +static int __maybe_unused pl330_resume(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + int ret; > + > + ret = amba_pclk_prepare(pcdev); > + if (ret) > + return ret; > + > + if (!pm_runtime_status_suspended(dev)) > + ret = amba_pclk_enable(pcdev); > + > + return ret; > +} > + > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); > + > static int > pl330_probe(struct amba_device *adev, const struct amba_id *id) > { > @@ -2853,6 +2889,7 @@ static struct amba_driver pl330_driver = { > .drv = { > .owner = THIS_MODULE, > .name = "dma-pl330", > + .pm = &pl330_pm, > }, > .id_table = pl330_ids, > .probe = pl330_probe, >
On 13 November 2014 02:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote: >> Add system suspend/resume capabilities to the pl330 driver so the amba >> bus clock could be also unprepared to conserve energy. >> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> --- >> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 37 insertions(+) >> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> index c3bd3584f261..fd71777fc565 100644 >> --- a/drivers/dma/pl330.c >> +++ b/drivers/dma/pl330.c >> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, >> return 0; >> } >> >> +/* >> + * Runtime PM callbacks are provided by amba/bus.c driver. >> + * >> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba >> + * bus driver will only disable/enable the clock in runtime PM callbacks. >> + */ >> +static int __maybe_unused pl330_suspend(struct device *dev) >> +{ >> + struct amba_device *pcdev = to_amba_device(dev); >> + >> + if (!pm_runtime_status_suspended(dev)) { > > It generally isn't safe to call that in .suspend(), because the status may still > change in theory. On the other hand, if you do that in .suspend_late(), you'll > be safe from that. > Just to clarify that statement; it's safe in a ->suspend_late() callback, because runtime PM has been disabled by the PM core. That's also the reason why the pm_runtime_force_suspend() helper is disabling runtime PM, such it may be used in some of the earlier phases of the system PM callbacks. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On czw, 2014-11-13 at 10:03 +0100, Ulf Hansson wrote: > On 13 November 2014 02:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote: > >> Add system suspend/resume capabilities to the pl330 driver so the amba > >> bus clock could be also unprepared to conserve energy. > >> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> --- > >> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 37 insertions(+) > >> > >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > >> index c3bd3584f261..fd71777fc565 100644 > >> --- a/drivers/dma/pl330.c > >> +++ b/drivers/dma/pl330.c > >> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > >> return 0; > >> } > >> > >> +/* > >> + * Runtime PM callbacks are provided by amba/bus.c driver. > >> + * > >> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba > >> + * bus driver will only disable/enable the clock in runtime PM callbacks. > >> + */ > >> +static int __maybe_unused pl330_suspend(struct device *dev) > >> +{ > >> + struct amba_device *pcdev = to_amba_device(dev); > >> + > >> + if (!pm_runtime_status_suspended(dev)) { > > > > It generally isn't safe to call that in .suspend(), because the status may still > > change in theory. On the other hand, if you do that in .suspend_late(), you'll > > be safe from that. > > > > Just to clarify that statement; it's safe in a ->suspend_late() > callback, because runtime PM has been disabled by the PM core. > > That's also the reason why the pm_runtime_force_suspend() helper is > disabling runtime PM, such it may be used in some of the earlier > phases of the system PM callbacks. So actually only pm_runtime_disable() is missing here (as you mentioned earlier)? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index c3bd3584f261..fd71777fc565 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, return 0; } +/* + * Runtime PM callbacks are provided by amba/bus.c driver. + * + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba + * bus driver will only disable/enable the clock in runtime PM callbacks. + */ +static int __maybe_unused pl330_suspend(struct device *dev) +{ + struct amba_device *pcdev = to_amba_device(dev); + + if (!pm_runtime_status_suspended(dev)) { + /* amba did not disable the clock */ + amba_pclk_disable(pcdev); + } + amba_pclk_unprepare(pcdev); + + return 0; +} + +static int __maybe_unused pl330_resume(struct device *dev) +{ + struct amba_device *pcdev = to_amba_device(dev); + int ret; + + ret = amba_pclk_prepare(pcdev); + if (ret) + return ret; + + if (!pm_runtime_status_suspended(dev)) + ret = amba_pclk_enable(pcdev); + + return ret; +} + +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); + static int pl330_probe(struct amba_device *adev, const struct amba_id *id) { @@ -2853,6 +2889,7 @@ static struct amba_driver pl330_driver = { .drv = { .owner = THIS_MODULE, .name = "dma-pl330", + .pm = &pl330_pm, }, .id_table = pl330_ids, .probe = pl330_probe,
Add system suspend/resume capabilities to the pl330 driver so the amba bus clock could be also unprepared to conserve energy. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)