COMMON_CLK_DISABLE_UNUSED

COMMON_CLK_DISABLE_UNUSED

[Andrew Lunn]

Hi Mike

I've been thinking about:

config COMMON_CLK_DISABLE_UNUSED
        bool "Disabled unused clocks at boot"
        depends on COMMON_CLK
        ---help---
          Traverses the entire clock tree and disables any clocks that are
          enabled in hardware but have not been enabled by any device drivers.
          This saves power and keeps the software model of the clock in line
          with reality.

          If in doubt, say "N".

I think in the long run, having this as an option is going to cause
problems, in particular for the One ARM Kernel. Some platforms are
going to want it enabled, others will want it disabled. Depending on
who looses out, there are going to be power regressions, or clocks
unexpectedly turned off.

I think it would be good to consider deleting this config option and
just have the code always enabled.

What do you think?

     Thanks
	Andrew

Re: COMMON_CLK_DISABLE_UNUSED

[Turquette, Mike]

On Mon, Apr 9, 2012 at 3:29 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> I think it would be good to consider deleting this config option and
> just have the code always enabled.

I agree.  We already support the CLK_IGNORE_UNUSED flag, so any
platforms that don't want this functionality wholesale can set that
bit for every clock.

I'll pull out that option.

Regards,
Mike

Re: COMMON_CLK_DISABLE_UNUSED

[Rob Herring]

On 04/09/2012 06:04 PM, Turquette, Mike wrote:
> On Mon, Apr 9, 2012 at 3:29 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> I think it would be good to consider deleting this config option and
>> just have the code always enabled.
> 
> I agree.  We already support the CLK_IGNORE_UNUSED flag, so any
> platforms that don't want this functionality wholesale can set that
> bit for every clock.
> 
> I'll pull out that option.
> 

CLK_IGNORE_UNUSED looks a bit broken to me. If you don't have the flag
set on the whole chain of parents, then a parent could be turned off.
This could happen at boot time or when another child get disabled and
the common parent's ref count goes to 0 and gets disabled.

I think a better solution would be a force enable or enable on boot flag
that sets ref counts correctly. Otherwise, each platform has to call
clk_prepare and clk_enable for all the clocks it wants to keep on.

Here's a simple case that needs work. You have a cpu clock controlled by
cpufreq driver. If the cpufreq driver is not loaded, we want the cpu
clock always enabled and don't want the clk infrastructure turning it
off. If the cpufreq driver is loaded, then we potentially want it to be
able to enable and disable the cpu clock if we switch parents. I guess
the easiest solution is the cpufreq driver needs to assume the cpu clock
is already enabled with a ref count of 1.

Rob

Re: COMMON_CLK_DISABLE_UNUSED

[Turquette, Mike]

On Tue, Apr 10, 2012 at 6:36 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 04/09/2012 06:04 PM, Turquette, Mike wrote:
>> On Mon, Apr 9, 2012 at 3:29 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> I think it would be good to consider deleting this config option and
>>> just have the code always enabled.
>>
>> I agree.  We already support the CLK_IGNORE_UNUSED flag, so any
>> platforms that don't want this functionality wholesale can set that
>> bit for every clock.
>>
>> I'll pull out that option.
>>
>
> CLK_IGNORE_UNUSED looks a bit broken to me. If you don't have the flag
> set on the whole chain of parents, then a parent could be turned off.
> This could happen at boot time or when another child get disabled and
> the common parent's ref count goes to 0 and gets disabled.

Chain of parents is irrelevant.  If the clock data is faulty and
doesn't mark a clock properly with CLK_IGNORE_UNUSED when it should
then bad things are expected to happen.

> I think a better solution would be a force enable or enable on boot flag
> that sets ref counts correctly. Otherwise, each platform has to call
> clk_prepare and clk_enable for all the clocks it wants to keep on.

I'm happy with platforms calling clk_prepare and clk_enable for clocks
that are not traditionally controlled by drivers.  Doing so makes for
a more easily understood clocking scheme and takes the magic out of
"why is that clock enabled/disabled?" during debug.

> Here's a simple case that needs work. You have a cpu clock controlled by
> cpufreq driver. If the cpufreq driver is not loaded, we want the cpu
> clock always enabled and don't want the clk infrastructure turning it
> off. If the cpufreq driver is loaded, then we potentially want it to be
> able to enable and disable the cpu clock if we switch parents. I guess
> the easiest solution is the cpufreq driver needs to assume the cpu clock
> is already enabled with a ref count of 1.

This feels cumbersome to me.  So the core increments the prepare and
enable usecounts and defers to the driver code to do the right thing?
This sounds like an easy way to end up with an imbalance of usecounts.
 Even worse drivers might end up doing something awful such as:

clk_enable(clk);
...
driver_does_stuff();
...
clk_disable(clk); // usecount is still 1
clk_disable(clk); // usecount is now zero

In the other thread on this topic started by Viresh a good question
was put forward by Shawn: can your hardware not tolerate having
.clk_enable run against a clock that is already enabled?  For SoCs
this is typically setting (or clearing) a bit that is already set (or
cleared, respectively).  That allows your cpufreq driver to still do
the typical clk_enable/clk_disable routine without having to make any
assumptions about the clock being enabled AND it allows proper use of
the CLK_IGNORE_UNUSED flag in the event that your cpufreq driver is
never loaded.

Regards,
Mike

> Rob

Re: COMMON_CLK_DISABLE_UNUSED

[Mike Turquette]

On 20120410-08:36, Rob Herring wrote:
> On 04/09/2012 06:04 PM, Turquette, Mike wrote:
> > On Mon, Apr 9, 2012 at 3:29 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> >> I think it would be good to consider deleting this config option and
> >> just have the code always enabled.
> > 
> > I agree.  We already support the CLK_IGNORE_UNUSED flag, so any
> > platforms that don't want this functionality wholesale can set that
> > bit for every clock.
> > 
> > I'll pull out that option.
> > 
> 
> CLK_IGNORE_UNUSED looks a bit broken to me. If you don't have the flag
> set on the whole chain of parents, then a parent could be turned off.
> This could happen at boot time or when another child get disabled and
> the common parent's ref count goes to 0 and gets disabled.
> 
> I think a better solution would be a force enable or enable on boot flag
> that sets ref counts correctly. Otherwise, each platform has to call
> clk_prepare and clk_enable for all the clocks it wants to keep on.
> 
> Here's a simple case that needs work. You have a cpu clock controlled by
> cpufreq driver. If the cpufreq driver is not loaded, we want the cpu
> clock always enabled and don't want the clk infrastructure turning it
> off. If the cpufreq driver is loaded, then we potentially want it to be
> able to enable and disable the cpu clock if we switch parents. I guess
> the easiest solution is the cpufreq driver needs to assume the cpu clock
> is already enabled with a ref count of 1.
> 

Hi Rob,

Apologies for the late reply.

I think having any driver (including a cpufreq driver) assume anything
about the usecount is broken.  Most code should follow the
tried-and-true pattern:

clk_get()
clk_prepare()
clk_enable()

cpufreq is no exception to this pattern.  The only concern that has been
raised to date is that of "re-enabling" a clock that is already enabled
in hardware (which the cpufreq driver would do).  That case is only
theoretical and no platform that I am aware of has a problem re-setting
the bit to enable a clock (or i2c message, or whatever).

Thanks,
Mike

> Rob