[PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[Vipul Kumar Samar]

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(-)

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[viresh kumar]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[Sergei Shtylyov]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[Linus Walleij]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[viresh kumar]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[Linus Walleij]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[viresh kumar]

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

Re: [PATCH] spi: pl022: Add clk_{un}prepare() support in runtime PM

[Linus Walleij]

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