[PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Benoit Cousson]

The Z clock frequency change is effective only after setting the kick
bit located in the FRQCRB register.
Without that, the CA15 CPUs clock rate will never change.

Fix that by checking if the kick bit is cleared and enable it to make
the clock rate change effective. The bit is cleared automatically upon
completion.

Note: The kick bit is used as well to change 3 other emulation clocks:
Debug Trace port clock (ZTR), Debug Trace bus clock (ZT), and Debug
clock (ZTRD2). It is not clear from the spec [1] if there
is any relation between the CPU clock rate and these emulation clocks.
Moreover, these clocks are probably supposed to be controled by an
external debugger / tracer like Lauterbach PowerTrace.
For all these reasons, the current implementation does not expose these
clock nodes and thus does not have to consider the multiple accesses
to the kick bit.

Signed-off-by: Benoit Cousson <bcousson+renesas@baylibre.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

[1] R-Car-H2-v0.6-en.pdf - page 186
---

Salut Laurent,

If you have any information about these emulation clocks, please let me
know.

Moreover, the CCF clock driver is shared with the r8a7791, so a test on 
that platform will be good.

Regards,
Benoit


  drivers/clk/shmobile/clk-rcar-gen2.c | 26 ++++++++++++++++++++++++--
  1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c 
b/drivers/clk/shmobile/clk-rcar-gen2.c
index a59ec21..9f12746 100644
--- a/drivers/clk/shmobile/clk-rcar-gen2.c
+++ b/drivers/clk/shmobile/clk-rcar-gen2.c
@@ -26,6 +26,8 @@ struct rcar_gen2_cpg {
  	void __iomem *reg;
  };

+#define CPG_FRQCRB			0x00000004
+#define CPG_FRQCRB_KICK			BIT(31)
  #define CPG_SDCKCR			0x00000074
  #define CPG_PLL0CR			0x000000d8
  #define CPG_FRQCRC			0x000000e0
@@ -45,6 +47,7 @@ struct rcar_gen2_cpg {
  struct cpg_z_clk {
  	struct clk_hw hw;
  	void __iomem *reg;
+	void __iomem *kick_reg;
  };

  #define to_z_clk(_hw)	container_of(_hw, struct cpg_z_clk, hw)
@@ -83,17 +86,35 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, 
unsigned long rate,
  {
  	struct cpg_z_clk *zclk = to_z_clk(hw);
  	unsigned int mult;
-	u32 val;
+	u32 val, kick;
+	int i;

  	mult = div_u64((u64)rate * 32, parent_rate);
  	mult = clamp(mult, 1U, 32U);

+	if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
+		return -EBUSY;
+
  	val = clk_readl(zclk->reg);
  	val &= ~CPG_FRQCRC_ZFC_MASK;
  	val |= (32 - mult) << CPG_FRQCRC_ZFC_SHIFT;
  	clk_writel(val, zclk->reg);

-	return 0;
+	/*
+	 * Set KICK bit in FRQCRB to update hardware setting and
+	 * wait for completion.
+	 */
+	kick = clk_readl(zclk->kick_reg);
+	kick |= CPG_FRQCRB_KICK;
+	clk_writel(kick, zclk->kick_reg);
+
+	for (i = 1000; i; i--)
+		if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
+			cpu_relax();
+		else
+			return 0;
+
+	return -ETIMEDOUT;
  }

  static const struct clk_ops cpg_z_clk_ops = {
@@ -120,6 +141,7 @@ static struct clk * __init cpg_z_clk_register(struct 
rcar_gen2_cpg *cpg)
  	init.num_parents = 1;

  	zclk->reg = cpg->reg + CPG_FRQCRC;
+	zclk->kick_reg = cpg->reg + CPG_FRQCRB;
  	zclk->hw.init = &init;

  	clk = clk_register(NULL, &zclk->hw);
-- 
1.8.3.2

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Laurent Pinchart]

Hi Benoit,

Thank you for the patch.

On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
> The Z clock frequency change is effective only after setting the kick
> bit located in the FRQCRB register.
> Without that, the CA15 CPUs clock rate will never change.
> 
> Fix that by checking if the kick bit is cleared and enable it to make
> the clock rate change effective. The bit is cleared automatically upon
> completion.
> 
> Note: The kick bit is used as well to change 3 other emulation clocks:
> Debug Trace port clock (ZTR), Debug Trace bus clock (ZT), and Debug
> clock (ZTRD2). It is not clear from the spec [1] if there
> is any relation between the CPU clock rate and these emulation clocks.
> Moreover, these clocks are probably supposed to be controled by an
> external debugger / tracer like Lauterbach PowerTrace.
> For all these reasons, the current implementation does not expose these
> clock nodes and thus does not have to consider the multiple accesses
> to the kick bit.
> 
> Signed-off-by: Benoit Cousson <bcousson+renesas@baylibre.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> [1] R-Car-H2-v0.6-en.pdf - page 186
> ---
> 
> Salut Laurent,
> 
> If you have any information about these emulation clocks, please let me
> know.

I don't I'm afraid. All I know is that we don't use them for now, so I 
wouldn't care :-)

> Moreover, the CCF clock driver is shared with the r8a7791, so a test on
> that platform will be good.

I can do that. I assume you're working on Lager then ?

How can I exercise the code ? Using cpufreq to change the CA15 clock frequency 
? A brief test procedure would be appreciated.

>   drivers/clk/shmobile/clk-rcar-gen2.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)

By the way the patch was corrupted with line wraps and additional spaces.

> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
> b/drivers/clk/shmobile/clk-rcar-gen2.c
> index a59ec21..9f12746 100644
> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
> @@ -26,6 +26,8 @@ struct rcar_gen2_cpg {
>   	void __iomem *reg;
>   };
> 
> +#define CPG_FRQCRB			0x00000004
> +#define CPG_FRQCRB_KICK			BIT(31)
>   #define CPG_SDCKCR			0x00000074
>   #define CPG_PLL0CR			0x000000d8
>   #define CPG_FRQCRC			0x000000e0
> @@ -45,6 +47,7 @@ struct rcar_gen2_cpg {
>   struct cpg_z_clk {
>   	struct clk_hw hw;
>   	void __iomem *reg;
> +	void __iomem *kick_reg;
>   };
> 
>   #define to_z_clk(_hw)	container_of(_hw, struct cpg_z_clk, hw)
> @@ -83,17 +86,35 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw,
> unsigned long rate,
>   {
>   	struct cpg_z_clk *zclk = to_z_clk(hw);
>   	unsigned int mult;
> -	u32 val;
> +	u32 val, kick;
> +	int i;

The loop counter will always be positive, please use an unsigned int.

> 
>   	mult = div_u64((u64)rate * 32, parent_rate);
>   	mult = clamp(mult, 1U, 32U);
> 
> +	if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
> +		return -EBUSY;
> +
>   	val = clk_readl(zclk->reg);
>   	val &= ~CPG_FRQCRC_ZFC_MASK;
>   	val |= (32 - mult) << CPG_FRQCRC_ZFC_SHIFT;
>   	clk_writel(val, zclk->reg);
> 
> -	return 0;
> +	/*
> +	 * Set KICK bit in FRQCRB to update hardware setting and
> +	 * wait for completion.
> +	 */
> +	kick = clk_readl(zclk->kick_reg);
> +	kick |= CPG_FRQCRB_KICK;
> +	clk_writel(kick, zclk->kick_reg);

Does CCF guarantee that two set_rate calls for different clocks will not occur 
concurrently ? If so a brief comment would be nice. Otherwise we need a lock 
around this.

> +
> +	for (i = 1000; i; i--)
> +		if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
> +			cpu_relax();
> +		else
> +			return 0;

Please put braces around the for() content.

How many iterations does this need in practice ? I also wonder if we really 
need to wait or if we could just return and assume the clock frequency will 
change soon enough.

> +
> +	return -ETIMEDOUT;
>   }
> 
>   static const struct clk_ops cpg_z_clk_ops = {
> @@ -120,6 +141,7 @@ static struct clk * __init cpg_z_clk_register(struct
> rcar_gen2_cpg *cpg)
>   	init.num_parents = 1;
> 
>   	zclk->reg = cpg->reg + CPG_FRQCRC;
> +	zclk->kick_reg = cpg->reg + CPG_FRQCRB;
>   	zclk->hw.init = &init;
> 
>   	clk = clk_register(NULL, &zclk->hw);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Benoit Cousson]

Hi Laurent,

On 25/02/2014 17:01, Laurent Pinchart wrote:
> Hi Benoit,
>
> Thank you for the patch.

You're welcome. Thanks for your comments.

> On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
>> The Z clock frequency change is effective only after setting the kick
>> bit located in the FRQCRB register.
>> Without that, the CA15 CPUs clock rate will never change.
>>
>> Fix that by checking if the kick bit is cleared and enable it to make
>> the clock rate change effective. The bit is cleared automatically upon
>> completion.
>>
>> Note: The kick bit is used as well to change 3 other emulation clocks:
>> Debug Trace port clock (ZTR), Debug Trace bus clock (ZT), and Debug
>> clock (ZTRD2). It is not clear from the spec [1] if there
>> is any relation between the CPU clock rate and these emulation clocks.
>> Moreover, these clocks are probably supposed to be controled by an
>> external debugger / tracer like Lauterbach PowerTrace.
>> For all these reasons, the current implementation does not expose these
>> clock nodes and thus does not have to consider the multiple accesses
>> to the kick bit.
>>
>> Signed-off-by: Benoit Cousson <bcousson+renesas@baylibre.com>
>> Cc: Mike Turquette <mturquette@linaro.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> [1] R-Car-H2-v0.6-en.pdf - page 186
>> ---
>>
>> Salut Laurent,
>>
>> If you have any information about these emulation clocks, please let me
>> know.
>
> I don't I'm afraid. All I know is that we don't use them for now, so I
> wouldn't care :-)
>
>> Moreover, the CCF clock driver is shared with the r8a7791, so a test on
>> that platform will be good.
>
> I can do that. I assume you're working on Lager then ?

Yes I am. I don't have any Koelsch board.

> How can I exercise the code ? Using cpufreq to change the CA15 clock frequency
> ? A brief test procedure would be appreciated.

Sure. First you can use dhrystone to check that the CPU frequency is 
changing or not.

So far, changing the clock rate will not affect the dhrystone value at 
all becasue the magic kick bit is not set.

debugfs does not seems to allow write access to the clk_rate, so using 
cpufreq is propably the best way to test it.

I have a small series to add DVFS for Lager (Multiplatform boot only), I 
can share you that code if you want.

Naive question: How did you test that z-clock code originally ?

>>    drivers/clk/shmobile/clk-rcar-gen2.c | 26 ++++++++++++++++++++++++--
>>    1 file changed, 24 insertions(+), 2 deletions(-)
>
> By the way the patch was corrupted with line wraps and additional spaces.

Gosh! I edited it slightly by hand with thunderbird before submitting... 
I probably messed it up. Sorry :-(

>
>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
>> b/drivers/clk/shmobile/clk-rcar-gen2.c
>> index a59ec21..9f12746 100644
>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
>> @@ -26,6 +26,8 @@ struct rcar_gen2_cpg {
>>    	void __iomem *reg;
>>    };
>>
>> +#define CPG_FRQCRB			0x00000004
>> +#define CPG_FRQCRB_KICK			BIT(31)
>>    #define CPG_SDCKCR			0x00000074
>>    #define CPG_PLL0CR			0x000000d8
>>    #define CPG_FRQCRC			0x000000e0
>> @@ -45,6 +47,7 @@ struct rcar_gen2_cpg {
>>    struct cpg_z_clk {
>>    	struct clk_hw hw;
>>    	void __iomem *reg;
>> +	void __iomem *kick_reg;
>>    };
>>
>>    #define to_z_clk(_hw)	container_of(_hw, struct cpg_z_clk, hw)
>> @@ -83,17 +86,35 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw,
>> unsigned long rate,
>>    {
>>    	struct cpg_z_clk *zclk = to_z_clk(hw);
>>    	unsigned int mult;
>> -	u32 val;
>> +	u32 val, kick;
>> +	int i;
>
> The loop counter will always be positive, please use an unsigned int.

OK.

>>    	mult = div_u64((u64)rate * 32, parent_rate);
>>    	mult = clamp(mult, 1U, 32U);
>>
>> +	if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
>> +		return -EBUSY;
>> +
>>    	val = clk_readl(zclk->reg);
>>    	val &= ~CPG_FRQCRC_ZFC_MASK;
>>    	val |= (32 - mult) << CPG_FRQCRC_ZFC_SHIFT;
>>    	clk_writel(val, zclk->reg);
>>
>> -	return 0;ir: cannot create directory `/home/ubuntu/projects/ti-glsdk_omap5-uevm_6_03_00_01/targetfs': Permission denied
>> +	/*
>> +	 * Set KICK bit in FRQCRB to update hardware setting and
>> +	 * wait for completion.
>> +	 */
>> +	kick = clk_readl(zclk->kick_reg);
>> +	kick |= CPG_FRQCRB_KICK;
>> +	clk_writel(kick, zclk->kick_reg);
>
> Does CCF guarantee that two set_rate calls for different clocks will not occur
> concurrently ? If so a brief comment would be nice. Otherwise we need a lock
> around this.

So far, this is the only user of the register. That's why there is no 
lock. It is explained in the changelog. But if you want I can re-use the 
same comment here.

>
>> +
>> +	for (i = 1000; i; i--)
>> +		if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
>> +			cpu_relax();
>> +		else
>> +			return 0;
>
> Please put braces around the for() content.

OK, no problem, if you prefer that.

> How many iterations does this need in practice ?

I don't have any clue :-)

I don't like these kind of magic numbers as well, but without further 
information, it is hard to know.

Based on TI experience, there is no way to get any accurate information 
about this kind of transition time even from HW enginners. Most of the 
time the best you can get is a tens of micro-sec or a hundred of clock 
cycles.

FWIW, this "value" is based on what was already done and merged for the 
zlock inside the clock-r8a73a4.c code (non-CCF).

> I also wonder if we really
> need to wait or if we could just return and assume the clock frequency will
> change soon enough.

We do need to wait, that the recommended procedure from the spec, and 
this is the only way to ensure the frequency is properly changed.

In the case of DVFS sequence, you do not want to start reducing the 
voltage before you are 100% sure the frequency was reduced before. So 
that's the only safe way to do that.

Regards,
Benoit


-- 
Benoît Cousson
BayLibre
Embedded Linux Technology Lab
www.baylibre.com

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Laurent Pinchart]

Hi Benoit,

On Tuesday 25 February 2014 18:07:54 Benoit Cousson wrote:
> On 25/02/2014 17:01, Laurent Pinchart wrote:
> > Hi Benoit,
> > 
> > Thank you for the patch.
> 
> You're welcome. Thanks for your comments.
> 
> > On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
> >> The Z clock frequency change is effective only after setting the kick
> >> bit located in the FRQCRB register.
> >> Without that, the CA15 CPUs clock rate will never change.
> >> 
> >> Fix that by checking if the kick bit is cleared and enable it to make
> >> the clock rate change effective. The bit is cleared automatically upon
> >> completion.
> >> 
> >> Note: The kick bit is used as well to change 3 other emulation clocks:
> >> Debug Trace port clock (ZTR), Debug Trace bus clock (ZT), and Debug
> >> clock (ZTRD2). It is not clear from the spec [1] if there
> >> is any relation between the CPU clock rate and these emulation clocks.
> >> Moreover, these clocks are probably supposed to be controled by an
> >> external debugger / tracer like Lauterbach PowerTrace.
> >> For all these reasons, the current implementation does not expose these
> >> clock nodes and thus does not have to consider the multiple accesses
> >> to the kick bit.
> >> 
> >> Signed-off-by: Benoit Cousson <bcousson+renesas@baylibre.com>
> >> Cc: Mike Turquette <mturquette@linaro.org>
> >> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >> 
> >> [1] R-Car-H2-v0.6-en.pdf - page 186
> >> ---
> >> 
> >> Salut Laurent,
> >> 
> >> If you have any information about these emulation clocks, please let me
> >> know.
> > 
> > I don't I'm afraid. All I know is that we don't use them for now, so I
> > wouldn't care :-)
> > 
> >> Moreover, the CCF clock driver is shared with the r8a7791, so a test on
> >> that platform will be good.
> > 
> > I can do that. I assume you're working on Lager then ?
> 
> Yes I am. I don't have any Koelsch board.
> 
> > How can I exercise the code ? Using cpufreq to change the CA15 clock
> > frequency ? A brief test procedure would be appreciated.
> 
> Sure. First you can use dhrystone to check that the CPU frequency is
> changing or not.

OK.

> So far, changing the clock rate will not affect the dhrystone value at
> all becasue the magic kick bit is not set.
> 
> debugfs does not seems to allow write access to the clk_rate, so using
> cpufreq is propably the best way to test it.
> 
> I have a small series to add DVFS for Lager (Multiplatform boot only), I
> can share you that code if you want.

That would be nice, thanks.

> Naive question: How did you test that z-clock code originally ?

Simple answer: I haven't :-)

> >>    drivers/clk/shmobile/clk-rcar-gen2.c | 26 ++++++++++++++++++++++++--
> >>    1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > By the way the patch was corrupted with line wraps and additional spaces.
> 
> Gosh! I edited it slightly by hand with thunderbird before submitting...
> I probably messed it up. Sorry :-(
> 
> >> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
> >> b/drivers/clk/shmobile/clk-rcar-gen2.c
> >> index a59ec21..9f12746 100644
> >> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
> >> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c

[snip]

> >> @@ -83,17 +86,35 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw,
> >> unsigned long rate,
> >>    {
> >>    	struct cpg_z_clk *zclk = to_z_clk(hw);
> >>    	unsigned int mult;
> >> -	u32 val;
> >> +	u32 val, kick;
> >> +	int i;
> > 
> > The loop counter will always be positive, please use an unsigned int.
> 
> OK.
> 
> >>    	mult = div_u64((u64)rate * 32, parent_rate);
> >>    	mult = clamp(mult, 1U, 32U);
> >> 
> >> +	if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
> >> +		return -EBUSY;
> >> +
> >>    	val = clk_readl(zclk->reg);
> >>    	val &= ~CPG_FRQCRC_ZFC_MASK;
> >>    	val |= (32 - mult) << CPG_FRQCRC_ZFC_SHIFT;
> >>    	clk_writel(val, zclk->reg);
> >> 
> >> -	return 0;ir: cannot create directory
> >> `/home/ubuntu/projects/ti-glsdk_omap5-uevm_6_03_00_01/targetfs':
> >> Permission denied
> >> +	/*
> >> +	 * Set KICK bit in FRQCRB to update hardware setting and
> >> +	 * wait for completion.
> >> +	 */
> >> +	kick = clk_readl(zclk->kick_reg);
> >> +	kick |= CPG_FRQCRB_KICK;
> >> +	clk_writel(kick, zclk->kick_reg);
> > 
> > Does CCF guarantee that two set_rate calls for different clocks will not
> > occur concurrently ? If so a brief comment would be nice. Otherwise we
> > need a lock around this.
> 
> So far, this is the only user of the register. That's why there is no
> lock. It is explained in the changelog. But if you want I can re-use the
> same comment here.

A comment would be nice, yes. You can make it shorter than the commit message.

> >> +
> >> +	for (i = 1000; i; i--)
> >> +		if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
> >> +			cpu_relax();
> >> +		else
> >> +			return 0;
> > 
> > Please put braces around the for() content.
> 
> OK, no problem, if you prefer that.
> 
> > How many iterations does this need in practice ?
> 
> I don't have any clue :-)

Could you test it ?

> I don't like these kind of magic numbers as well, but without further
> information, it is hard to know.
> 
> Based on TI experience, there is no way to get any accurate information
> about this kind of transition time even from HW enginners. Most of the
> time the best you can get is a tens of micro-sec or a hundred of clock
> cycles.
> 
> FWIW, this "value" is based on what was already done and merged for the
> zlock inside the clock-r8a73a4.c code (non-CCF).
> 
> > I also wonder if we really need to wait or if we could just return and
> > assume the clock frequency will change soon enough.
> 
> We do need to wait, that the recommended procedure from the spec, and
> this is the only way to ensure the frequency is properly changed.
> 
> In the case of DVFS sequence, you do not want to start reducing the
> voltage before you are 100% sure the frequency was reduced before. So
> that's the only safe way to do that.

Good point.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Mike Turquette]

Quoting Laurent Pinchart (2014-02-25 09:48:38)
> On Tuesday 25 February 2014 18:07:54 Benoit Cousson wrote:
> > On 25/02/2014 17:01, Laurent Pinchart wrote:
> > > On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
> > >> +  /*
> > >> +   * Set KICK bit in FRQCRB to update hardware setting and
> > >> +   * wait for completion.
> > >> +   */
> > >> +  kick = clk_readl(zclk->kick_reg);
> > >> +  kick |= CPG_FRQCRB_KICK;
> > >> +  clk_writel(kick, zclk->kick_reg);
> > > 
> > > Does CCF guarantee that two set_rate calls for different clocks will not
> > > occur concurrently ? If so a brief comment would be nice. Otherwise we
> > > need a lock around this.
> > 
> > So far, this is the only user of the register. That's why there is no
> > lock. It is explained in the changelog. But if you want I can re-use the
> > same comment here.
> 
> A comment would be nice, yes. You can make it shorter than the commit message.

CCF holds a global mutex during a call to clk_set_rate. So clk_prepare,
clk_unprepare, clk_set_parent or a competing clk_set_rate will not touch
this register during the critical section.

However it is possible to reenter the framework, but usually you control
that code flow.

The main two reasons to introduce your own more granular register-level
locking are:

1) clk_enable & clk_disable hold a separate global spinlock (not the
global mutex), so if this register is used for set_rate operations AND
enable/disable operations then you'll need a spinlock.

2) Other stuff outside of the clk framework touches this register
(sounds like it is not the case here).

Regards,
Mike

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Laurent Pinchart]

Hi Mike,

On Wednesday 26 February 2014 12:54:55 Mike Turquette wrote:
> Quoting Laurent Pinchart (2014-02-25 09:48:38)
> > On Tuesday 25 February 2014 18:07:54 Benoit Cousson wrote:
> > > On 25/02/2014 17:01, Laurent Pinchart wrote:
> > > > On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
> > > >> +  /*
> > > >> +   * Set KICK bit in FRQCRB to update hardware setting and
> > > >> +   * wait for completion.
> > > >> +   */
> > > >> +  kick = clk_readl(zclk->kick_reg);
> > > >> +  kick |= CPG_FRQCRB_KICK;
> > > >> +  clk_writel(kick, zclk->kick_reg);
> > > > 
> > > > Does CCF guarantee that two set_rate calls for different clocks will
> > > > not occur concurrently ? If so a brief comment would be nice.
> > > > Otherwise we need a lock around this.
> > > 
> > > So far, this is the only user of the register. That's why there is no
> > > lock. It is explained in the changelog. But if you want I can re-use the
> > > same comment here.
> > 
> > A comment would be nice, yes. You can make it shorter than the commit
> > message.
>
> CCF holds a global mutex during a call to clk_set_rate. So clk_prepare,
> clk_unprepare, clk_set_parent or a competing clk_set_rate will not touch
> this register during the critical section.
> 
> However it is possible to reenter the framework, but usually you control
> that code flow.
> 
> The main two reasons to introduce your own more granular register-level
> locking are:
> 
> 1) clk_enable & clk_disable hold a separate global spinlock (not the
> global mutex), so if this register is used for set_rate operations AND
> enable/disable operations then you'll need a spinlock.
> 
> 2) Other stuff outside of the clk framework touches this register
> (sounds like it is not the case here).

Thanks a lot for the explanation. I've cooked up a small documentation patch 
to avoid the need to repeat this over and over, I'll send it separately.

Looking at the implementation I found something that looked like a locking bug 
in the Qualcomm clock drivers at first sight.

clk-rpg.c calls __clk_is_enabled() from within its configure_bank() function. 
That function ends up being called from within the .set_rate, .set_parent and 
.set_rate_and_parent operations. This leads to __clk_is_enabled() being called 
without the enable spinlock held.

Now, clk-rpg.c provides an .is_enabled handler (clk_is_enabled_regmap) so 
we're at least not accessing the clock enable_count counter without the proper 
lock being held. I don't know whether clk_is_enabled_regmap() will handle 
locking properly though.

Exporting __clk_is_enabled() looks a bit dangerous to me. It might make sense 
to replace the __clk_is_enabled() call in clk-rpg.c with a direct call to 
clk_is_enabled_regmap(), but we still have two other users (namely 
drivers/cpufreq/kirkwood-cpufreq.c and arch/arm/mach-omap2/pm24xx.c) in the 
mainline kernel.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Stephen Boyd]

On 02/26/14 15:43, Mike Turquette wrote:
> Quoting Laurent Pinchart (2014-02-26 13:53:03)
>> Hi Mike,
>>
>> On Wednesday 26 February 2014 12:54:55 Mike Turquette wrote:
>>> Quoting Laurent Pinchart (2014-02-25 09:48:38)
>>>> On Tuesday 25 February 2014 18:07:54 Benoit Cousson wrote:
>>>>> On 25/02/2014 17:01, Laurent Pinchart wrote:
>>>>>> On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote:
>>>>>>> +  /*
>>>>>>> +   * Set KICK bit in FRQCRB to update hardware setting and
>>>>>>> +   * wait for completion.
>>>>>>> +   */
>>>>>>> +  kick = clk_readl(zclk->kick_reg);
>>>>>>> +  kick |= CPG_FRQCRB_KICK;
>>>>>>> +  clk_writel(kick, zclk->kick_reg);
>>>>>> Does CCF guarantee that two set_rate calls for different clocks will
>>>>>> not occur concurrently ? If so a brief comment would be nice.
>>>>>> Otherwise we need a lock around this.
>>>>> So far, this is the only user of the register. That's why there is no
>>>>> lock. It is explained in the changelog. But if you want I can re-use the
>>>>> same comment here.
>>>> A comment would be nice, yes. You can make it shorter than the commit
>>>> message.
>>> CCF holds a global mutex during a call to clk_set_rate. So clk_prepare,
>>> clk_unprepare, clk_set_parent or a competing clk_set_rate will not touch
>>> this register during the critical section.
>>>
>>> However it is possible to reenter the framework, but usually you control
>>> that code flow.
>>>
>>> The main two reasons to introduce your own more granular register-level
>>> locking are:
>>>
>>> 1) clk_enable & clk_disable hold a separate global spinlock (not the
>>> global mutex), so if this register is used for set_rate operations AND
>>> enable/disable operations then you'll need a spinlock.
>>>
>>> 2) Other stuff outside of the clk framework touches this register
>>> (sounds like it is not the case here).
>> Thanks a lot for the explanation. I've cooked up a small documentation patch 
>> to avoid the need to repeat this over and over, I'll send it separately.
>>
>> Looking at the implementation I found something that looked like a locking bug 
>> in the Qualcomm clock drivers at first sight.
>>
>> clk-rpg.c calls __clk_is_enabled() from within its configure_bank() function. 
>> That function ends up being called from within the .set_rate, .set_parent and 
>> .set_rate_and_parent operations. This leads to __clk_is_enabled() being called 
>> without the enable spinlock held.
>>
>> Now, clk-rpg.c provides an .is_enabled handler (clk_is_enabled_regmap) so 
>> we're at least not accessing the clock enable_count counter without the proper 
>> lock being held. I don't know whether clk_is_enabled_regmap() will handle 
>> locking properly though.
>>
>> Exporting __clk_is_enabled() looks a bit dangerous to me. It might make sense 
>> to replace the __clk_is_enabled() call in clk-rpg.c with a direct call to 
>> clk_is_enabled_regmap(), but we still have two other users (namely 
>> drivers/cpufreq/kirkwood-cpufreq.c and arch/arm/mach-omap2/pm24xx.c) in the 
>> mainline kernel.

(I'm guessing clk-rpg.c is actually clk-rcg.c?)

It's safe for the set_parent and set_parent_and_rate case because of the
way we do the parent switch (see __clk_set_parent_before() in
particular). The enable/prepare state of the clock cannot change while
these ops are called. If the clock is prepared before configure_bank()
is called then __clk_set_parent_before() has enabled the clock for us.
If the clock isn't prepared before configure_bank() is called then we're
holding the prepare lock and nobody can enable the clock without first
preparing the clock (which means they would need to grab the prepare
lock and wait for us to be done).

It looks like the only case where it actually is racy is a plain
set_rate, in which case we're not switching parents and a
clk_enable()/disable() could happen in the middle of configure_bank().
Sad. I doubt anyone will ever actually do that in practice, but sure
theoretical problems are still problems. This patch should fix it.

---8<---
From: Stephen Boyd <sboyd@codeaurora.org>
Subject: [PATCH] clk: qcom: Synchronize configure_bank() with enable/disable

Calling __clk_is_enabled() in configure_bank() isn't safe without
synchronizing with enable/disable of this clock. Introduce a
global spinlock that we grab in enable, disable and across this
bank configuration function so that the clock can't be enabled or
disabled while configure_bank() is running.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/qcom/clk-rcg.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
index abfc2b675aea..51c08bc22378 100644
--- a/drivers/clk/qcom/clk-rcg.c
+++ b/drivers/clk/qcom/clk-rcg.c
@@ -22,6 +22,9 @@
 
 #include "clk-rcg.h"
 
+/* Synchronize .enable/.disable with plain .set_rate */
+static DEFINE_SPINLOCK(rcg_enable_lock);
+
 static u32 ns_to_src(struct src_sel *s, u32 ns)
 {
 	ns >>= s->src_sel_shift;
@@ -202,7 +205,9 @@ static void configure_bank(struct clk_dyn_rcg *rcg, const struct freq_tbl *f)
 	u32 bank_reg;
 	bool banked_mn = !!rcg->mn[1].width;
 	struct clk_hw *hw = &rcg->clkr.hw;
+	unsigned long flags;
 
+	spin_lock_irqsave(&rcg_enable_lock, flags);
 	enabled = __clk_is_enabled(hw->clk);
 
 	regmap_read(rcg->clkr.regmap, rcg->ns_reg, &ns);
@@ -251,6 +256,7 @@ static void configure_bank(struct clk_dyn_rcg *rcg, const struct freq_tbl *f)
 		*regp ^= BIT(rcg->mux_sel_bit);
 		regmap_write(rcg->clkr.regmap, bank_reg, *regp);
 	}
+	spin_unlock_irqrestore(&rcg_enable_lock, flags);
 }
 
 static int clk_dyn_rcg_set_parent(struct clk_hw *hw, u8 index)
@@ -492,6 +498,27 @@ static int clk_dyn_rcg_set_rate_and_parent(struct clk_hw *hw,
 	return __clk_dyn_rcg_set_rate(hw, rate);
 }
 
+static int clk_dyn_rcg_enable(struct clk_hw *hw)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rcg_enable_lock, flags);
+	ret = clk_enable_regmap(hw);
+	spin_unlock_irqrestore(&rcg_enable_lock, flags);
+
+	return ret;
+}
+
+static void clk_dyn_rcg_disable(struct clk_hw *hw)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&rcg_enable_lock, flags);
+	clk_disable_regmap(hw);
+	spin_unlock_irqrestore(&rcg_enable_lock, flags);
+}
+
 const struct clk_ops clk_rcg_ops = {
 	.enable = clk_enable_regmap,
 	.disable = clk_disable_regmap,
@@ -504,9 +531,9 @@ const struct clk_ops clk_rcg_ops = {
 EXPORT_SYMBOL_GPL(clk_rcg_ops);
 
 const struct clk_ops clk_dyn_rcg_ops = {
-	.enable = clk_enable_regmap,
+	.enable = clk_dyn_rcg_enable,
 	.is_enabled = clk_is_enabled_regmap,
-	.disable = clk_disable_regmap,
+	.disable = clk_dyn_rcg_disable,
 	.get_parent = clk_dyn_rcg_get_parent,
 	.set_parent = clk_dyn_rcg_set_parent,
 	.recalc_rate = clk_dyn_rcg_recalc_rate,

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Laurent Pinchart]

Hi Stephen,

On Wednesday 26 February 2014 16:50:29 Stephen Boyd wrote:
> On 02/26/14 15:43, Mike Turquette wrote:
> > Quoting Laurent Pinchart (2014-02-26 13:53:03)
> >> On Wednesday 26 February 2014 12:54:55 Mike Turquette wrote:

[snip]

> >>> CCF holds a global mutex during a call to clk_set_rate. So clk_prepare,
> >>> clk_unprepare, clk_set_parent or a competing clk_set_rate will not touch
> >>> this register during the critical section.
> >>> 
> >>> However it is possible to reenter the framework, but usually you control
> >>> that code flow.
> >>> 
> >>> The main two reasons to introduce your own more granular register-level
> >>> locking are:
> >>> 
> >>> 1) clk_enable & clk_disable hold a separate global spinlock (not the
> >>> global mutex), so if this register is used for set_rate operations AND
> >>> enable/disable operations then you'll need a spinlock.
> >>> 
> >>> 2) Other stuff outside of the clk framework touches this register
> >>> (sounds like it is not the case here).
> >> 
> >> Thanks a lot for the explanation. I've cooked up a small documentation
> >> patch to avoid the need to repeat this over and over, I'll send it
> >> separately.
> >> 
> >> Looking at the implementation I found something that looked like a
> >> locking bug in the Qualcomm clock drivers at first sight.
> >> 
> >> clk-rpg.c calls __clk_is_enabled() from within its configure_bank()
> >> function. That function ends up being called from within the .set_rate,
> >> .set_parent and .set_rate_and_parent operations. This leads to
> >> __clk_is_enabled() being called without the enable spinlock held.
> >> 
> >> Now, clk-rpg.c provides an .is_enabled handler (clk_is_enabled_regmap) so
> >> we're at least not accessing the clock enable_count counter without the
> >> proper lock being held. I don't know whether clk_is_enabled_regmap()
> >> will handle locking properly though.
> >> 
> >> Exporting __clk_is_enabled() looks a bit dangerous to me. It might make
> >> sense to replace the __clk_is_enabled() call in clk-rpg.c with a direct
> >> call to clk_is_enabled_regmap(), but we still have two other users
> >> (namely drivers/cpufreq/kirkwood-cpufreq.c and
> >> arch/arm/mach-omap2/pm24xx.c) in the mainline kernel.
> 
> (I'm guessing clk-rpg.c is actually clk-rcg.c?)

Yes, sorry.

> It's safe for the set_parent and set_parent_and_rate case because of the
> way we do the parent switch (see __clk_set_parent_before() in
> particular). The enable/prepare state of the clock cannot change while
> these ops are called. If the clock is prepared before configure_bank()
> is called then __clk_set_parent_before() has enabled the clock for us.
> If the clock isn't prepared before configure_bank() is called then we're
> holding the prepare lock and nobody can enable the clock without first
> preparing the clock (which means they would need to grab the prepare
> lock and wait for us to be done).
> 
> It looks like the only case where it actually is racy is a plain
> set_rate, in which case we're not switching parents and a
> clk_enable()/disable() could happen in the middle of configure_bank().
> Sad. I doubt anyone will ever actually do that in practice, but sure
> theoretical problems are still problems. This patch should fix it.

I'm not familiar with the hardware, but the patch below looks good to me. 
While you're at it, what would you think about replacing the 
__clk_is_enabled() call with a direct call to clk_is_enabled_regmap() ? Any 
user of __clk_is_enabled() outside the CCF core is suspect from a locking 
point of view, and exporting that function seems like a dangerous idea. On the 
other hand, if you had done so already I wouldn't have found the race 
condition.

> ---8<---
> From: Stephen Boyd <sboyd@codeaurora.org>
> Subject: [PATCH] clk: qcom: Synchronize configure_bank() with enable/disable
> 
> Calling __clk_is_enabled() in configure_bank() isn't safe without
> synchronizing with enable/disable of this clock. Introduce a
> global spinlock that we grab in enable, disable and across this
> bank configuration function so that the clock can't be enabled or
> disabled while configure_bank() is running.
> 
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-rcg.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
> index abfc2b675aea..51c08bc22378 100644
> --- a/drivers/clk/qcom/clk-rcg.c
> +++ b/drivers/clk/qcom/clk-rcg.c
> @@ -22,6 +22,9 @@
> 
>  #include "clk-rcg.h"
> 
> +/* Synchronize .enable/.disable with plain .set_rate */
> +static DEFINE_SPINLOCK(rcg_enable_lock);
> +
>  static u32 ns_to_src(struct src_sel *s, u32 ns)
>  {
>  	ns >>= s->src_sel_shift;
> @@ -202,7 +205,9 @@ static void configure_bank(struct clk_dyn_rcg *rcg,
> const struct freq_tbl *f) u32 bank_reg;
>  	bool banked_mn = !!rcg->mn[1].width;
>  	struct clk_hw *hw = &rcg->clkr.hw;
> +	unsigned long flags;
> 
> +	spin_lock_irqsave(&rcg_enable_lock, flags);
>  	enabled = __clk_is_enabled(hw->clk);
> 
>  	regmap_read(rcg->clkr.regmap, rcg->ns_reg, &ns);
> @@ -251,6 +256,7 @@ static void configure_bank(struct clk_dyn_rcg *rcg,
> const struct freq_tbl *f) *regp ^= BIT(rcg->mux_sel_bit);
>  		regmap_write(rcg->clkr.regmap, bank_reg, *regp);
>  	}
> +	spin_unlock_irqrestore(&rcg_enable_lock, flags);
>  }
> 
>  static int clk_dyn_rcg_set_parent(struct clk_hw *hw, u8 index)
> @@ -492,6 +498,27 @@ static int clk_dyn_rcg_set_rate_and_parent(struct
> clk_hw *hw, return __clk_dyn_rcg_set_rate(hw, rate);
>  }
> 
> +static int clk_dyn_rcg_enable(struct clk_hw *hw)
> +{
> +	int ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rcg_enable_lock, flags);
> +	ret = clk_enable_regmap(hw);
> +	spin_unlock_irqrestore(&rcg_enable_lock, flags);
> +
> +	return ret;
> +}
> +
> +static void clk_dyn_rcg_disable(struct clk_hw *hw)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rcg_enable_lock, flags);
> +	clk_disable_regmap(hw);
> +	spin_unlock_irqrestore(&rcg_enable_lock, flags);
> +}
> +
>  const struct clk_ops clk_rcg_ops = {
>  	.enable = clk_enable_regmap,
>  	.disable = clk_disable_regmap,
> @@ -504,9 +531,9 @@ const struct clk_ops clk_rcg_ops = {
>  EXPORT_SYMBOL_GPL(clk_rcg_ops);
> 
>  const struct clk_ops clk_dyn_rcg_ops = {
> -	.enable = clk_enable_regmap,
> +	.enable = clk_dyn_rcg_enable,
>  	.is_enabled = clk_is_enabled_regmap,
> -	.disable = clk_disable_regmap,
> +	.disable = clk_dyn_rcg_disable,
>  	.get_parent = clk_dyn_rcg_get_parent,
>  	.set_parent = clk_dyn_rcg_set_parent,
>  	.recalc_rate = clk_dyn_rcg_recalc_rate,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change

[Stephen Boyd]

On 02/27/14 14:46, Laurent Pinchart wrote:
> I'm not familiar with the hardware, but the patch below looks good to me. 
> While you're at it, what would you think about replacing the 
> __clk_is_enabled() call with a direct call to clk_is_enabled_regmap() ? Any 
> user of __clk_is_enabled() outside the CCF core is suspect from a locking 
> point of view, and exporting that function seems like a dangerous idea. On the 
> other hand, if you had done so already I wouldn't have found the race 
> condition.

Hm.. I guess that's ok. I like not having to care about what op is
assigned to the .is_enabled op though and we can't easily access the
.ops structure because it's hidden by the clk framework.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation