| Message ID | 1415955033-29093-6-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 14 November 2014 09:50, 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 | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index c3bd3584f261..e499bb118f0a 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -2627,6 +2627,46 @@ 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); > + > + pm_runtime_disable(dev); > + > + if (!pm_runtime_status_suspended(dev)) { > + /* amba did not disable the clock */ > + amba_pclk_disable(pcdev); > + } > + amba_pclk_unprepare(pcdev); I would also invoke pm_runtime_set_suspended() here, to reflect that's the current runtime PM state of the device. I guess I sounds like broken record :-), but using pm_runtime_force_suspend() would help to prevent some code duplication here. Something like this: pm_runtime_force_suspend() if (pm_runtime_is_irq_safe()) amba_pclk_unprepare(pcdev); Kind regards Uffe > + > + 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); > + > + pm_runtime_enable(dev); > + > + 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 +2893,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 -- 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 pi?, 2014-11-14 at 14:31 +0100, Ulf Hansson wrote: > On 14 November 2014 09:50, 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 | 41 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index c3bd3584f261..e499bb118f0a 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -2627,6 +2627,46 @@ 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); > > + > > + pm_runtime_disable(dev); > > + > > + if (!pm_runtime_status_suspended(dev)) { > > + /* amba did not disable the clock */ > > + amba_pclk_disable(pcdev); > > + } > > + amba_pclk_unprepare(pcdev); > > I would also invoke pm_runtime_set_suspended() here, to reflect that's > the current runtime PM state of the device. Strictly speaking the device is not runtime suspended in that moment. PM runtime callbacks were not called. Although the device status looks like runtime suspended (clocks disabled) but I think the whole goal here was to avoid touching runtime PM because this is system sleep. > I guess I sounds like broken record :-), but using > pm_runtime_force_suspend() would help to prevent some code duplication > here. > > Something like this: > > pm_runtime_force_suspend() > if (pm_runtime_is_irq_safe()) > amba_pclk_unprepare(pcdev); With !CONFIG_PM_RUNTIME this would leave clocks prepared... Thanks for feedback! 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
On 14 November 2014 14:47, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > On pi?, 2014-11-14 at 14:31 +0100, Ulf Hansson wrote: >> On 14 November 2014 09:50, 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 | 41 +++++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 41 insertions(+) >> > >> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> > index c3bd3584f261..e499bb118f0a 100644 >> > --- a/drivers/dma/pl330.c >> > +++ b/drivers/dma/pl330.c >> > @@ -2627,6 +2627,46 @@ 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); >> > + >> > + pm_runtime_disable(dev); >> > + >> > + if (!pm_runtime_status_suspended(dev)) { >> > + /* amba did not disable the clock */ >> > + amba_pclk_disable(pcdev); >> > + } >> > + amba_pclk_unprepare(pcdev); >> >> I would also invoke pm_runtime_set_suspended() here, to reflect that's >> the current runtime PM state of the device. > > Strictly speaking the device is not runtime suspended in that moment. PM > runtime callbacks were not called. Although the device status looks like > runtime suspended (clocks disabled) but I think the whole goal here was > to avoid touching runtime PM because this is system sleep. If someone outside, like a PM domain monitors the runtime PM status of the device it would get the wrong impression of the device. This is quite similar to why the amba bus needs to update the runtime PM status during probe(), using pm_runtime_set_active(). Shouldn't we do that either? > > >> I guess I sounds like broken record :-), but using >> pm_runtime_force_suspend() would help to prevent some code duplication >> here. >> >> Something like this: >> >> pm_runtime_force_suspend() >> if (pm_runtime_is_irq_safe()) >> amba_pclk_unprepare(pcdev); > > With !CONFIG_PM_RUNTIME this would leave clocks prepared... No. pm_runtime_is_irq_safe() returns false, and thus the amba bus do a clk_disable_unprepare() from it's runtime PM suspend callback when it's invoked via pm_runtime_force_suspend(). 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 pi?, 2014-11-14 at 15:22 +0100, Ulf Hansson wrote: > On 14 November 2014 14:47, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > On pi?, 2014-11-14 at 14:31 +0100, Ulf Hansson wrote: > >> On 14 November 2014 09:50, 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 | 41 +++++++++++++++++++++++++++++++++++++++++ > >> > 1 file changed, 41 insertions(+) > >> > > >> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > >> > index c3bd3584f261..e499bb118f0a 100644 > >> > --- a/drivers/dma/pl330.c > >> > +++ b/drivers/dma/pl330.c > >> > @@ -2627,6 +2627,46 @@ 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); > >> > + > >> > + pm_runtime_disable(dev); > >> > + > >> > + if (!pm_runtime_status_suspended(dev)) { > >> > + /* amba did not disable the clock */ > >> > + amba_pclk_disable(pcdev); > >> > + } > >> > + amba_pclk_unprepare(pcdev); > >> > >> I would also invoke pm_runtime_set_suspended() here, to reflect that's > >> the current runtime PM state of the device. > > > > Strictly speaking the device is not runtime suspended in that moment. PM > > runtime callbacks were not called. Although the device status looks like > > runtime suspended (clocks disabled) but I think the whole goal here was > > to avoid touching runtime PM because this is system sleep. > > If someone outside, like a PM domain monitors the runtime PM status of > the device it would get the wrong impression of the device. > > This is quite similar to why the amba bus needs to update the runtime > PM status during probe(), using pm_runtime_set_active(). Shouldn't we > do that either? Hmm... I got your point. However now device is not runtime resumed if it was runtime suspended before (clock is not enabled). The runtime status is preserved between system sleep. Going your way I would have to either: 1. store runtime status somewhere and use it during resume, 2. always runtime resume during resuming (which would be error-prone because there won't be a real runtime resume). Current solution looks nice and simple to me. > >> I guess I sounds like broken record :-), but using > >> pm_runtime_force_suspend() would help to prevent some code duplication > >> here. > >> > >> Something like this: > >> > >> pm_runtime_force_suspend() > >> if (pm_runtime_is_irq_safe()) > >> amba_pclk_unprepare(pcdev); > > > > With !CONFIG_PM_RUNTIME this would leave clocks prepared... > > No. > > pm_runtime_is_irq_safe() returns false, and thus the amba bus do a > clk_disable_unprepare() from it's runtime PM suspend callback when > it's invoked via pm_runtime_force_suspend(). Aaa, right! But during system resume that would leave the device always runtime resumed. Now we can avoid this :). Anyway this is a conceptual discussion. I don't mind the force-suspend way but I got an impression that current way was preferred (separated sleep and runtime paths). 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..e499bb118f0a 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2627,6 +2627,46 @@ 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); + + pm_runtime_disable(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); + + pm_runtime_enable(dev); + + 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 +2893,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 | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)