clk: ux500: assume PRCC clocks are off by default

Commit Message

Linus Walleij Nov. 27, 2012, 1:51 p.m. UTC

From: Linus Walleij <[e-mail]>

The code for the PRCC clock assumed that on boot all the PRCC
clocks were on, defining them as enabled. However some such
clocks like the clock for the GPIO block 6 in the PER2
peripheral group are not on at all.

This would manifest itself as a GPIO block seeming to be
clocked and working from the debugfs point of view when
actually it was not clocked at all.

So instead assume that the PRCC clocks are *not* on, and
everything starts working. This may cause a few extra writes
to the enable registers but it's worth it. We cannot read
the status registers to find out if the clock is on at
this point as that means we first have to turn on the
clock to the peripheral cluster.

Reported-by: Lee Jones <[e-mail]>
Cc: Ulf Hansson <[e-mail]>
Cc: Philippe Begnic <[e-mail]>
Cc: Per Forlin <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
Mike: this needs to go into v3.7. (Yes, sorry for not seeing
this earlier, but the problem did manifest itself in very
strange ways.) If v3.7 is not possible, make sure to add the
Cc: [e-mail] tag to it.
---
 drivers/clk/ux500/clk-prcc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ulf Hansson Nov. 27, 2012, 1:58 p.m. UTC | #1

On 27 November 2012 14:51, Linus Walleij <[e-mail]> wrote:
> From: Linus Walleij <[e-mail]>
>
> The code for the PRCC clock assumed that on boot all the PRCC
> clocks were on, defining them as enabled. However some such
> clocks like the clock for the GPIO block 6 in the PER2
> peripheral group are not on at all.
>
> This would manifest itself as a GPIO block seeming to be
> clocked and working from the debugfs point of view when
> actually it was not clocked at all.
>
> So instead assume that the PRCC clocks are *not* on, and
> everything starts working. This may cause a few extra writes
> to the enable registers but it's worth it. We cannot read
> the status registers to find out if the clock is on at
> this point as that means we first have to turn on the
> clock to the peripheral cluster.
>
> Reported-by: Lee Jones <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Philippe Begnic <[e-mail]>
> Cc: Per Forlin <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
> Mike: this needs to go into v3.7. (Yes, sorry for not seeing
> this earlier, but the problem did manifest itself in very
> strange ways.) If v3.7 is not possible, make sure to add the

Hold on!

I see what you are trying to solve. But the next problem when going
ahead with this patch is that some clocks, which are default on, will
be kept on unless some client explicit disables it. This is not OK. I
will get back to you with some more ideas how to proceed.

Kind regards
Ulf Hansson

Linus Walleij Nov. 27, 2012, 2:16 p.m. UTC | #2

On Tue, Nov 27, 2012 at 2:58 PM, Ulf Hansson <[e-mail]> wrote:
> On 27 November 2012 14:51, Linus Walleij <[e-mail]> wrote:
>> From: Linus Walleij <[e-mail]>

>> So instead assume that the PRCC clocks are *not* on, and
>> everything starts working. This may cause a few extra writes
>> to the enable registers but it's worth it. We cannot read
>> the status registers to find out if the clock is on at
>> this point as that means we first have to turn on the
>> clock to the peripheral cluster.
>>
>> Reported-by: Lee Jones <[e-mail]>
>> Cc: Ulf Hansson <[e-mail]>
>> Cc: Philippe Begnic <[e-mail]>
>> Cc: Per Forlin <[e-mail]>
>> Signed-off-by: Linus Walleij <[e-mail]>
>> ---
>> Mike: this needs to go into v3.7. (Yes, sorry for not seeing
>> this earlier, but the problem did manifest itself in very
>> strange ways.) If v3.7 is not possible, make sure to add the
>
> Hold on!
>
> I see what you are trying to solve. But the next problem when going
> ahead with this patch is that some clocks, which are default on, will
> be kept on unless some client explicit disables it. This is not OK. I
> will get back to you with some more ideas how to proceed.

OK ... well that means if we don't come up with a better solution
we need to choose the lesser of two evils:

- Apply this and get a power regression

- Not apply this and get a GPIO regression which cascades
  to child devices, for example STMPE keypad and SD Card
  detect on older boards is
  not working at all without this.

So let's see if we can fix the perfect patch to avoid having to
choose...

Yours,
Linus Walleij

Linus Walleij Nov. 27, 2012, 7:04 p.m. UTC | #3

On Tue, Nov 27, 2012 at 2:51 PM, Linus Walleij
<[e-mail]> wrote:

> From: Linus Walleij <[e-mail]>
>
> The code for the PRCC clock assumed that on boot all the PRCC
> clocks were on, defining them as enabled. However some such
> clocks like the clock for the GPIO block 6 in the PER2
> peripheral group are not on at all.

Bah, the problem I'm describing is probably true, but I was
mistaken (of course). I have a more ACK:able patch now...

Forget about this patch, check the next one.

Yours,
Linus Walleij

Patch

diff --git a/drivers/clk/ux500/clk-prcc.c b/drivers/clk/ux500/clk-prcc.c
index 7eee7f7..0a20a06 100644
--- a/drivers/clk/ux500/clk-prcc.c
+++ b/drivers/clk/ux500/clk-prcc.c
@@ -120,7 +120,7 @@  static struct clk *clk_reg_prcc(const char *name,
 		goto free_clk;
 
 	clk->cg_sel = cg_sel;
-	clk->is_enabled = 1;
+	clk->is_enabled = 0;
 
 	clk_prcc_init.name = name;
 	clk_prcc_init.ops = clk_prcc_ops;