This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org --- drivers/clk/mediatek/Makefile | 2 +- drivers/clk/mediatek/clk-cpumux.c | 119 +++++++++++++++++++++++++++++++++ drivers/clk/mediatek/clk-cpumux.h | 32 +++++++++ drivers/clk/mediatek/clk-mt8173.c | 23 +++++++ drivers/clk/mediatek/clk-mtk.c | 39 +++++++++++ drivers/clk/mediatek/clk-mtk.h | 4 ++ include/dt-bindings/clock/mt8173-clk.h | 4 +- 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 drivers/clk/mediatek/clk-cpumux.c create mode 100644 drivers/clk/mediatek/clk-cpumux.h
diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile index 8e4b2a4..299917a 100644 --- a/drivers/clk/mediatek/Makefile +++ b/drivers/clk/mediatek/Makefile @@ -1,4 +1,4 @@ -obj-y += clk-mtk.o clk-pll.o clk-gate.o +obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o obj-$(CONFIG_RESET_CONTROLLER) += reset.o obj-y += clk-mt8135.o obj-y += clk-mt8173.o diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c new file mode 100644 index 0000000..2026d56 --- /dev/null +++ b/drivers/clk/mediatek/clk-cpumux.c @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2015 Linaro Ltd. + * Author: Pi-Cheng Chen pi-cheng.chen@linaro.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#include "clk-cpumux.h" + +#define ARMPLL_INDEX 1 +#define MAINPLL_INDEX 2 + +static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw) +{ + return container_of(_hw, struct mtk_clk_cpumux, hw); +} + +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate, + unsigned long min_rate, + unsigned long max_rate, + unsigned long *best_parent_rate, + struct clk_hw **best_parent_p) +{ + struct clk *clk = hw->clk, *parent; + unsigned long parent_rate; + int i; + + for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) { + parent = clk_get_parent_by_index(clk, i); + if (!parent) + return 0; + + if (i == MAINPLL_INDEX) { + parent_rate = __clk_get_rate(parent); + if (parent_rate == rate) + break; + } + + parent_rate = __clk_round_rate(parent, rate); + } + + *best_parent_rate = parent_rate; + *best_parent_p = __clk_get_hw(parent); + return parent_rate; +} + +static u8 clk_cpumux_get_parent(struct clk_hw *hw) +{ + struct mtk_clk_cpumux *mux = to_clk_mux(hw); + int num_parents = __clk_get_num_parents(hw->clk); + u32 val; + + regmap_read(mux->regmap, mux->reg, &val); + val = (val & mux->mask) >> mux->shift; + + if (val >= num_parents) + return -EINVAL; + + return val; +} + +static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index) +{ + struct mtk_clk_cpumux *mux = to_clk_mux(hw); + + return regmap_update_bits(mux->regmap, mux->reg, mux->mask, + index << mux->shift); +} + +static struct clk_ops clk_cpumux_ops = { + .get_parent = clk_cpumux_get_parent, + .set_parent = clk_cpumux_set_parent, + .determine_rate = clk_cpumux_determine_rate, +}; + +struct clk *mtk_clk_register_cpumux(const char *name, const char **parent_names, + u8 num_parents, struct regmap *regmap, + u32 reg, u8 shift, u8 width) +{ + struct mtk_clk_cpumux *mux; + struct clk *clk; + struct clk_init_data init; + u32 mask = (BIT(width) - 1) << shift; + + mux = kzalloc(sizeof(*mux), GFP_KERNEL); + if (!mux) + return ERR_PTR(-ENOMEM); + + init.name = name; + init.ops = &clk_cpumux_ops; + init.parent_names = parent_names; + init.num_parents = num_parents; + init.flags = CLK_SET_RATE_PARENT; + + mux->regmap = regmap; + mux->reg = reg; + mux->shift = shift; + mux->mask = mask; + mux->hw.init = &init; + + clk = clk_register(NULL, &mux->hw); + if (IS_ERR(clk)) + kfree(mux); + + return clk; +} + diff --git a/drivers/clk/mediatek/clk-cpumux.h b/drivers/clk/mediatek/clk-cpumux.h new file mode 100644 index 0000000..e1c8369 --- /dev/null +++ b/drivers/clk/mediatek/clk-cpumux.h @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2015 Linaro Ltd. + * Author: Pi-Cheng Chen pi-cheng.chen@linaro.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __DRV_CLK_CPUMUX_H +#define __DRV_CLK_CPUMUX_H + +#include <linux/regmap.h> + +struct mtk_clk_cpumux { + struct clk_hw hw; + struct regmap *regmap; + u32 reg; + u32 mask; + u8 shift; +}; + +struct clk *mtk_clk_register_cpumux(const char *name, const char **parent_names, + u8 num_parents, struct regmap *regmap, + u32 reg, u8 shift, u8 width); + +#endif /* __DRV_CLK_CPUMUX_H */ diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c index 014e552..124c7da 100644 --- a/drivers/clk/mediatek/clk-mt8173.c +++ b/drivers/clk/mediatek/clk-mt8173.c @@ -19,6 +19,7 @@
#include "clk-mtk.h" #include "clk-gate.h" +#include "clk-cpumux.h"
#include <dt-bindings/clock/mt8173-clk.h>
@@ -517,6 +518,25 @@ static const char *i2s3_b_ck_parents[] __initconst = { "apll2_div5" };
+static const char *ca53_parents[] __initconst = { + "clk26m", + "armca7pll", + "mainpll", + "univpll" +}; + +static const char *ca57_parents[] __initconst = { + "clk26m", + "armca15pll", + "mainpll", + "univpll" +}; + +static struct mtk_composite cpu_muxes[] __initdata = { + MUX(INFRA_CA53SEL, "infra_ca53_sel", ca53_parents, 0x0000, 0, 2), + MUX(INFRA_CA57SEL, "infra_ca57_sel", ca57_parents, 0x0000, 2, 2), +}; + static struct mtk_composite top_muxes[] __initdata = { /* CLK_CFG_0 */ MUX(TOP_AXI_SEL, "axi_sel", axi_parents, 0x0040, 0, 3), @@ -745,6 +765,9 @@ static void __init mtk_infrasys_init(struct device_node *node) mtk_clk_register_gates(node, infra_clks, ARRAY_SIZE(infra_clks), clk_data);
+ mtk_clk_register_cpumuxes(node, cpu_muxes, ARRAY_SIZE(cpu_muxes), + clk_data); + r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); if (r) pr_err("%s(): could not register clock provider: %d\n", diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c index 2bcade0..5c65cc4 100644 --- a/drivers/clk/mediatek/clk-mtk.c +++ b/drivers/clk/mediatek/clk-mtk.c @@ -23,6 +23,7 @@
#include "clk-mtk.h" #include "clk-gate.h" +#include "clk-cpumux.h"
struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num) { @@ -193,3 +194,41 @@ err_out:
return ERR_PTR(ret); } + +int mtk_clk_register_cpumuxes(struct device_node *node, + struct mtk_composite *clks, int num, + struct clk_onecell_data *clk_data) +{ + int i; + struct clk *clk; + struct regmap *regmap; + + if (!clk_data) + return -ENOMEM; + + regmap = syscon_node_to_regmap(node); + if (IS_ERR(regmap)) { + pr_err("Cannot find regmap for %s: %ld\n", node->full_name, + PTR_ERR(regmap)); + return PTR_ERR(regmap); + } + + for (i = 0; i < num; i++) { + struct mtk_composite *mux = &clks[i]; + + clk = mtk_clk_register_cpumux(mux->name, mux->parent_names, + mux->num_parents, regmap, + mux->mux_reg, mux->mux_shift, + mux->mux_width); + + if (IS_ERR(clk)) { + pr_err("Failed to register clk %s: %ld\n", + mux->name, PTR_ERR(clk)); + continue; + } + + clk_data->clks[mux->id] = clk; + } + + return 0; +} diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h index 8f5190c..f3b1840 100644 --- a/drivers/clk/mediatek/clk-mtk.h +++ b/drivers/clk/mediatek/clk-mtk.h @@ -126,6 +126,10 @@ struct mtk_gate { int mtk_clk_register_gates(struct device_node *node, struct mtk_gate *clks, int num, struct clk_onecell_data *clk_data);
+int mtk_clk_register_cpumuxes(struct device_node *node, + struct mtk_composite *clks, int num, + struct clk_onecell_data *clk_data); + struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num);
#define HAVE_RST_BAR BIT(0) diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h index e648b28..2503b03 100644 --- a/include/dt-bindings/clock/mt8173-clk.h +++ b/include/dt-bindings/clock/mt8173-clk.h @@ -187,7 +187,9 @@ #define INFRA_CEC 9 #define INFRA_PMICSPI 10 #define INFRA_PMICWRAP 11 -#define INFRA_NR_CLK 12 +#define INFRA_CA53SEL 12 +#define INFRA_CA57SEL 13 +#define INFRA_NR_CLK 14
/* PERI_SYS */
On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_p)
+{
- struct clk *clk = hw->clk, *parent;
- unsigned long parent_rate;
- int i;
- for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
parent = clk_get_parent_by_index(clk, i);
if (!parent)
return 0;
if (i == MAINPLL_INDEX) {
parent_rate = __clk_get_rate(parent);
if (parent_rate == rate)
break;
}
parent_rate = __clk_round_rate(parent, rate);
- }
- *best_parent_rate = parent_rate;
- *best_parent_p = __clk_get_hw(parent);
- return parent_rate;
+}
Why this determine_rate hook? If you want to switch the clock to some intermediate parent I would assume you do this explicitly by setting the parent and not implicitly by setting a rate.
+int mtk_clk_register_cpumuxes(struct device_node *node,
struct mtk_composite *clks, int num,
struct clk_onecell_data *clk_data)
+{
- int i;
- struct clk *clk;
- struct regmap *regmap;
- if (!clk_data)
return -ENOMEM;
- regmap = syscon_node_to_regmap(node);
- if (IS_ERR(regmap)) {
pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
PTR_ERR(regmap));
return PTR_ERR(regmap);
- }
- for (i = 0; i < num; i++) {
struct mtk_composite *mux = &clks[i];
clk = mtk_clk_register_cpumux(mux->name, mux->parent_names,
mux->num_parents, regmap,
mux->mux_reg, mux->mux_shift,
mux->mux_width);
Pass 'mux' directly instead of dispatching this struct into the individual fields. Also, probably better to move this function to drivers/clk/mediatek/clk-cpumux.c
Sascha
Hi Sascha,
On 4 March 2015 at 19:21, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_p)
+{
struct clk *clk = hw->clk, *parent;
unsigned long parent_rate;
int i;
for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
parent = clk_get_parent_by_index(clk, i);
if (!parent)
return 0;
if (i == MAINPLL_INDEX) {
parent_rate = __clk_get_rate(parent);
if (parent_rate == rate)
break;
}
parent_rate = __clk_round_rate(parent, rate);
}
*best_parent_rate = parent_rate;
*best_parent_p = __clk_get_hw(parent);
return parent_rate;
+}
Why this determine_rate hook? If you want to switch the clock to some intermediate parent I would assume you do this explicitly by setting the parent and not implicitly by setting a rate.
I use determine_rate hook here because I am using generic cpufreq-dt driver and I want to make clock switching transparent to cpufreq-dt. I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I call clk_set_rate() with the rate of MAINPLL, and determine_rate will select MAINPLL as the new parent.
+int mtk_clk_register_cpumuxes(struct device_node *node,
struct mtk_composite *clks, int num,
struct clk_onecell_data *clk_data)
+{
int i;
struct clk *clk;
struct regmap *regmap;
if (!clk_data)
return -ENOMEM;
regmap = syscon_node_to_regmap(node);
if (IS_ERR(regmap)) {
pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
PTR_ERR(regmap));
return PTR_ERR(regmap);
}
for (i = 0; i < num; i++) {
struct mtk_composite *mux = &clks[i];
clk = mtk_clk_register_cpumux(mux->name, mux->parent_names,
mux->num_parents, regmap,
mux->mux_reg, mux->mux_shift,
mux->mux_width);
Pass 'mux' directly instead of dispatching this struct into the individual fields. Also, probably better to move this function to drivers/clk/mediatek/clk-cpumux.c
Will do it. Thanks for reviewing.
Best Regards, Pi-Cheng
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
+Cc Viresh Kumar
Viresh, this is the patch for the underlying clocks for the Mediatek cpufreq driver.
On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
Hi Sascha,
On 4 March 2015 at 19:21, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_p)
+{
struct clk *clk = hw->clk, *parent;
unsigned long parent_rate;
int i;
for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
parent = clk_get_parent_by_index(clk, i);
if (!parent)
return 0;
if (i == MAINPLL_INDEX) {
parent_rate = __clk_get_rate(parent);
if (parent_rate == rate)
break;
}
parent_rate = __clk_round_rate(parent, rate);
}
*best_parent_rate = parent_rate;
*best_parent_p = __clk_get_hw(parent);
return parent_rate;
+}
Why this determine_rate hook? If you want to switch the clock to some intermediate parent I would assume you do this explicitly by setting the parent and not implicitly by setting a rate.
I use determine_rate hook here because I am using generic cpufreq-dt driver and I want to make clock switching transparent to cpufreq-dt. I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I call clk_set_rate() with the rate of MAINPLL, and determine_rate will select MAINPLL as the new parent.
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent - call clk_set_rate() for the real parent PLL - switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
Sascha
On 5 March 2015 at 15:42, Sascha Hauer s.hauer@pengutronix.de wrote:
+Cc Viresh Kumar
Viresh, this is the patch for the underlying clocks for the Mediatek cpufreq driver.
On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
Hi Sascha,
On 4 March 2015 at 19:21, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_p)
+{
struct clk *clk = hw->clk, *parent;
unsigned long parent_rate;
int i;
for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
parent = clk_get_parent_by_index(clk, i);
if (!parent)
return 0;
if (i == MAINPLL_INDEX) {
parent_rate = __clk_get_rate(parent);
if (parent_rate == rate)
break;
}
parent_rate = __clk_round_rate(parent, rate);
}
*best_parent_rate = parent_rate;
*best_parent_p = __clk_get_hw(parent);
return parent_rate;
+}
Why this determine_rate hook? If you want to switch the clock to some intermediate parent I would assume you do this explicitly by setting the parent and not implicitly by setting a rate.
I use determine_rate hook here because I am using generic cpufreq-dt driver and I want to make clock switching transparent to cpufreq-dt. I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I call clk_set_rate() with the rate of MAINPLL, and determine_rate will select MAINPLL as the new parent.
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
Hi,
Yes. But the frequency of intermediate PLL parent is different from the original frequency and the frequency after changed of original PLL clock in most cases. If the frequency of intermediate PLL parent is higher than the original frequency, then we have to scale up the voltage first before switch mux to intermediate PLL parent. I am not sure this could be properly handled if I implement it in the way you suggested. For now, I put the mux switching logic in the determine_rate hook so that the voltage scaling could be handled by target_intermediate hook of cpufreq-dt which I am also working on[1].
[1] http://marc.info/?l=linux-kernel&m=142545898313351&w=2
Best Regards, Pi-Cheng
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
CPUFreq wants to change to intermediate frequency by itself against some magic change behind the scene. The major requirement for that comes from the fact that we want to send PRE/POST freq notifiers on which loops-per-jiffie depends.
On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
The sequence to change the CPU frequency on the Mediatek SoCs is like this:
- Change CPU from CPU PLL to another clock source (intermediate source) - Change CPU PLL frequency - wait until PLL has settled - switch back to CPU PLL
The frequency of the intermediate source is irrelevant, the important thing is that the CPU is switched to this source while the CPU PLL is reconfigured.
In Pi-Chengs patches the switch to th eintermediate clock is done like:
rate = clk_get_rate(intermediate_clk); clk_set_rate(cpu_clk, rate);
Now the clk framework does the switch not because it's told to switch to another parent, but only because the other parent happens to be the only provider for that rate. That's rubbish, when the parent must be changed, then it should be done explicitly. What if the CPU PLL and the intermediate clk happen to have the same rate? Then the clk_set_rate above simply does nothing, no parent is changed and the following rate change of the CPU PLL just crashes the system.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
CPUFreq wants to change to intermediate frequency by itself against some magic change behind the scene. The major requirement for that comes from the fact that we want to send PRE/POST freq notifiers on which loops-per-jiffie depends.
Maybe, I don't know the internals of CPUFreq. But here the important thing is the rate change, not the parent change.
Sascha
On 5 March 2015 at 17:19, Sascha Hauer s.hauer@pengutronix.de wrote:
On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
The sequence to change the CPU frequency on the Mediatek SoCs is like this:
- Change CPU from CPU PLL to another clock source (intermediate source)
- Change CPU PLL frequency
- wait until PLL has settled
- switch back to CPU PLL
The frequency of the intermediate source is irrelevant, the important thing is that the CPU is switched to this source while the CPU PLL is reconfigured.
In Pi-Chengs patches the switch to th eintermediate clock is done like:
rate = clk_get_rate(intermediate_clk); clk_set_rate(cpu_clk, rate);
Now the clk framework does the switch not because it's told to switch to another parent, but only because the other parent happens to be the only provider for that rate. That's rubbish, when the parent must be changed, then it should be done explicitly.
Hi,
Please correct me if I was wrong. But I think that's the reason why we have a determine_rate hook here, isn't it?
What if the CPU PLL and the intermediate clk happen to have the same rate? Then the clk_set_rate above simply does nothing, no parent is changed and the following rate change of the CPU PLL just crashes the system.
I think what I am trying to do in the determine_rate hook of cpumux is:
* if the target rate of clk_set_rate is equal to the rate of MAINPLL, then switch the mux to MAINPLL * otherwise, set the rate directly on ARMPLL and switch the mux back to ARMPLL if the current parent of mux is not ARMPLL
And if the CPU PLL and the intermediate clk happen to have the same rate, I think the cpufreq-dt driver will change nothing on the clk framework. Or do I misunderstand your point?
Thanks.
Best Regards, Pi-Cheng
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
CPUFreq wants to change to intermediate frequency by itself against some magic change behind the scene. The major requirement for that comes from the fact that we want to send PRE/POST freq notifiers on which loops-per-jiffie depends.
Maybe, I don't know the internals of CPUFreq. But here the important thing is the rate change, not the parent change.
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Mar 05, 2015 at 05:39:12PM +0800, Pi-Cheng Chen wrote:
On 5 March 2015 at 17:19, Sascha Hauer s.hauer@pengutronix.de wrote:
On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
The sequence to change the CPU frequency on the Mediatek SoCs is like this:
- Change CPU from CPU PLL to another clock source (intermediate source)
- Change CPU PLL frequency
- wait until PLL has settled
- switch back to CPU PLL
The frequency of the intermediate source is irrelevant, the important thing is that the CPU is switched to this source while the CPU PLL is reconfigured.
In Pi-Chengs patches the switch to th eintermediate clock is done like:
rate = clk_get_rate(intermediate_clk); clk_set_rate(cpu_clk, rate);
Now the clk framework does the switch not because it's told to switch to another parent, but only because the other parent happens to be the only provider for that rate. That's rubbish, when the parent must be changed, then it should be done explicitly.
Hi,
Please correct me if I was wrong. But I think that's the reason why we have a determine_rate hook here, isn't it?
What if the CPU PLL and the intermediate clk happen to have the same rate? Then the clk_set_rate above simply does nothing, no parent is changed and the following rate change of the CPU PLL just crashes the system.
I think what I am trying to do in the determine_rate hook of cpumux is:
- if the target rate of clk_set_rate is equal to the rate of MAINPLL,
then switch the mux to MAINPLL
- otherwise, set the rate directly on ARMPLL and switch the mux back to ARMPLL if the current parent of mux is not ARMPLL
And if the CPU PLL and the intermediate clk happen to have the same rate, I think the cpufreq-dt driver will change nothing on the clk framework. Or do I misunderstand your point?
Imagine the board starts with both the CPU PLL and Intermediate PLL running with the same frequency with the CPU driven from the CPU PLL. Now the cpufreq driver does it's very first frequency transition. Now when clk_set_rate is used with the intention to switch to the intermediate PLL nothing will happen because the CPU PLL already runs at the desired frequency. Reconfiguring the CPU PLL then crashes your system.
Another reason why abusing clk_set_rate to change the parent is this: You have this in your SoC:
--- CPU_PLL ---| | | | | |----- CPU | | IM_PLL ----| | ---
Now you do a clk_set_rate(CPU, clk_get_rate(IM_PLL)) which (often) works in your case. Many SoCs have an additional divider though, like this:
--- CPU_PLL ---| | | | ---- | |--| :x | --- CPU | | ---- IM_PLL ----| | ---
Now clk_set_rate(CPU, clk_get_rate(IM_PLL)) doesn't give anything sensible anymore.
Given the variance of different SoCs I don't think it makes sense to try to handle all these cases. Instead the cpufreq-dt driver should just call clk_set_rate() on the CPU clock with the desired target frequency. Everything else should be handled in the clock driver which has intimate knowledge about the SoC anyway.
Sascha
On 5 March 2015 at 16:21, Sascha Hauer s.hauer@pengutronix.de wrote:
Given the variance of different SoCs I don't think it makes sense to try to handle all these cases. Instead the cpufreq-dt driver should just call clk_set_rate() on the CPU clock with the desired target frequency. Everything else should be handled in the clock driver which has intimate knowledge about the SoC anyway.
I agree..
@Russell: I wanted to ask you this since sometime..
On CPU-freq changes we fire PRE/POST notifiers and they are used for updating loops_per_jiffies which then controls delays.
Now, it is fine to do that for normal frequencies, but what should be the approach for intermediate frequencies ?
Intermediate freqs: On some platforms changing PLL's straight away isn't considered safe and so we switch parent to another stable clock, change PLL rate and switch back.
The *wild* thought I earlier had was to fire these notifiers for even these intermediate frequencies, otherwise some of the delays will end before they should have and that *might* cause other problems.
I wanted to know what do you (and other champs) think about this..
Thanks in advance for your advice.
Quoting Viresh Kumar (2015-03-05 03:02:06)
On 5 March 2015 at 16:21, Sascha Hauer s.hauer@pengutronix.de wrote:
Given the variance of different SoCs I don't think it makes sense to try to handle all these cases. Instead the cpufreq-dt driver should just call clk_set_rate() on the CPU clock with the desired target frequency. Everything else should be handled in the clock driver which has intimate knowledge about the SoC anyway.
I agree..
@Russell: I wanted to ask you this since sometime..
On CPU-freq changes we fire PRE/POST notifiers and they are used for updating loops_per_jiffies which then controls delays.
Now, it is fine to do that for normal frequencies, but what should be the approach for intermediate frequencies ?
Intermediate freqs: On some platforms changing PLL's straight away isn't considered safe and so we switch parent to another stable clock, change PLL rate and switch back.
The *wild* thought I earlier had was to fire these notifiers for even these intermediate frequencies, otherwise some of the delays will end before they should have and that *might* cause other problems.
I wanted to know what do you (and other champs) think about this..
Thanks in advance for your advice.
Sorry, I am not who you asked for advice but I will chime in anyways ;-)
I really hate this intermediate frequency stuff in cpufreq. As we discussed over lunch in Hong Kong, it does not scale. What if there two intermediate frequencies? What if a processor steps frequency up in 5KHz increments (albeit very quickly) until it converges to the target rate?
Or what if we have more complex levels of intermediate frequencies? Stealing Sascha's excellent ascii art:
--- CPU_PLL ---| | | | | |----- CPU | | IM_PLL ----| | ---
Where the sequence is as follows:
1) set IM_PLL to a new freq 2) reparent CPU to IM_PLL 3) configure CPU_PLL 4) reparent CPU to CPU_PLL 5) restore IM_PLL to original frequency
Steps #1 and #5 are new in this example.
I don't see how the concept of an intermediate frequency in the cpufreq core could ever scale gracefully to handle corner cases. They may be hypothetical now but I'd rather see us dodge this mistake.
Furthermore any intermediate-frequency property in a Devicetree binding would suffer the same fate. Trying to neatly encode some weird sequence into this generic thing will get very ugly very fast.
For proof please look at clk-divider.c, clk-gate.c, clk-mux.c or clk-composite.c and you'll see the result of the slow accumulation of lots and lots of hardware corner cases onto generic code. If I had known then what I know now I would not have created those generic clock types and I would have tried for an abstraction layer between generic stuff (e.g. find the best divider) and the real hardware stuff (write to the register). Instead I kept all of it together and now things are super ugly.
Regards, Mike
On 11 March 2015 at 05:43, Mike Turquette mturquette@linaro.org wrote:
Sorry, I am not who you asked for advice but I will chime in anyways ;-)
Always welcome :)
I really hate this intermediate frequency stuff in cpufreq. As we
I am starting to :)
Furthermore any intermediate-frequency property in a Devicetree binding would suffer the same fate. Trying to neatly encode some weird sequence into this generic thing will get very ugly very fast.
Hmm..
For proof please look at clk-divider.c, clk-gate.c, clk-mux.c or clk-composite.c and you'll see the result of the slow accumulation of lots and lots of hardware corner cases onto generic code. If I had known then what I know now I would not have created those generic clock types and I would have tried for an abstraction layer between generic stuff (e.g. find the best divider) and the real hardware stuff (write to the register). Instead I kept all of it together and now things are super ugly.
Yeah.
On Thu, Mar 05, 2015 at 04:32:06PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 16:21, Sascha Hauer s.hauer@pengutronix.de wrote:
Given the variance of different SoCs I don't think it makes sense to try to handle all these cases. Instead the cpufreq-dt driver should just call clk_set_rate() on the CPU clock with the desired target frequency. Everything else should be handled in the clock driver which has intimate knowledge about the SoC anyway.
I agree..
@Russell: I wanted to ask you this since sometime..
Remember that I've been "away" from mail for several days last week, as I had widely published on google+ and on the lists.
On CPU-freq changes we fire PRE/POST notifiers and they are used for updating loops_per_jiffies which then controls delays.
Their main purpose is to allow drivers to shut down their peripherals before the change and bring them back up after the change on SoCs where the peripherals are clocked off the CPU clock or a derivative of it.
For example, SA11x0 SoCs were never actually designed for CPU frequency scaling, but we made it work - and in doing so, you have to reprogram a bunch of peripherals such as PCMCIA timings, LCD controller, and SDRAM controller - and because of the need to quiesce the SDRAM controller over the transition, we need to shut down anything which may cause RAM to be accessed (such as DMA.)
Now, it is fine to do that for normal frequencies, but what should be the approach for intermediate frequencies ?
I don't think so - calling the notifiers can be expensive where peripheral drivers are involved - peripherals may need to wait for the hardware to quiesce before they can allow the notifier to continue. You could make it a different reason code (so that drivers such as the SA11x0 stuff can ignore these intermediate steps.)
The *wild* thought I earlier had was to fire these notifiers for even these intermediate frequencies, otherwise some of the delays will end before they should have and that *might* cause other problems.
The real answer is not to use the software delay at all when cpufreq is enabled, and use a timer based delay which is then independent of the CPU clock rate.
The only case where a software delay is acceptable is as a last resort fallback if the platform has no other option, or the CPU clock is stable.
On 5 March 2015 at 18:51, Sascha Hauer s.hauer@pengutronix.de wrote:
On Thu, Mar 05, 2015 at 05:39:12PM +0800, Pi-Cheng Chen wrote:
On 5 March 2015 at 17:19, Sascha Hauer s.hauer@pengutronix.de wrote:
On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
The sequence to change the CPU frequency on the Mediatek SoCs is like this:
- Change CPU from CPU PLL to another clock source (intermediate source)
- Change CPU PLL frequency
- wait until PLL has settled
- switch back to CPU PLL
The frequency of the intermediate source is irrelevant, the important thing is that the CPU is switched to this source while the CPU PLL is reconfigured.
In Pi-Chengs patches the switch to th eintermediate clock is done like:
rate = clk_get_rate(intermediate_clk); clk_set_rate(cpu_clk, rate);
Now the clk framework does the switch not because it's told to switch to another parent, but only because the other parent happens to be the only provider for that rate. That's rubbish, when the parent must be changed, then it should be done explicitly.
Hi,
Please correct me if I was wrong. But I think that's the reason why we have a determine_rate hook here, isn't it?
What if the CPU PLL and the intermediate clk happen to have the same rate? Then the clk_set_rate above simply does nothing, no parent is changed and the following rate change of the CPU PLL just crashes the system.
I think what I am trying to do in the determine_rate hook of cpumux is:
- if the target rate of clk_set_rate is equal to the rate of MAINPLL,
then switch the mux to MAINPLL
- otherwise, set the rate directly on ARMPLL and switch the mux back to ARMPLL if the current parent of mux is not ARMPLL
And if the CPU PLL and the intermediate clk happen to have the same rate, I think the cpufreq-dt driver will change nothing on the clk framework. Or do I misunderstand your point?
Imagine the board starts with both the CPU PLL and Intermediate PLL running with the same frequency with the CPU driven from the CPU PLL. Now the cpufreq driver does it's very first frequency transition. Now when clk_set_rate is used with the intention to switch to the intermediate PLL nothing will happen because the CPU PLL already runs at the desired frequency. Reconfiguring the CPU PLL then crashes your system.
Another reason why abusing clk_set_rate to change the parent is this: You have this in your SoC:
---
CPU_PLL ---| | | | | |----- CPU | | IM_PLL ----| | ---
Now you do a clk_set_rate(CPU, clk_get_rate(IM_PLL)) which (often) works in your case. Many SoCs have an additional divider though, like this:
---
CPU_PLL ---| | | | ---- | |--| :x | --- CPU | | ---- IM_PLL ----| | ---
Now clk_set_rate(CPU, clk_get_rate(IM_PLL)) doesn't give anything sensible anymore.
Given the variance of different SoCs I don't think it makes sense to try to handle all these cases. Instead the cpufreq-dt driver should just call clk_set_rate() on the CPU clock with the desired target frequency. Everything else should be handled in the clock driver which has intimate knowledge about the SoC anyway.
Now I got it. Thanks for your elaboration. I will investigate the way you suggested to implement intermediate clock switching.
Best Regards, Pi-Cheng
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 5 March 2015 at 14:49, Sascha Hauer s.hauer@pengutronix.de wrote:
The sequence to change the CPU frequency on the Mediatek SoCs is like this:
- Change CPU from CPU PLL to another clock source (intermediate source)
- Change CPU PLL frequency
- wait until PLL has settled
- switch back to CPU PLL
This should be the case for most of the intermediate-freq users..
The frequency of the intermediate source is irrelevant, the important thing is that the CPU is switched to this source while the CPU PLL is reconfigured.
Right.
In Pi-Chengs patches the switch to th eintermediate clock is done like:
rate = clk_get_rate(intermediate_clk); clk_set_rate(cpu_clk, rate);
Now the clk framework does the switch not because it's told to switch to another parent, but only because the other parent happens to be the only provider for that rate. That's rubbish, when the parent must be changed, then it should be done explicitly. What if the CPU PLL and the intermediate clk happen to have the same rate? Then the clk_set_rate above simply does nothing, no parent is changed and the following rate change of the CPU PLL just crashes the system.
The problem is that the code is common across platforms that need to reparent or just change rate for intermediate clocks. And the best we can do is clk_set_rate() and so probably the clk driver need to take care of this somehow and make sure we don't result in a crash like you just demonstrated.
On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
CPUFreq wants to change to intermediate frequency by itself against some magic change behind the scene. The major requirement for that comes from the fact that we want to send PRE/POST freq notifiers on which loops-per-jiffie depends.
You could register a clk notifier using clk_notifier_register(). The notifer callback can then call the cpufreq notifiers. The clk notifiers can also be used to change the CPU voltage.
Sascha
Quoting Viresh Kumar (2015-03-05 00:59:50)
On 5 March 2015 at 13:12, Sascha Hauer s.hauer@pengutronix.de wrote:
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
clk_set_rate() for CPUs clock is responsible to change clock rate of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
Agreed.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
CPUFreq wants to change to intermediate frequency by itself against some magic change behind the scene. The major requirement for that comes from the fact that we want to send PRE/POST freq notifiers on which loops-per-jiffie depends.
I assume you are saying that you want to update loops-per-jiffie while at an intermediate frequency. Why? This operation should not take very long.
Imagine a (hypothetical?) processor that changes frequency in many small steps until it converges to the target rate. Would you want to update lpj for every step?
Regards, Mike
On 11 March 2015 at 05:29, Mike Turquette mturquette@linaro.org wrote:
I assume you are saying that you want to update loops-per-jiffie while at an intermediate frequency. Why? This operation should not take very long.
Imagine a (hypothetical?) processor that changes frequency in many small steps until it converges to the target rate. Would you want to update lpj for every step?
That's why I have asked Russell specifically about this but haven't got a reply yet. But it looks wrong to me as well now.. Probably we don't need to care about it at all..
On 5 March 2015 at 15:42, Sascha Hauer s.hauer@pengutronix.de wrote:
+Cc Viresh Kumar
Viresh, this is the patch for the underlying clocks for the Mediatek cpufreq driver.
On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
Hi Sascha,
On 4 March 2015 at 19:21, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for intermediate clock source switching. This patch is based on Mediatek clock driver patches[1].
[1] http://thread.gmane.org/gmane.linux.kernel/1892436
Signed-off-by: pi-cheng.chen pi-cheng.chen@linaro.org
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_p)
+{
struct clk *clk = hw->clk, *parent;
unsigned long parent_rate;
int i;
for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
parent = clk_get_parent_by_index(clk, i);
if (!parent)
return 0;
if (i == MAINPLL_INDEX) {
parent_rate = __clk_get_rate(parent);
if (parent_rate == rate)
break;
}
parent_rate = __clk_round_rate(parent, rate);
}
*best_parent_rate = parent_rate;
*best_parent_p = __clk_get_hw(parent);
return parent_rate;
+}
Why this determine_rate hook? If you want to switch the clock to some intermediate parent I would assume you do this explicitly by setting the parent and not implicitly by setting a rate.
I use determine_rate hook here because I am using generic cpufreq-dt driver and I want to make clock switching transparent to cpufreq-dt. I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I call clk_set_rate() with the rate of MAINPLL, and determine_rate will select MAINPLL as the new parent.
We have clk_set_parent for changing the parent and clk_set_rate to change the rate. Use the former for changing the parent and the latter for changing the rate. What you are interested in is changing the parent, so use clk_set_parent for this and not abuse a side effect of clk_set_rate.
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
Hi Sascha,
Thanks for your suggestion. I've tried to take this approach, but there's some issues here.
Calling clk_set_rate() inside the set_rate callback of cpumux will cause an infinite recursive calling in the clock framework: mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...
I've also tries to update pll register settings in the set_rate() callback of cpumux, but the PLL clock information will not be correctly updated in this case.
How do you think to create a new "CPU PLL" type of clock and do underlying mux switching in the set_rate callback of "CPU PLL"?
Best Regards, Pi-Cheng
This way the things happening behind the scenes are completely transparent to the cpufreq driver and you can use cpufreq-dt as is without changes.
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Mar 10, 2015 at 09:53:19AM +0800, Pi-Cheng Chen wrote:
On 5 March 2015 at 15:42, Sascha Hauer s.hauer@pengutronix.de wrote:
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
Hi Sascha,
Thanks for your suggestion. I've tried to take this approach, but there's some issues here.
Calling clk_set_rate() inside the set_rate callback of cpumux will cause an infinite recursive calling in the clock framework: mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...
I don't understand why setting the PLL rate should call into the mux set_rate. Are you sure you call clk_set_rate for the mux parent clk? I think the general approach should work, drivers/clk/sirf/clk-common.c does something similar in cpu_clk_set_rate(). If you like you can send me your work in progress state privatly, I'll have a look then.
I've also tries to update pll register settings in the set_rate() callback of cpumux, but the PLL clock information will not be correctly updated in this case.
No, that won't work.
Sascha
On Tue, Mar 10, 2015 at 3:55 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Tue, Mar 10, 2015 at 09:53:19AM +0800, Pi-Cheng Chen wrote:
On 5 March 2015 at 15:42, Sascha Hauer s.hauer@pengutronix.de wrote:
My suggestion is to take another approach. Implement clk_set_rate for these muxes and in the set_rate hook:
- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL
Hi Sascha,
Thanks for your suggestion. I've tried to take this approach, but there's some issues here.
Calling clk_set_rate() inside the set_rate callback of cpumux will cause an infinite recursive calling in the clock framework: mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...
I don't understand why setting the PLL rate should call into the mux set_rate. Are you sure you call clk_set_rate for the mux parent clk? I think the general approach should work, drivers/clk/sirf/clk-common.c does something similar in cpu_clk_set_rate(). If you like you can send me your work in progress state privatly, I'll have a look then.
Thanks for pointing me out the reference. I think I misunderstood the way you suggested to do it. I'll post the new version once the design including cpufreq part is finalized.
Best Regards, Pi-Cheng
I've also tries to update pll register settings in the set_rate() callback of cpumux, but the PLL clock information will not be correctly updated in this case.
No, that won't work.
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
linaro-kernel@lists.linaro.org