[RFC PATCH 0/2] add dts property to force update a clock setting

[RFC PATCH 0/2] add dts property to force update a clock setting

[Kever Yang]

When we assgined a clock rate in dts, we may need to update
the clock setting like PLLs who can get the same output rate with
different parameter even if we don't need to change the rate.


Kever Yang (2):
  clk: add property for force to update clock setting
  dt-bindings: clk: add document for assigned-clock-force-rates usage

 .../devicetree/bindings/clock/clock-bindings.txt   |  7 +++--
 drivers/clk/clk-conf.c                             | 33 +++++++++++++++++++++-
 2 files changed, 37 insertions(+), 3 deletions(-)

-- 
1.9.1

[RFC PATCH 1/2] clk: add property for force to update clock setting

[Kever Yang]

Usually we assigned a clock to a default rate in dts,
there is a situation that the clock already initialized to the rate
we intend to set before kernel(hardware default or init in uboot etc).
For the PLLs we can get a rate from different PLL parameter configure,
we can't change the PLL parameter if the rate is not changed by now.

This patch adds a option property 'assigned-clock-force-rates'
to make sure we update all the setting even if we don't need to
update the clock rate.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/clk/clk-conf.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..0c9df48 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -84,7 +84,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 	struct clk *clk;
 	u32 rate;
 
-	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
+	of_property_for_each_u32(node, "assigned-force-rates", prop, cur, rate) {
 		if (rate) {
 			rc = of_parse_phandle_with_args(node, "assigned-clocks",
 					"#clock-cells",	index, &clkspec);
@@ -104,7 +104,38 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 					index, node->full_name);
 				return PTR_ERR(clk);
 			}
+			/* change the old rate to 0 to make sure we can get into
+			 * clk_change_rate */
+			clk->rate = 0;
+			rc = clk_set_rate(clk, rate);
+			if (rc < 0)
+				pr_err("clk: couldn't set %s clock rate: %d\n",
+				       __clk_get_name(clk), rc);
+			clk_put(clk);
+		}
+		index++;
+	}
 
+	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
+		if (rate) {
+			rc = of_parse_phandle_with_args(node, "assigned-clocks",
+					"#clock-cells",	index, &clkspec);
+			if (rc < 0) {
+				/* skip empty (null) phandles */
+				if (rc == -ENOENT)
+					continue;
+				else
+					return rc;
+			}
+			if (clkspec.np == node && !clk_supplier)
+				return 0;
+
+			clk = of_clk_get_by_clkspec(&clkspec);
+			if (IS_ERR(clk)) {
+				pr_warn("clk: couldn't get clock %d for %s\n",
+					index, node->full_name);
+				return PTR_ERR(clk);
+			}
 			rc = clk_set_rate(clk, rate);
 			if (rc < 0)
 				pr_err("clk: couldn't set %s clock rate: %d\n",
-- 
1.9.1

[RFC PATCH 2/2] dt-bindings: clk: add document for assigned-clock-force-rates usage

[Kever Yang]

This patch adds document for how to use the opetion property
assigned-clock-force-rates.

We may use this property to force update a clock setting.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 Documentation/devicetree/bindings/clock/clock-bindings.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 06fc6d5..fb5d3a5 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -154,11 +154,14 @@ set to 0, or can be omitted if it is not followed by any non-zero entry.
 
         assigned-clocks = <&clkcon 0>, <&pll 2>;
         assigned-clock-parents = <&pll 2>;
+        assigned-clock-force-rates = <500000>, <0>;
         assigned-clock-rates = <0>, <460800>;
     };
 
-In this example the <&pll 2> clock is set as parent of clock <&clkcon 0> and
-the <&pll 2> clock is assigned a frequency value of 460800 Hz.
+In this example the <&pll 2> clock is set as parent of clock <&clkcon 0>,
+the <&clkcon 0> clock is assigned a frequency value of 500000 Hz and make
+sure the we get into assigned-clock-rates even if there is no rate change,
+and the <&pll 2> clock is assigned a frequency value of 460800 Hz.
 
 Configuring a clock's parent and rate through the device node that consumes
 the clock can be done only for clocks that have a single user. Specifying
-- 
1.9.1

Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Heiko Stübner]

Am Donnerstag, 13. November 2014, 21:20:25 schrieb Kever Yang:
> Usually we assigned a clock to a default rate in dts,
> there is a situation that the clock already initialized to the rate
> we intend to set before kernel(hardware default or init in uboot etc).
> For the PLLs we can get a rate from different PLL parameter configure,
> we can't change the PLL parameter if the rate is not changed by now.
> 
> This patch adds a option property 'assigned-clock-force-rates'
> to make sure we update all the setting even if we don't need to
> update the clock rate.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
>  drivers/clk/clk-conf.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index aad4796..0c9df48 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -84,7 +84,7 @@ static int __set_clk_rates(struct device_node *node, bool
> clk_supplier) struct clk *clk;
>  	u32 rate;
> 
> -	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
> +	of_property_for_each_u32(node, "assigned-force-rates", prop, cur, rate) {
>  		if (rate) {
>  			rc = of_parse_phandle_with_args(node, "assigned-clocks",
>  					"#clock-cells",	index, &clkspec);
> @@ -104,7 +104,38 @@ static int __set_clk_rates(struct device_node *node,
> bool clk_supplier) index, node->full_name);
>  				return PTR_ERR(clk);
>  			}
> +			/* change the old rate to 0 to make sure we can get into
> +			 * clk_change_rate */
> +			clk->rate = 0;
> +			rc = clk_set_rate(clk, rate);
> +			if (rc < 0)
> +				pr_err("clk: couldn't set %s clock rate: %d\n",
> +				       __clk_get_name(clk), rc);
> +			clk_put(clk);

Forcing clocks to 0 at first will probably create issues on some platfoms.
I think what Doug meant was something like [0], which would then enable
the clk_conf part to force the rate change. I haven't tested this yet, but it
seems the check in clk_set_rate is the only one checking for identical new
and old rates.

My one-for-all assigned-clock-force-rates param might be debatable.


Heiko


[0]

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..421422f 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -83,6 +83,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 	int rc, index = 0;
 	struct clk *clk;
 	u32 rate;
+	bool force = of_property_read_bool(node, "assigned-clock-force-rates");
 
 	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
 		if (rate) {
@@ -105,7 +106,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 				return PTR_ERR(clk);
 			}
 
-			rc = clk_set_rate(clk, rate);
+			rc = __clk_set_rate(clk, rate, force);
 			if (rc < 0)
 				pr_err("clk: couldn't set %s clock rate: %d\n",
 				       __clk_get_name(clk), rc);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4896ae9..26d183d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1528,7 +1528,7 @@ static void clk_change_rate(struct clk *clk)
  *
  * Returns 0 on success, -EERROR otherwise.
  */
-int clk_set_rate(struct clk *clk, unsigned long rate)
+int __clk_set_rate(struct clk *clk, unsigned long rate, bool force)
 {
 	struct clk *top, *fail_clk;
 	int ret = 0;
@@ -1540,7 +1540,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	clk_prepare_lock();
 
 	/* bail early if nothing to do */
-	if (rate == clk_get_rate(clk))
+	if (rate == clk_get_rate(clk) && !force)
 		goto out;
 
 	if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
@@ -1573,6 +1573,11 @@ out:
 
 	return ret;
 }
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	return __clk_set_rate(clk, rate, false);
+}
 EXPORT_SYMBOL_GPL(clk_set_rate);
 
 /**
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index be21af1..c7c3a37 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -555,6 +555,7 @@ struct clk *__clk_lookup(const char *name);
 long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
 			      unsigned long *best_parent_rate,
 			      struct clk **best_parent_p);
+int __clk_set_rate(struct clk *clk, unsigned long rate, bool force);
 
 /*
  * FIXME clock api without lock protection

Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Doug Anderson]

Hi,

On Thu, Nov 13, 2014 at 6:53 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Donnerstag, 13. November 2014, 21:20:25 schrieb Kever Yang:
>> Usually we assigned a clock to a default rate in dts,
>> there is a situation that the clock already initialized to the rate
>> we intend to set before kernel(hardware default or init in uboot etc).
>> For the PLLs we can get a rate from different PLL parameter configure,
>> we can't change the PLL parameter if the rate is not changed by now.
>>
>> This patch adds a option property 'assigned-clock-force-rates'
>> to make sure we update all the setting even if we don't need to
>> update the clock rate.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>  drivers/clk/clk-conf.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
>> index aad4796..0c9df48 100644
>> --- a/drivers/clk/clk-conf.c
>> +++ b/drivers/clk/clk-conf.c
>> @@ -84,7 +84,7 @@ static int __set_clk_rates(struct device_node *node, bool
>> clk_supplier) struct clk *clk;
>>       u32 rate;
>>
>> -     of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
>> +     of_property_for_each_u32(node, "assigned-force-rates", prop, cur, rate) {
>>               if (rate) {
>>                       rc = of_parse_phandle_with_args(node, "assigned-clocks",
>>                                       "#clock-cells", index, &clkspec);
>> @@ -104,7 +104,38 @@ static int __set_clk_rates(struct device_node *node,
>> bool clk_supplier) index, node->full_name);
>>                               return PTR_ERR(clk);
>>                       }
>> +                     /* change the old rate to 0 to make sure we can get into
>> +                      * clk_change_rate */
>> +                     clk->rate = 0;
>> +                     rc = clk_set_rate(clk, rate);
>> +                     if (rc < 0)
>> +                             pr_err("clk: couldn't set %s clock rate: %d\n",
>> +                                    __clk_get_name(clk), rc);
>> +                     clk_put(clk);
>
> Forcing clocks to 0 at first will probably create issues on some platfoms.
> I think what Doug meant was something like [0], which would then enable
> the clk_conf part to force the rate change. I haven't tested this yet, but it
> seems the check in clk_set_rate is the only one checking for identical new
> and old rates.

Hrm, I was actually not thinking of adding a new device tree property.
I was thinking that we'd _always_ call "force" for
"assigned-clock-rates".  Really the check in clk_set_rate() is an
optimization (right?), not for correctness.  Thus it should be OK to
bypass it at bootup.

Actually, maybe even better: for all clocks you should always skip the
"clk_get_rate()" check the first time through.  Then you'd ensure that
you aren't using some default or firmware-assigned clock settings.
AKA, something like this untested patch:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 59d853d..56db138 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1618,9 +1618,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
        /* prevent racing with updates to the clock topology */
        clk_prepare_lock();

-       /* bail early if nothing to do */
-       if (rate == clk_get_rate(clk))
+       /* bail early if nothing to do; linux should always set the rate once */
+       if (rate == clk_get_rate(clk) && clk->did_set_rate)
                goto out;
+       clk->did_set_rate = true;

        if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
                ret = -EBUSY;

I'm ducking now in case Mike decides to throw a tomato at me.

-Doug

Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Heiko Stübner]

Hi Mike,

Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette:
> Quoting Doug Anderson (2014-11-13 15:27:32)

[...]

> All of the above is to say that perhaps the solution to this problem
> belongs in the driver. In the end we're talking about details for
> correctly programming hardware, which sounds an awful lot like what
> drivers are supposed to do.
> 
> Let me know if the ->init() callback holds any promise for you. If not
> we can figure something out.

>From my theoretical musings, ->init() sounds like a nice idea - but of
course it comes with a "but".

I guess the general idea would be to have the pll clk-type simply reset
to the same rate but forcing it to use the parameters from its parameter
table - when the rate params differ [0].

The only problem would be the apll supplying the cpu cores. After all clocks
are registered, our armclk makes sure that the core clock gets reparented
before changing the underlying apll [dpll is safe, as it is read-only currently].

At the moment the order would be
clk_register(apll)
apll->init()
clk_register(armclk);

I'm currently unsure if simply exchanging the register-order of armclk and
the plls would help, but as the orphan handling is done before the ->init
call I guess it might help.


Heiko


[0] (compile-tested only)
diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index a3e886a..7f59579 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -257,6 +257,40 @@ static int rockchip_rk3066_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3066_PLLCON3_PWRDOWN);
 }
 
+static void rockchip_rk3066_pll_init(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	const struct rockchip_pll_rate_table *rate;
+	unsigned int nf, nr, no, bwadj;
+	unsigned long drate;
+	u32 pllcon;
+
+	drate = __clk_get_rate(hw->clk);
+	rate = rockchip_get_pll_settings(pll, drate);
+
+	/* when no rate setting for the current rate, rely on clk_set_rate */
+	if (!rate)
+		return;
+
+	pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(0));
+	nr = ((pllcon >> RK3066_PLLCON0_NR_SHIFT) & RK3066_PLLCON0_NR_MASK) + 1;
+	no = ((pllcon >> RK3066_PLLCON0_OD_SHIFT) & RK3066_PLLCON0_OD_MASK) + 1;
+
+	pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(1));
+	nf = ((pllcon >> RK3066_PLLCON1_NF_SHIFT) & RK3066_PLLCON1_NF_MASK) + 1;
+
+	pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(2));
+	bwadj = (pllcon >> RK3066_PLLCON2_BWADJ_SHIFT) & RK3066_PLLCON2_BWADJ_MASK;
+
+	if (rate->nr != nr || rate->no != no || rate->nf != nf
+					     || rate->bwadj != bwadj) {
+		struct clk *parent = __clk_get_parent(hw->clk);
+		unsigned long prate = __clk_get_rate(parent);
+
+		rockchip_rk3066_pll_set_rate(hw, drate, prate);
+	}
+}
+
 static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
 	.recalc_rate = rockchip_rk3066_pll_recalc_rate,
 	.enable = rockchip_rk3066_pll_enable,
@@ -271,6 +305,7 @@ static const struct clk_ops rockchip_rk3066_pll_clk_ops = {
 	.enable = rockchip_rk3066_pll_enable,
 	.disable = rockchip_rk3066_pll_disable,
 	.is_enabled = rockchip_rk3066_pll_is_enabled,
+	.init = rockchip_rk3066_pll_init,
 };
 
 /*

Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Doug Anderson]

Hi,

On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Heiko Stübner (2014-11-14 10:06:47)
>> Hi Mike,
>>
>> Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette:
>> > Quoting Doug Anderson (2014-11-13 15:27:32)
>>
>> [...]
>>
>> > All of the above is to say that perhaps the solution to this problem
>> > belongs in the driver. In the end we're talking about details for
>> > correctly programming hardware, which sounds an awful lot like what
>> > drivers are supposed to do.
>> >
>> > Let me know if the ->init() callback holds any promise for you. If not
>> > we can figure something out.
>>
>> From my theoretical musings, ->init() sounds like a nice idea - but of
>> course it comes with a "but".
>>
>> I guess the general idea would be to have the pll clk-type simply reset
>> to the same rate but forcing it to use the parameters from its parameter
>> table - when the rate params differ [0].
>>
>> The only problem would be the apll supplying the cpu cores. After all clocks
>> are registered, our armclk makes sure that the core clock gets reparented
>> before changing the underlying apll [dpll is safe, as it is read-only currently].

DPLL probably won't be read only forever...


>> At the moment the order would be
>> clk_register(apll)
>> apll->init()
>> clk_register(armclk);
>
> Sorry, but I don't understand the problem. The at registration-time,
> apll is re-programmed to a correct value for its current rate. Then
> armclk is registered which might change apll's rate. Any change to the
> apll which is issued from armclk should insure that apll is programmed
> correctly.

I think Heiko is worried that until the "armclk" is registered that
nobody is there to reparent the ARM core to GPLL while APLL changes
(that's armclk's job).  This is potentially unsafe.

NOTE: it actually might not be unsafe, just slow.  I think we'll
actually swap the PLL into "slow" mode before changing it (24MHz) so
we won't die we'll just run at 24MHz at boot time while we wait for
APLL to re-lock.


One option would be to just add yet another per-pll parameter.  We'll
only cleanup CPLL, GPLL, and NPLL.  If APLL (ARM clock) and DPLL
(memory clock) are set differently by firmware then we're just SOL.
Of course if firmware boots us on GPLL then I guess we're back to
square one (would firmware really be that malicious?)

-Doug

Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Heiko Stübner]

Am Dienstag, 18. November 2014, 09:59:56 schrieb Doug Anderson:
> Hi,
> 
> On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette <mturquette@linaro.org> 
wrote:
> > Quoting Heiko Stübner (2014-11-14 10:06:47)
> > 
> >> Hi Mike,
> >> 
> >> Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette:
> >> > Quoting Doug Anderson (2014-11-13 15:27:32)
> >> 
> >> [...]
> >> 
> >> > All of the above is to say that perhaps the solution to this problem
> >> > belongs in the driver. In the end we're talking about details for
> >> > correctly programming hardware, which sounds an awful lot like what
> >> > drivers are supposed to do.
> >> > 
> >> > Let me know if the ->init() callback holds any promise for you. If not
> >> > we can figure something out.
> >> 
> >> From my theoretical musings, ->init() sounds like a nice idea - but of
> >> course it comes with a "but".
> >> 
> >> I guess the general idea would be to have the pll clk-type simply reset
> >> to the same rate but forcing it to use the parameters from its parameter
> >> table - when the rate params differ [0].
> >> 
> >> The only problem would be the apll supplying the cpu cores. After all
> >> clocks are registered, our armclk makes sure that the core clock gets
> >> reparented before changing the underlying apll [dpll is safe, as it is
> >> read-only currently].
> DPLL probably won't be read only forever...
> 
> >> At the moment the order would be
> >> clk_register(apll)
> >> apll->init()
> >> clk_register(armclk);
> > 
> > Sorry, but I don't understand the problem. The at registration-time,
> > apll is re-programmed to a correct value for its current rate. Then
> > armclk is registered which might change apll's rate. Any change to the
> > apll which is issued from armclk should insure that apll is programmed
> > correctly.
> 
> I think Heiko is worried that until the "armclk" is registered that
> nobody is there to reparent the ARM core to GPLL while APLL changes
> (that's armclk's job).  This is potentially unsafe.
> 
> NOTE: it actually might not be unsafe, just slow.  I think we'll
> actually swap the PLL into "slow" mode before changing it (24MHz) so
> we won't die we'll just run at 24MHz at boot time while we wait for
> APLL to re-lock.

that is correct, we switch the pll to slow-mode (which simply passes through 
the xin24m) before touching its params, which is also the behaviour described 
in the manual for this.


> One option would be to just add yet another per-pll parameter.  We'll
> only cleanup CPLL, GPLL, and NPLL.  If APLL (ARM clock) and DPLL
> (memory clock) are set differently by firmware then we're just SOL.
> Of course if firmware boots us on GPLL then I guess we're back to
> square one (would firmware really be that malicious?)

I was talking with Kever about the same thing today. My best idea would be to 
give our plls a flag property - like all the other clocks have (divider_flags, 
mux_flags, etc) - because who knows if later pll designs will need other 
smaller adjustments. Then adding a ROCKCHIP_PLL_FLAG_SYNC_RATE triggering the 
init function to check the rate params.


Heiko