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
--
1.7.12.rc2.18.g61b472e
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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_ */
--
1.7.12.rc2.18.g61b472e
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 <[email protected]>
---
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 {
--
1.7.12.rc2.18.g61b472e
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 <[email protected]>
---
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
--
1.7.12.rc2.18.g61b472e
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 <[email protected]>
---
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.
--
1.7.12.rc2.18.g61b472e
Tegra requires cpufreq-cpu0 driver to be compiled in and hence we select it from
the defconfig.
Signed-off-by: Viresh Kumar <[email protected]>
---
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
--
1.7.12.rc2.18.g61b472e
We are using generic cpufreq-cpu0 driver, so lets get rid of platform specific
tegra-cpufreq.c driver.
Signed-off-by: Viresh Kumar <[email protected]>
---
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 <[email protected]>
- * 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 <[email protected]>");
-MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2");
-MODULE_LICENSE("GPL");
-module_init(tegra_cpufreq_init);
-module_exit(tegra_cpufreq_exit);
--
1.7.12.rc2.18.g61b472e
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 <[email protected]>
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 <[email protected]>
> + *
> + * 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_ */
> --
> 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 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 1000000
> + 312000 1000000
> + 456000 1000000
> + 608000 1000000
> + 760000 1000000
> + 816000 1000000
> + 912000 1000000
> + 1000000 1000000
> + >;
> + clock-latency = <300000>;
> };
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:08, Stephen Warren <[email protected]> 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 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 08/07/2013 11:45 AM, Viresh Kumar wrote:
> On 7 August 2013 23:08, Stephen Warren <[email protected]> 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:16, Stephen Warren <[email protected]> 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..
http://permalink.gmane.org/gmane.linux.kernel.cpufreq/9018
On 08/07/2013 11:49 AM, Viresh Kumar wrote:
> On 7 August 2013 23:16, Stephen Warren <[email protected]> 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..
>
> http://permalink.gmane.org/gmane.linux.kernel.cpufreq/9018
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:18, Stephen Warren <[email protected]> wrote:
> On 08/07/2013 11:45 AM, Viresh Kumar wrote:
>> On 7 August 2013 23:08, Stephen Warren <[email protected]> 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 7 August 2013 23:23, Stephen Warren <[email protected]> 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 7 August 2013 23:12, Stephen Warren <[email protected]> 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 7 August 2013 23:13, Stephen Warren <[email protected]> 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..
On 08/07/2013 11:54 AM, Viresh Kumar wrote:
> On 7 August 2013 23:18, Stephen Warren <[email protected]> wrote:
>> On 08/07/2013 11:45 AM, Viresh Kumar wrote:
>>> On 7 August 2013 23:08, Stephen Warren <[email protected]> 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 08/07/2013 11:59 AM, Viresh Kumar wrote:
> On 7 August 2013 23:23, Stephen Warren <[email protected]> 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 08/07/2013 12:06 PM, Viresh Kumar wrote:
> On 7 August 2013 23:12, Stephen Warren <[email protected]> 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:20, Stephen Warren <[email protected]> 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 8 August 2013 00:21, Stephen Warren <[email protected]> wrote:
> On 08/07/2013 11:59 AM, Viresh Kumar wrote:
>> On 7 August 2013 23:23, Stephen Warren <[email protected]> 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?
On 8 August 2013 00:25, Stephen Warren <[email protected]> wrote:
> On 08/07/2013 12:06 PM, Viresh Kumar wrote:
>> On 7 August 2013 23:12, Stephen Warren <[email protected]> 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 Wed, Aug 7, 2013 at 10:46 PM, Viresh Kumar <[email protected]> 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 <[email protected]> wrote:
> On Wed, Aug 7, 2013 at 10:46 PM, Viresh Kumar <[email protected]> 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..
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 <[email protected]> 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
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 8 August 2013 19:28, Lucas Stach <[email protected]> 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 <[email protected]> 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
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 8 August 2013 19:52, Lucas Stach <[email protected]> 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 <[email protected]> wrote:
> On 8 August 2013 19:52, Lucas Stach <[email protected]> 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
On 08/07/2013 08:42 PM, Viresh Kumar wrote:
> On 8 August 2013 00:20, Stephen Warren <[email protected]> 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 08/07/2013 08:57 PM, Viresh Kumar wrote:
> On 8 August 2013 00:25, Stephen Warren <[email protected]> wrote:
>> On 08/07/2013 12:06 PM, Viresh Kumar wrote:
>>> On 7 August 2013 23:12, Stephen Warren <[email protected]> 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.
On 9 August 2013 00:20, Stephen Warren <[email protected]> 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