[PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Laurent Pinchart]

The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
bit value should thus be inverted in the is_enabled() operation.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/clk/shmobile/clk-mstp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Geert, I believe this patch should fix the problem we've noticed with
clk_disable_unused() not disabling unused clocks. Could you please test it ?

diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c
index 42d5912..be45b45 100644
--- a/drivers/clk/shmobile/clk-mstp.c
+++ b/drivers/clk/shmobile/clk-mstp.c
@@ -112,7 +112,7 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
 	else
 		value = clk_readl(group->smstpcr);
 
-	return !!(value & BIT(clock->bit_index));
+	return !(value & BIT(clock->bit_index));
 }
 
 static const struct clk_ops cpg_mstp_clock_ops = {
-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Mar 12, 2014 at 10:04 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
> bit value should thus be inverted in the is_enabled() operation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/clk/shmobile/clk-mstp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Geert, I believe this patch should fix the problem we've noticed with
> clk_disable_unused() not disabling unused clocks. Could you please test it ?

Yes, now it disables the hardware clock bits for all clocks that are not in
use according to CCF:

MSTP tmu0 OFF
MSTP i2c0 OFF
MSTP i2c1 OFF
MSTP i2c2 OFF
MSTP i2c3 OFF
MSTP i2c4 OFF
MSTP i2c5 OFF
MSTP ether OFF
MSTP msiof1 OFF
MSTP msiof2 OFF
MSTP msiof0 OFF
MSTP qspi_mod OFF

and booting stops...

So this exposes that all of the clocks above haven't really been enabled
through CCF. The only reason they worked before is "sheer luck" (read:
they were enabled before we booted Linux, due to reset state or boot
loader).

The only clocks we do enable are those we register with
shmobile_clk_workaround():

MSTP cmt0 ON
MSTP scifa0 ON
MSTP scifa1 ON
MSTP scifb0 ON
MSTP scifb1 ON
MSTP scifb2 ON
MSTP scifa2 ON
MSTP scif0 ON
MSTP scif1 ON
MSTP scif2 ON
MSTP scif3 ON
MSTP scif4 ON
MSTP scif5 ON
MSTP scifa3 ON
MSTP scifa4 ON
MSTP scifa5 ON
MSTP sata0 ON
MSTP scif1 ON
MSTP scif1 ON

To check yourself, add

    #include <linux/clk-private.h>

to drivers/clk/shmobile/clk-mstp.c, and add

    pr_info("MSTP %s %s\n", hw->clk->name, enable ? "ON" : "OFF")

to the top of cpg_mstp_clock_endisable().

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] clk: shmobile: mstp: Fix the is_enabled() operation

[Ben Dooks]

On 13/03/14 11:27, Geert Uytterhoeven wrote:
> Hi Laurent,
>
> On Wed, Mar 12, 2014 at 10:04 PM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
>> bit value should thus be inverted in the is_enabled() operation.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>   drivers/clk/shmobile/clk-mstp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Geert, I believe this patch should fix the problem we've noticed with
>> clk_disable_unused() not disabling unused clocks. Could you please test it ?
>
> Yes, now it disables the hardware clock bits for all clocks that are not in
> use according to CCF:

Do you have the drivers/sh fix in?

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

Re: [PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Geert Uytterhoeven]

Hi Ben,

On Thu, Mar 13, 2014 at 12:35 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> Yes, now it disables the hardware clock bits for all clocks that are not
>> in
>> use according to CCF:
>
> Do you have the drivers/sh fix in?

No, I removed that due to bad interaction with userspace:

Unhandled fault: imprecise external abort (0x1406) at 0x00000000

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] clk: shmobile: mstp: Fix the is_enabled() operation

[Geert Uytterhoeven]

On Thu, Mar 13, 2014 at 12:27 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> The only clocks we do enable are those we register with
> shmobile_clk_workaround():
>
> MSTP sata0 ON

Sata0 is not registered with shmobile_clk_workaround(), so probably is
managed manually by its driver.

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] clk: shmobile: mstp: Fix the is_enabled() operation

[Ben Dooks]

On 13/03/14 11:37, Geert Uytterhoeven wrote:
> Hi Ben,
>
> On Thu, Mar 13, 2014 at 12:35 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>> Yes, now it disables the hardware clock bits for all clocks that are not
>>> in
>>> use according to CCF:
>>
>> Do you have the drivers/sh fix in?
>
> No, I removed that due to bad interaction with userspace:
>
> Unhandled fault: imprecise external abort (0x1406) at 0x00000000

Hmm, so if you disable clocks that peripherals need then there's no
surprise that the system fails to boot :/ I added an patch to stop
the abort being fatal to allow continued boot.

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

Re: [PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Laurent Pinchart]

Hi Geert,

On Thursday 13 March 2014 12:27:43 Geert Uytterhoeven wrote:
> On Wed, Mar 12, 2014 at 10:04 PM, Laurent Pinchart wrote:
> > The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
> > bit value should thus be inverted in the is_enabled() operation.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/clk/shmobile/clk-mstp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Geert, I believe this patch should fix the problem we've noticed with
> > clk_disable_unused() not disabling unused clocks. Could you please test it
> > ?
>
> Yes, now it disables the hardware clock bits for all clocks that are not in
> use according to CCF:
> 
> MSTP tmu0 OFF
> MSTP i2c0 OFF
> MSTP i2c1 OFF
> MSTP i2c2 OFF
> MSTP i2c3 OFF
> MSTP i2c4 OFF
> MSTP i2c5 OFF
> MSTP ether OFF
> MSTP msiof1 OFF
> MSTP msiof2 OFF
> MSTP msiof0 OFF
> MSTP qspi_mod OFF
> 
> and booting stops...

In a way, that's good, the results are consistent. I propose delaying this 
patch a bit though :-)

The tmu0 clock is supposed to be off, as we don't instantiate the device for 
r8a779[01].

The i2c driver acquires the device clock explicitly but doesn't enable/disable 
it.

The msiof and rspi drivers seem to enable/disable clocks explicitly, so they 
should probably work. However the rspi driver seems to be missing 
clk_prepare/clk_unprepare calls. Do you plan to fix that ?

The sh-eth driver doesn't perform any clock management.

The question is, how should we fix it in the short term ? Should we target 
runtime PM clock management straight away, or should we add manual clock 
handling to drivers as a first step ?

> So this exposes that all of the clocks above haven't really been enabled
> through CCF. The only reason they worked before is "sheer luck" (read:
> they were enabled before we booted Linux, due to reset state or boot
> loader).
> 
> The only clocks we do enable are those we register with
> shmobile_clk_workaround():
> 
> MSTP cmt0 ON
> MSTP scifa0 ON
> MSTP scifa1 ON
> MSTP scifb0 ON
> MSTP scifb1 ON
> MSTP scifb2 ON
> MSTP scifa2 ON
> MSTP scif0 ON
> MSTP scif1 ON
> MSTP scif2 ON
> MSTP scif3 ON
> MSTP scif4 ON
> MSTP scif5 ON
> MSTP scifa3 ON
> MSTP scifa4 ON
> MSTP scifa5 ON
> MSTP sata0 ON
> MSTP scif1 ON
> MSTP scif1 ON
> 
> To check yourself, add
> 
>     #include <linux/clk-private.h>
> 
> to drivers/clk/shmobile/clk-mstp.c, and add
> 
>     pr_info("MSTP %s %s\n", hw->clk->name, enable ? "ON" : "OFF")
> 
> to the top of cpg_mstp_clock_endisable().

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Ben Dooks]

On 13/03/14 11:50, Laurent Pinchart wrote:
> Hi Geert,
>
> On Thursday 13 March 2014 12:27:43 Geert Uytterhoeven wrote:
>> On Wed, Mar 12, 2014 at 10:04 PM, Laurent Pinchart wrote:
>>> The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
>>> bit value should thus be inverted in the is_enabled() operation.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>   drivers/clk/shmobile/clk-mstp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Geert, I believe this patch should fix the problem we've noticed with
>>> clk_disable_unused() not disabling unused clocks. Could you please test it
>>> ?
>>
>> Yes, now it disables the hardware clock bits for all clocks that are not in
>> use according to CCF:
>>
>> MSTP tmu0 OFF
>> MSTP i2c0 OFF
>> MSTP i2c1 OFF
>> MSTP i2c2 OFF
>> MSTP i2c3 OFF
>> MSTP i2c4 OFF
>> MSTP i2c5 OFF
>> MSTP ether OFF
>> MSTP msiof1 OFF
>> MSTP msiof2 OFF
>> MSTP msiof0 OFF
>> MSTP qspi_mod OFF
>>
>> and booting stops...
>
> In a way, that's good, the results are consistent. I propose delaying this
> patch a bit though :-)
>
> The tmu0 clock is supposed to be off, as we don't instantiate the device for
> r8a779[01].
>
> The i2c driver acquires the device clock explicitly but doesn't enable/disable
> it.

It expects runtime-pm

>
> The msiof and rspi drivers seem to enable/disable clocks explicitly, so they
> should probably work. However the rspi driver seems to be missing
> clk_prepare/clk_unprepare calls. Do you plan to fix that ?

If you are manually managing the clocks, then the clk_prepare_enable and
the unprepare counterpart are probably what you want to use as long as
you are not going to be called from a non-sleepable context.

> The sh-eth driver doesn't perform any clock management.

It again expects the runtime-pm to manage it.


> The question is, how should we fix it in the short term ? Should we target
> runtime PM clock management straight away, or should we add manual clock
> handling to drivers as a first step ?

The drivers/sh was supposed to enable the use of the runtime pm as
that is what the non-multiplatorm SHMOBILE stuff was using.

This sort of thing is why I do not like the current implicit clock
management as it is so difficult to diagnose what is going on and there
is no error feedback if it is not present :(


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

Re: [PATCH] clk: shmobile: mstp: Fix the is_enabled() operation

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Mar 13, 2014 at 12:50 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The msiof and rspi drivers seem to enable/disable clocks explicitly, so they
> should probably work. However the rspi driver seems to be missing
> clk_prepare/clk_unprepare calls. Do you plan to fix that ?

Those haven been removed in the conversion of the rspi driver to spi
core auto_runtime_pm.

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] clk: shmobile: mstp: Fix the is_enabled() operation

[Laurent Pinchart]

On Thursday 13 March 2014 11:54:44 Ben Dooks wrote:
> On 13/03/14 11:50, Laurent Pinchart wrote:
> > On Thursday 13 March 2014 12:27:43 Geert Uytterhoeven wrote:
> >> On Wed, Mar 12, 2014 at 10:04 PM, Laurent Pinchart wrote:
> >>> The MSTP[SC]R registers have clock stop bits, not clock enable bits. The
> >>> bit value should thus be inverted in the is_enabled() operation.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>   drivers/clk/shmobile/clk-mstp.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> Geert, I believe this patch should fix the problem we've noticed with
> >>> clk_disable_unused() not disabling unused clocks. Could you please test
> >>> it
> >>> ?
> >> 
> >> Yes, now it disables the hardware clock bits for all clocks that are not
> >> in use according to CCF:
> >> 
> >> MSTP tmu0 OFF
> >> MSTP i2c0 OFF
> >> MSTP i2c1 OFF
> >> MSTP i2c2 OFF
> >> MSTP i2c3 OFF
> >> MSTP i2c4 OFF
> >> MSTP i2c5 OFF
> >> MSTP ether OFF
> >> MSTP msiof1 OFF
> >> MSTP msiof2 OFF
> >> MSTP msiof0 OFF
> >> MSTP qspi_mod OFF
> >> 
> >> and booting stops...
> > 
> > In a way, that's good, the results are consistent. I propose delaying this
> > patch a bit though :-)
> > 
> > The tmu0 clock is supposed to be off, as we don't instantiate the device
> > for r8a779[01].
> > 
> > The i2c driver acquires the device clock explicitly but doesn't
> > enable/disable it.
> 
> It expects runtime-pm
> 
> > The msiof and rspi drivers seem to enable/disable clocks explicitly, so
> > they should probably work. However the rspi driver seems to be missing
> > clk_prepare/clk_unprepare calls. Do you plan to fix that ?
> 
> If you are manually managing the clocks, then the clk_prepare_enable and
> the unprepare counterpart are probably what you want to use as long as
> you are not going to be called from a non-sleepable context.
> 
> > The sh-eth driver doesn't perform any clock management.
> 
> It again expects the runtime-pm to manage it.
> 
> > The question is, how should we fix it in the short term ? Should we target
> > runtime PM clock management straight away, or should we add manual clock
> > handling to drivers as a first step ?
> 
> The drivers/sh was supposed to enable the use of the runtime pm as
> that is what the non-multiplatorm SHMOBILE stuff was using.
> 
> This sort of thing is why I do not like the current implicit clock
> management as it is so difficult to diagnose what is going on and there
> is no error feedback if it is not present :(

I hear your concern. We should try to add proper feedback to the runtime PM 
clock management code.

-- 
Regards,

Laurent Pinchart