| Message ID | 20141209084653.GR14482@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hallo Sascha, On Tue, Dec 09, 2014 at 09:46:53AM +0100, Sascha Hauer wrote: > On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote: > > For those clk gates which hold share count, since its is_enabled > > callback is only checking the share count rather than reading > > the hardware register setting, in the late phase of kernel bootup, > > the clk_disable_unused action will NOT handle the scenario of > > share_count is 0 but the hardware setting is enabled, actually, > > uboot normally enables all clk gates, then those shared clk gates > > will be always enabled until they are used by some modules. > > > > So the problem would be: when kernel boot up, the usecount cat > > from clk tree is 0, but the clk gates actually is enabled in > > hardware register, it will confuse user and bring unnecessary power > > consumption, take i.MX6SX for example, the ESAI clk info > > is as below, the use count is 0, but the hardware register read > > from CCM_CCGR1_CG8 is ungated. > > I believe the problem is that the shared count is not increased > correctly during registration. When it would be increased during > registration for each clock that is enabled in hardware clock_disable_unused > could do its job like intended. > > Could you test the attached patch? > > -------------------------------8<-------------------------- > > From 8095ee6e407f33887b2afa504767d00fcca4f10c Mon Sep 17 00:00:00 2001 > From: Sascha Hauer <s.hauer@pengutronix.de> > Date: Tue, 9 Dec 2014 09:39:06 +0100 > Subject: [PATCH] ARM: i.MX: clk-gate2: Fix shared clock enable counting > > When during registration of a shared clock the clock is enabled > in hardware the shared_count must be increased. This makes sure > that during clk_disable the shared_count is decreased first before > actually diabling the clock. When done like this the is_enabled > callback can return the hardware state of the clock, like it is > intended. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm/mach-imx/clk-gate2.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c > index 5a75cdc..a673364 100644 > --- a/arch/arm/mach-imx/clk-gate2.c > +++ b/arch/arm/mach-imx/clk-gate2.c > @@ -96,10 +96,7 @@ static int clk_gate2_is_enabled(struct clk_hw *hw) > { > struct clk_gate2 *gate = to_clk_gate2(hw); > > - if (gate->share_count) > - return !!__clk_get_enable_count(hw->clk); > - else > - return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx); > + return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx); > } > > static struct clk_ops clk_gate2_ops = { > @@ -129,6 +126,9 @@ struct clk *clk_register_gate2(struct device *dev, const char *name, > gate->lock = lock; > gate->share_count = share_count; > > + if (share_count && clk_gate2_reg_is_enabled(reg, bit_idx)) > + (*share_count)++; > + This is equivalent to calling clk_enable on this clk. So the result is that clk_disable_unused now succeeds to disable if all related clks are unused. But it introduces a new problem I think. Consider that the bootloader starts linux with the clk on, so assuming 3 shared clks share_count ends at 3 after the clocks are registered. Then a driver makes use of one of them, calls clk_prepare_enable on it, share_count becomes 4, enable_count = 1. clk_disable_unused disables the two unused clks making share_count = 2. After the driver now calls clk_disable the clk stays on. -> bad Best regards Uwe
diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c index 5a75cdc..a673364 100644 --- a/arch/arm/mach-imx/clk-gate2.c +++ b/arch/arm/mach-imx/clk-gate2.c @@ -96,10 +96,7 @@ static int clk_gate2_is_enabled(struct clk_hw *hw) { struct clk_gate2 *gate = to_clk_gate2(hw); - if (gate->share_count) - return !!__clk_get_enable_count(hw->clk); - else - return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx); + return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx); } static struct clk_ops clk_gate2_ops = { @@ -129,6 +126,9 @@ struct clk *clk_register_gate2(struct device *dev, const char *name, gate->lock = lock; gate->share_count = share_count; + if (share_count && clk_gate2_reg_is_enabled(reg, bit_idx)) + (*share_count)++; + init.name = name; init.ops = &clk_gate2_ops; init.flags = flags;