| Message ID | 1f784b72399e0e96414f00866b209e2e218065af.1347877746.git.vipulkumar.samar@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 17, 2012 at 4:07 PM, Vipul Kumar Samar <vipulkumar.samar@st.com> wrote: > clk_{un}prepare is mandatory for platforms using common clock framework. Add > clk_{un}prepare() support for spi-pl022 runtime PM. You are not calling these routines in actualy patch.. Fix commit log and add my Reviewed-by. > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> > --- > drivers/spi/spi-pl022.c | 9 ++++++--- > 1 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c > index f2a80ff..500e75e 100644 > --- a/drivers/spi/spi-pl022.c > +++ b/drivers/spi/spi-pl022.c > @@ -2334,7 +2334,7 @@ static int pl022_runtime_suspend(struct device *dev) > { > struct pl022 *pl022 = dev_get_drvdata(dev); > > - clk_disable(pl022->clk); > + clk_disable_unprepare(pl022->clk); > > return 0; > } > @@ -2342,10 +2342,13 @@ static int pl022_runtime_suspend(struct device *dev) > static int pl022_runtime_resume(struct device *dev) > { > struct pl022 *pl022 = dev_get_drvdata(dev); > + int ret = 0; > > - clk_enable(pl022->clk); > + ret = clk_prepare_enable(pl022->clk); > + if (ret) > + dev_err(dev, "could not enable SSP/SPI bus clock\n"); > > - return 0; > + return ret; > } > #endif > > -- > 1.7.2.2 > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > spi-devel-general mailing list > spi-devel-general@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Hello. On 17-09-2012 14:37, Vipul Kumar Samar wrote: > clk_{un}prepare is mandatory for platforms using common clock framework. Add > clk_{un}prepare() support for spi-pl022 runtime PM. > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> [...] > @@ -2342,10 +2342,13 @@ static int pl022_runtime_suspend(struct device *dev) > static int pl022_runtime_resume(struct device *dev) > { > struct pl022 *pl022 = dev_get_drvdata(dev); > + int ret = 0; Don't need to init it at all. > - clk_enable(pl022->clk); > + ret = clk_prepare_enable(pl022->clk); > + if (ret) > + dev_err(dev, "could not enable SSP/SPI bus clock\n"); > > - return 0; > + return ret; > } > #endif WBR, Sergei
On Mon, Sep 17, 2012 at 12:37 PM, Vipul Kumar Samar <vipulkumar.samar@st.com> wrote: > clk_{un}prepare is mandatory for platforms using common clock framework. Add > clk_{un}prepare() support for spi-pl022 runtime PM. > > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> This driver does clk_prepare/unprepare at probe and removed, so I guess what you're trying to say is that on your platform the clk_unprepare() process context call is needed to save power? Please elaborate... Yours, Linus Walleij
On Mon, Sep 17, 2012 at 7:09 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > This driver does clk_prepare/unprepare at probe > and removed, so I guess what you're trying to say is that > on your platform the clk_unprepare() process context call > is needed to save power? > > Please elaborate... Hi Linus, Yes, we don't need to call prepare() again atleast for SPEAr. You are correct. I saw the driver after a long time :) Can you please elaborate, why can't i see any clk_disable/enable calls anywhere else from probe. If i remember correctly, earlier we used to enable/disable clk after transfers and also during suspend/resume. The amba layer is taking care of interface clock only and not functional clock. So i believe that's not the magic code. :) -- viresh
On Tue, Sep 18, 2012 at 6:09 AM, viresh kumar <viresh.kumar@linaro.org> wrote: > Yes, we don't need to call prepare() again atleast for SPEAr. You are correct. > I saw the driver after a long time :) I'm asking because it's actually OK to do this, I was more asking whether it was really needed by any platforms... > Can you please elaborate, why can't i see any clk_disable/enable calls anywhere > else from probe. If i remember correctly, earlier we used to enable/disable > clk after transfers and also during suspend/resume. We clk_disable() at runtime_suspend() and clk_enable() at runtime resume, and the driver gives hints to the runtime PM layer to autosuspend the driver whenever it's unused. Check the pm_runtime_* calls. > The amba layer is taking care of interface clock only and not > functional clock. So > i believe that's not the magic code. :) This clock is the one for the external bus. In some designs these two clocks are one and the same, and these won't currently get into any clock disabled states, sadly. (We need to fix that some day.) Yours, Linus Walleij
On Tue, Sep 18, 2012 at 5:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Sep 18, 2012 at 6:09 AM, viresh kumar <viresh.kumar@linaro.org> wrote: > >> Yes, we don't need to call prepare() again atleast for SPEAr. You are correct. >> I saw the driver after a long time :) > > I'm asking because it's actually OK to do this, I was more asking whether it > was really needed by any platforms... Yes, I got that. Patch from Vipul is correct and should be there for any platforms which do anything in prepare/unprepare. But Atleast for SPEAr we don't need it. But i would still insist in keeping it for completeness. :) > We clk_disable() at runtime_suspend() and clk_enable() at runtime resume, > and the driver gives hints to the runtime PM layer to autosuspend the > driver whenever it's unused. Check the pm_runtime_* calls. Ahh.. How could i miss it. >> The amba layer is taking care of interface clock only and not >> functional clock. So >> i believe that's not the magic code. :) > > This clock is the one for the external bus. In some designs these two > clocks are one and the same, and these won't currently get into any clock > disabled states, sadly. (We need to fix that some day.) I went through the code and found following in amba/bus.c: static int amba_pm_runtime_suspend(struct device *dev) { struct amba_device *pcdev = to_amba_device(dev); int ret = pm_generic_runtime_suspend(dev); if (ret == 0 && dev->driver) clk_disable(pcdev->pclk); return ret; } static int amba_pm_runtime_resume(struct device *dev) { struct amba_device *pcdev = to_amba_device(dev); int ret; if (dev->driver) { ret = clk_enable(pcdev->pclk); /* Failure is probably fatal to the system, but... */ if (ret) return ret; } return pm_generic_runtime_resume(dev); } If i am not wrong, these routines also get called with runtiime suspend/resume of pl022? If that is the case, the even the interface clock of pl022 is getting disabled when not in used. And so for Architectures like SPEAr (where functional and interface clock are controlled by a single bit), we don't need anything else for power saving, with respect to clocks. Isn't it so? @Vipul/Vinit: Can you please confirm this behavior? -- viresh
On Wed, Sep 19, 2012 at 5:31 AM, viresh kumar <viresh.kumar@linaro.org> wrote: > On Tue, Sep 18, 2012 at 5:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Sep 18, 2012 at 6:09 AM, viresh kumar <viresh.kumar@linaro.org> wrote: > >>> The amba layer is taking care of interface clock only and not >>> functional clock. So >>> i believe that's not the magic code. :) >> >> This clock is the one for the external bus. In some designs these two >> clocks are one and the same, and these won't currently get into any clock >> disabled states, sadly. (We need to fix that some day.) > > I went through the code and found following in amba/bus.c: > > > static int amba_pm_runtime_suspend(struct device *dev) > { > struct amba_device *pcdev = to_amba_device(dev); > int ret = pm_generic_runtime_suspend(dev); > > if (ret == 0 && dev->driver) > clk_disable(pcdev->pclk); > > return ret; > } > > static int amba_pm_runtime_resume(struct device *dev) > { > struct amba_device *pcdev = to_amba_device(dev); > int ret; > > if (dev->driver) { > ret = clk_enable(pcdev->pclk); > /* Failure is probably fatal to the system, but... */ > if (ret) > return ret; > } > > return pm_generic_runtime_resume(dev); > } > > If i am not wrong, these routines also get called with runtiime suspend/resume > of pl022? Maybe. And that is part of the problem. Check this in drivers/base/power/runtime.c, rpm_suspend(): if (dev->pm_domain) callback = dev->pm_domain->ops.runtime_suspend; else if (dev->type && dev->type->pm) callback = dev->type->pm->runtime_suspend; else if (dev->class && dev->class->pm) callback = dev->class->pm->runtime_suspend; else if (dev->bus && dev->bus->pm) callback = dev->bus->pm->runtime_suspend; else callback = NULL; So as you see the AMBA bus-level runtime PM callbacks will be called UNLESS there is a class and UNLESS there is a type and UNLESS there is a voltage domain for this device. In Ux500 we solved this by calling the AMBA bus-level code from the voltage domain so it's not completely overridden, but the semantics are complex here. :-/ (And then we have yet to bring common suspend/resume into the picture ... which makes is ever more complex.) If the SPEAr is not using any voltage domains for example, it'll be unaffected and work fine. > If that is the case, the even the interface clock of pl022 is getting > disabled when not in used. Yes, hopefully... > And so for Architectures like SPEAr (where functional and interface > clock are controlled > by a single bit), we don't need anything else for power saving, with > respect to clocks. > Isn't it so? If you have no power domains I hope the ref goes down to zero and gates off the clock, so yes! Yours, Linus Walleij
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index f2a80ff..500e75e 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -2334,7 +2334,7 @@ static int pl022_runtime_suspend(struct device *dev) { struct pl022 *pl022 = dev_get_drvdata(dev); - clk_disable(pl022->clk); + clk_disable_unprepare(pl022->clk); return 0; } @@ -2342,10 +2342,13 @@ static int pl022_runtime_suspend(struct device *dev) static int pl022_runtime_resume(struct device *dev) { struct pl022 *pl022 = dev_get_drvdata(dev); + int ret = 0; - clk_enable(pl022->clk); + ret = clk_prepare_enable(pl022->clk); + if (ret) + dev_err(dev, "could not enable SSP/SPI bus clock\n"); - return 0; + return ret; } #endif
clk_{un}prepare is mandatory for platforms using common clock framework. Add clk_{un}prepare() support for spi-pl022 runtime PM. Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> --- drivers/spi/spi-pl022.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)