Hi Stephen,
This is the first attempt to get rid of tegra-cpufreq driver. This patchset tries to add supporting infrastructure for tegra to use cpufreq-cpu0 driver.
I don't have hardware to test it and so is compiled tested only.. Few bits may be missing as I couldn't think of all aspects and so may need your help getting them fixed.
Once this is tested by you, I would like to take it through my ARM cpufreq tree if nobody else has a problem with it.
Thanks
-- Viresh.
Viresh Kumar (6): clk: Tegra: Add CPU0 clock driver ARM: Tegra: Add CPU's OPPs for using cpufreq-cpu0 driver ARM: Tegra: Enable OPP library ARM: Tegra: defconfig: select cpufreq-cpu0 driver ARM: Tegra: start using cpufreq-cpu0 driver cpufreq: Tegra: Remove tegra-cpufreq driver
arch/arm/boot/dts/tegra114.dtsi | 12 ++ arch/arm/boot/dts/tegra20.dtsi | 12 ++ arch/arm/boot/dts/tegra30.dtsi | 12 ++ arch/arm/configs/tegra_defconfig | 1 + arch/arm/mach-tegra/Kconfig | 2 + arch/arm/mach-tegra/tegra.c | 2 + drivers/clk/tegra/Makefile | 1 + drivers/clk/tegra/clk-cpu.c | 164 ++++++++++++++++++++++ drivers/clk/tegra/clk-tegra30.c | 4 + drivers/cpufreq/Kconfig.arm | 8 -- drivers/cpufreq/Makefile | 1 - drivers/cpufreq/tegra-cpufreq.c | 291 --------------------------------------- include/linux/clk/tegra.h | 1 + 13 files changed, 211 insertions(+), 300 deletions(-) create mode 100644 drivers/clk/tegra/clk-cpu.c delete mode 100644 drivers/cpufreq/tegra-cpufreq.c
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clk/tegra/Makefile | 1 + drivers/clk/tegra/clk-cpu.c | 164 ++++++++++++++++++++++++++++++++++++++++ drivers/clk/tegra/clk-tegra30.c | 4 + include/linux/clk/tegra.h | 1 + 4 files changed, 170 insertions(+) create mode 100644 drivers/clk/tegra/clk-cpu.c
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile index f49fac2..0e818c0 100644 --- a/drivers/clk/tegra/Makefile +++ b/drivers/clk/tegra/Makefile @@ -10,3 +10,4 @@ obj-y += clk-super.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o +obj-$(CONFIG_GENERIC_CPUFREQ_CPU0) += clk-cpu.o diff --git a/drivers/clk/tegra/clk-cpu.c b/drivers/clk/tegra/clk-cpu.c new file mode 100644 index 0000000..01716d6 --- /dev/null +++ b/drivers/clk/tegra/clk-cpu.c @@ -0,0 +1,164 @@ +/* + * Copyright (C) 2013 Linaro + * + * Author: Viresh Kumar viresh.kumar@linaro.org + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +/* + * Responsible for setting cpu0 clk as requested by cpufreq-cpu0 driver + * + * All platform specific bits are taken from tegra-cpufreq driver. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/slab.h> + +#define to_clk_cpu0(_hw) container_of(_hw, struct clk_cpu0, hw) + +struct clk_cpu0 { + struct clk_hw hw; + spinlock_t *lock; +}; + +static struct clk *cpu_clk; +static struct clk *pll_x_clk; +static struct clk *pll_p_clk; +static struct clk *emc_clk; + +static unsigned long cpu0_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + return clk_get_rate(cpu_clk); +} + +static long cpu0_round_rate(struct clk_hw *hw, unsigned long drate, + unsigned long *parent_rate) +{ + return clk_round_rate(cpu_clk, drate); +} + +static int cpu0_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + int ret; + + /* + * Vote on memory bus frequency based on cpu frequency + * This sets the minimum frequency, display or avp may request higher + */ + if (rate >= 816000000) + clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */ + else if (rate >= 456000000) + clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */ + else + clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ + + /* + * Take an extra reference to the main pll so it doesn't turn + * off when we move the cpu off of it + */ + clk_prepare_enable(pll_x_clk); + + ret = clk_set_parent(cpu_clk, pll_p_clk); + if (ret) { + pr_err("%s: Failed to switch cpu to clock pll_p\n", __func__); + goto out; + } + + if (rate == clk_get_rate(pll_p_clk)) + goto out; + + ret = clk_set_rate(pll_x_clk, rate); + if (ret) { + pr_err("Failed to change pll_x to %lu\n", rate); + goto out; + } + + ret = clk_set_parent(cpu_clk, pll_x_clk); + if (ret) { + pr_err("Failed to switch cpu to clock pll_x\n"); + goto out; + } + +out: + clk_disable_unprepare(pll_x_clk); + return ret; +} + +static struct clk_ops clk_cpu0_ops = { + .recalc_rate = cpu0_recalc_rate, + .round_rate = cpu0_round_rate, + .set_rate = cpu0_set_rate, +}; + +struct clk *tegra_clk_register_cpu0(void) +{ + struct clk_init_data init; + struct clk_cpu0 *cpu0; + struct clk *clk; + + cpu0 = kzalloc(sizeof(*cpu0), GFP_KERNEL); + if (!cpu0) { + pr_err("%s: could not allocate cpu0 clk\n", __func__); + return ERR_PTR(-ENOMEM); + } + + cpu_clk = clk_get_sys(NULL, "cpu"); + if (IS_ERR(cpu_clk)) { + clk = cpu_clk; + goto free_mem; + } + + pll_x_clk = clk_get_sys(NULL, "pll_x"); + if (IS_ERR(pll_x_clk)) { + clk = pll_x_clk; + goto put_cpu_clk; + } + + pll_p_clk = clk_get_sys(NULL, "pll_p_cclk"); + if (IS_ERR(pll_p_clk)) { + clk = pll_p_clk; + goto put_pll_x_clk; + } + + emc_clk = clk_get_sys("cpu", "emc"); + if (IS_ERR(emc_clk)) { + clk = emc_clk; + goto put_pll_p_clk; + } + + cpu0->hw.init = &init; + + init.name = "cpu0"; + init.ops = &clk_cpu0_ops; + init.flags = CLK_IS_ROOT | CLK_GET_RATE_NOCACHE; + init.num_parents = 0; + + clk = clk_register(NULL, &cpu0->hw); + if (!IS_ERR(clk)) + return clk; + + clk_prepare_enable(emc_clk); + clk_prepare_enable(cpu_clk); + + clk_put(emc_clk); +put_pll_p_clk: + clk_put(pll_p_clk); +put_pll_x_clk: + clk_put(pll_x_clk); +put_cpu_clk: + clk_put(cpu_clk); +free_mem: + kfree(cpu0); + + pr_err("%s: clk register failed\n", __func__); + + return NULL; +} diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c index e2c6ca0..1cabeea 100644 --- a/drivers/clk/tegra/clk-tegra30.c +++ b/drivers/clk/tegra/clk-tegra30.c @@ -1396,6 +1396,10 @@ static void __init tegra30_super_clk_init(void) CLK_SET_RATE_PARENT, 1, 2); clk_register_clkdev(clk, "twd", NULL); clks[twd] = clk; + + /* cpu0 clk for cpufreq driver */ + clk = tegra_clk_register_cpu0(); + clk_register_clkdev(clk, NULL, "cpu0"); }
static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p", diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h index 23a0cee..d416138 100644 --- a/include/linux/clk/tegra.h +++ b/include/linux/clk/tegra.h @@ -128,5 +128,6 @@ static inline void tegra_periph_reset_deassert(struct clk *c) {} static inline void tegra_periph_reset_assert(struct clk *c) {} #endif void tegra_clocks_apply_init_table(void); +struct clk *tegra_clk_register_cpu0(void);
#endif /* __LINUX_CLK_TEGRA_H_ */
Quoting Viresh Kumar (2013-08-07 07:46:43)
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Viresh,
It is nice to see more CPUfreq consolidation.
I'm currently hacking on a patch to introduce clk_coordinate_rates(). That function may be a better fit for this sort of thing compared to overloading the .set_rate callback. I'll try to get the patches on the list ASAP and will Cc you.
Regards, Mike
drivers/clk/tegra/Makefile | 1 + drivers/clk/tegra/clk-cpu.c | 164 ++++++++++++++++++++++++++++++++++++++++ drivers/clk/tegra/clk-tegra30.c | 4 + include/linux/clk/tegra.h | 1 + 4 files changed, 170 insertions(+) create mode 100644 drivers/clk/tegra/clk-cpu.c
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile index f49fac2..0e818c0 100644 --- a/drivers/clk/tegra/Makefile +++ b/drivers/clk/tegra/Makefile @@ -10,3 +10,4 @@ obj-y += clk-super.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o +obj-$(CONFIG_GENERIC_CPUFREQ_CPU0) += clk-cpu.o diff --git a/drivers/clk/tegra/clk-cpu.c b/drivers/clk/tegra/clk-cpu.c new file mode 100644 index 0000000..01716d6 --- /dev/null +++ b/drivers/clk/tegra/clk-cpu.c @@ -0,0 +1,164 @@ +/*
- Copyright (C) 2013 Linaro
- Author: Viresh Kumar viresh.kumar@linaro.org
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+/*
- Responsible for setting cpu0 clk as requested by cpufreq-cpu0 driver
- All platform specific bits are taken from tegra-cpufreq driver.
- */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/slab.h>
+#define to_clk_cpu0(_hw) container_of(_hw, struct clk_cpu0, hw)
+struct clk_cpu0 {
struct clk_hw hw;spinlock_t *lock;+};
+static struct clk *cpu_clk; +static struct clk *pll_x_clk; +static struct clk *pll_p_clk; +static struct clk *emc_clk;
+static unsigned long cpu0_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)+{
return clk_get_rate(cpu_clk);+}
+static long cpu0_round_rate(struct clk_hw *hw, unsigned long drate,
unsigned long *parent_rate)+{
return clk_round_rate(cpu_clk, drate);+}
+static int cpu0_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)+{
int ret;/** Vote on memory bus frequency based on cpu frequency* This sets the minimum frequency, display or avp may request higher*/if (rate >= 816000000)clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */else if (rate >= 456000000)clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */elseclk_set_rate(emc_clk, 100000000); /* emc 50Mhz *//** Take an extra reference to the main pll so it doesn't turn* off when we move the cpu off of it*/clk_prepare_enable(pll_x_clk);ret = clk_set_parent(cpu_clk, pll_p_clk);if (ret) {pr_err("%s: Failed to switch cpu to clock pll_p\n", __func__);goto out;}if (rate == clk_get_rate(pll_p_clk))goto out;ret = clk_set_rate(pll_x_clk, rate);if (ret) {pr_err("Failed to change pll_x to %lu\n", rate);goto out;}ret = clk_set_parent(cpu_clk, pll_x_clk);if (ret) {pr_err("Failed to switch cpu to clock pll_x\n");goto out;}+out:
clk_disable_unprepare(pll_x_clk);return ret;+}
+static struct clk_ops clk_cpu0_ops = {
.recalc_rate = cpu0_recalc_rate,.round_rate = cpu0_round_rate,.set_rate = cpu0_set_rate,+};
+struct clk *tegra_clk_register_cpu0(void) +{
struct clk_init_data init;struct clk_cpu0 *cpu0;struct clk *clk;cpu0 = kzalloc(sizeof(*cpu0), GFP_KERNEL);if (!cpu0) {pr_err("%s: could not allocate cpu0 clk\n", __func__);return ERR_PTR(-ENOMEM);}cpu_clk = clk_get_sys(NULL, "cpu");if (IS_ERR(cpu_clk)) {clk = cpu_clk;goto free_mem;}pll_x_clk = clk_get_sys(NULL, "pll_x");if (IS_ERR(pll_x_clk)) {clk = pll_x_clk;goto put_cpu_clk;}pll_p_clk = clk_get_sys(NULL, "pll_p_cclk");if (IS_ERR(pll_p_clk)) {clk = pll_p_clk;goto put_pll_x_clk;}emc_clk = clk_get_sys("cpu", "emc");if (IS_ERR(emc_clk)) {clk = emc_clk;goto put_pll_p_clk;}cpu0->hw.init = &init;init.name = "cpu0";init.ops = &clk_cpu0_ops;init.flags = CLK_IS_ROOT | CLK_GET_RATE_NOCACHE;init.num_parents = 0;clk = clk_register(NULL, &cpu0->hw);if (!IS_ERR(clk))return clk;clk_prepare_enable(emc_clk);clk_prepare_enable(cpu_clk);clk_put(emc_clk);+put_pll_p_clk:
clk_put(pll_p_clk);+put_pll_x_clk:
clk_put(pll_x_clk);+put_cpu_clk:
clk_put(cpu_clk);+free_mem:
kfree(cpu0);pr_err("%s: clk register failed\n", __func__);return NULL;+} diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c index e2c6ca0..1cabeea 100644 --- a/drivers/clk/tegra/clk-tegra30.c +++ b/drivers/clk/tegra/clk-tegra30.c @@ -1396,6 +1396,10 @@ static void __init tegra30_super_clk_init(void) CLK_SET_RATE_PARENT, 1, 2); clk_register_clkdev(clk, "twd", NULL); clks[twd] = clk;
/* cpu0 clk for cpufreq driver */clk = tegra_clk_register_cpu0();clk_register_clkdev(clk, NULL, "cpu0");} static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p", diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h index 23a0cee..d416138 100644 --- a/include/linux/clk/tegra.h +++ b/include/linux/clk/tegra.h @@ -128,5 +128,6 @@ static inline void tegra_periph_reset_deassert(struct clk *c) {} static inline void tegra_periph_reset_assert(struct clk *c) {} #endif void tegra_clocks_apply_init_table(void); +struct clk *tegra_clk_register_cpu0(void);
#endif /* __LINUX_CLK_TEGRA_H_ */
1.7.12.rc2.18.g61b472e
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".
On 7 August 2013 23:08, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".
I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.
This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.
On 08/07/2013 11:45 AM, Viresh Kumar wrote:
On 7 August 2013 23:08, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".
I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.
Sure.
This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.
But the additional management logic on top of the raw clock is exactly what the cpufreq driver is for. This patch series is basically moving the cpufreq driver code inside the clock code instead.
On 7 August 2013 23:18, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 11:45 AM, Viresh Kumar wrote:
On 7 August 2013 23:08, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".
I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.
Sure.
This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.
But the additional management logic on top of the raw clock is exactly what the cpufreq driver is for. This patch series is basically moving the cpufreq driver code inside the clock code instead.
Above "sure" didn't go very well with what you wrote here :)
The additional management that we are required to do isn't cpufreq driver specific but cpu or platform specific. cpufreq shouldn't care about how CPU's clock is set to a particular frequency, its headache of CPU's clk driver instead. cpu is yet another device and so clk_set_rate() must be enough to set its frequency....
There might be other frameworks that need to set frequency of this device later on and surely we don't want to replicate such piece of code to every user..
Does it make sense to you?
On 08/07/2013 11:54 AM, Viresh Kumar wrote:
On 7 August 2013 23:18, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 11:45 AM, Viresh Kumar wrote:
On 7 August 2013 23:08, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.
Most of the platform specific bits are picked from tegra-cpufreq.c.
Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".
I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.
Sure.
This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.
But the additional management logic on top of the raw clock is exactly what the cpufreq driver is for. This patch series is basically moving the cpufreq driver code inside the clock code instead.
Above "sure" didn't go very well with what you wrote here :)
The additional management that we are required to do isn't cpufreq driver specific but cpu or platform specific.
Right, and that's *exactly* what having a cpufreq driver is for; to implement the details of CPU clock management.
On 8 August 2013 00:20, Stephen Warren swarren@wwwdotorg.org wrote:
Right, and that's *exactly* what having a cpufreq driver is for; to implement the details of CPU clock management.
cpufreq drivers used to keep such information since a long time, probably because there wasn't another place to keep them and provide generic API's (like generic clock framework).. And so this replication started to get in place which we are trying to get rid of now.
All cpufreq drivers share a lot of common code which can go away and so cpufreq-cpu0 was introduced..
With this patchset this replication goes away for tegra atleast at the cost of a platform specific clk-cpu driver.. I think that's a good deal, isn't it?
And that's the only way you can use these generic drivers that we have...
On 08/07/2013 08:42 PM, Viresh Kumar wrote:
On 8 August 2013 00:20, Stephen Warren swarren@wwwdotorg.org wrote:
Right, and that's *exactly* what having a cpufreq driver is for; to implement the details of CPU clock management.
cpufreq drivers used to keep such information since a long time, probably because there wasn't another place to keep them and provide generic API's (like generic clock framework).. And so this replication started to get in place which we are trying to get rid of now.
All cpufreq drivers share a lot of common code which can go away and so cpufreq-cpu0 was introduced..
With this patchset this replication goes away for tegra atleast at the cost of a platform specific clk-cpu driver.. I think that's a good deal, isn't it?
I think this patch series is simply moving the custom per-SoC code somewhere else (clock driver) so that the cpufreq drivers can be simpler. However, the clock drivers are more complex, and now represent concepts that aren't really clocks.
So, no I'm not sure it's good.
And that's the only way you can use these generic drivers that we have...
I don't think so. I think it's reasonable to have a per-SoC cpufreq driver whose primary content is the parameterization data and/or custom hooks for a unified core cpufreq driver. The duplicate parts of each cpufreq driver can be moved into the core cpufreq driver, but the non-duplicate parts remain. That's how many other subsystems work (MMC, USB, ASoC spring to mind).
On 9 August 2013 00:20, Stephen Warren swarren@wwwdotorg.org wrote:
I don't think so. I think it's reasonable to have a per-SoC cpufreq driver whose primary content is the parameterization data and/or custom hooks for a unified core cpufreq driver. The duplicate parts of each cpufreq driver can be moved into the core cpufreq driver, but the non-duplicate parts remain. That's how many other subsystems work (MMC, USB, ASoC spring to mind).
Guys in the --to list:
Please shout before its too late...
I can understand why Stephen is asking not to implement a virtual clock driver for cpu as there is no clock corresponding to that.. We are just playing with existing clocks there..
But I thought these clock APIs can be considered as hooks that we were looking for and so shouldn't be a problem..
But yes, different people see things differently..
So, if I take Stephen's suggestions then I need to implement hooks into cpufreq-cpu0 driver for taking freq-table/ setting clk rates, etc...
Let me know if anybody has a issue with that before we actually implement that..
-- viresh
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/boot/dts/tegra114.dtsi | 12 ++++++++++++ arch/arm/boot/dts/tegra20.dtsi | 12 ++++++++++++ arch/arm/boot/dts/tegra30.dtsi | 12 ++++++++++++ 3 files changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi index abf6c40..730e0d9 100644 --- a/arch/arm/boot/dts/tegra114.dtsi +++ b/arch/arm/boot/dts/tegra114.dtsi @@ -438,6 +438,18 @@ device_type = "cpu"; compatible = "arm,cortex-a15"; reg = <0>; + operating-points = < + /* kHz ignored */ + 216000 1000000 + 312000 1000000 + 456000 1000000 + 608000 1000000 + 760000 1000000 + 816000 1000000 + 912000 1000000 + 1000000 1000000 + >; + clock-latency = <300000>; };
cpu@1 { diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi index 9653fd8..5696f98 100644 --- a/arch/arm/boot/dts/tegra20.dtsi +++ b/arch/arm/boot/dts/tegra20.dtsi @@ -577,6 +577,18 @@ device_type = "cpu"; compatible = "arm,cortex-a9"; reg = <0>; + operating-points = < + /* kHz ignored */ + 216000 1000000 + 312000 1000000 + 456000 1000000 + 608000 1000000 + 760000 1000000 + 816000 1000000 + 912000 1000000 + 1000000 1000000 + >; + clock-latency = <300000>; };
cpu@1 { diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi index d8783f0..5930290 100644 --- a/arch/arm/boot/dts/tegra30.dtsi +++ b/arch/arm/boot/dts/tegra30.dtsi @@ -569,6 +569,18 @@ device_type = "cpu"; compatible = "arm,cortex-a9"; reg = <0>; + operating-points = < + /* kHz ignored */ + 216000 1000000 + 312000 1000000 + 456000 1000000 + 608000 1000000 + 760000 1000000 + 816000 1000000 + 912000 1000000 + 1000000 1000000 + >; + clock-latency = <300000>; };
cpu@1 {
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Similarly, we can't put invalid voltages into the DT, since if a later kernel version starts actually using that field, the HW will no longer work correctly. Unless perhaps we put 0 into the DT and make the binding define that 0 means "you can't change the voltage at all away from the boot value"?
Is the operating-points property documented in Documentation/devicetree/bindings/ somewhere?
(Also Cc'ing the DT mailing list and maintainers)
diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi index abf6c40..730e0d9 100644 --- a/arch/arm/boot/dts/tegra114.dtsi +++ b/arch/arm/boot/dts/tegra114.dtsi @@ -438,6 +438,18 @@ device_type = "cpu"; compatible = "arm,cortex-a15"; reg = <0>;
operating-points = </* kHz ignored */216000 1000000312000 1000000456000 1000000608000 1000000760000 1000000816000 1000000912000 10000001000000 1000000>;clock-latency = <300000>;
On 7 August 2013 23:12, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
:(
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt
Or you meant, Tegra never required voltage levels and we are getting them in here.
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.
Btw, you must have some specific voltage level for each freq, we can get them here..
Similarly, we can't put invalid voltages into the DT, since if a later kernel version starts actually using that field, the HW will no longer work correctly. Unless perhaps we put 0 into the DT and make the binding define that 0 means "you can't change the voltage at all away from the boot value"?
Hmm.. so if you help me in getting actual voltage levels for these freqs then this problem will be resolved. Otherwise I can check what will happen if we pass zero to voltage.
Is the operating-points property documented in Documentation/devicetree/bindings/ somewhere?
Check above.
(Also Cc'ing the DT mailing list and maintainers)
Thanks.
On 08/07/2013 12:06 PM, Viresh Kumar wrote:
On 7 August 2013 23:12, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
:(
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt
Or you meant, Tegra never required voltage levels and we are getting them in here.
The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.
Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.
Btw, you must have some specific voltage level for each freq, we can get them here..
Yes, I'm sure we do, but I have no idea what they are:-( It may even be board-specific or SoC-SKU-specific. I think we should defer this aspect for now.
On 8 August 2013 00:25, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 12:06 PM, Viresh Kumar wrote:
On 7 August 2013 23:12, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
:(
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt
Or you meant, Tegra never required voltage levels and we are getting them in here.
The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.
But that can't be true always.. Specially when we are moving things to DT...
For example, we are moving your DMA driver to DT and hence in the platform code, we are making a new DT node + removing static platform device.
Now, old DT can't work with new kernel... That is just not possible. That statement might be true for cases where we are just upgrading existing DT support (but I doubt it there as well :) )..
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.
Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.
That would be something similar to what we are discussing on other thread about new platform device...
You are asking me to go back to platform specific code instead of DT. When there exists a generic enough way of providing this information via DT, why should we put this in a driver?
Btw, you must have some specific voltage level for each freq, we can get them here..
Yes, I'm sure we do, but I have no idea what they are:-( It may even be board-specific or SoC-SKU-specific. I think we should defer this aspect for now.
Ok.
On 08/07/2013 08:57 PM, Viresh Kumar wrote:
On 8 August 2013 00:25, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 12:06 PM, Viresh Kumar wrote:
On 7 August 2013 23:12, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
:(
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt
Or you meant, Tegra never required voltage levels and we are getting them in here.
The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.
But that can't be true always.. Specially when we are moving things to DT...
It has to be, or DT isn't an ABI.
Since DT is defined as being an ABI, it has to be true.
The solution here that allows DT to be an ABI is to be the data into the drivers (or core SoC support code) rather than DT.
For example, we are moving your DMA driver to DT and hence in the platform code, we are making a new DT node + removing static platform device.
Now, old DT can't work with new kernel... That is just not possible. That statement might be true for cases where we are just upgrading existing DT support (but I doubt it there as well :) )..
Well yes, converting existing platforms to DT piece-meal was probably a mistake in retrospect. What we should have done is added parallel DT and non-DT support, and only allow features to be enabled when booting DT if they were triggered by DT nodes, and never allowed additional drivers to be registered by board files.
Your point is indeed why suddenly deciding that DT is an ABI when it wasn't being enforced before is painful.
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.
Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.
That would be something similar to what we are discussing on other thread about new platform device...
You are asking me to go back to platform specific code instead of DT. When there exists a generic enough way of providing this information via DT, why should we put this in a driver?
I think that drivers should include all data that doesn't need to vary; there's no point putting data into DT just to parse it out into the same tables that the driver could have embedded itself from the start.
Am Mittwoch, den 07.08.2013, 12:55 -0600 schrieb Stephen Warren:
On 08/07/2013 12:06 PM, Viresh Kumar wrote:
On 7 August 2013 23:12, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.
Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.
This patch is problematic w.r.t. DT being an ABI.
:(
We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.
To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt
Or you meant, Tegra never required voltage levels and we are getting them in here.
The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.
As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.
Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.
Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.
Btw, you must have some specific voltage level for each freq, we can get them here..
Yes, I'm sure we do, but I have no idea what they are:-( It may even be board-specific or SoC-SKU-specific. I think we should defer this aspect for now.
From what I learned those voltage levels are dependent on both the
Speedo and the process ID of the specific Tegra processor. So you really get a two dimensional mapping table instead of a single OPP. Also you can not scale the CPU voltage on it's own, but have to make sure the core voltage isn't too far away from. Then core voltage also depends on the operating states of engines like GR2D or even display.
Regards, Lucas
On 8 August 2013 19:28, Lucas Stach l.stach@pengutronix.de wrote:
From what I learned those voltage levels are dependent on both the Speedo and the process ID of the specific Tegra processor. So you really get a two dimensional mapping table instead of a single OPP. Also you can not scale the CPU voltage on it's own, but have to make sure the core voltage isn't too far away from. Then core voltage also depends on the operating states of engines like GR2D or even display.
So if they depend on a certain type of SoC, which they should, then we can get these initialized from that SoC's dts/dtsi file instead of a common file.. And so that would resolve the issue you just reported.
Now I haven't proposed in the patch that we will change these voltage levels at all.. This is regulator specific code and would come into play only when regulators are registered for cpu.. Otherwise we will just play with frequency..
Passing OPP instead of just list of frequencies is the generic way this is done now a days..
Am Donnerstag, den 08.08.2013, 19:41 +0530 schrieb Viresh Kumar:
On 8 August 2013 19:28, Lucas Stach l.stach@pengutronix.de wrote:
From what I learned those voltage levels are dependent on both the Speedo and the process ID of the specific Tegra processor. So you really get a two dimensional mapping table instead of a single OPP. Also you can not scale the CPU voltage on it's own, but have to make sure the core voltage isn't too far away from. Then core voltage also depends on the operating states of engines like GR2D or even display.
So if they depend on a certain type of SoC, which they should, then we can get these initialized from that SoC's dts/dtsi file instead of a common file.. And so that would resolve the issue you just reported.
You can certainly define the mapping table in DT where a specialized Tegra cpufreq driver could read it in and then map frequency to voltage. But that's a runtime decision, as Speedo and process ID are fuse values and can not be represented in DT.
Now I haven't proposed in the patch that we will change these voltage levels at all.. This is regulator specific code and would come into play only when regulators are registered for cpu.. Otherwise we will just play with frequency..
Passing OPP instead of just list of frequencies is the generic way this is done now a days..
The problem with this is that the hardware description now associates voltages with certain frequencies and even if they are not used by the Linux driver they are plain wrong.
Regards, Lucas
On 8 August 2013 19:52, Lucas Stach l.stach@pengutronix.de wrote:
You can certainly define the mapping table in DT where a specialized Tegra cpufreq driver could read it in and then map frequency to voltage. But that's a runtime decision, as Speedo and process ID are fuse values and can not be represented in DT.
The problem with this is that the hardware description now associates voltages with certain frequencies and even if they are not used by the Linux driver they are plain wrong.
Hmm. I understand. Then we probably need mach-tegra/opp.c to call opp_add() for all such OPPs.. Neither DT nor cpufreq driver are the right place for this.
On Thu, Aug 8, 2013 at 9:37 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 8 August 2013 19:52, Lucas Stach l.stach@pengutronix.de wrote:
You can certainly define the mapping table in DT where a specialized Tegra cpufreq driver could read it in and then map frequency to voltage. But that's a runtime decision, as Speedo and process ID are fuse values and can not be represented in DT.
The problem with this is that the hardware description now associates voltages with certain frequencies and even if they are not used by the Linux driver they are plain wrong.
Hmm. I understand. Then we probably need mach-tegra/opp.c to call opp_add() for all such OPPs.. Neither DT nor cpufreq driver are the right place for this.
This is similar to what I suspected might be the case on other platforms (in addition to known iMx and OMAP). Could you see/comment on [1] to see if it meets your needs.
We should like to avoid dealing custom SoC specific OPP, if we are able to generalize the need. ofcourse, I am yet to submit a official proposal, but more SoCs the current proposal can handle, the better it will be for all of us.
[1] http://marc.info/?l=linux-pm&m=137589225305971&w=2 -- Regards, Nishanth Menon
cpufreq-cpu0 driver is dependent on OPP library and hence we need to enable it for Tegra as we are going to use cpufreq-cpu0.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-tegra/Kconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig index ef3a8da..63875c5 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig @@ -1,6 +1,8 @@ config ARCH_TEGRA bool "NVIDIA Tegra" if ARCH_MULTI_V7 select ARCH_HAS_CPUFREQ + select ARCH_HAS_OPP + select PM_OPP if PM select ARCH_REQUIRE_GPIOLIB select CLKDEV_LOOKUP select CLKSRC_MMIO
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver is dependent on OPP library and hence we need to enable it for Tegra as we are going to use cpufreq-cpu0.
Shouldn't these be selected by something in drivers/cpufreq/Kconfig?
On 7 August 2013 23:13, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver is dependent on OPP library and hence we need to enable it for Tegra as we are going to use cpufreq-cpu0.
Shouldn't these be selected by something in drivers/cpufreq/Kconfig?
There is nothing tegra specific in that directory now and so probably this is the best place :)
Creating a config option only for this doesn't sound great to me..
Tegra requires cpufreq-cpu0 driver to be compiled in and hence we select it from the defconfig.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/configs/tegra_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig index 1effb43..3fcec8f 100644 --- a/arch/arm/configs/tegra_defconfig +++ b/arch/arm/configs/tegra_defconfig @@ -38,6 +38,7 @@ CONFIG_ZBOOT_ROM_BSS=0x0 CONFIG_KEXEC=y CONFIG_CPU_FREQ=y CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y +CONFIG_GENERIC_CPUFREQ_CPU0=y CONFIG_CPU_IDLE=y CONFIG_VFP=y CONFIG_PM_RUNTIME=y
cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.
Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-tegra/tegra.c | 2 ++ drivers/cpufreq/Kconfig.arm | 8 -------- 2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c index 0d1e412..6ab3f69 100644 --- a/arch/arm/mach-tegra/tegra.c +++ b/arch/arm/mach-tegra/tegra.c @@ -82,11 +82,13 @@ static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
static void __init tegra_dt_init(void) { + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; struct device *parent = NULL;
tegra_clocks_apply_init_table(); + platform_device_register_full(&devinfo);
soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); if (!soc_dev_attr) diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index de4d5d9..9472160 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -215,11 +215,3 @@ config ARM_SPEAR_CPUFREQ default y help This adds the CPUFreq driver support for SPEAr SOCs. - -config ARM_TEGRA_CPUFREQ - bool "TEGRA CPUFreq support" - depends on ARCH_TEGRA - select CPU_FREQ_TABLE - default y - help - This adds the CPUFreq driver support for TEGRA SOCs.
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.
Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
static void __init tegra_dt_init(void) {
- struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
static? const?
struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; struct device *parent = NULL; tegra_clocks_apply_init_table();
- platform_device_register_full(&devinfo);
This seems awfully like going back to board files. Shouldn't something that binds to the CPU nodes register the cpufreq device automatically, based on the CPU's compatible value?
On 7 August 2013 23:16, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.
Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
static void __init tegra_dt_init(void) {
struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };static? const?
static: yes const: no, as it might be modified by platform_device_register_full()
struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; struct device *parent = NULL; tegra_clocks_apply_init_table();
platform_device_register_full(&devinfo);This seems awfully like going back to board files. Shouldn't something that binds to the CPU nodes register the cpufreq device automatically, based on the CPU's compatible value?
This link has got some information why we can't have a node for cpufreq or a compatibility value..
On 08/07/2013 11:49 AM, Viresh Kumar wrote:
On 7 August 2013 23:16, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.
Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
static void __init tegra_dt_init(void) {
struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; struct device *parent = NULL; tegra_clocks_apply_init_table();platform_device_register_full(&devinfo);This seems awfully like going back to board files. Shouldn't something that binds to the CPU nodes register the cpufreq device automatically, based on the CPU's compatible value?
This link has got some information why we can't have a node for cpufreq or a compatibility value..
That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:
compatible = "arm,cortex-a9";
... should instantiate any virtual devices that relate to the CPU.
Doing so would be similar to how the Tegra I2S driver instantiates the internal struct device that ASoC needs for the PCM/DMA device, rather than having board-dt-tegra20.c do it, like it would have done in board-file days.
On 7 August 2013 23:23, Stephen Warren swarren@wwwdotorg.org wrote:
That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:
compatible = "arm,cortex-a9";... should instantiate any virtual devices that relate to the CPU.
But how would we know here if platform really wants us to probe cpufreq-cpu0 driver? On multiplatform kernel there can be multiple cpufreq drivers available and there has to be some sort of code in DT or platform code that reflects which driver we want to use.
We never required a device node for cpufreq, platform device was added just to solve this issue.
Doing so would be similar to how the Tegra I2S driver instantiates the internal struct device that ASoC needs for the PCM/DMA device, rather than having board-dt-tegra20.c do it, like it would have done in board-file days.
I haven't gone through it yet though :)
On 08/07/2013 11:59 AM, Viresh Kumar wrote:
On 7 August 2013 23:23, Stephen Warren swarren@wwwdotorg.org wrote:
That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:
compatible = "arm,cortex-a9";... should instantiate any virtual devices that relate to the CPU.
But how would we know here if platform really wants us to probe cpufreq-cpu0 driver? On multiplatform kernel there can be multiple cpufreq drivers available and there has to be some sort of code in DT or platform code that reflects which driver we want to use.
Presumably the code would look at the top-level DT node's compatible value (e.g. "nvidia,tegra20").
On 8 August 2013 00:21, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/07/2013 11:59 AM, Viresh Kumar wrote:
On 7 August 2013 23:23, Stephen Warren swarren@wwwdotorg.org wrote:
That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:
compatible = "arm,cortex-a9";... should instantiate any virtual devices that relate to the CPU.
But how would we know here if platform really wants us to probe cpufreq-cpu0 driver? On multiplatform kernel there can be multiple cpufreq drivers available and there has to be some sort of code in DT or platform code that reflects which driver we want to use.
Presumably the code would look at the top-level DT node's compatible value (e.g. "nvidia,tegra20").
So you are actually asking us to get a compatibility list inside cpufreq-cpu0 driver which will list all the platforms for which this driver would work?
Honestly speaking I wasn't in favor of getting a platform-device registered for cpufreq-cpu0 earlier and had few discussion on the thread I passed to you.
The problem with the new solution you just proposed is, for every new platform that comes in we need to update this file.. And that's it probably..
Don't know how others would see it... @Rafael/Rob/Shawn: Any suggestions here?
We are using generic cpufreq-cpu0 driver, so lets get rid of platform specific tegra-cpufreq.c driver.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/Makefile | 1 - drivers/cpufreq/tegra-cpufreq.c | 291 ---------------------------------------- 2 files changed, 292 deletions(-) delete mode 100644 drivers/cpufreq/tegra-cpufreq.c
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index ad5866c..e74b3ee 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -76,7 +76,6 @@ obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o -obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
################################################################################## # PowerPC platform drivers diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c deleted file mode 100644 index cd66b85..0000000 --- a/drivers/cpufreq/tegra-cpufreq.c +++ /dev/null @@ -1,291 +0,0 @@ -/* - * Copyright (C) 2010 Google, Inc. - * - * Author: - * Colin Cross ccross@google.com - * Based on arch/arm/plat-omap/cpu-omap.c, (C) 2005 Nokia Corporation - * - * This software is licensed under the terms of the GNU General Public - * License version 2, as published by the Free Software Foundation, and - * may be copied, distributed, and modified under those terms. - * - * 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/kernel.h> -#include <linux/module.h> -#include <linux/types.h> -#include <linux/sched.h> -#include <linux/cpufreq.h> -#include <linux/delay.h> -#include <linux/init.h> -#include <linux/err.h> -#include <linux/clk.h> -#include <linux/io.h> -#include <linux/suspend.h> - -static struct cpufreq_frequency_table freq_table[] = { - { .frequency = 216000 }, - { .frequency = 312000 }, - { .frequency = 456000 }, - { .frequency = 608000 }, - { .frequency = 760000 }, - { .frequency = 816000 }, - { .frequency = 912000 }, - { .frequency = 1000000 }, - { .frequency = CPUFREQ_TABLE_END }, -}; - -#define NUM_CPUS 2 - -static struct clk *cpu_clk; -static struct clk *pll_x_clk; -static struct clk *pll_p_clk; -static struct clk *emc_clk; - -static unsigned long target_cpu_speed[NUM_CPUS]; -static DEFINE_MUTEX(tegra_cpu_lock); -static bool is_suspended; - -static int tegra_verify_speed(struct cpufreq_policy *policy) -{ - return cpufreq_frequency_table_verify(policy, freq_table); -} - -static unsigned int tegra_getspeed(unsigned int cpu) -{ - unsigned long rate; - - if (cpu >= NUM_CPUS) - return 0; - - rate = clk_get_rate(cpu_clk) / 1000; - return rate; -} - -static int tegra_cpu_clk_set_rate(unsigned long rate) -{ - int ret; - - /* - * Take an extra reference to the main pll so it doesn't turn - * off when we move the cpu off of it - */ - clk_prepare_enable(pll_x_clk); - - ret = clk_set_parent(cpu_clk, pll_p_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_p\n"); - goto out; - } - - if (rate == clk_get_rate(pll_p_clk)) - goto out; - - ret = clk_set_rate(pll_x_clk, rate); - if (ret) { - pr_err("Failed to change pll_x to %lu\n", rate); - goto out; - } - - ret = clk_set_parent(cpu_clk, pll_x_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_x\n"); - goto out; - } - -out: - clk_disable_unprepare(pll_x_clk); - return ret; -} - -static int tegra_update_cpu_speed(struct cpufreq_policy *policy, - unsigned long rate) -{ - int ret = 0; - struct cpufreq_freqs freqs; - - freqs.old = tegra_getspeed(0); - freqs.new = rate; - - if (freqs.old == freqs.new) - return ret; - - /* - * Vote on memory bus frequency based on cpu frequency - * This sets the minimum frequency, display or avp may request higher - */ - if (rate >= 816000) - clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */ - else if (rate >= 456000) - clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */ - else - clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ - - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); - -#ifdef CONFIG_CPU_FREQ_DEBUG - printk(KERN_DEBUG "cpufreq-tegra: transition: %u --> %u\n", - freqs.old, freqs.new); -#endif - - ret = tegra_cpu_clk_set_rate(freqs.new * 1000); - if (ret) { - pr_err("cpu-tegra: Failed to set cpu frequency to %d kHz\n", - freqs.new); - freqs.new = freqs.old; - } - - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); - - return ret; -} - -static unsigned long tegra_cpu_highest_speed(void) -{ - unsigned long rate = 0; - int i; - - for_each_online_cpu(i) - rate = max(rate, target_cpu_speed[i]); - return rate; -} - -static int tegra_target(struct cpufreq_policy *policy, - unsigned int target_freq, - unsigned int relation) -{ - unsigned int idx; - unsigned int freq; - int ret = 0; - - mutex_lock(&tegra_cpu_lock); - - if (is_suspended) { - ret = -EBUSY; - goto out; - } - - cpufreq_frequency_table_target(policy, freq_table, target_freq, - relation, &idx); - - freq = freq_table[idx].frequency; - - target_cpu_speed[policy->cpu] = freq; - - ret = tegra_update_cpu_speed(policy, tegra_cpu_highest_speed()); - -out: - mutex_unlock(&tegra_cpu_lock); - return ret; -} - -static int tegra_pm_notify(struct notifier_block *nb, unsigned long event, - void *dummy) -{ - mutex_lock(&tegra_cpu_lock); - if (event == PM_SUSPEND_PREPARE) { - struct cpufreq_policy *policy = cpufreq_cpu_get(0); - is_suspended = true; - pr_info("Tegra cpufreq suspend: setting frequency to %d kHz\n", - freq_table[0].frequency); - tegra_update_cpu_speed(policy, freq_table[0].frequency); - cpufreq_cpu_put(policy); - } else if (event == PM_POST_SUSPEND) { - is_suspended = false; - } - mutex_unlock(&tegra_cpu_lock); - - return NOTIFY_OK; -} - -static struct notifier_block tegra_cpu_pm_notifier = { - .notifier_call = tegra_pm_notify, -}; - -static int tegra_cpu_init(struct cpufreq_policy *policy) -{ - if (policy->cpu >= NUM_CPUS) - return -EINVAL; - - clk_prepare_enable(emc_clk); - clk_prepare_enable(cpu_clk); - - cpufreq_frequency_table_cpuinfo(policy, freq_table); - cpufreq_frequency_table_get_attr(freq_table, policy->cpu); - policy->cur = tegra_getspeed(policy->cpu); - target_cpu_speed[policy->cpu] = policy->cur; - - /* FIXME: what's the actual transition time? */ - policy->cpuinfo.transition_latency = 300 * 1000; - - cpumask_copy(policy->cpus, cpu_possible_mask); - - if (policy->cpu == 0) - register_pm_notifier(&tegra_cpu_pm_notifier); - - return 0; -} - -static int tegra_cpu_exit(struct cpufreq_policy *policy) -{ - cpufreq_frequency_table_cpuinfo(policy, freq_table); - clk_disable_unprepare(emc_clk); - return 0; -} - -static struct freq_attr *tegra_cpufreq_attr[] = { - &cpufreq_freq_attr_scaling_available_freqs, - NULL, -}; - -static struct cpufreq_driver tegra_cpufreq_driver = { - .verify = tegra_verify_speed, - .target = tegra_target, - .get = tegra_getspeed, - .init = tegra_cpu_init, - .exit = tegra_cpu_exit, - .name = "tegra", - .attr = tegra_cpufreq_attr, -}; - -static int __init tegra_cpufreq_init(void) -{ - cpu_clk = clk_get_sys(NULL, "cpu"); - if (IS_ERR(cpu_clk)) - return PTR_ERR(cpu_clk); - - pll_x_clk = clk_get_sys(NULL, "pll_x"); - if (IS_ERR(pll_x_clk)) - return PTR_ERR(pll_x_clk); - - pll_p_clk = clk_get_sys(NULL, "pll_p_cclk"); - if (IS_ERR(pll_p_clk)) - return PTR_ERR(pll_p_clk); - - emc_clk = clk_get_sys("cpu", "emc"); - if (IS_ERR(emc_clk)) { - clk_put(cpu_clk); - return PTR_ERR(emc_clk); - } - - return cpufreq_register_driver(&tegra_cpufreq_driver); -} - -static void __exit tegra_cpufreq_exit(void) -{ - cpufreq_unregister_driver(&tegra_cpufreq_driver); - clk_put(emc_clk); - clk_put(cpu_clk); -} - - -MODULE_AUTHOR("Colin Cross ccross@android.com"); -MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2"); -MODULE_LICENSE("GPL"); -module_init(tegra_cpufreq_init); -module_exit(tegra_cpufreq_exit);
On Wed, Aug 7, 2013 at 10:46 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Stephen,
This is the first attempt to get rid of tegra-cpufreq driver. This patchset tries to add supporting infrastructure for tegra to use cpufreq-cpu0 driver.
If tegra has only 4-core fast cpu, I would agree with the patch set. But now I'm not so sure. Tegra cpuquiet driver and cluster switch may need special settings of cpufreq driver.
Thanks Richard
On 8 August 2013 14:01, Richard Zhao linuxzsc@gmail.com wrote:
On Wed, Aug 7, 2013 at 10:46 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Stephen,
This is the first attempt to get rid of tegra-cpufreq driver. This patchset tries to add supporting infrastructure for tegra to use cpufreq-cpu0 driver.
If tegra has only 4-core fast cpu, I would agree with the patch set. But now I'm not so sure. Tegra cpuquiet driver and cluster switch may need special settings of cpufreq driver.
I am not familiar with the latest happenings.. This change should be good enough for not breaking anything that is working with current tegra cpufreq driver.. If there is a new SoC with different needs then we can talk about it separately.. As that might not be able to use tegra-cpufreq driver anyway..
linaro-kernel@lists.linaro.org
-
Lucas Stach -
Mike Turquette -
Nishanth Menon -
Richard Zhao -
Stephen Warren -
Viresh Kumar