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

[Ben Dooks <ben.dooks@codethink.co.uk>]

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