[PATCH v1 00/11] Clean up Tegra20 cpufreq driver

[PATCH v1 00/11] Clean up Tegra20 cpufreq driver

[Dmitry Osipenko]

Hello,

Recently Peter Geis (who is working on Tegra30 cpufreq driver) asked me how
tegra20-cpufreq driver is getting loaded. After taking a look at the code
it became apparent that the drivers code has been rusted a tad and so this
series is intended to refresh the drivers code by disallowing module to be
loaded on non-Tegra20 machines, by cleaning whitespaces in the code, removing
dead EMC code and in the end by allowing tegra20-cpufreq to be built as a
loadable module.

Please review, thanks.

Dmitry Osipenko (11):
  cpufreq: tegra20: Change module description
  cpufreq: tegra20: Clean up whitespaces in the code
  cpufreq: tegra20: Remove EMC clock usage
  cpufreq: tegra20: Release clocks properly
  cpufreq: tegra20: Clean up included headers
  cpufreq: tegra20: Remove unneeded check in tegra_cpu_init
  cpufreq: tegra20: Check if this is Tegra20 machine
  cpufreq: tegra20: Remove unneeded variable initialization
  cpufreq: tegra20: Allow cpufreq driver to be built as loadable module
  cpufreq: tegra20: Wrap cpufreq into platform driver
  ARM: tegra: Create platform device for tegra20-cpufreq driver

 arch/arm/mach-tegra/tegra.c       |   4 +
 drivers/cpufreq/Kconfig.arm       |   2 +-
 drivers/cpufreq/tegra20-cpufreq.c | 172 +++++++++++++++++-------------
 3 files changed, 101 insertions(+), 77 deletions(-)

-- 
2.17.0

[PATCH v1 01/11] cpufreq: tegra20: Change module description

[Dmitry Osipenko]

Change module description to be in line with the other Tegra drivers, just
for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 2bd62845e9d5..10793498355e 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -210,7 +210,7 @@ static void __exit tegra_cpufreq_exit(void)
 
 
 MODULE_AUTHOR("Colin Cross <ccross@android.com>");
-MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2");
+MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
 MODULE_LICENSE("GPL");
 module_init(tegra_cpufreq_init);
 module_exit(tegra_cpufreq_exit);
-- 
2.17.0

[PATCH v1 02/11] cpufreq: tegra20: Clean up whitespaces in the code

[Dmitry Osipenko]

Remove unneeded blank line and replace whitespaces with a tab in the code
for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 10793498355e..dd8a76a64a8e 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -203,12 +203,11 @@ static int __init tegra_cpufreq_init(void)
 
 static void __exit tegra_cpufreq_exit(void)
 {
-        cpufreq_unregister_driver(&tegra_cpufreq_driver);
+	cpufreq_unregister_driver(&tegra_cpufreq_driver);
 	clk_put(emc_clk);
 	clk_put(cpu_clk);
 }
 
-
 MODULE_AUTHOR("Colin Cross <ccross@android.com>");
 MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
 MODULE_LICENSE("GPL");
-- 
2.17.0

[PATCH v1 03/11] cpufreq: tegra20: Remove EMC clock usage

[Dmitry Osipenko]

The EMC driver has been gone 4 years ago, since the commit a7cbe92cef27
("ARM: tegra: remove tegra EMC scaling driver"). Remove the EMC clock
usage as it does nothing. We may consider re-implementing the EMC scaling
later, probably using PM Memory Bandwidth QoS API.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index dd8a76a64a8e..693f9067ba8a 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -44,7 +44,6 @@ static struct cpufreq_frequency_table freq_table[] = {
 static struct clk *cpu_clk;
 static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
-static struct clk *emc_clk;
 static bool pll_x_prepared;
 
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
@@ -95,17 +94,6 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
 	int ret = 0;
 
-	/*
-	 * 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 */
-
 	/*
 	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
 	 * as it isn't used anymore.
@@ -141,14 +129,12 @@ 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);
 
 	/* FIXME: what's the actual transition time? */
 	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
 	if (ret) {
 		clk_disable_unprepare(cpu_clk);
-		clk_disable_unprepare(emc_clk);
 		return ret;
 	}
 
@@ -160,7 +146,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 static int tegra_cpu_exit(struct cpufreq_policy *policy)
 {
 	clk_disable_unprepare(cpu_clk);
-	clk_disable_unprepare(emc_clk);
 	return 0;
 }
 
@@ -192,19 +177,12 @@ static int __init tegra_cpufreq_init(void)
 	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);
 }
 
-- 
2.17.0

[PATCH v1 04/11] cpufreq: tegra20: Release clocks properly

[Dmitry Osipenko]

Properly put requested clocks in the module init/exit code.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 693f9067ba8a..69f033d297e1 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -165,24 +165,45 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
 
 static int __init tegra_cpufreq_init(void)
 {
+	int err;
+
 	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);
+	if (IS_ERR(pll_x_clk)) {
+		err = PTR_ERR(pll_x_clk);
+		goto put_cpu;
+	}
 
 	pll_p_clk = clk_get_sys(NULL, "pll_p");
-	if (IS_ERR(pll_p_clk))
-		return PTR_ERR(pll_p_clk);
+	if (IS_ERR(pll_p_clk)) {
+		err = PTR_ERR(pll_p_clk);
+		goto put_pll_x;
+	}
+
+	err = cpufreq_register_driver(&tegra_cpufreq_driver);
+	if (err)
+		goto put_pll_p;
+
+	return 0;
+
+put_pll_p:
+	clk_put(pll_p_clk);
+put_pll_x:
+	clk_put(pll_x_clk);
+put_cpu:
+	clk_put(cpu_clk);
 
-	return cpufreq_register_driver(&tegra_cpufreq_driver);
+	return err;
 }
 
 static void __exit tegra_cpufreq_exit(void)
 {
 	cpufreq_unregister_driver(&tegra_cpufreq_driver);
+	clk_put(pll_p_clk);
+	clk_put(pll_x_clk);
 	clk_put(cpu_clk);
 }
 
-- 
2.17.0

[PATCH v1 05/11] cpufreq: tegra20: Clean up included headers

[Dmitry Osipenko]

Remove unused/unneeded headers and sort them in the alphabet order.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 69f033d297e1..61f00d1cba26 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -16,16 +16,9 @@
  *
  */
 
-#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/cpufreq.h>
+#include <linux/module.h>
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
-- 
2.17.0

[PATCH v1 06/11] cpufreq: tegra20: Remove unneeded check in tegra_cpu_init

[Dmitry Osipenko]

Remove checking of the CPU number for consistency as it won't ever fail
unless there is a severe bug in the cpufreq core.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 61f00d1cba26..147ae3e14f18 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -32,8 +32,6 @@ static struct cpufreq_frequency_table freq_table[] = {
 	{ .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;
@@ -119,9 +117,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
 	int ret;
 
-	if (policy->cpu >= NUM_CPUS)
-		return -EINVAL;
-
 	clk_prepare_enable(cpu_clk);
 
 	/* FIXME: what's the actual transition time? */
-- 
2.17.0

[PATCH v1 07/11] cpufreq: tegra20: Check if this is Tegra20 machine

[Dmitry Osipenko]

Don't even try to request the clocks during of module initialization on
non-Tegra20 machines (this is the case for a multi-platform kernel) for
consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 147ae3e14f18..797c61c74b65 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
@@ -155,6 +156,9 @@ static int __init tegra_cpufreq_init(void)
 {
 	int err;
 
+	if (!of_machine_is_compatible("nvidia,tegra20"))
+		return -ENODEV;
+
 	cpu_clk = clk_get_sys(NULL, "cclk");
 	if (IS_ERR(cpu_clk))
 		return PTR_ERR(cpu_clk);
-- 
2.17.0

[PATCH v1 08/11] cpufreq: tegra20: Remove unneeded variable initialization

[Dmitry Osipenko]

Remove unneeded variable initialization solely for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 797c61c74b65..c0a7b5a78aa6 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -84,7 +84,7 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	unsigned long rate = freq_table[index].frequency;
 	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
-- 
2.17.0

[PATCH v1 09/11] cpufreq: tegra20: Allow cpufreq driver to be built as loadable module

[Dmitry Osipenko]

Nothing prevents Tegra20 CPUFreq module to be unloaded, hence allow it to
be built as a non-builtin kernel module.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/Kconfig.arm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 96b35b8b3606..a8a2e210c624 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -264,7 +264,7 @@ config ARM_TANGO_CPUFREQ
 	default y
 
 config ARM_TEGRA20_CPUFREQ
-	bool "Tegra20 CPUFreq support"
+	tristate "Tegra20 CPUFreq support"
 	depends on ARCH_TEGRA
 	default y
 	help
-- 
2.17.0

[PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Dmitry Osipenko]

Currently tegra20-cpufreq kernel module isn't getting autoloaded because
there is no device associated with the module, this is one of two patches
that resolves the module autoloading. This patch adds a module alias that
will associate the tegra20-cpufreq kernel module with the platform device,
other patch will instantiate the actual platform device. And now it makes
sense to wrap cpufreq driver into a platform driver for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/cpufreq/tegra20-cpufreq.c | 116 +++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index c0a7b5a78aa6..f9d02a28df9e 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -19,7 +19,7 @@
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/platform_device.h>
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
@@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = CPUFREQ_TABLE_END },
 };
 
-static struct clk *cpu_clk;
-static struct clk *pll_x_clk;
-static struct clk *pll_p_clk;
-static bool pll_x_prepared;
+struct tegra20_cpufreq_data {
+	struct device *dev;
+	struct clk *cpu_clk;
+	struct clk *pll_x_clk;
+	struct clk *pll_p_clk;
+	bool pll_x_prepared;
+};
 
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
 					   unsigned int index)
 {
-	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+	struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
+	unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
 
 	/*
 	 * Don't switch to intermediate freq if:
@@ -57,6 +61,7 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
 static int tegra_target_intermediate(struct cpufreq_policy *policy,
 				     unsigned int index)
 {
+	struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
 	int ret;
 
 	/*
@@ -69,21 +74,22 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy,
 	 * Also, we wouldn't be using pll_x anymore and must not take extra
 	 * reference to it, as it can be disabled now to save some power.
 	 */
-	clk_prepare_enable(pll_x_clk);
+	clk_prepare_enable(data->pll_x_clk);
 
-	ret = clk_set_parent(cpu_clk, pll_p_clk);
+	ret = clk_set_parent(data->cpu_clk, data->pll_p_clk);
 	if (ret)
-		clk_disable_unprepare(pll_x_clk);
+		clk_disable_unprepare(data->pll_x_clk);
 	else
-		pll_x_prepared = true;
+		data->pll_x_prepared = true;
 
 	return ret;
 }
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
+	struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
 	unsigned long rate = freq_table[index].frequency;
-	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+	unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
 	int ret;
 
 	/*
@@ -91,14 +97,14 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 	 * as it isn't used anymore.
 	 */
 	if (rate == ifreq)
-		return clk_set_parent(cpu_clk, pll_p_clk);
+		return clk_set_parent(data->cpu_clk, data->pll_p_clk);
 
-	ret = clk_set_rate(pll_x_clk, rate * 1000);
+	ret = clk_set_rate(data->pll_x_clk, rate * 1000);
 	/* Restore to earlier frequency on error, i.e. pll_x */
 	if (ret)
-		pr_err("Failed to change pll_x to %lu\n", rate);
+		dev_err(data->dev, "Failed to change pll_x to %lu\n", rate);
 
-	ret = clk_set_parent(cpu_clk, pll_x_clk);
+	ret = clk_set_parent(data->cpu_clk, data->pll_x_clk);
 	/* This shouldn't fail while changing or restoring */
 	WARN_ON(ret);
 
@@ -106,9 +112,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 	 * Drop count to pll_x clock only if we switched to intermediate freq
 	 * earlier while transitioning to a target frequency.
 	 */
-	if (pll_x_prepared) {
-		clk_disable_unprepare(pll_x_clk);
-		pll_x_prepared = false;
+	if (data->pll_x_prepared) {
+		clk_disable_unprepare(data->pll_x_clk);
+		data->pll_x_prepared = false;
 	}
 
 	return ret;
@@ -116,25 +122,28 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
+	struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
 	int ret;
 
-	clk_prepare_enable(cpu_clk);
+	clk_prepare_enable(data->cpu_clk);
 
 	/* FIXME: what's the actual transition time? */
 	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
 	if (ret) {
-		clk_disable_unprepare(cpu_clk);
+		clk_disable_unprepare(data->cpu_clk);
 		return ret;
 	}
 
-	policy->clk = cpu_clk;
+	policy->clk = data->cpu_clk;
 	policy->suspend_freq = freq_table[0].frequency;
 	return 0;
 }
 
 static int tegra_cpu_exit(struct cpufreq_policy *policy)
 {
-	clk_disable_unprepare(cpu_clk);
+	struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
+
+	clk_disable_unprepare(data->cpu_clk);
 	return 0;
 }
 
@@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
 	.suspend		= cpufreq_generic_suspend,
 };
 
-static int __init tegra_cpufreq_init(void)
+static int tegra20_cpufreq_probe(struct platform_device *pdev)
 {
+	struct tegra20_cpufreq_data *data;
 	int err;
 
-	if (!of_machine_is_compatible("nvidia,tegra20"))
-		return -ENODEV;
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
-	cpu_clk = clk_get_sys(NULL, "cclk");
-	if (IS_ERR(cpu_clk))
-		return PTR_ERR(cpu_clk);
+	data->cpu_clk = clk_get_sys(NULL, "cclk");
+	if (IS_ERR(data->cpu_clk))
+		return PTR_ERR(data->cpu_clk);
 
-	pll_x_clk = clk_get_sys(NULL, "pll_x");
-	if (IS_ERR(pll_x_clk)) {
-		err = PTR_ERR(pll_x_clk);
+	data->pll_x_clk = clk_get_sys(NULL, "pll_x");
+	if (IS_ERR(data->pll_x_clk)) {
+		err = PTR_ERR(data->pll_x_clk);
 		goto put_cpu;
 	}
 
-	pll_p_clk = clk_get_sys(NULL, "pll_p");
-	if (IS_ERR(pll_p_clk)) {
-		err = PTR_ERR(pll_p_clk);
+	data->pll_p_clk = clk_get_sys(NULL, "pll_p");
+	if (IS_ERR(data->pll_p_clk)) {
+		err = PTR_ERR(data->pll_p_clk);
 		goto put_pll_x;
 	}
 
+	data->dev = &pdev->dev;
+
+	tegra_cpufreq_driver.driver_data = data;
+
 	err = cpufreq_register_driver(&tegra_cpufreq_driver);
 	if (err)
 		goto put_pll_p;
 
+	platform_set_drvdata(pdev, data);
+
 	return 0;
 
 put_pll_p:
-	clk_put(pll_p_clk);
+	clk_put(data->pll_p_clk);
 put_pll_x:
-	clk_put(pll_x_clk);
+	clk_put(data->pll_x_clk);
 put_cpu:
-	clk_put(cpu_clk);
+	clk_put(data->cpu_clk);
 
 	return err;
 }
 
-static void __exit tegra_cpufreq_exit(void)
+static int tegra20_cpufreq_remove(struct platform_device *pdev)
 {
+	struct tegra20_cpufreq_data *data = platform_get_drvdata(pdev);
+
 	cpufreq_unregister_driver(&tegra_cpufreq_driver);
-	clk_put(pll_p_clk);
-	clk_put(pll_x_clk);
-	clk_put(cpu_clk);
+
+	clk_put(data->pll_p_clk);
+	clk_put(data->pll_x_clk);
+	clk_put(data->cpu_clk);
+
+	return 0;
 }
 
+static struct platform_driver tegra20_cpufreq_driver = {
+	.probe		= tegra20_cpufreq_probe,
+	.remove		= tegra20_cpufreq_remove,
+	.driver		= {
+		.name	= "tegra20-cpufreq",
+	},
+};
+module_platform_driver(tegra20_cpufreq_driver);
+
 MODULE_AUTHOR("Colin Cross <ccross@android.com>");
+MODULE_ALIAS("platform:tegra20-cpufreq");
 MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
 MODULE_LICENSE("GPL");
-module_init(tegra_cpufreq_init);
-module_exit(tegra_cpufreq_exit);
-- 
2.17.0

[PATCH v1 11/11] ARM: tegra: Create platform device for tegra20-cpufreq driver

[Dmitry Osipenko]

Tegra20-cpufreq driver require a platform device in order to be loaded,
instantiate a simple platform device for the driver during of the machines
late initialization.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/mach-tegra/tegra.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 02e712d2ea30..f9587be48235 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -97,6 +97,10 @@ static void __init tegra_dt_init_late(void)
 	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
 	    of_machine_is_compatible("compal,paz00"))
 		tegra_paz00_wifikill_init();
+
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
+	    of_machine_is_compatible("nvidia,tegra20"))
+		platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
 }
 
 static const char * const tegra_dt_board_compat[] = {
-- 
2.17.0

Re: [PATCH v1 01/11] cpufreq: tegra20: Change module description

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Change module description to be in line with the other Tegra drivers, just
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 2bd62845e9d5..10793498355e 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -210,7 +210,7 @@ static void __exit tegra_cpufreq_exit(void)
>  
>  
>  MODULE_AUTHOR("Colin Cross <ccross@android.com>");
> -MODULE_DESCRIPTION("cpufreq driver for Nvidia Tegra2");
> +MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
>  MODULE_LICENSE("GPL");
>  module_init(tegra_cpufreq_init);
>  module_exit(tegra_cpufreq_exit);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 02/11] cpufreq: tegra20: Clean up whitespaces in the code

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Remove unneeded blank line and replace whitespaces with a tab in the code
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 10793498355e..dd8a76a64a8e 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -203,12 +203,11 @@ static int __init tegra_cpufreq_init(void)
>  
>  static void __exit tegra_cpufreq_exit(void)
>  {
> -        cpufreq_unregister_driver(&tegra_cpufreq_driver);
> +	cpufreq_unregister_driver(&tegra_cpufreq_driver);
>  	clk_put(emc_clk);
>  	clk_put(cpu_clk);
>  }
>  
> -
>  MODULE_AUTHOR("Colin Cross <ccross@android.com>");
>  MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
>  MODULE_LICENSE("GPL");

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 03/11] cpufreq: tegra20: Remove EMC clock usage

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> The EMC driver has been gone 4 years ago, since the commit a7cbe92cef27
> ("ARM: tegra: remove tegra EMC scaling driver"). Remove the EMC clock
> usage as it does nothing. We may consider re-implementing the EMC scaling
> later, probably using PM Memory Bandwidth QoS API.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 22 ----------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index dd8a76a64a8e..693f9067ba8a 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -44,7 +44,6 @@ static struct cpufreq_frequency_table freq_table[] = {
>  static struct clk *cpu_clk;
>  static struct clk *pll_x_clk;
>  static struct clk *pll_p_clk;
> -static struct clk *emc_clk;
>  static bool pll_x_prepared;
>  
>  static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
> @@ -95,17 +94,6 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>  	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
>  	int ret = 0;
>  
> -	/*
> -	 * 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 */
> -
>  	/*
>  	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
>  	 * as it isn't used anymore.
> @@ -141,14 +129,12 @@ 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);
>  
>  	/* FIXME: what's the actual transition time? */
>  	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
>  	if (ret) {
>  		clk_disable_unprepare(cpu_clk);
> -		clk_disable_unprepare(emc_clk);
>  		return ret;
>  	}
>  
> @@ -160,7 +146,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
>  static int tegra_cpu_exit(struct cpufreq_policy *policy)
>  {
>  	clk_disable_unprepare(cpu_clk);
> -	clk_disable_unprepare(emc_clk);
>  	return 0;
>  }
>  
> @@ -192,19 +177,12 @@ static int __init tegra_cpufreq_init(void)
>  	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);
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 04/11] cpufreq: tegra20: Release clocks properly

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Properly put requested clocks in the module init/exit code.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 693f9067ba8a..69f033d297e1 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -165,24 +165,45 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>  
>  static int __init tegra_cpufreq_init(void)
>  {
> +	int err;
> +
>  	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);
> +	if (IS_ERR(pll_x_clk)) {
> +		err = PTR_ERR(pll_x_clk);
> +		goto put_cpu;
> +	}
>  
>  	pll_p_clk = clk_get_sys(NULL, "pll_p");
> -	if (IS_ERR(pll_p_clk))
> -		return PTR_ERR(pll_p_clk);
> +	if (IS_ERR(pll_p_clk)) {
> +		err = PTR_ERR(pll_p_clk);
> +		goto put_pll_x;
> +	}
> +
> +	err = cpufreq_register_driver(&tegra_cpufreq_driver);
> +	if (err)
> +		goto put_pll_p;
> +
> +	return 0;
> +
> +put_pll_p:
> +	clk_put(pll_p_clk);
> +put_pll_x:
> +	clk_put(pll_x_clk);
> +put_cpu:
> +	clk_put(cpu_clk);
>  
> -	return cpufreq_register_driver(&tegra_cpufreq_driver);
> +	return err;
>  }
>  
>  static void __exit tegra_cpufreq_exit(void)
>  {
>  	cpufreq_unregister_driver(&tegra_cpufreq_driver);
> +	clk_put(pll_p_clk);
> +	clk_put(pll_x_clk);
>  	clk_put(cpu_clk);
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 05/11] cpufreq: tegra20: Clean up included headers

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Remove unused/unneeded headers and sort them in the alphabet order.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 69f033d297e1..61f00d1cba26 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -16,16 +16,9 @@
>   *
>   */
>  
> -#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/cpufreq.h>
> +#include <linux/module.h>

Ideally you should keep all the headers whose declarations your code
is using directly. It may happen that removing above still compiles
because cpufreq.h has included the headers indirectly for you. But
that will break the day cpufreq.h doesn't need those headers anymore.

So just make sure you aren't using any of them in your code. For
example you are using bool in your code and so you shouldn't remove
types.h ? Same for init.h as you are using __init and __exit.

>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
> -- 
> 2.17.0

-- 
viresh

Re: [PATCH v1 06/11] cpufreq: tegra20: Remove unneeded check in tegra_cpu_init

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Remove checking of the CPU number for consistency as it won't ever fail
> unless there is a severe bug in the cpufreq core.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 61f00d1cba26..147ae3e14f18 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -32,8 +32,6 @@ static struct cpufreq_frequency_table freq_table[] = {
>  	{ .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;
> @@ -119,9 +117,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
>  {
>  	int ret;
>  
> -	if (policy->cpu >= NUM_CPUS)
> -		return -EINVAL;
> -
>  	clk_prepare_enable(cpu_clk);
>  
>  	/* FIXME: what's the actual transition time? */

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 07/11] cpufreq: tegra20: Check if this is Tegra20 machine

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Don't even try to request the clocks during of module initialization on
> non-Tegra20 machines (this is the case for a multi-platform kernel) for
> consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 147ae3e14f18..797c61c74b65 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -19,6 +19,7 @@
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
> @@ -155,6 +156,9 @@ static int __init tegra_cpufreq_init(void)
>  {
>  	int err;
>  
> +	if (!of_machine_is_compatible("nvidia,tegra20"))
> +		return -ENODEV;
> +
>  	cpu_clk = clk_get_sys(NULL, "cclk");
>  	if (IS_ERR(cpu_clk))
>  		return PTR_ERR(cpu_clk);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 08/11] cpufreq: tegra20: Remove unneeded variable initialization

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Remove unneeded variable initialization solely for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index 797c61c74b65..c0a7b5a78aa6 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -84,7 +84,7 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>  {
>  	unsigned long rate = freq_table[index].frequency;
>  	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
> -	int ret = 0;
> +	int ret;
>  
>  	/*
>  	 * target freq == pll_p, don't need to take extra reference to pll_x_clk

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 09/11] cpufreq: tegra20: Allow cpufreq driver to be built as loadable module

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Nothing prevents Tegra20 CPUFreq module to be unloaded, hence allow it to
> be built as a non-builtin kernel module.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/Kconfig.arm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 96b35b8b3606..a8a2e210c624 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -264,7 +264,7 @@ config ARM_TANGO_CPUFREQ
>  	default y
>  
>  config ARM_TEGRA20_CPUFREQ
> -	bool "Tegra20 CPUFreq support"
> +	tristate "Tegra20 CPUFreq support"
>  	depends on ARCH_TEGRA
>  	default y
>  	help

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> +	struct tegra20_cpufreq_data *data;
>  	int err;
>  
> -	if (!of_machine_is_compatible("nvidia,tegra20"))
> -		return -ENODEV;

So this stuff wasn't really required as you are getting rid of that in
the same series. Should we really add it then ? Maybe ..

> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>  
> -	cpu_clk = clk_get_sys(NULL, "cclk");
> -	if (IS_ERR(cpu_clk))
> -		return PTR_ERR(cpu_clk);
> +	data->cpu_clk = clk_get_sys(NULL, "cclk");
> +	if (IS_ERR(data->cpu_clk))
> +		return PTR_ERR(data->cpu_clk);
>  
> -	pll_x_clk = clk_get_sys(NULL, "pll_x");
> -	if (IS_ERR(pll_x_clk)) {
> -		err = PTR_ERR(pll_x_clk);
> +	data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> +	if (IS_ERR(data->pll_x_clk)) {
> +		err = PTR_ERR(data->pll_x_clk);
>  		goto put_cpu;
>  	}
>  
> -	pll_p_clk = clk_get_sys(NULL, "pll_p");
> -	if (IS_ERR(pll_p_clk)) {
> -		err = PTR_ERR(pll_p_clk);
> +	data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> +	if (IS_ERR(data->pll_p_clk)) {
> +		err = PTR_ERR(data->pll_p_clk);
>  		goto put_pll_x;
>  	}
>  
> +	data->dev = &pdev->dev;
> +
> +	tegra_cpufreq_driver.driver_data = data;
> +
>  	err = cpufreq_register_driver(&tegra_cpufreq_driver);
>  	if (err)
>  		goto put_pll_p;
>  
> +	platform_set_drvdata(pdev, data);
> +
>  	return 0;
>  
>  put_pll_p:
> -	clk_put(pll_p_clk);
> +	clk_put(data->pll_p_clk);
>  put_pll_x:
> -	clk_put(pll_x_clk);
> +	clk_put(data->pll_x_clk);
>  put_cpu:
> -	clk_put(cpu_clk);
> +	clk_put(data->cpu_clk);
>  
>  	return err;
>  }
>  
> -static void __exit tegra_cpufreq_exit(void)
> +static int tegra20_cpufreq_remove(struct platform_device *pdev)
>  {
> +	struct tegra20_cpufreq_data *data = platform_get_drvdata(pdev);
> +
>  	cpufreq_unregister_driver(&tegra_cpufreq_driver);
> -	clk_put(pll_p_clk);
> -	clk_put(pll_x_clk);
> -	clk_put(cpu_clk);
> +
> +	clk_put(data->pll_p_clk);
> +	clk_put(data->pll_x_clk);
> +	clk_put(data->cpu_clk);
> +
> +	return 0;
>  }
>  
> +static struct platform_driver tegra20_cpufreq_driver = {
> +	.probe		= tegra20_cpufreq_probe,
> +	.remove		= tegra20_cpufreq_remove,
> +	.driver		= {
> +		.name	= "tegra20-cpufreq",
> +	},
> +};
> +module_platform_driver(tegra20_cpufreq_driver);
> +
>  MODULE_AUTHOR("Colin Cross <ccross@android.com>");
> +MODULE_ALIAS("platform:tegra20-cpufreq");
>  MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
>  MODULE_LICENSE("GPL");
> -module_init(tegra_cpufreq_init);
> -module_exit(tegra_cpufreq_exit);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 11/11] ARM: tegra: Create platform device for tegra20-cpufreq driver

[Viresh Kumar]

On 17-05-18, 21:00, Dmitry Osipenko wrote:
> Tegra20-cpufreq driver require a platform device in order to be loaded,
> instantiate a simple platform device for the driver during of the machines
> late initialization.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  arch/arm/mach-tegra/tegra.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
> index 02e712d2ea30..f9587be48235 100644
> --- a/arch/arm/mach-tegra/tegra.c
> +++ b/arch/arm/mach-tegra/tegra.c
> @@ -97,6 +97,10 @@ static void __init tegra_dt_init_late(void)
>  	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
>  	    of_machine_is_compatible("compal,paz00"))
>  		tegra_paz00_wifikill_init();
> +
> +	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
> +	    of_machine_is_compatible("nvidia,tegra20"))
> +		platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
>  }
>  
>  static const char * const tegra_dt_board_compat[] = {

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 00/11] Clean up Tegra20 cpufreq driver

[Rafael J. Wysocki]

On Thursday, May 17, 2018 8:00:45 PM CEST Dmitry Osipenko wrote:
> Hello,
> 
> Recently Peter Geis (who is working on Tegra30 cpufreq driver) asked me how
> tegra20-cpufreq driver is getting loaded. After taking a look at the code
> it became apparent that the drivers code has been rusted a tad and so this
> series is intended to refresh the drivers code by disallowing module to be
> loaded on non-Tegra20 machines, by cleaning whitespaces in the code, removing
> dead EMC code and in the end by allowing tegra20-cpufreq to be built as a
> loadable module.
> 
> Please review, thanks.
> 
> Dmitry Osipenko (11):
>   cpufreq: tegra20: Change module description
>   cpufreq: tegra20: Clean up whitespaces in the code
>   cpufreq: tegra20: Remove EMC clock usage
>   cpufreq: tegra20: Release clocks properly
>   cpufreq: tegra20: Clean up included headers
>   cpufreq: tegra20: Remove unneeded check in tegra_cpu_init
>   cpufreq: tegra20: Check if this is Tegra20 machine
>   cpufreq: tegra20: Remove unneeded variable initialization
>   cpufreq: tegra20: Allow cpufreq driver to be built as loadable module
>   cpufreq: tegra20: Wrap cpufreq into platform driver
>   ARM: tegra: Create platform device for tegra20-cpufreq driver
> 
>  arch/arm/mach-tegra/tegra.c       |   4 +
>  drivers/cpufreq/Kconfig.arm       |   2 +-
>  drivers/cpufreq/tegra20-cpufreq.c | 172 +++++++++++++++++-------------
>  3 files changed, 101 insertions(+), 77 deletions(-)
> 
> 

It looks like Viresh has ACKed the majority of the patches in this series,
but there are a few where he had comments.

Please fix up these and resend the entire series with the Acked-by tags from
Viresh added where applicable.

Thanks,
Rafael

Re: [PATCH v1 05/11] cpufreq: tegra20: Clean up included headers

[Dmitry Osipenko]

On 18.05.2018 04:58, Viresh Kumar wrote:
> On 17-05-18, 21:00, Dmitry Osipenko wrote:
>> Remove unused/unneeded headers and sort them in the alphabet order.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/cpufreq/tegra20-cpufreq.c | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>> index 69f033d297e1..61f00d1cba26 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -16,16 +16,9 @@
>>   *
>>   */
>>  
>> -#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/cpufreq.h>
>> +#include <linux/module.h>
> 
> Ideally you should keep all the headers whose declarations your code
> is using directly. It may happen that removing above still compiles
> because cpufreq.h has included the headers indirectly for you. But
> that will break the day cpufreq.h doesn't need those headers anymore.
> 
> So just make sure you aren't using any of them in your code. For
> example you are using bool in your code and so you shouldn't remove
> types.h ? Same for init.h as you are using __init and __exit.

The preference on includes seems to vary among maintainers. I've seen other
opinion that encouraged to minimize included headers and only add the headers
when compilation breaks.

I'll revisit this patch and keep init.h and others in v2 since you prefer that
way. Thank you for the review.

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Dmitry Osipenko]

On 18.05.2018 05:07, Viresh Kumar wrote:
> On 17-05-18, 21:00, Dmitry Osipenko wrote:
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +	struct tegra20_cpufreq_data *data;
>>  	int err;
>>  
>> -	if (!of_machine_is_compatible("nvidia,tegra20"))
>> -		return -ENODEV;
> 
> So this stuff wasn't really required as you are getting rid of that in
> the same series. Should we really add it then ? Maybe ..
> 

It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
kinda shows the way that led to the final result.

[snip]

Re: [PATCH v1 00/11] Clean up Tegra20 cpufreq driver

[Dmitry Osipenko]

On 18.05.2018 10:30, Rafael J. Wysocki wrote:
> On Thursday, May 17, 2018 8:00:45 PM CEST Dmitry Osipenko wrote:
>> Hello,
>>
>> Recently Peter Geis (who is working on Tegra30 cpufreq driver) asked me how
>> tegra20-cpufreq driver is getting loaded. After taking a look at the code
>> it became apparent that the drivers code has been rusted a tad and so this
>> series is intended to refresh the drivers code by disallowing module to be
>> loaded on non-Tegra20 machines, by cleaning whitespaces in the code, removing
>> dead EMC code and in the end by allowing tegra20-cpufreq to be built as a
>> loadable module.
>>
>> Please review, thanks.
>>
>> Dmitry Osipenko (11):
>>   cpufreq: tegra20: Change module description
>>   cpufreq: tegra20: Clean up whitespaces in the code
>>   cpufreq: tegra20: Remove EMC clock usage
>>   cpufreq: tegra20: Release clocks properly
>>   cpufreq: tegra20: Clean up included headers
>>   cpufreq: tegra20: Remove unneeded check in tegra_cpu_init
>>   cpufreq: tegra20: Check if this is Tegra20 machine
>>   cpufreq: tegra20: Remove unneeded variable initialization
>>   cpufreq: tegra20: Allow cpufreq driver to be built as loadable module
>>   cpufreq: tegra20: Wrap cpufreq into platform driver
>>   ARM: tegra: Create platform device for tegra20-cpufreq driver
>>
>>  arch/arm/mach-tegra/tegra.c       |   4 +
>>  drivers/cpufreq/Kconfig.arm       |   2 +-
>>  drivers/cpufreq/tegra20-cpufreq.c | 172 +++++++++++++++++-------------
>>  3 files changed, 101 insertions(+), 77 deletions(-)
>>
>>
> 
> It looks like Viresh has ACKed the majority of the patches in this series,
> but there are a few where he had comments.
> 
> Please fix up these and resend the entire series with the Acked-by tags from
> Viresh added where applicable.

Sure, I'll address the Viresh's review comments in the v2 that I'll probably
prepare and send out later today. I'll also add another minor-cleanup patch to
the series that removes unnecessary brackets in the code.

Thierry / Jon, please let me if there is anything you disagree-with in the
series as I'd like to avoid re-sending multiple times.

Thanks.

Re: [PATCH v1 01/11] cpufreq: tegra20: Change module description

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

On Thu, May 17, 2018 at 09:00:46PM +0300, Dmitry Osipenko wrote:
> Change module description to be in line with the other Tegra drivers, just
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 02/11] cpufreq: tegra20: Clean up whitespaces in the code

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Thu, May 17, 2018 at 09:00:47PM +0300, Dmitry Osipenko wrote:
> Remove unneeded blank line and replace whitespaces with a tab in the code
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 03/11] cpufreq: tegra20: Remove EMC clock usage

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Thu, May 17, 2018 at 09:00:48PM +0300, Dmitry Osipenko wrote:
> The EMC driver has been gone 4 years ago, since the commit a7cbe92cef27
> ("ARM: tegra: remove tegra EMC scaling driver"). Remove the EMC clock
> usage as it does nothing. We may consider re-implementing the EMC scaling
> later, probably using PM Memory Bandwidth QoS API.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 22 ----------------------
>  1 file changed, 22 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 04/11] cpufreq: tegra20: Release clocks properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On Thu, May 17, 2018 at 09:00:49PM +0300, Dmitry Osipenko wrote:
> Properly put requested clocks in the module init/exit code.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 05/11] cpufreq: tegra20: Clean up included headers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

On Fri, May 18, 2018 at 11:05:58AM +0300, Dmitry Osipenko wrote:
> On 18.05.2018 04:58, Viresh Kumar wrote:
> > On 17-05-18, 21:00, Dmitry Osipenko wrote:
> >> Remove unused/unneeded headers and sort them in the alphabet order.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/cpufreq/tegra20-cpufreq.c | 11 ++---------
> >>  1 file changed, 2 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> >> index 69f033d297e1..61f00d1cba26 100644
> >> --- a/drivers/cpufreq/tegra20-cpufreq.c
> >> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> >> @@ -16,16 +16,9 @@
> >>   *
> >>   */
> >>  
> >> -#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/cpufreq.h>
> >> +#include <linux/module.h>
> > 
> > Ideally you should keep all the headers whose declarations your code
> > is using directly. It may happen that removing above still compiles
> > because cpufreq.h has included the headers indirectly for you. But
> > that will break the day cpufreq.h doesn't need those headers anymore.
> > 
> > So just make sure you aren't using any of them in your code. For
> > example you are using bool in your code and so you shouldn't remove
> > types.h ? Same for init.h as you are using __init and __exit.
> 
> The preference on includes seems to vary among maintainers. I've seen other
> opinion that encouraged to minimize included headers and only add the headers
> when compilation breaks.

I think this probably depends on the type of include. For headers in
include/linux the reasoning is that they can change often as part of
some rework and this has in the past caused unrelated files to break
builds because suddenly a file no longer has all required definitions
available.

It's slightly different for header files that have a narrower exposure
because the chances of breaking things randomly are much lower.

> I'll revisit this patch and keep init.h and others in v2 since you prefer that
> way. Thank you for the review.

With those changes, this is:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 06/11] cpufreq: tegra20: Remove unneeded check in tegra_cpu_init

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

On Thu, May 17, 2018 at 09:00:51PM +0300, Dmitry Osipenko wrote:
> Remove checking of the CPU number for consistency as it won't ever fail
> unless there is a severe bug in the cpufreq core.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 5 -----
>  1 file changed, 5 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 07/11] cpufreq: tegra20: Check if this is Tegra20 machine

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Thu, May 17, 2018 at 09:00:52PM +0300, Dmitry Osipenko wrote:
> Don't even try to request the clocks during of module initialization on
> non-Tegra20 machines (this is the case for a multi-platform kernel) for
> consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 08/11] cpufreq: tegra20: Remove unneeded variable initialization

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Thu, May 17, 2018 at 09:00:53PM +0300, Dmitry Osipenko wrote:
> Remove unneeded variable initialization solely for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 09/11] cpufreq: tegra20: Allow cpufreq driver to be built as loadable module

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On Thu, May 17, 2018 at 09:00:54PM +0300, Dmitry Osipenko wrote:
> Nothing prevents Tegra20 CPUFreq module to be unloaded, hence allow it to
> be built as a non-builtin kernel module.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/Kconfig.arm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Viresh Kumar]

On 18-05-18, 11:09, Dmitry Osipenko wrote:
> On 18.05.2018 05:07, Viresh Kumar wrote:
> > On 17-05-18, 21:00, Dmitry Osipenko wrote:
> >> -static int __init tegra_cpufreq_init(void)
> >> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
> >>  {
> >> +	struct tegra20_cpufreq_data *data;
> >>  	int err;
> >>  
> >> -	if (!of_machine_is_compatible("nvidia,tegra20"))
> >> -		return -ENODEV;
> > 
> > So this stuff wasn't really required as you are getting rid of that in
> > the same series. Should we really add it then ? Maybe ..
> > 
> 
> It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
> kinda shows the way that led to the final result.

Okay.

-- 
viresh

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
> there is no device associated with the module, this is one of two patches
> that resolves the module autoloading. This patch adds a module alias that
> will associate the tegra20-cpufreq kernel module with the platform device,
> other patch will instantiate the actual platform device. And now it makes
> sense to wrap cpufreq driver into a platform driver for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index c0a7b5a78aa6..f9d02a28df9e 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -19,7 +19,7 @@
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/platform_device.h>
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = 216000 },
> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>  	{ .frequency = CPUFREQ_TABLE_END },
>  };
>  
> -static struct clk *cpu_clk;
> -static struct clk *pll_x_clk;
> -static struct clk *pll_p_clk;
> -static bool pll_x_prepared;
> +struct tegra20_cpufreq_data {

Nit: I'm not a big fan of _data suffixes because they are completely
redundant. Any data structure by definition hosts data, so I'd just drop
that.

[...]
> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>  	.suspend		= cpufreq_generic_suspend,
>  };
>  
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> +	struct tegra20_cpufreq_data *data;
>  	int err;
>  
> -	if (!of_machine_is_compatible("nvidia,tegra20"))
> -		return -ENODEV;
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>  
> -	cpu_clk = clk_get_sys(NULL, "cclk");
> -	if (IS_ERR(cpu_clk))
> -		return PTR_ERR(cpu_clk);
> +	data->cpu_clk = clk_get_sys(NULL, "cclk");
> +	if (IS_ERR(data->cpu_clk))
> +		return PTR_ERR(data->cpu_clk);
>  
> -	pll_x_clk = clk_get_sys(NULL, "pll_x");
> -	if (IS_ERR(pll_x_clk)) {
> -		err = PTR_ERR(pll_x_clk);
> +	data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> +	if (IS_ERR(data->pll_x_clk)) {
> +		err = PTR_ERR(data->pll_x_clk);
>  		goto put_cpu;
>  	}
>  
> -	pll_p_clk = clk_get_sys(NULL, "pll_p");
> -	if (IS_ERR(pll_p_clk)) {
> -		err = PTR_ERR(pll_p_clk);
> +	data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> +	if (IS_ERR(data->pll_p_clk)) {
> +		err = PTR_ERR(data->pll_p_clk);
>  		goto put_pll_x;
>  	}
>  
> +	data->dev = &pdev->dev;
> +
> +	tegra_cpufreq_driver.driver_data = data;

Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
everything but this into a per-device data structure seems half-baked.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 11/11] ARM: tegra: Create platform device for tegra20-cpufreq driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Thu, May 17, 2018 at 09:00:56PM +0300, Dmitry Osipenko wrote:
> Tegra20-cpufreq driver require a platform device in order to be loaded,
> instantiate a simple platform device for the driver during of the machines
> late initialization.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  arch/arm/mach-tegra/tegra.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
> index 02e712d2ea30..f9587be48235 100644
> --- a/arch/arm/mach-tegra/tegra.c
> +++ b/arch/arm/mach-tegra/tegra.c
> @@ -97,6 +97,10 @@ static void __init tegra_dt_init_late(void)
>  	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
>  	    of_machine_is_compatible("compal,paz00"))
>  		tegra_paz00_wifikill_init();
> +
> +	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
> +	    of_machine_is_compatible("nvidia,tegra20"))
> +		platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
>  }
>  
>  static const char * const tegra_dt_board_compat[] = {

Tegra124 has a CPU frequency driver that is similar to this and it
contains code that will instantiate the platform device from the CPU
frequency driver's module_init function.

I think the primary reason for doing that was to not tie the code to
32-bit ARM, even though it never runs on anything but that, so it's
slightly over-engineered.

I don't mind either way, and it's easy enough to change this to
something else later on if we want. I'll pick this up into the Tegra
tree.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

[Dmitry Osipenko]

On 18.05.2018 12:07, Thierry Reding wrote:
> On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
>> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
>> there is no device associated with the module, this is one of two patches
>> that resolves the module autoloading. This patch adds a module alias that
>> will associate the tegra20-cpufreq kernel module with the platform device,
>> other patch will instantiate the actual platform device. And now it makes
>> sense to wrap cpufreq driver into a platform driver for consistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++++++++++++++++++-----------
>>  1 file changed, 73 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>> index c0a7b5a78aa6..f9d02a28df9e 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/cpufreq.h>
>>  #include <linux/module.h>
>> -#include <linux/of.h>
>> +#include <linux/platform_device.h>
>>  
>>  static struct cpufreq_frequency_table freq_table[] = {
>>  	{ .frequency = 216000 },
>> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>>  	{ .frequency = CPUFREQ_TABLE_END },
>>  };
>>  
>> -static struct clk *cpu_clk;
>> -static struct clk *pll_x_clk;
>> -static struct clk *pll_p_clk;
>> -static bool pll_x_prepared;
>> +struct tegra20_cpufreq_data {
> 
> Nit: I'm not a big fan of _data suffixes because they are completely
> redundant. Any data structure by definition hosts data, so I'd just drop
> that.

Okay, I'll drop it in v2.

> [...]
>> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>>  	.suspend		= cpufreq_generic_suspend,
>>  };
>>  
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +	struct tegra20_cpufreq_data *data;
>>  	int err;
>>  
>> -	if (!of_machine_is_compatible("nvidia,tegra20"))
>> -		return -ENODEV;
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>>  
>> -	cpu_clk = clk_get_sys(NULL, "cclk");
>> -	if (IS_ERR(cpu_clk))
>> -		return PTR_ERR(cpu_clk);
>> +	data->cpu_clk = clk_get_sys(NULL, "cclk");
>> +	if (IS_ERR(data->cpu_clk))
>> +		return PTR_ERR(data->cpu_clk);
>>  
>> -	pll_x_clk = clk_get_sys(NULL, "pll_x");
>> -	if (IS_ERR(pll_x_clk)) {
>> -		err = PTR_ERR(pll_x_clk);
>> +	data->pll_x_clk = clk_get_sys(NULL, "pll_x");
>> +	if (IS_ERR(data->pll_x_clk)) {
>> +		err = PTR_ERR(data->pll_x_clk);
>>  		goto put_cpu;
>>  	}
>>  
>> -	pll_p_clk = clk_get_sys(NULL, "pll_p");
>> -	if (IS_ERR(pll_p_clk)) {
>> -		err = PTR_ERR(pll_p_clk);
>> +	data->pll_p_clk = clk_get_sys(NULL, "pll_p");
>> +	if (IS_ERR(data->pll_p_clk)) {
>> +		err = PTR_ERR(data->pll_p_clk);
>>  		goto put_pll_x;
>>  	}
>>  
>> +	data->dev = &pdev->dev;
>> +
>> +	tegra_cpufreq_driver.driver_data = data;
> 
> Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
> everything but this into a per-device data structure seems half-baked.

That's a good suggestions, thank you.

Re: [PATCH v1 11/11] ARM: tegra: Create platform device for tegra20-cpufreq driver

[Dmitry Osipenko]

On 18.05.2018 12:13, Thierry Reding wrote:
> On Thu, May 17, 2018 at 09:00:56PM +0300, Dmitry Osipenko wrote:
>> Tegra20-cpufreq driver require a platform device in order to be loaded,
>> instantiate a simple platform device for the driver during of the machines
>> late initialization.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  arch/arm/mach-tegra/tegra.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
>> index 02e712d2ea30..f9587be48235 100644
>> --- a/arch/arm/mach-tegra/tegra.c
>> +++ b/arch/arm/mach-tegra/tegra.c
>> @@ -97,6 +97,10 @@ static void __init tegra_dt_init_late(void)
>>  	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
>>  	    of_machine_is_compatible("compal,paz00"))
>>  		tegra_paz00_wifikill_init();
>> +
>> +	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
>> +	    of_machine_is_compatible("nvidia,tegra20"))
>> +		platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
>>  }
>>  
>>  static const char * const tegra_dt_board_compat[] = {
> 
> Tegra124 has a CPU frequency driver that is similar to this and it
> contains code that will instantiate the platform device from the CPU
> frequency driver's module_init function.
> 
> I think the primary reason for doing that was to not tie the code to
> 32-bit ARM, even though it never runs on anything but that, so it's
> slightly over-engineered.

The tegra124-cpufreq driver likely to be broken in regards to the module loading
as platform_device_register_simple() seem to be allowed invoked only from the
kernel itself.

Having platform_device_register_simple in the kernels module leads to a such
errors on the module loading:

# section 5 reloc 7 sym 'memset': relocation 10 out of range (0xbf805030 ->
0xc088c481)

Initially I wanted to make tegra124-cpufreq a module_platform_driver, but
decided to postpone that, as the driver is shared with ARM64 Tegra132 which
doesn't have machine-init code. Maybe we could move the devices instantiation to
something common like driver/soc/tegra/devices.c later.

> I don't mind either way, and it's easy enough to change this to
> something else later on if we want. I'll pick this up into the Tegra
> tree.
Okay, thanks.