[PATCH v3 0/2] Remodel Tegra cpufreq drivers to support Tegra series SoC

[PATCH v3 0/2] Remodel Tegra cpufreq drivers to support Tegra series SoC

[Bill Huang]

This patch series remodel Tegra cpufreq driver to make it more easy to
add new SoC support, in addition to that, adding probe function in the
driver to let probe defer can be used to control init sequence when we
are going to support DVFS.

Changes since v2:

- Fix Kconfig.
- Rebase on top of branch 'cpufreq-next' on git://git.linaro.org/people/vireshk/linux.git.

Changes since v1:

- Split up patches.
- Split configuration-time data out of structure "tegra_cpufreq_data".
- Bug fixes.

Bill Huang (2):
  cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine
    code
  cpufreq: tegra: Re-model Tegra cpufreq driver

 arch/arm/mach-tegra/tegra.c       |    2 +
 drivers/cpufreq/Kconfig.arm       |   12 +++
 drivers/cpufreq/Makefile          |    1 +
 drivers/cpufreq/tegra-cpufreq.c   |  188 +++++++++++++++++--------------------
 drivers/cpufreq/tegra-cpufreq.h   |   40 ++++++++
 drivers/cpufreq/tegra20-cpufreq.c |  136 +++++++++++++++++++++++++++
 include/linux/tegra-soc.h         |   11 ++-
 7 files changed, 287 insertions(+), 103 deletions(-)
 create mode 100644 drivers/cpufreq/tegra-cpufreq.h
 create mode 100644 drivers/cpufreq/tegra20-cpufreq.c

-- 
1.7.9.5

[PATCH v3 1/2] cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine code

[Bill Huang]

Move the call from module_init to Tegra machine codes so it won't be
called in a multi-platform kernel running on non-Tegra SoCs.

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
---
 arch/arm/mach-tegra/tegra.c     |    2 ++
 drivers/cpufreq/tegra-cpufreq.c |   13 ++-----------
 include/linux/tegra-soc.h       |   11 ++++++++++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 7336817..14490ad 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -34,6 +34,7 @@
 #include <linux/usb/tegra_usb_phy.h>
 #include <linux/clk/tegra.h>
 #include <linux/irqchip.h>
+#include <linux/tegra-soc.h>
 
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach-types.h>
@@ -160,6 +161,7 @@ static void __init tegra_dt_init_late(void)
 {
 	int i;
 
+	tegra_cpufreq_init();
 	tegra_init_suspend();
 	tegra_cpuidle_init();
 	tegra_powergate_debugfs_init();
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 63f0059..ae1c0f1 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -155,7 +155,7 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
 #endif
 };
 
-static int __init tegra_cpufreq_init(void)
+int __init tegra_cpufreq_init(void)
 {
 	cpu_clk = clk_get_sys(NULL, "cclk");
 	if (IS_ERR(cpu_clk))
@@ -177,17 +177,8 @@ static int __init tegra_cpufreq_init(void)
 
 	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);
-}
-
+EXPORT_SYMBOL(tegra_cpufreq_init);
 
 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);
diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h
index 95f611d..a179aa5 100644
--- a/include/linux/tegra-soc.h
+++ b/include/linux/tegra-soc.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2012,2013, NVIDIA CORPORATION.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -17,6 +17,15 @@
 #ifndef __LINUX_TEGRA_SOC_H_
 #define __LINUX_TEGRA_SOC_H_
 
+#ifdef CONFIG_ARM_TEGRA_CPUFREQ
+int tegra_cpufreq_init(void);
+#else
+static inline int tegra_cpufreq_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 u32 tegra_read_chipid(void);
 
 #endif /* __LINUX_TEGRA_SOC_H_ */
-- 
1.7.9.5

Re: [PATCH v3 1/2] cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine code

[Stephen Warren]

On 12/05/2013 12:44 AM, Bill Huang wrote:
> Move the call from module_init to Tegra machine codes so it won't be
> called in a multi-platform kernel running on non-Tegra SoCs.

> diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h

It might be better to create <linux/tegra-cpufreq.h> for the interface
to the cpufreq driver; tegra-soc.h is for the interface to core Tegra
code *from* other drivers.

> +#ifdef CONFIG_ARM_TEGRA_CPUFREQ
> +int tegra_cpufreq_init(void);
> +#else
> +static inline int tegra_cpufreq_init(void)
> +{
> +	return -EINVAL;
> +}
> +#endif

Probably best to "return 0" from the !CONFIG_ARM_TEGRA_CPUFREQ case; the
whole point is to isolate callers from having to care whether
CONFIG_ARM_TEGRA_CPUFREQ is enabled, and making the function act like it
worked OK is part of that isolation.

Re: [PATCH v3 1/2] cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine code

[bilhuang]

On 12/06/2013 06:54 AM, Stephen Warren wrote:
> On 12/05/2013 12:44 AM, Bill Huang wrote:
>> Move the call from module_init to Tegra machine codes so it won't be
>> called in a multi-platform kernel running on non-Tegra SoCs.
>
>> diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h
>
> It might be better to create <linux/tegra-cpufreq.h> for the interface
> to the cpufreq driver; tegra-soc.h is for the interface to core Tegra
> code *from* other drivers.
Thanks, will do.
>
>> +#ifdef CONFIG_ARM_TEGRA_CPUFREQ
>> +int tegra_cpufreq_init(void);
>> +#else
>> +static inline int tegra_cpufreq_init(void)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>
> Probably best to "return 0" from the !CONFIG_ARM_TEGRA_CPUFREQ case; the
> whole point is to isolate callers from having to care whether
> CONFIG_ARM_TEGRA_CPUFREQ is enabled, and making the function act like it
> worked OK is part of that isolation.
>
OK thanks.

Re: [PATCH v3 1/2] cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine code

[Viresh Kumar]

On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> wrote:
> Move the call from module_init to Tegra machine codes so it won't be
> called in a multi-platform kernel running on non-Tegra SoCs.
>
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> ---
>  arch/arm/mach-tegra/tegra.c     |    2 ++
>  drivers/cpufreq/tegra-cpufreq.c |   13 ++-----------
>  include/linux/tegra-soc.h       |   11 ++++++++++-
>  3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
> index 7336817..14490ad 100644
> --- a/arch/arm/mach-tegra/tegra.c
> +++ b/arch/arm/mach-tegra/tegra.c
> @@ -34,6 +34,7 @@
>  #include <linux/usb/tegra_usb_phy.h>
>  #include <linux/clk/tegra.h>
>  #include <linux/irqchip.h>
> +#include <linux/tegra-soc.h>
>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/mach-types.h>
> @@ -160,6 +161,7 @@ static void __init tegra_dt_init_late(void)
>  {
>         int i;
>
> +       tegra_cpufreq_init();
>         tegra_init_suspend();
>         tegra_cpuidle_init();
>         tegra_powergate_debugfs_init();
> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
> index 63f0059..ae1c0f1 100644
> --- a/drivers/cpufreq/tegra-cpufreq.c
> +++ b/drivers/cpufreq/tegra-cpufreq.c
> @@ -155,7 +155,7 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>  #endif
>  };

Remove module.h as well ??

> -static int __init tegra_cpufreq_init(void)
> +int __init tegra_cpufreq_init(void)
>  {
>         cpu_clk = clk_get_sys(NULL, "cclk");
>         if (IS_ERR(cpu_clk))
> @@ -177,17 +177,8 @@ static int __init tegra_cpufreq_init(void)
>
>         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);
> -}
> -
> +EXPORT_SYMBOL(tegra_cpufreq_init);
>
>  MODULE_AUTHOR("Colin Cross <ccross@android.com>");
>  MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2");
>  MODULE_LICENSE("GPL");

Remove these as well? As they don't have a meaning for Tegra
cpufreq driver which can't be compiled as module.

> -module_init(tegra_cpufreq_init);
> -module_exit(tegra_cpufreq_exit);

Rest as what Stephen suggested.

Re: [PATCH v3 1/2] cpufreq: tegra: Call tegra_cpufreq_init() specifically in machine code

[bilhuang]

On 12/17/2013 02:31 PM, Viresh Kumar wrote:
> On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> wrote:
>> Move the call from module_init to Tegra machine codes so it won't be
>> called in a multi-platform kernel running on non-Tegra SoCs.
>>
>> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
>> ---
>>   arch/arm/mach-tegra/tegra.c     |    2 ++
>>   drivers/cpufreq/tegra-cpufreq.c |   13 ++-----------
>>   include/linux/tegra-soc.h       |   11 ++++++++++-
>>   3 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
>> index 7336817..14490ad 100644
>> --- a/arch/arm/mach-tegra/tegra.c
>> +++ b/arch/arm/mach-tegra/tegra.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/usb/tegra_usb_phy.h>
>>   #include <linux/clk/tegra.h>
>>   #include <linux/irqchip.h>
>> +#include <linux/tegra-soc.h>
>>
>>   #include <asm/hardware/cache-l2x0.h>
>>   #include <asm/mach-types.h>
>> @@ -160,6 +161,7 @@ static void __init tegra_dt_init_late(void)
>>   {
>>          int i;
>>
>> +       tegra_cpufreq_init();
>>          tegra_init_suspend();
>>          tegra_cpuidle_init();
>>          tegra_powergate_debugfs_init();
>> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
>> index 63f0059..ae1c0f1 100644
>> --- a/drivers/cpufreq/tegra-cpufreq.c
>> +++ b/drivers/cpufreq/tegra-cpufreq.c
>> @@ -155,7 +155,7 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>>   #endif
>>   };
>
> Remove module.h as well ??
>
>> -static int __init tegra_cpufreq_init(void)
>> +int __init tegra_cpufreq_init(void)
>>   {
>>          cpu_clk = clk_get_sys(NULL, "cclk");
>>          if (IS_ERR(cpu_clk))
>> @@ -177,17 +177,8 @@ static int __init tegra_cpufreq_init(void)
>>
>>          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);
>> -}
>> -
>> +EXPORT_SYMBOL(tegra_cpufreq_init);
>>
>>   MODULE_AUTHOR("Colin Cross <ccross@android.com>");
>>   MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2");
>>   MODULE_LICENSE("GPL");
>
> Remove these as well? As they don't have a meaning for Tegra
> cpufreq driver which can't be compiled as module.
OK thanks.
>
>> -module_init(tegra_cpufreq_init);
>> -module_exit(tegra_cpufreq_exit);
>
> Rest as what Stephen suggested.
>

[PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Bill Huang]

Re-model Tegra cpufreq driver to support all Tegra series of SoCs.

* Make tegra-cpufreq.c a generic Tegra cpufreq driver.
* Move Tegra20 specific codes into tegra20-cpufreq.c.
* Bind Tegra cpufreq dirver with a fake device so defer probe would work
  when we're going to get regulator in the driver to support voltage
  scaling (DVFS).

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
---
 drivers/cpufreq/Kconfig.arm       |   12 +++
 drivers/cpufreq/Makefile          |    1 +
 drivers/cpufreq/tegra-cpufreq.c   |  185 ++++++++++++++++++-------------------
 drivers/cpufreq/tegra-cpufreq.h   |   40 ++++++++
 drivers/cpufreq/tegra20-cpufreq.c |  136 +++++++++++++++++++++++++++
 5 files changed, 278 insertions(+), 96 deletions(-)
 create mode 100644 drivers/cpufreq/tegra-cpufreq.h
 create mode 100644 drivers/cpufreq/tegra20-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index ce52ed9..1cc9213 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -225,6 +225,18 @@ config ARM_TEGRA_CPUFREQ
 	help
 	  This adds the CPUFreq driver support for TEGRA SOCs.
 
+config ARM_TEGRA20_CPUFREQ
+	bool "NVIDIA TEGRA20"
+	depends on ARM_TEGRA_CPUFREQ && ARCH_TEGRA_2x_SOC
+	default y
+	help
+	  This enables Tegra20 cpufreq functionality, it adds
+	  Tegra20 CPU frequency ladder and the call back functions
+	  to set CPU rate. All the non-SoC dependant codes are
+	  controlled by the config ARM_TEGRA20_CPUFREQ.
+
+	  If in doubt, say N.
+
 config ARM_VEXPRESS_SPC_CPUFREQ
         tristate "Versatile Express SPC based CPUfreq driver"
         select ARM_BIG_LITTLE_CPUFREQ
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 7494565..331964b 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -74,6 +74,7 @@ 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
+obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 
 ##################################################################################
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index ae1c0f1..5f19c18 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2010 Google, Inc.
+ * Copyright (c) 2013, NVIDIA Corporation.
  *
  * Author:
  *	Colin Cross <ccross@google.com>
@@ -18,69 +19,17 @@
 
 #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>
-
-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
+#include <linux/cpu.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
 
-static struct clk *cpu_clk;
-static struct clk *pll_x_clk;
-static struct clk *pll_p_clk;
-static struct clk *emc_clk;
-
-static int tegra_cpu_clk_set_rate(unsigned long rate)
-{
-	int ret;
+#include "tegra-cpufreq.h"
 
-	/*
-	 * 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 struct tegra_cpufreq_data *tegra_data;
+static const struct tegra_cpufreq_config *soc_config;
 
 static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 		unsigned long rate)
@@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 	 * 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 */
-
-	ret = tegra_cpu_clk_set_rate(rate * 1000);
+	if (soc_config->vote_emc_on_cpu_rate)
+		soc_config->vote_emc_on_cpu_rate(rate);
+
+	ret = soc_config->cpu_clk_set_rate(rate * 1000);
 	if (ret)
 		pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
 			rate);
@@ -108,6 +53,8 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
+	struct cpufreq_frequency_table *freq_table = tegra_data->freq_table;
+
 	return tegra_update_cpu_speed(policy, freq_table[index].frequency);
 }
 
@@ -115,29 +62,27 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
 	int ret;
 
-	if (policy->cpu >= NUM_CPUS)
-		return -EINVAL;
-
-	clk_prepare_enable(emc_clk);
-	clk_prepare_enable(cpu_clk);
+	if (soc_config->cpufreq_clk_init)
+		soc_config->cpufreq_clk_init();
 
 	/* FIXME: what's the actual transition time? */
-	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
+	ret = cpufreq_generic_init(policy, tegra_data->freq_table, 300 * 1000);
 	if (ret) {
-		clk_disable_unprepare(cpu_clk);
-		clk_disable_unprepare(emc_clk);
+		if (soc_config->cpufreq_clk_exit)
+			soc_config->cpufreq_clk_exit();
 		return ret;
 	}
 
-	policy->clk = cpu_clk;
-	policy->suspend_freq = freq_table[0].frequency;
+	policy->clk = tegra_data->cpu_clk;
+	policy->suspend_freq = tegra_data->freq_table[0].frequency;
 	return 0;
 }
 
 static int tegra_cpu_exit(struct cpufreq_policy *policy)
 {
-	clk_disable_unprepare(cpu_clk);
-	clk_disable_unprepare(emc_clk);
+	cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table);
+	if (soc_config->cpufreq_clk_exit)
+		soc_config->cpufreq_clk_exit();
 	return 0;
 }
 
@@ -155,27 +100,75 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
 #endif
 };
 
-int __init tegra_cpufreq_init(void)
+static struct {
+	char *compat;
+	int (*init)(struct tegra_cpufreq_data *,
+			const struct tegra_cpufreq_config **);
+} tegra_init_funcs[] = {
+	{ "nvidia,tegra20", tegra20_cpufreq_init },
+};
+
+static int tegra_cpufreq_probe(struct platform_device *pdev)
 {
-	cpu_clk = clk_get_sys(NULL, "cclk");
-	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");
-	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);
+	int i;
+	int ret = -EINVAL;
+
+	tegra_data = devm_kzalloc(&pdev->dev,
+			sizeof(*tegra_data), GFP_KERNEL);
+	if (!tegra_data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	tegra_data->dev = &pdev->dev;
+
+	for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
+		if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
+			ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
+			if (!ret)
+				break;
+			else
+				goto out;
+		}
 	}
+	if (i == ARRAY_SIZE(tegra_init_funcs))
+		goto out;
+
+	ret = cpufreq_register_driver(&tegra_cpufreq_driver);
+	if (ret) {
+		dev_err(tegra_data->dev,
+			"%s: failed to register cpufreq driver\n", __func__);
+		goto out;
+	}
+
+	return 0;
+out:
+	return ret;
+}
+
+static int tegra_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&tegra_cpufreq_driver);
+	return 0;
+}
 
-	return cpufreq_register_driver(&tegra_cpufreq_driver);
+static struct platform_driver tegra_cpufreq_platdrv = {
+	.driver = {
+		.name	= "tegra-cpufreq",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= tegra_cpufreq_probe,
+	.remove		= tegra_cpufreq_remove,
+};
+module_platform_driver(tegra_cpufreq_platdrv);
+
+int __init tegra_cpufreq_init(void)
+{
+	struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
+
+	platform_device_register_full(&devinfo);
+
+	return 0;
 }
 EXPORT_SYMBOL(tegra_cpufreq_init);
 
diff --git a/drivers/cpufreq/tegra-cpufreq.h b/drivers/cpufreq/tegra-cpufreq.h
new file mode 100644
index 0000000..0556354
--- /dev/null
+++ b/drivers/cpufreq/tegra-cpufreq.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2013, NVIDIA Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef __LINUX_TEGRA_CPUFREQ_H
+#define __LINUX_TEGRA_CPUFREQ_H
+
+struct tegra_cpufreq_config {
+	void (*vote_emc_on_cpu_rate)(unsigned long);
+	int (*cpu_clk_set_rate)(unsigned long);
+	void (*cpufreq_clk_init)(void);
+	void (*cpufreq_clk_exit)(void);
+};
+
+struct tegra_cpufreq_data {
+	struct device	*dev;
+	struct clk	*cpu_clk;
+	struct cpufreq_frequency_table	*freq_table;
+};
+
+#ifdef CONFIG_ARM_TEGRA20_CPUFREQ
+int tegra20_cpufreq_init(struct tegra_cpufreq_data *data,
+		const struct tegra_cpufreq_config **config);
+#else
+static inline int tegra20_cpufreq_init(struct tegra_cpufreq_data *data,
+		const struct tegra_cpufreq_config **config)
+{ return -EINVAL; }
+#endif
+
+#endif /* __LINUX_TEGRA_CPUFREQ_H_ */
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
new file mode 100644
index 0000000..b5b4e95
--- /dev/null
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -0,0 +1,136 @@
+/*
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/cpufreq.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/suspend.h>
+#include <linux/cpu.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#include "tegra-cpufreq.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 },
+};
+
+static struct clk *cpu_clk;
+static struct clk *pll_x_clk;
+static struct clk *pll_p_clk;
+static struct clk *emc_clk;
+
+static void tegra20_vote_emc_on_cpu_rate(unsigned long rate)
+{
+	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 */
+}
+
+static int tegra20_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 void tegra20_cpufreq_clk_init(void)
+{
+	clk_prepare_enable(emc_clk);
+	clk_prepare_enable(cpu_clk);
+}
+
+static void tegra20_cpufreq_clk_exit(void)
+{
+	clk_disable_unprepare(cpu_clk);
+	clk_disable_unprepare(emc_clk);
+}
+
+static const struct tegra_cpufreq_config tegra20_cpufreq_config = {
+	.vote_emc_on_cpu_rate = tegra20_vote_emc_on_cpu_rate,
+	.cpu_clk_set_rate = tegra20_cpu_clk_set_rate,
+	.cpufreq_clk_init = tegra20_cpufreq_clk_init,
+	.cpufreq_clk_exit = tegra20_cpufreq_clk_exit,
+};
+
+int tegra20_cpufreq_init(struct tegra_cpufreq_data *data,
+		const struct tegra_cpufreq_config **soc_config)
+{
+	cpu_clk = clk_get_sys(NULL, "cclk");
+	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");
+	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);
+	}
+
+	data->cpu_clk = cpu_clk;
+	data->freq_table = freq_table;
+	*soc_config = &tegra20_cpufreq_config;
+
+	return 0;
+}
+EXPORT_SYMBOL(tegra20_cpufreq_init);
-- 
1.7.9.5

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Stephen Warren]

On 12/05/2013 12:44 AM, Bill Huang wrote:
> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
> 
> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
> * Move Tegra20 specific codes into tegra20-cpufreq.c.
> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>   when we're going to get regulator in the driver to support voltage
>   scaling (DVFS).

> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c

> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
...
> +	if (soc_config->vote_emc_on_cpu_rate)
> +		soc_config->vote_emc_on_cpu_rate(rate);
> +
> +	ret = soc_config->cpu_clk_set_rate(rate * 1000);
>  	if (ret)
>  		pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>  			rate);

Is there any/much shared code left in this file after this patch? It
seems like all this file does now is make each cpufreq callback function
call soc_config->the_same_function_name(). If so, wouldn't it be better
to simply implement completely separate tegar20-cpufreq and
tegra30-cpufreq drivers, and register them each directly with the
cpufreq core, to avoid this file doing all the indirection?


> -int __init tegra_cpufreq_init(void)
> +static struct {
> +	char *compat;
> +	int (*init)(struct tegra_cpufreq_data *,
> +			const struct tegra_cpufreq_config **);
> +} tegra_init_funcs[] = {
> +	{ "nvidia,tegra20", tegra20_cpufreq_init },
> +};
> +
> +static int tegra_cpufreq_probe(struct platform_device *pdev)
...
> +	for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
> +		if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
> +			ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
> +			if (!ret)
> +				break;
> +			else
> +				goto out;
> +		}
>  	}
> +	if (i == ARRAY_SIZE(tegra_init_funcs))
> +		goto out;

I think there are better ways of doing this than open-coding it. Perhaps
of_match_device() or the platform-driver equivalent could be made to work?

> +int __init tegra_cpufreq_init(void)
> +{
> +	struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
> +
> +	platform_device_register_full(&devinfo);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(tegra_cpufreq_init);

Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could
dynamically construct the device name based on the DT's root compatible
value, register "${root_compatible}-cpufreq", e.g.
"nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow
the kernel's standard device/driver matching mechanism to pick the
correct driver to instantiate. Perhaps you could even dynamically
register an OF device so that you can use of_match_device() in probe, if
there's some advantage of having a single driver that supports N chips.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/06/2013 07:04 AM, Stephen Warren wrote:
> On 12/05/2013 12:44 AM, Bill Huang wrote:
>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>
>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>>    when we're going to get regulator in the driver to support voltage
>>    scaling (DVFS).
>
>> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
>
>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
> ...
>> +	if (soc_config->vote_emc_on_cpu_rate)
>> +		soc_config->vote_emc_on_cpu_rate(rate);
>> +
>> +	ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>   	if (ret)
>>   		pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>>   			rate);
>
> Is there any/much shared code left in this file after this patch? It
> seems like all this file does now is make each cpufreq callback function
> call soc_config->the_same_function_name(). If so, wouldn't it be better
> to simply implement completely separate tegar20-cpufreq and
> tegra30-cpufreq drivers, and register them each directly with the
> cpufreq core, to avoid this file doing all the indirection?
I think this file is needed since we can shared the registration and 
probe logic for different SoCs.
>
>
>> -int __init tegra_cpufreq_init(void)
>> +static struct {
>> +	char *compat;
>> +	int (*init)(struct tegra_cpufreq_data *,
>> +			const struct tegra_cpufreq_config **);
>> +} tegra_init_funcs[] = {
>> +	{ "nvidia,tegra20", tegra20_cpufreq_init },
>> +};
>> +
>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
> ...
>> +	for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>> +		if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>> +			ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>> +			if (!ret)
>> +				break;
>> +			else
>> +				goto out;
>> +		}
>>   	}
>> +	if (i == ARRAY_SIZE(tegra_init_funcs))
>> +		goto out;
>
> I think there are better ways of doing this than open-coding it. Perhaps
> of_match_device() or the platform-driver equivalent could be made to work?
Open coding is everywhere in OF helper functions actually. I doubt if we 
can use of_match_device() if we're not adding node in DT.
If we're matching the platform device then we might need open coding, no?
>
>> +int __init tegra_cpufreq_init(void)
>> +{
>> +	struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
>> +
>> +	platform_device_register_full(&devinfo);
>> +
>> +	return 0;
>>   }
>>   EXPORT_SYMBOL(tegra_cpufreq_init);
>
> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could
> dynamically construct the device name based on the DT's root compatible
> value, register "${root_compatible}-cpufreq", e.g.
> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow
> the kernel's standard device/driver matching mechanism to pick the
> correct driver to instantiate. Perhaps you could even dynamically
> register an OF device so that you can use of_match_device() in probe, if
I guess what you meant dynamically register an OF device is registering 
an fake OF device by calling of_device_add(), no? If yes then what 
of_node should we give?
> there's some advantage of having a single driver that supports N chips.
>

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Stephen Warren]

On 12/09/2013 01:44 AM, bilhuang wrote:
> On 12/06/2013 07:04 AM, Stephen Warren wrote:
>> On 12/05/2013 12:44 AM, Bill Huang wrote:
>>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>>
>>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>>> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>>>    when we're going to get regulator in the driver to support voltage
>>>    scaling (DVFS).
>>
>>> diff --git a/drivers/cpufreq/tegra-cpufreq.c
>>> b/drivers/cpufreq/tegra-cpufreq.c
>>
>>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct
>>> cpufreq_policy *policy,
>> ...
>>> +    if (soc_config->vote_emc_on_cpu_rate)
>>> +        soc_config->vote_emc_on_cpu_rate(rate);
>>> +
>>> +    ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>>       if (ret)
>>>           pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>>>               rate);
>>
>> Is there any/much shared code left in this file after this patch? It
>> seems like all this file does now is make each cpufreq callback function
>> call soc_config->the_same_function_name(). If so, wouldn't it be better
>> to simply implement completely separate tegar20-cpufreq and
>> tegra30-cpufreq drivers, and register them each directly with the
>> cpufreq core, to avoid this file doing all the indirection?
>
> I think this file is needed since we can shared the registration and
> probe logic for different SoCs.

But there's basically nothing in probe() already, and if we have a
separate driver for each SoC, then there's even less code; just a call
to devm_kzalloc() for the device-specific data (which will be
SoC-specific in size anyway), and a call to cpufreq_register_driver(). I
don't think it's worth sharing that if it means that every other
function needs to be an indirect function call.

>>> -int __init tegra_cpufreq_init(void)
>>> +static struct {
>>> +    char *compat;
>>> +    int (*init)(struct tegra_cpufreq_data *,
>>> +            const struct tegra_cpufreq_config **);
>>> +} tegra_init_funcs[] = {
>>> +    { "nvidia,tegra20", tegra20_cpufreq_init },
>>> +};
>>> +
>>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
>> ...
>>> +    for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>>> +        if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>>> +            ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>>> +            if (!ret)
>>> +                break;
>>> +            else
>>> +                goto out;
>>> +        }
>>>       }
>>> +    if (i == ARRAY_SIZE(tegra_init_funcs))
>>> +        goto out;
>>
>> I think there are better ways of doing this than open-coding it. Perhaps
>> of_match_device() or the platform-driver equivalent could be made to
>> work?
>
> Open coding is everywhere in OF helper functions actually. I doubt if we
> can use of_match_device() if we're not adding node in DT.
> If we're matching the platform device then we might need open coding, no?

For platform devices, you can set up the id_table of struct
platform_driver, and then simply call platform_get_device_id(pdev)
inside probe() to find the matching entry. drivers/i2c/busses/i2c-at91.c
is an example of how this works (just some random driver I found using
grep).

>>> +int __init tegra_cpufreq_init(void)
>>> +{
>>> +    struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
>>> +
>>> +    platform_device_register_full(&devinfo);
>>> +
>>> +    return 0;
>>>   }
>>>   EXPORT_SYMBOL(tegra_cpufreq_init);
>>
>> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could
>> dynamically construct the device name based on the DT's root compatible
>> value, register "${root_compatible}-cpufreq", e.g.
>> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow
>> the kernel's standard device/driver matching mechanism to pick the
>> correct driver to instantiate. Perhaps you could even dynamically
>> register an OF device so that you can use of_match_device() in probe, if
>
> I guess what you meant dynamically register an OF device is registering
> an fake OF device by calling of_device_add(), no? If yes then what
> of_node should we give?

Yes. Good question about which node. I guess the root node would be the
only one that made any sense at all, and admittedly it doesn't make a
huge amount of sense. Perhaps registers a platform device rather than an
OF device would make more sense. See platform_device_register() I think.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/10/2013 01:32 AM, Stephen Warren wrote:
> On 12/09/2013 01:44 AM, bilhuang wrote:
>> On 12/06/2013 07:04 AM, Stephen Warren wrote:
>>> On 12/05/2013 12:44 AM, Bill Huang wrote:
>>>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>>>
>>>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>>>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>>>> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>>>>     when we're going to get regulator in the driver to support voltage
>>>>     scaling (DVFS).
>>>
>>>> diff --git a/drivers/cpufreq/tegra-cpufreq.c
>>>> b/drivers/cpufreq/tegra-cpufreq.c
>>>
>>>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct
>>>> cpufreq_policy *policy,
>>> ...
>>>> +    if (soc_config->vote_emc_on_cpu_rate)
>>>> +        soc_config->vote_emc_on_cpu_rate(rate);
>>>> +
>>>> +    ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>>>        if (ret)
>>>>            pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>>>>                rate);
>>>
>>> Is there any/much shared code left in this file after this patch? It
>>> seems like all this file does now is make each cpufreq callback function
>>> call soc_config->the_same_function_name(). If so, wouldn't it be better
>>> to simply implement completely separate tegar20-cpufreq and
>>> tegra30-cpufreq drivers, and register them each directly with the
>>> cpufreq core, to avoid this file doing all the indirection?
>>
>> I think this file is needed since we can shared the registration and
>> probe logic for different SoCs.
>
> But there's basically nothing in probe() already, and if we have a
> separate driver for each SoC, then there's even less code; just a call
> to devm_kzalloc() for the device-specific data (which will be
> SoC-specific in size anyway), and a call to cpufreq_register_driver(). I
> don't think it's worth sharing that if it means that every other
> function needs to be an indirect function call.
OK that makes sense.
>
>>>> -int __init tegra_cpufreq_init(void)
>>>> +static struct {
>>>> +    char *compat;
>>>> +    int (*init)(struct tegra_cpufreq_data *,
>>>> +            const struct tegra_cpufreq_config **);
>>>> +} tegra_init_funcs[] = {
>>>> +    { "nvidia,tegra20", tegra20_cpufreq_init },
>>>> +};
>>>> +
>>>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
>>> ...
>>>> +    for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>>>> +        if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>>>> +            ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>>>> +            if (!ret)
>>>> +                break;
>>>> +            else
>>>> +                goto out;
>>>> +        }
>>>>        }
>>>> +    if (i == ARRAY_SIZE(tegra_init_funcs))
>>>> +        goto out;
>>>
>>> I think there are better ways of doing this than open-coding it. Perhaps
>>> of_match_device() or the platform-driver equivalent could be made to
>>> work?
>>
>> Open coding is everywhere in OF helper functions actually. I doubt if we
>> can use of_match_device() if we're not adding node in DT.
>> If we're matching the platform device then we might need open coding, no?
>
> For platform devices, you can set up the id_table of struct
> platform_driver, and then simply call platform_get_device_id(pdev)
> inside probe() to find the matching entry. drivers/i2c/busses/i2c-at91.c
> is an example of how this works (just some random driver I found using
> grep).
If we're going to have separate driver for each SoC, then we don't need 
platform_get_device_id(pdev) stuffs...

What I would like to do is creating platform cpufreq device with name 
"${root_compatible}-cpufreq" then each SoC cpufreq driver can bind to 
it, but the question is, which file is the best place to do this? Create 
a new file for this or use existing file like arch/arm/mach-tegra/tegra.c?
>
>>>> +int __init tegra_cpufreq_init(void)
>>>> +{
>>>> +    struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
>>>> +
>>>> +    platform_device_register_full(&devinfo);
>>>> +
>>>> +    return 0;
>>>>    }
>>>>    EXPORT_SYMBOL(tegra_cpufreq_init);
>>>
>>> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could
>>> dynamically construct the device name based on the DT's root compatible
>>> value, register "${root_compatible}-cpufreq", e.g.
>>> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow
>>> the kernel's standard device/driver matching mechanism to pick the
>>> correct driver to instantiate. Perhaps you could even dynamically
>>> register an OF device so that you can use of_match_device() in probe, if
>>
>> I guess what you meant dynamically register an OF device is registering
>> an fake OF device by calling of_device_add(), no? If yes then what
>> of_node should we give?
>
> Yes. Good question about which node. I guess the root node would be the
> only one that made any sense at all, and admittedly it doesn't make a
> huge amount of sense. Perhaps registers a platform device rather than an
> OF device would make more sense. See platform_device_register() I think.
>

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Stephen Warren]

On 12/11/2013 04:18 AM, bilhuang wrote:
> On 12/10/2013 01:32 AM, Stephen Warren wrote:
>> On 12/09/2013 01:44 AM, bilhuang wrote:
>>> On 12/06/2013 07:04 AM, Stephen Warren wrote:
>>>> On 12/05/2013 12:44 AM, Bill Huang wrote:
>>>>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>>>>
>>>>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>>>>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>>>>> * Bind Tegra cpufreq dirver with a fake device so defer probe would
>>>>> work
>>>>>     when we're going to get regulator in the driver to support voltage
>>>>>     scaling (DVFS).
>>>>
>>>>> diff --git a/drivers/cpufreq/tegra-cpufreq.c
>>>>> b/drivers/cpufreq/tegra-cpufreq.c
>>>>
>>>>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct
>>>>> cpufreq_policy *policy,
>>>> ...
>>>>> +    if (soc_config->vote_emc_on_cpu_rate)
>>>>> +        soc_config->vote_emc_on_cpu_rate(rate);
>>>>> +
>>>>> +    ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>>>>        if (ret)
>>>>>            pr_err("cpu-tegra: Failed to set cpu frequency to %lu
>>>>> kHz\n",
>>>>>                rate);
>>>>
>>>> Is there any/much shared code left in this file after this patch? It
>>>> seems like all this file does now is make each cpufreq callback
>>>> function
>>>> call soc_config->the_same_function_name(). If so, wouldn't it be better
>>>> to simply implement completely separate tegar20-cpufreq and
>>>> tegra30-cpufreq drivers, and register them each directly with the
>>>> cpufreq core, to avoid this file doing all the indirection?
>>>
>>> I think this file is needed since we can shared the registration and
>>> probe logic for different SoCs.
>>
>> But there's basically nothing in probe() already, and if we have a
>> separate driver for each SoC, then there's even less code; just a call
>> to devm_kzalloc() for the device-specific data (which will be
>> SoC-specific in size anyway), and a call to cpufreq_register_driver(). I
>> don't think it's worth sharing that if it means that every other
>> function needs to be an indirect function call.
> OK that makes sense.
>>
>>>>> -int __init tegra_cpufreq_init(void)
>>>>> +static struct {
>>>>> +    char *compat;
>>>>> +    int (*init)(struct tegra_cpufreq_data *,
>>>>> +            const struct tegra_cpufreq_config **);
>>>>> +} tegra_init_funcs[] = {
>>>>> +    { "nvidia,tegra20", tegra20_cpufreq_init },
>>>>> +};
>>>>> +
>>>>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
>>>> ...
>>>>> +    for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>>>>> +        if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>>>>> +            ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>>>>> +            if (!ret)
>>>>> +                break;
>>>>> +            else
>>>>> +                goto out;
>>>>> +        }
>>>>>        }
>>>>> +    if (i == ARRAY_SIZE(tegra_init_funcs))
>>>>> +        goto out;
>>>>
>>>> I think there are better ways of doing this than open-coding it.
>>>> Perhaps
>>>> of_match_device() or the platform-driver equivalent could be made to
>>>> work?
>>>
>>> Open coding is everywhere in OF helper functions actually. I doubt if we
>>> can use of_match_device() if we're not adding node in DT.
>>> If we're matching the platform device then we might need open coding,
>>> no?
>>
>> For platform devices, you can set up the id_table of struct
>> platform_driver, and then simply call platform_get_device_id(pdev)
>> inside probe() to find the matching entry. drivers/i2c/busses/i2c-at91.c
>> is an example of how this works (just some random driver I found using
>> grep).
>
> If we're going to have separate driver for each SoC, then we don't need
> platform_get_device_id(pdev) stuffs...

True.

> What I would like to do is creating platform cpufreq device with name
> "${root_compatible}-cpufreq" then each SoC cpufreq driver can bind to
> it, but the question is, which file is the best place to do this? Create
> a new file for this or use existing file like arch/arm/mach-tegra/tegra.c?

I think create the device in
drivers/cpufreq/cpufreq-tegra.c:tegra_cpufreq_init() (which is possibly
all that file would contain), and call that function from
arch/arm/mach-tegra/tegra.c. That way, we'll be able to share the
implementation of tegra_cpufreq_init() with any ARMv8 CPUs that might
appear.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Viresh Kumar]

On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> wrote:
> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>
> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
> * Move Tegra20 specific codes into tegra20-cpufreq.c.
> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>   when we're going to get regulator in the driver to support voltage
>   scaling (DVFS).

I strongly feel we must reuse cpufreq-cpu0 driver here after adding a
clk/regulator driver for tegra to support all that.

@Stephen: If you want we can keep all that tegra specific stuff
(clk/regulator) in
tegra-cpufreq.c, but we can easily use cpufreq-cpu0 driver without much
complications..

I have tried it earlier, got some comments and then got busy in other stuff..
https://lkml.org/lkml/2013/8/7/364

>  static int tegra_cpu_exit(struct cpufreq_policy *policy)
>  {
> -       clk_disable_unprepare(cpu_clk);
> -       clk_disable_unprepare(emc_clk);
> +       cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table);

Btw, why do you need this here?

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/17/2013 02:54 PM, Viresh Kumar wrote:
> On 5 December 2013 13:14, Bill Huang <bilhuang@nvidia.com> wrote:
>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>
>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>>    when we're going to get regulator in the driver to support voltage
>>    scaling (DVFS).
>
> I strongly feel we must reuse cpufreq-cpu0 driver here after adding a
> clk/regulator driver for tegra to support all that.

Tegra20 DVFS is a little bit complicated due to the fact that we can't 
scale VDD_CPU directly, there are constraints or relationship to other 
power rails so I don't think it is a good idea to use generic 
cpufreq-cpu0 driver if we're going to support voltage scaling.
>
> @Stephen: If you want we can keep all that tegra specific stuff
> (clk/regulator) in
> tegra-cpufreq.c, but we can easily use cpufreq-cpu0 driver without much
> complications..
>
> I have tried it earlier, got some comments and then got busy in other stuff..
> https://lkml.org/lkml/2013/8/7/364
>
>>   static int tegra_cpu_exit(struct cpufreq_policy *policy)
>>   {
>> -       clk_disable_unprepare(cpu_clk);
>> -       clk_disable_unprepare(emc_clk);
>> +       cpufreq_frequency_table_cpuinfo(policy, tegra_data->freq_table);
>
> Btw, why do you need this here?
>
Actually the latest version is v4 which is quite different against v3.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Viresh Kumar]

On 17 December 2013 16:22, bilhuang <bilhuang@nvidia.com> wrote:
> Tegra20 DVFS is a little bit complicated due to the fact that we can't scale
> VDD_CPU directly, there are constraints or relationship to other power rails
> so I don't think it is a good idea to use generic cpufreq-cpu0 driver if
> we're going to support voltage scaling.

But why can't we handle that in a CPU specific regulator code?

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/18/2013 07:11 PM, Viresh Kumar wrote:
> On 17 December 2013 16:22, bilhuang <bilhuang@nvidia.com> wrote:
>> Tegra20 DVFS is a little bit complicated due to the fact that we can't scale
>> VDD_CPU directly, there are constraints or relationship to other power rails
>> so I don't think it is a good idea to use generic cpufreq-cpu0 driver if
>> we're going to support voltage scaling.
>
> But why can't we handle that in a CPU specific regulator code?
>
cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly 
according to the pre-defined OPP freq/volt pairs, the regulator drivers 
could be shared by other SoC so is not suitable to handle this, or do I 
misunderstand?

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Viresh Kumar]

On 18 December 2013 17:03, bilhuang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly according
> to the pre-defined OPP freq/volt pairs, the regulator drivers could be
> shared by other SoC so is not suitable to handle this, or do I
> misunderstand?

In case regulator's driver is shared, then you can probably add another
virtual regulator for CPU which would have the special code you want.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/18/2013 10:39 PM, Viresh Kumar wrote:
> On 18 December 2013 17:03, bilhuang <bilhuang@nvidia.com> wrote:
>> cpufreq-cpu0 driver will call regulator_set_voltage_tol() directly according
>> to the pre-defined OPP freq/volt pairs, the regulator drivers could be
>> shared by other SoC so is not suitable to handle this, or do I
>> misunderstand?
>
> In case regulator's driver is shared, then you can probably add another
> virtual regulator for CPU which would have the special code you want.
>
I'm not sure virtual regulator for CPU is a good idea, in addition to 
that, we don't have a single SoC OPP table, we need several which are 
speedo-id and process-id dependant, but generic cpufreq-cpu0 is assuming 
there is only one statically, for some SoC the frequency table is not 
fixed, they are created at runtime combining our fast and slow CPU 
frequency table and dvfs table. So I'm really not sure is it worth 
adding so many tweaks in order to use the generic cpufreq-cpu0 driver.

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[Viresh Kumar]

On 19 December 2013 10:56, bilhuang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> I'm not sure virtual regulator for CPU is a good idea, in addition to that,
> we don't have a single SoC OPP table, we need several which are speedo-id
> and process-id dependant, but generic cpufreq-cpu0 is assuming there is only
> one statically

Can't that be handled via DT ?

> for some SoC the frequency table is not fixed, they are
> created at runtime combining our fast and slow CPU frequency table and dvfs
> table. So I'm really not sure is it worth adding so many tweaks in order to
> use the generic cpufreq-cpu0 driver.

Hmm, maybe I got confused because I don't have a clear picture in my mind.
It might be better to go ahead with your implementation for now and after
everything is set, we can choose to use cpufreq-cpu0 if it is worth it.

--
viresh

Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang]

On 12/19/2013 01:29 PM, Viresh Kumar wrote:
> On 19 December 2013 10:56, bilhuang <bilhuang@nvidia.com> wrote:
>> I'm not sure virtual regulator for CPU is a good idea, in addition to that,
>> we don't have a single SoC OPP table, we need several which are speedo-id
>> and process-id dependant, but generic cpufreq-cpu0 is assuming there is only
>> one statically
>
> Can't that be handled via DT ?
I don't think it can be handled via DT unless we separate DTB according 
to different CPU speedo/process-id but that is not a good idea.
>
>> for some SoC the frequency table is not fixed, they are
>> created at runtime combining our fast and slow CPU frequency table and dvfs
>> table. So I'm really not sure is it worth adding so many tweaks in order to
>> use the generic cpufreq-cpu0 driver.
>
> Hmm, maybe I got confused because I don't have a clear picture in my mind.
> It might be better to go ahead with your implementation for now and after
> everything is set, we can choose to use cpufreq-cpu0 if it is worth it.
>
This makes more sense to me, thanks.
> --
> viresh
>

Re: [PATCH v3 0/2] Remodel Tegra cpufreq drivers to support Tegra series SoC

[Viresh Kumar]

On 5 December 2013 13:14, Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> This patch series remodel Tegra cpufreq driver to make it more easy to
> add new SoC support, in addition to that, adding probe function in the
> driver to let probe defer can be used to control init sequence when we
> are going to support DVFS.
>
> Changes since v2:
>
> - Fix Kconfig.
> - Rebase on top of branch 'cpufreq-next' on git://git.linaro.org/people/vireshk/linux.git.

That's wrong base.. This branch is used only for Rafael to pull in
stuff and so it doesn't
get updated everyday..

You must rebase on Rafael's linux-next branch all the time.

Re: [PATCH v3 0/2] Remodel Tegra cpufreq drivers to support Tegra series SoC

[bilhuang]

On 12/17/2013 02:26 PM, Viresh Kumar wrote:
> On 5 December 2013 13:14, Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> This patch series remodel Tegra cpufreq driver to make it more easy to
>> add new SoC support, in addition to that, adding probe function in the
>> driver to let probe defer can be used to control init sequence when we
>> are going to support DVFS.
>>
>> Changes since v2:
>>
>> - Fix Kconfig.
>> - Rebase on top of branch 'cpufreq-next' on git://git.linaro.org/people/vireshk/linux.git.
>
> That's wrong base.. This branch is used only for Rafael to pull in
> stuff and so it doesn't
> get updated everyday..
>
> You must rebase on Rafael's linux-next branch all the time.
>
OK thanks, is it branch pm-cpufreq?

Re: [PATCH v3 0/2] Remodel Tegra cpufreq drivers to support Tegra series SoC

[Viresh Kumar]

On 17 December 2013 16:17, bilhuang <bilhuang@nvidia.com> wrote:
> OK thanks, is it branch pm-cpufreq?

Rebase on linux-next..