Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Am Dienstag, 18. November 2014, 09:59:56 schrieb Doug Anderson: > Hi, > > On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Hi, On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Hi, On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette mturque...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Am Dienstag, 18. November 2014, 09:59:56 schrieb Doug Anderson: Hi, On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette mturque...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
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,
};
/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
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,
};
/*
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Hi,
On Thu, Nov 13, 2014 at 6:53 AM, Heiko Stübner 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
>> ---
>>
>> 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, );
>> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
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
> ---
>
> 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, );
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message
[RFC PATCH 1/2] clk: add property for force to update clock setting
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
---
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, );
@@ -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, );
+ 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();
+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 1/2] clk: add property for force to update clock setting
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.y...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
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.y...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More
Re: [RFC PATCH 1/2] clk: add property for force to update clock setting
Hi,
On Thu, Nov 13, 2014 at 6:53 AM, Heiko Stübner he...@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.y...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
