[PATCH] power: fixup clock_ops

[PATCH] power: fixup clock_ops

[Ben Dooks]

The drivers/base/power/clock_ops.c file is causing warnings from
the clock driver (as shown below) due to failing to do a clk_prepare()
call before enabling a clock. It also fails to check the balance of
prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call.

This bug has probably been in since commit b2476490e ("clk: introduce
the common clock framework") as the warning was part of the original
commit. It is strange that it has not been noticed (although this has
also been coupled with a failure for certain SH builds to not build the
necessary glue to use this method of controlling the clocks).

In summary, this is probably needed in several stable branches but need
advice on which ones.

On the Renesas Lager board, this causes numerous warnings of the following
and even worse the clock system will not enable clocks, causing drivers
that are in development to fail to work:

WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0()

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/base/power/clock_ops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 9d8fde7..b9dd8fa 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -43,6 +43,7 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 	if (IS_ERR(ce->clk)) {
 		ce->status = PCE_STATUS_ERROR;
 	} else {
+		clk_prepare(ce->clk);
 		ce->status = PCE_STATUS_ACQUIRED;
 		dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
 	}
@@ -99,10 +100,12 @@ static void __pm_clk_remove(struct pm_clock_entry *ce)
 
 	if (ce->status < PCE_STATUS_ERROR) {
 		if (ce->status == PCE_STATUS_ENABLED)
-			clk_disable_unprepare(ce->clk);
+			clk_disable(ce->clk);
 
-		if (ce->status >= PCE_STATUS_ACQUIRED)
+		if (ce->status >= PCE_STATUS_ACQUIRED) {
+			clk_unprepare(ce->clk);
 			clk_put(ce->clk);
+		}
 	}
 
 	kfree(ce->con_id);
-- 
1.8.5.2

Re: [PATCH] power: fixup clock_ops

[Ben Dooks]

Sorry, as a note the patch title /should/ have been:

power: fixup clock_ops clk prepare count usage

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] power: fixup clock_ops

[Geert Uytterhoeven]

Hi Ben,

On Fri, Jan 10, 2014 at 4:06 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On the Renesas Lager board, this causes numerous warnings of the following
> and even worse the clock system will not enable clocks, causing drivers
> that are in development to fail to work:

Did you see a message like

    rcar_thermal e61f0000.thermal: thermal sensor was broken

on Lager?

If yes, was it fixed by this patch?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] power: fixup clock_ops

[Ben Dooks]

On 10/01/14 15:59, Geert Uytterhoeven wrote:
> Hi Ben,
>
> On Fri, Jan 10, 2014 at 4:06 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> On the Renesas Lager board, this causes numerous warnings of the following
>> and even worse the clock system will not enable clocks, causing drivers
>> that are in development to fail to work:
>
> Did you see a message like
>
>      rcar_thermal e61f0000.thermal: thermal sensor was broken
>
> on Lager?

rcar_thermal e61f0000.thermal: 1 sensor probed 


but I've a few more fixes in flight due to other interesting issues...

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] power: fixup clock_ops

[Geert Uytterhoeven]

On Fri, Jan 10, 2014 at 5:23 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 10/01/14 15:59, Geert Uytterhoeven wrote:
>> On Fri, Jan 10, 2014 at 4:06 PM, Ben Dooks <ben.dooks@codethink.co.uk>
>> wrote:
>>>
>>> On the Renesas Lager board, this causes numerous warnings of the
>>> following
>>> and even worse the clock system will not enable clocks, causing drivers
>>> that are in development to fail to work:
>>
>>
>> Did you see a message like
>>
>>      rcar_thermal e61f0000.thermal: thermal sensor was broken
>>
>> on Lager?
>
> rcar_thermal e61f0000.thermal: 1 sensor probed

Good. So it's working.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] power: fixup clock_ops

[Ben Dooks]

On 10/01/14 17:09, Geert Uytterhoeven wrote:
> On Fri, Jan 10, 2014 at 5:23 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> On 10/01/14 15:59, Geert Uytterhoeven wrote:
>>> On Fri, Jan 10, 2014 at 4:06 PM, Ben Dooks <ben.dooks@codethink.co.uk>
>>> wrote:
>>>>
>>>> On the Renesas Lager board, this causes numerous warnings of the
>>>> following
>>>> and even worse the clock system will not enable clocks, causing drivers
>>>> that are in development to fail to work:
>>>
>>>
>>> Did you see a message like
>>>
>>>       rcar_thermal e61f0000.thermal: thermal sensor was broken
>>>
>>> on Lager?
>>
>> rcar_thermal e61f0000.thermal: 1 sensor probed
>
> Good. So it's working.

I will try and get everything out on list before Monday.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius