Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330701 > unrolled thread

[PATCH] clk: clk_register: Correctly initialize enable_count

Back to article view | Back to linux.kernel

Contents

  [PATCH] clk: clk_register: Correctly initialize enable_count Rhyland Klein <rklein@nvidia.com> - 2016-02-09 23:50 +0100
    Re: [PATCH] clk: clk_register: Correctly initialize enable_count Emilio López <emilio@elopez.com.ar> - 2016-02-10 04:20 +0100
      Re: [PATCH] clk: clk_register: Correctly initialize enable_count Rhyland Klein <rklein@nvidia.com> - 2016-02-10 19:40 +0100
        Re: [PATCH] clk: clk_register: Correctly initialize enable_count Michael Turquette <mturquette@baylibre.com> - 2016-02-11 22:40 +0100
          Re: [PATCH] clk: clk_register: Correctly initialize enable_count Rhyland Klein <rklein@nvidia.com> - 2016-02-11 23:00 +0100

#1330701 — [PATCH] clk: clk_register: Correctly initialize enable_count

When clocks are registered, they could be enabled already in
hardware. As of now, the enable count will start at 0. When this
happens, it means a clock is enabled and the framework doesn't know
that, so it will always report it as disabled.

After the first call of clk_enable(), the enable_count will be
correct, as it will simply try to enable an already enabled clock.

However, in some instances, some clocks may be left on from the boot
logic and because of the enable_count is inaccurate, drivers won't be
able to simply use clk_disable() to turn it off.

This patch will correctly set the enable_count to 1 if the clk is
already on when it is registered. This allows clk_disable to work as
expected.

To prevent the situation where the enable_count is always 1 greater
than the number of calls to clk_enable() for that clk, we add a flag
which will prevent incrementing enable_count the first time someone
calls clk_enable() for a clk that was on at boot.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
Perhaps this code should be something optional right now? I can't test
all boards that use this framework, and some boards may be using the
clocks that are on but thought off without realizing it.

 drivers/clk/clk.c            | 18 +++++++++++++++++-
 include/linux/clk-provider.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index bb01ed6cc63e..70d5ae7dd7a5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -682,6 +682,8 @@ static void clk_core_disable(struct clk_core *core)
 	if (--core->enable_count > 0)
 		return;
 
+	core->flags &= ~CLK_BOOT_ON_FIRST_ENABLE;
+
 	trace_clk_disable(core);
 
 	if (core->ops->disable)
@@ -729,7 +731,8 @@ static int clk_core_enable(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 0))
 		return -ESHUTDOWN;
 
-	if (core->enable_count == 0) {
+	if (core->enable_count == 0 ||
+	    (core->flags & CLK_BOOT_ON_FIRST_ENABLE)) {
 		ret = clk_core_enable(core->parent);
 
 		if (ret)
@@ -748,6 +751,10 @@ static int clk_core_enable(struct clk_core *core)
 		}
 	}
 
+	if (core->flags & CLK_BOOT_ON_FIRST_ENABLE) {
+		core->flags &= ~CLK_BOOT_ON_FIRST_ENABLE;
+		return 0;
+	}
 	core->enable_count++;
 	return 0;
 }
@@ -2513,6 +2520,15 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	core->max_rate = ULONG_MAX;
 	hw->core = core;
 
+	/* clocks can be enabled before being registered. This makes
+	 * their enable_count inherently incorrect. During register,
+	 * check to see if the clk is already enabled.
+	 */
+	if (clk_core_is_enabled(core)) {
+		core->enable_count++;
+		core->flags |= CLK_BOOT_ON_FIRST_ENABLE;
+	}
+
 	/* allocate local copy in case parent_names is __initdata */
 	core->parent_names = kcalloc(core->num_parents, sizeof(char *),
 					GFP_KERNEL);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index fabe5bedbba6..dacc28ebbf96 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -32,6 +32,7 @@
 #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
 #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
 #define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
+#define CLK_BOOT_ON_FIRST_ENABLE BIT(11) /* clk on at boot, skip 1st enable */
 
 struct clk;
 struct clk_hw;
-- 
1.9.1

[toc] | [next] | [standalone]

#1330874

Hi,

El 09/02/16 a las 19:48, Rhyland Klein escribió:
> When clocks are registered, they could be enabled already in
> hardware. As of now, the enable count will start at 0. When this
> happens, it means a clock is enabled and the framework doesn't know
> that, so it will always report it as disabled.

Keep in mind that during the boot process, towards the end, unused
clocks get disabled, so the state remains in sync. If suddenly the
enable_count on unused clocks is not 0, this will break and unused
clocks will remain on, wasting power.

http://lxr.free-electrons.com/source/drivers/clk/clk.c#L244

What issue were you having that prompted you to write this patch?

Cheers,
Emilio

[toc] | [prev] | [next] | [standalone]

#1331431

On 2/9/2016 9:56 PM, Emilio López wrote:
> Hi,
> 
> El 09/02/16 a las 19:48, Rhyland Klein escribió:
>> When clocks are registered, they could be enabled already in
>> hardware. As of now, the enable count will start at 0. When this
>> happens, it means a clock is enabled and the framework doesn't know
>> that, so it will always report it as disabled.
> 
> Keep in mind that during the boot process, towards the end, unused
> clocks get disabled, so the state remains in sync. If suddenly the
> enable_count on unused clocks is not 0, this will break and unused
> clocks will remain on, wasting power.

Hmm, I had misread the logic in clk_disable_unused_subtree(), namely I
inverted the check on enable_count when I was looking at it. It does
seem like it would take care of the clocks I was referring to.

> 
> http://lxr.free-electrons.com/source/drivers/clk/clk.c#L244
> 
> What issue were you having that prompted you to write this patch?

I ran into the situation where a peripheral clock was enabled before the
kernel loaded, and I was trying to disable it in the clk-tegra210
driver. Calling clk_disable() won't work, as the clock doesn't have an
enable_count.

I do think that the clk_disable_unused_subtree() should pick up the
slack, but I was trying to disable it before the cleanup code to remove
unused clocks ran.

I think you are right and the clean up code should cover the situation I
was looking at.

Thanks.
-rhyland


-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1332385

Quoting Rhyland Klein (2016-02-10 10:34:16)
> On 2/9/2016 9:56 PM, Emilio López wrote:
> > Hi,
> > 
> > El 09/02/16 a las 19:48, Rhyland Klein escribió:
> >> When clocks are registered, they could be enabled already in
> >> hardware. As of now, the enable count will start at 0. When this
> >> happens, it means a clock is enabled and the framework doesn't know
> >> that, so it will always report it as disabled.

Nak.

> > 
> > Keep in mind that during the boot process, towards the end, unused
> > clocks get disabled, so the state remains in sync. If suddenly the
> > enable_count on unused clocks is not 0, this will break and unused
> > clocks will remain on, wasting power.
> 
> Hmm, I had misread the logic in clk_disable_unused_subtree(), namely I
> inverted the check on enable_count when I was looking at it. It does
> seem like it would take care of the clocks I was referring to.

clk_disable_unused() will gate clocks left on by the bootloader AND not
yet claimed and enabled by a clock consumer.

> 
> > 
> > http://lxr.free-electrons.com/source/drivers/clk/clk.c#L244
> > 
> > What issue were you having that prompted you to write this patch?
> 
> I ran into the situation where a peripheral clock was enabled before the
> kernel loaded, and I was trying to disable it in the clk-tegra210
> driver. Calling clk_disable() won't work, as the clock doesn't have an
> enable_count.

Please check out include/linux/clk.h one more time. Calling
clk_disable() before ever calling clk_enable() is a violation of the
api. We try our best to avoid playing refcount games by enforcing that
rule.

> 
> I do think that the clk_disable_unused_subtree() should pick up the
> slack, but I was trying to disable it before the cleanup code to remove
> unused clocks ran.

clk_disable_unused() handles the case where spurious clocks were left
enabled by the bootloader and need to be gated. This sounds like the
only thing you were after.

We also have a case where clocks are gated by clk_disable_unused()
because no driver has claimed them yet, but we really want those clocks
to be left enabled. For example, clocks supplying DDR & CPU shouldn't be
cut, ever. Alternatively, your bootloader put something on the display
but your display controller module hasn't loaded yet. Cutting the clock
isn't fatal but causes unnecessary screen flicker because the module has
not loaded up.

To solve those sets of problems there is the critical clocks and handoff
clocks thread[0].

[0] http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturquette@baylibre.com>

Best regards,
Mike

> 
> I think you are right and the clean up code should cover the situation I
> was looking at.
> 
> Thanks.
> -rhyland
> 
> 
> -- 
> nvpublic

[toc] | [prev] | [next] | [standalone]

#1332399

On 2/11/2016 4:32 PM, Michael Turquette wrote:
> Quoting Rhyland Klein (2016-02-10 10:34:16)
>> On 2/9/2016 9:56 PM, Emilio López wrote:
>>> Hi,
>>>
>>> El 09/02/16 a las 19:48, Rhyland Klein escribió:
>>>> When clocks are registered, they could be enabled already in
>>>> hardware. As of now, the enable count will start at 0. When this
>>>> happens, it means a clock is enabled and the framework doesn't know
>>>> that, so it will always report it as disabled.
...
> 
> clk_disable_unused() handles the case where spurious clocks were left
> enabled by the bootloader and need to be gated. This sounds like the
> only thing you were after.
> 
> We also have a case where clocks are gated by clk_disable_unused()
> because no driver has claimed them yet, but we really want those clocks
> to be left enabled. For example, clocks supplying DDR & CPU shouldn't be
> cut, ever. Alternatively, your bootloader put something on the display
> but your display controller module hasn't loaded yet. Cutting the clock
> isn't fatal but causes unnecessary screen flicker because the module has
> not loaded up.
> 
> To solve those sets of problems there is the critical clocks and handoff
> clocks thread[0].
> 
> [0] http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturquette@baylibre.com>
> 
> Best regards,
> Mike
> 

Thanks. I agree the clk_disable_unused should clear up the situation I
was in. And the critical clocks and handoff logic should be useful. I am
sure we can make use of it once its in for Tegra support.

-rhyland

-- 
nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web