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

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 <redacted>
---
 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

On Mon, Sep 17, 2012 at 4:07 PM, Vipul Kumar Samar
[off-list ref] 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.

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 <redacted>

[...]

    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
[off-list ref] 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 <redacted>

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 [off-list ref] 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 [off-list ref] 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 [off-list ref] wrote:

On Tue, Sep 18, 2012 at 6:09 AM, viresh kumar [off-list ref] wrote:

quoted

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.

quoted

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 [off-list ref] wrote:

On Tue, Sep 18, 2012 at 5:20 PM, Linus Walleij [off-list ref] wrote:

quoted

On Tue, Sep 18, 2012 at 6:09 AM, viresh kumar [off-list ref] wrote:

quoted

quoted

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