[PATCH V2 0/3] cpufreq: dt: Create platform device from generic code

[PATCH V2 0/3] cpufreq: dt: Create platform device from generic code

[Viresh Kumar]

Multiple platforms are using the generic cpufreq-dt driver now, and all
of them are required to create a platform device with name "cpufreq-dt",
in order to get the cpufreq-dt probed.

Many of them do it from platform code, others have special drivers just
to do that.

It would be more sensible to do this at a generic place, where all such
platform can mark their entries.

The first patch fixes an issue that becomes visible only after the
second patch is applied. The second one creates a new driver to create
platform-device based on current platform and the last one converts
exynos platform to use this common infrastructure.

I will migrate rest of the platforms after this is accepted as the right
way ahead.

V1->V2:
- Updated 3/3 to rebase on top of latest exynos changes
- Reviewed-by tags added

Viresh Kumar (3):
  cpufreq: dt: Include types.h from cpufreq-dt.h
  cpufreq: dt: Add generic platform-device creation support
  cpufreq: exynos: Use generic platdev driver

 arch/arm/mach-exynos/exynos.c        | 29 ------------------
 drivers/cpufreq/Kconfig              | 11 +++++++
 drivers/cpufreq/Makefile             |  1 +
 drivers/cpufreq/cpufreq-dt-platdev.c | 57 ++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq-dt.h           |  2 ++
 5 files changed, 71 insertions(+), 29 deletions(-)
 create mode 100644 drivers/cpufreq/cpufreq-dt-platdev.c

-- 
2.7.1.410.g6faf27b

[PATCH V1 Resend 1/3] cpufreq: dt: Include types.h from cpufreq-dt.h

[Viresh Kumar]

cpufreq-dt.h uses 'bool' data type but doesn't include types.h. It works
fine for now as the files that include cpufreq-dt.h, also include
types.h directly or indirectly.

But, when a file includes cpufreq-dt.h without including types.h, we get
a build error. Avoid such errors by including types.h in cpufreq-dt
itself.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/cpufreq-dt.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
index 0414009e2c30..a87335a1660c 100644
--- a/include/linux/cpufreq-dt.h
+++ b/include/linux/cpufreq-dt.h
@@ -10,6 +10,8 @@
 #ifndef __CPUFREQ_DT_H__
 #define __CPUFREQ_DT_H__
 
+#include <linux/types.h>
+
 struct cpufreq_dt_platform_data {
 	/*
 	 * True when each CPU has its own clock to control its
-- 
2.7.1.410.g6faf27b

[PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

Multiple platforms are using the generic cpufreq-dt driver now, and all
of them are required to create a platform device with name "cpufreq-dt",
in order to get the cpufreq-dt probed.

Many of them do it from platform code, others have special drivers just
to do that.

It would be more sensible to do this at a generic place, where all such
platform can mark their entries.

This patch adds a separate file to get this device created. Currently
the compat list of platforms that we support is empty, and will be
filled in as and when we move platforms to use it.

It always compiles as part of the kernel and so doesn't need a
module-exit operation.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/cpufreq/Kconfig              | 11 +++++++++
 drivers/cpufreq/Makefile             |  1 +
 drivers/cpufreq/cpufreq-dt-platdev.c | 48 ++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)
 create mode 100644 drivers/cpufreq/cpufreq-dt-platdev.c

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index a7f45853c103..08573d54105b 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -191,6 +191,7 @@ config CPUFREQ_DT
 	depends on HAVE_CLK && OF
 	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
 	depends on !CPU_THERMAL || THERMAL
+	select CPUFREQ_DT_PLATDEV
 	select PM_OPP
 	help
 	  This adds a generic DT based cpufreq driver for frequency management.
@@ -199,6 +200,16 @@ config CPUFREQ_DT
 
 	  If in doubt, say N.
 
+config CPUFREQ_DT_PLATDEV
+	bool
+	depends on CPUFREQ_DT
+	help
+	  This adds a generic DT based cpufreq platdev driver for frequency
+	  management.  This creates a 'cpufreq-dt' platform device, on the
+	  supported platforms.
+
+	  If in doubt, say N.
+
 if X86
 source "drivers/cpufreq/Kconfig.x86"
 endif
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 9e63fb1b09f8..b9224fdb8322 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
 obj-$(CONFIG_CPU_FREQ_GOV_COMMON)		+= cpufreq_governor.o
 
 obj-$(CONFIG_CPUFREQ_DT)		+= cpufreq-dt.o
+obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 
 ##################################################################################
 # x86 drivers.
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
new file mode 100644
index 000000000000..18b81724ca0b
--- /dev/null
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2016 Linaro.
+ * Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/cpufreq-dt.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct cpufreq_dt_compat {
+	const char *compatible;
+	const void *data;
+	size_t size;
+};
+
+static struct cpufreq_dt_compat compat[] = {
+};
+
+static int __init cpufreq_dt_platdev_init(void)
+{
+	struct platform_device *pdev;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(compat); i++) {
+		if (!of_machine_is_compatible(compat[i].compatible))
+			continue;
+
+		pdev = platform_device_register_data(NULL, "cpufreq-dt", -1,
+						     compat[i].data,
+						     compat[i].size);
+
+		return PTR_ERR_OR_ZERO(pdev);
+	}
+
+	return -ENODEV;
+}
+module_init(cpufreq_dt_platdev_init);
+
+MODULE_ALIAS("cpufreq-dt-platdev");
+MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.org>");
+MODULE_DESCRIPTION("cpufreq-dt platdev driver");
+MODULE_LICENSE("GPL");
-- 
2.7.1.410.g6faf27b

[PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Viresh Kumar]

The cpufreq-dt-platdev driver supports creation of cpufreq-dt platform
device now, reuse that and remove similar code from platform code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/mach-exynos/exynos.c        | 29 -----------------------------
 drivers/cpufreq/cpufreq-dt-platdev.c |  9 +++++++++
 2 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index bbf51a46f772..4d3b056fd786 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -213,33 +213,6 @@ static void __init exynos_init_irq(void)
 	exynos_map_pmu();
 }
 
-static const struct of_device_id exynos_cpufreq_matches[] = {
-	{ .compatible = "samsung,exynos3250", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4210", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4212", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4412", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos5250", .data = "cpufreq-dt" },
-#ifndef CONFIG_BL_SWITCHER
-	{ .compatible = "samsung,exynos5420", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos5800", .data = "cpufreq-dt" },
-#endif
-	{ /* sentinel */ }
-};
-
-static void __init exynos_cpufreq_init(void)
-{
-	struct device_node *root = of_find_node_by_path("/");
-	const struct of_device_id *match;
-
-	match = of_match_node(exynos_cpufreq_matches, root);
-	if (!match) {
-		platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
-		return;
-	}
-
-	platform_device_register_simple(match->data, -1, NULL, 0);
-}
-
 static void __init exynos_dt_machine_init(void)
 {
 	/*
@@ -262,8 +235,6 @@ static void __init exynos_dt_machine_init(void)
 	    of_machine_is_compatible("samsung,exynos5250"))
 		platform_device_register(&exynos_cpuidle);
 
-	exynos_cpufreq_init();
-
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 18b81724ca0b..c5a70b683c7c 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -20,6 +20,15 @@ struct cpufreq_dt_compat {
 };
 
 static struct cpufreq_dt_compat compat[] = {
+	{ "samsung,exynos3250",		NULL,		0 },
+	{ "samsung,exynos4210",		NULL,		0 },
+	{ "samsung,exynos4212",		NULL,		0 },
+	{ "samsung,exynos4412",		NULL,		0 },
+	{ "samsung,exynos5250",		NULL,		0 },
+#ifndef CONFIG_BL_SWITCHER
+	{ "samsung,exynos5420",		NULL,		0 },
+	{ "samsung,exynos5800",		NULL,		0 },
+#endif
 };
 
 static int __init cpufreq_dt_platdev_init(void)
-- 
2.7.1.410.g6faf27b

[PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Viresh Kumar]

The cpufreq-dt-platdev driver supports creation of cpufreq-dt platform
device now, reuse that and remove similar code from platform code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/mach-exynos/exynos.c        | 29 -----------------------------
 drivers/cpufreq/cpufreq-dt-platdev.c |  9 +++++++++
 2 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index bbf51a46f772..4d3b056fd786 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -213,33 +213,6 @@ static void __init exynos_init_irq(void)
 	exynos_map_pmu();
 }
 
-static const struct of_device_id exynos_cpufreq_matches[] = {
-	{ .compatible = "samsung,exynos3250", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4210", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4212", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos4412", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos5250", .data = "cpufreq-dt" },
-#ifndef CONFIG_BL_SWITCHER
-	{ .compatible = "samsung,exynos5420", .data = "cpufreq-dt" },
-	{ .compatible = "samsung,exynos5800", .data = "cpufreq-dt" },
-#endif
-	{ /* sentinel */ }
-};
-
-static void __init exynos_cpufreq_init(void)
-{
-	struct device_node *root = of_find_node_by_path("/");
-	const struct of_device_id *match;
-
-	match = of_match_node(exynos_cpufreq_matches, root);
-	if (!match) {
-		platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
-		return;
-	}
-
-	platform_device_register_simple(match->data, -1, NULL, 0);
-}
-
 static void __init exynos_dt_machine_init(void)
 {
 	/*
@@ -262,8 +235,6 @@ static void __init exynos_dt_machine_init(void)
 	    of_machine_is_compatible("samsung,exynos5250"))
 		platform_device_register(&exynos_cpuidle);
 
-	exynos_cpufreq_init();
-
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 18b81724ca0b..c5a70b683c7c 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -20,6 +20,15 @@ struct cpufreq_dt_compat {
 };
 
 static struct cpufreq_dt_compat compat[] = {
+	{ "samsung,exynos3250",		NULL,		0 },
+	{ "samsung,exynos4210",		NULL,		0 },
+	{ "samsung,exynos4212",		NULL,		0 },
+	{ "samsung,exynos4412",		NULL,		0 },
+	{ "samsung,exynos5250",		NULL,		0 },
+#ifndef CONFIG_BL_SWITCHER
+	{ "samsung,exynos5420",		NULL,		0 },
+	{ "samsung,exynos5800",		NULL,		0 },
+#endif
 };
 
 static int __init cpufreq_dt_platdev_init(void)
-- 
2.7.1.410.g6faf27b

Re: [PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Arnd Bergmann]

On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
> -static void __init exynos_cpufreq_init(void)
> -{
> -       struct device_node *root = of_find_node_by_path("/");
> -       const struct of_device_id *match;
> -
> -       match = of_match_node(exynos_cpufreq_matches, root);
> -       if (!match) {
> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
> -               return;
> -       }
> -
> -       platform_device_register_simple(match->data, -1, NULL, 0);
> -}

How is the "exynos-cpufreq" case handled now? Is that no longer used now?
I assume the patch is correct based on Krzysztof's review, but it might be
good to explain this better.

	Arnd

[PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Arnd Bergmann]

On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
> -static void __init exynos_cpufreq_init(void)
> -{
> -       struct device_node *root = of_find_node_by_path("/");
> -       const struct of_device_id *match;
> -
> -       match = of_match_node(exynos_cpufreq_matches, root);
> -       if (!match) {
> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
> -               return;
> -       }
> -
> -       platform_device_register_simple(match->data, -1, NULL, 0);
> -}

How is the "exynos-cpufreq" case handled now? Is that no longer used now?
I assume the patch is correct based on Krzysztof's review, but it might be
good to explain this better.

	Arnd

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Arnd Bergmann]

On Tuesday 29 March 2016 12:09:48 Viresh Kumar wrote:
> Multiple platforms are using the generic cpufreq-dt driver now, and all
> of them are required to create a platform device with name "cpufreq-dt",
> in order to get the cpufreq-dt probed.
> 
> Many of them do it from platform code, others have special drivers just
> to do that.
> 
> It would be more sensible to do this at a generic place, where all such
> platform can mark their entries.
> 
> This patch adds a separate file to get this device created. Currently
> the compat list of platforms that we support is empty, and will be
> filled in as and when we move platforms to use it.
> 
> It always compiles as part of the kernel and so doesn't need a
> module-exit operation.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/cpufreq/Kconfig              | 11 +++++++++
>  drivers/cpufreq/Makefile             |  1 +
>  drivers/cpufreq/cpufreq-dt-platdev.c | 48 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+)
>  create mode 100644 drivers/cpufreq/cpufreq-dt-platdev.c
> 
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index a7f45853c103..08573d54105b 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -191,6 +191,7 @@ config CPUFREQ_DT
>  	depends on HAVE_CLK && OF
>  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
>  	depends on !CPU_THERMAL || THERMAL
> +	select CPUFREQ_DT_PLATDEV
>  	select PM_OPP
>  	help
>  	  This adds a generic DT based cpufreq driver for frequency management.
> @@ -199,6 +200,16 @@ config CPUFREQ_DT
>  
>  	  If in doubt, say N.
>  
> +config CPUFREQ_DT_PLATDEV
> +	bool
> +	depends on CPUFREQ_DT

The 'depends on' line is redundant as you always 'select' the code
from CPUFREQ_DT. Since they are always set together, you can also
just drop the new symbol, or put the new code into the same file.

> +struct cpufreq_dt_compat {
> +	const char *compatible;
> +	const void *data;
> +	size_t size;
> +};
> +
> +static struct cpufreq_dt_compat compat[] = {
> +};

The 'data' here is alway 'struct cpufreq_dt_platform_data' or NULL,
and the size can be derived from that. If we add this into the opp
platform interfaces, both can go away.

> +static int __init cpufreq_dt_platdev_init(void)
> +{
> +	struct platform_device *pdev;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(compat); i++) {
> +		if (!of_machine_is_compatible(compat[i].compatible))
> +			continue;
> +
> +		pdev = platform_device_register_data(NULL, "cpufreq-dt", -1,
> +						     compat[i].data,
> +						     compat[i].size);
> +

and then this can become 

	if (of_device_match(of_root, compat))
		platform_device_register_simple(NULL, "cpufreq-dt", 0, NULL, 0);

	Arnd

Re: [PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Viresh Kumar]

On 29 March 2016 at 20:34, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
>> -static void __init exynos_cpufreq_init(void)
>> -{
>> -       struct device_node *root = of_find_node_by_path("/");
>> -       const struct of_device_id *match;
>> -
>> -       match = of_match_node(exynos_cpufreq_matches, root);
>> -       if (!match) {
>> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
>> -               return;
>> -       }
>> -
>> -       platform_device_register_simple(match->data, -1, NULL, 0);
>> -}
>
> How is the "exynos-cpufreq" case handled now? Is that no longer used now?
> I assume the patch is correct based on Krzysztof's review, but it might be
> good to explain this better.

That's stale code, the driver got removed some time back.

[PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Viresh Kumar]

On 29 March 2016 at 20:34, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
>> -static void __init exynos_cpufreq_init(void)
>> -{
>> -       struct device_node *root = of_find_node_by_path("/");
>> -       const struct of_device_id *match;
>> -
>> -       match = of_match_node(exynos_cpufreq_matches, root);
>> -       if (!match) {
>> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
>> -               return;
>> -       }
>> -
>> -       platform_device_register_simple(match->data, -1, NULL, 0);
>> -}
>
> How is the "exynos-cpufreq" case handled now? Is that no longer used now?
> I assume the patch is correct based on Krzysztof's review, but it might be
> good to explain this better.

That's stale code, the driver got removed some time back.

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

On 29-03-16, 17:14, Arnd Bergmann wrote:
> On Tuesday 29 March 2016 12:09:48 Viresh Kumar wrote:
> > +config CPUFREQ_DT_PLATDEV
> > +	bool
> > +	depends on CPUFREQ_DT
> 
> The 'depends on' line is redundant as you always 'select' the code
> from CPUFREQ_DT. Since they are always set together, you can also
> just drop the new symbol, or put the new code into the same file.

I would like to keep a separate file for this, it looks awkward to update the
driver for any new user platform.

I added a separate symbol to take care of the Modular case of cpufreq-dt. i.e. I
didn't wanted to add module_exit() code in the new file. Currently, it gets
included in the kernel even if cpufreq-dt is selected as a Module, so we just
create the platform-device just once and never touch it again.

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

On 29-03-16, 17:14, Arnd Bergmann wrote:
> 	if (of_device_match(of_root, compat))

You meant of_match_node() here, right?

> 		platform_device_register_simple(NULL, "cpufreq-dt", 0, NULL, 0);

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Arnd Bergmann]

On Tuesday 29 March 2016 22:06:49 Viresh Kumar wrote:
> On 29-03-16, 17:14, Arnd Bergmann wrote:
> > On Tuesday 29 March 2016 12:09:48 Viresh Kumar wrote:
> > > +config CPUFREQ_DT_PLATDEV
> > > +	bool
> > > +	depends on CPUFREQ_DT
> > 
> > The 'depends on' line is redundant as you always 'select' the code
> > from CPUFREQ_DT. Since they are always set together, you can also
> > just drop the new symbol, or put the new code into the same file.
> 
> I would like to keep a separate file for this, it looks awkward to update the
> driver for any new user platform.
>
> I added a separate symbol to take care of the Modular case of cpufreq-dt. i.e. I
> didn't wanted to add module_exit() code in the new file. Currently, it gets
> included in the kernel even if cpufreq-dt is selected as a Module, so we just
> create the platform-device just once and never touch it again.

Ok, that makes sense.

Regarding new platforms, I'd hope that we could manage to define an extension
to the oppv2 binding that marks a machine as compatible with opp, so we can
just look at that instead of the root compatible string. Your current series
is nice in the way it gets the hack outside of my code, but it would be ideal
if we could avoid propagating it further.

	Arnd

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Arnd Bergmann]

On Tuesday 29 March 2016 22:12:09 you wrote:
> On 29-03-16, 17:14, Arnd Bergmann wrote:
> >       if (of_device_match(of_root, compat))
> 
> You meant of_match_node() here, right?
> 

Yes, sorry for the confusion.

	Arnd

Re: [PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Krzysztof Kozlowski]

On 30.03.2016 00:22, Viresh Kumar wrote:
> On 29 March 2016 at 20:34, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
>>> -static void __init exynos_cpufreq_init(void)
>>> -{
>>> -       struct device_node *root = of_find_node_by_path("/");
>>> -       const struct of_device_id *match;
>>> -
>>> -       match = of_match_node(exynos_cpufreq_matches, root);
>>> -       if (!match) {
>>> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
>>> -               return;
>>> -       }
>>> -
>>> -       platform_device_register_simple(match->data, -1, NULL, 0);
>>> -}
>>
>> How is the "exynos-cpufreq" case handled now? Is that no longer used now?
>> I assume the patch is correct based on Krzysztof's review, but it might be
>> good to explain this better.
> 
> That's stale code, the driver got removed some time back.

Yeah, that's old driver, no longer existing. It makes sense to split the
patch - first remove the stale exynos-cpufreq and then move cpufreq-dt.
This would help avoid confusion when grepping through the history.

Best regards,
Krzysztof

[PATCH V2 3/3] cpufreq: exynos: Use generic platdev driver

[Krzysztof Kozlowski]

On 30.03.2016 00:22, Viresh Kumar wrote:
> On 29 March 2016 at 20:34, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:
>>> -static void __init exynos_cpufreq_init(void)
>>> -{
>>> -       struct device_node *root = of_find_node_by_path("/");
>>> -       const struct of_device_id *match;
>>> -
>>> -       match = of_match_node(exynos_cpufreq_matches, root);
>>> -       if (!match) {
>>> -               platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
>>> -               return;
>>> -       }
>>> -
>>> -       platform_device_register_simple(match->data, -1, NULL, 0);
>>> -}
>>
>> How is the "exynos-cpufreq" case handled now? Is that no longer used now?
>> I assume the patch is correct based on Krzysztof's review, but it might be
>> good to explain this better.
> 
> That's stale code, the driver got removed some time back.

Yeah, that's old driver, no longer existing. It makes sense to split the
patch - first remove the stale exynos-cpufreq and then move cpufreq-dt.
This would help avoid confusion when grepping through the history.

Best regards,
Krzysztof

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

On 29-03-16, 21:45, Arnd Bergmann wrote:
> Regarding new platforms, I'd hope that we could manage to define an extension
> to the oppv2 binding that marks a machine as compatible with opp, so we can

The extension of oppv2 binding or compatible string is for platforms that really
need extend oppv2 (like ST, which already have a new compatible string). I am
not sure if we should be putting that into the Machine's compatible area, as I
though it is something internal to OPP layer or their driver.

But we can see that when we handle it.

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Arnd Bergmann]

On Wednesday 30 March 2016 08:52:40 Viresh Kumar wrote:
> On 29-03-16, 21:45, Arnd Bergmann wrote:
> > Regarding new platforms, I'd hope that we could manage to define an extension
> > to the oppv2 binding that marks a machine as compatible with opp, so we can
> 
> The extension of oppv2 binding or compatible string is for platforms that really
> need extend oppv2 (like ST, which already have a new compatible string). I am
> not sure if we should be putting that into the Machine's compatible area, as I
> though it is something internal to OPP layer or their driver.
> 
> But we can see that when we handle it.
> 

I think it should be something in the /cpus or the /opp_table hierarchy,
not the root of the device tree, but other than that I don't care much
whether it's a variation of the oppv2 compatible string or an additional
property in any of the nodes.

	Arnd

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

Cc'ing Rob and Mason.

On 30-03-16, 09:53, Arnd Bergmann wrote:
> I think it should be something in the /cpus or the /opp_table hierarchy,
> not the root of the device tree, but other than that I don't care much
> whether it's a variation of the oppv2 compatible string or an additional
> property in any of the nodes.

So you mean for future DT files we can have something like this:

	cpus {
                compatible = "operation-points-v2";
		#address-cells = <1>;
		#size-cells = <0>;

		cpu@0 {
			compatible = "arm,cortex-a9";
			reg = <0>;
			next-level-cache = <&L2>;
			operating-points-v2 = <&cpu0_opp_table>;
		};
	};

	cpu0_opp_table: opp_table0 {
		opp@1000000000 {
			opp-hz = /bits/ 64 <1000000000>;
			opp-microvolt = <970000 975000 985000>;
			opp-microamp = <70000>;
			clock-latency-ns = <300000>;
			opp-suspend;
		};
		opp@1100000000 {
			opp-hz = /bits/ 64 <1100000000>;
			opp-microvolt = <980000 1000000 1010000>;
			opp-microamp = <80000>;
			clock-latency-ns = <310000>;
		};
	};
};


And the cpufreq-dt driver can match /cpus node's compatible string against
"operating-points-v2" and create a device at runtime ?

@Rob: Will that be acceptable to you? We are discussing (again) about how to
probe cpufreq-dt driver automatically for platforms :)

The cpus node doesn't have any 'compatible' property today, and I will be
required to add that in this case.

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Mason]

On 01/04/2016 12:23, Viresh Kumar wrote:

> Cc'ing Rob and Mason.
> 
> On 30-03-16, 09:53, Arnd Bergmann wrote:
>
>> I think it should be something in the /cpus or the /opp_table hierarchy,
>> not the root of the device tree, but other than that I don't care much
>> whether it's a variation of the oppv2 compatible string or an additional
>> property in any of the nodes.
> 
> So you mean for future DT files we can have something like this:
> 
> 	cpus {
>               compatible = "operation-points-v2";
>               [...]
> 
> And the cpufreq-dt driver can match /cpus node's compatible string against
> "operating-points-v2" and create a device at runtime ?

Hmmm... I'm using the older operating-points prop in my platform's DT.
Why can't we define a new property (e.g. "enable-generic-cpufreq")
which registers the "cpufreq-dt" pseudo-device?

And platforms that manually register "cpufreq-dt" would be
automatically white-listed, even if they don't have the new
property, to maintain backward-compat?

> @Rob: Will that be acceptable to you? We are discussing (again) about how to
> probe cpufreq-dt driver automatically for platforms :)
> 
> The cpus node doesn't have any 'compatible' property today, and I will be
> required to add that in this case.

Why does it need a compatible prop?
Why isn't a bool prop enough?

Regards.

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

On 01-04-16, 14:30, Mason wrote:
> Hmmm... I'm using the older operating-points prop in my platform's DT.
> Why can't we define a new property (e.g. "enable-generic-cpufreq")
> which registers the "cpufreq-dt" pseudo-device?

DT is all about expressing hardware in a file. The same bindings
should be usable across any operating system, not just Linux. And so
we shouldn't have any OS or software-implementation specific
properties here.

> And platforms that manually register "cpufreq-dt" would be
> automatically white-listed, even if they don't have the new
> property, to maintain backward-compat?

Its not about just making it work, otherwise we would have done it
long time back by creating a DT node for cpufreq-dt driver.

> > @Rob: Will that be acceptable to you? We are discussing (again) about how to
> > probe cpufreq-dt driver automatically for platforms :)
> > 
> > The cpus node doesn't have any 'compatible' property today, and I will be
> > required to add that in this case.
> 
> Why does it need a compatible prop?

That compatible property will describe how to parse/describe
operating-points for a CPU. Any driver, which has the ability to parse
those bindings can do things base on such a compatibility string.

I hope you got the idea.

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Rob Herring]

On Fri, Apr 1, 2016 at 5:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Cc'ing Rob and Mason.
>
> On 30-03-16, 09:53, Arnd Bergmann wrote:
>> I think it should be something in the /cpus or the /opp_table hierarchy,
>> not the root of the device tree, but other than that I don't care much
>> whether it's a variation of the oppv2 compatible string or an additional
>> property in any of the nodes.
>
> So you mean for future DT files we can have something like this:
>
>         cpus {
>                 compatible = "operation-points-v2";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 cpu@0 {
>                         compatible = "arm,cortex-a9";
>                         reg = <0>;
>                         next-level-cache = <&L2>;
>                         operating-points-v2 = <&cpu0_opp_table>;
>                 };
>         };
>
>         cpu0_opp_table: opp_table0 {
>                 opp@1000000000 {
>                         opp-hz = /bits/ 64 <1000000000>;
>                         opp-microvolt = <970000 975000 985000>;
>                         opp-microamp = <70000>;
>                         clock-latency-ns = <300000>;
>                         opp-suspend;
>                 };
>                 opp@1100000000 {
>                         opp-hz = /bits/ 64 <1100000000>;
>                         opp-microvolt = <980000 1000000 1010000>;
>                         opp-microamp = <80000>;
>                         clock-latency-ns = <310000>;
>                 };
>         };
> };
>
>
> And the cpufreq-dt driver can match /cpus node's compatible string against
> "operating-points-v2" and create a device at runtime ?
>
> @Rob: Will that be acceptable to you? We are discussing (again) about how to
> probe cpufreq-dt driver automatically for platforms :)

No, I don't think that belongs in /cpus.

Part of the problem is this requires a DT change if you switch between
a platform-specific driver and generic driver.

I don't understand the issue having a little bit of code to parse the
DT and create the device. If you are worried about having a long list
of platforms, you could instead check the tree for operating-points-v2
property in the cpu node and create the device unless the platform is
black-listed.

Rob

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Viresh Kumar]

On 01-04-16, 09:15, Rob Herring wrote:
> On Fri, Apr 1, 2016 at 5:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > So you mean for future DT files we can have something like this:
> >
> >         cpus {
> >                 compatible = "operation-points-v2";
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 cpu@0 {
> >                         compatible = "arm,cortex-a9";
> >                         reg = <0>;
> >                         next-level-cache = <&L2>;
> >                         operating-points-v2 = <&cpu0_opp_table>;
> >                 };
> >         };
> >
> >         cpu0_opp_table: opp_table0 {
> >                 opp@1000000000 {
> >                         opp-hz = /bits/ 64 <1000000000>;
> >                         opp-microvolt = <970000 975000 985000>;
> >                         opp-microamp = <70000>;
> >                         clock-latency-ns = <300000>;
> >                         opp-suspend;
> >                 };
> >                 opp@1100000000 {
> >                         opp-hz = /bits/ 64 <1100000000>;
> >                         opp-microvolt = <980000 1000000 1010000>;
> >                         opp-microamp = <80000>;
> >                         clock-latency-ns = <310000>;
> >                 };
> >         };
> > };
> >
> >
> > And the cpufreq-dt driver can match /cpus node's compatible string against
> > "operating-points-v2" and create a device at runtime ?
> >
> > @Rob: Will that be acceptable to you? We are discussing (again) about how to
> > probe cpufreq-dt driver automatically for platforms :)
> 
> No, I don't think that belongs in /cpus.
> 
> Part of the problem is this requires a DT change if you switch between
> a platform-specific driver and generic driver.

Right.

> I don't understand the issue having a little bit of code to parse the
> DT and create the device.

I am fine with that, we were just re-evaluating our options :)

> If you are worried about having a long list
> of platforms,

At least I am not.

> you could instead check the tree for operating-points-v2
> property in the cpu node and create the device unless the platform is
> black-listed.

I don't really like the black-list idea much. It forces a Non
cpufreq-dt platform to edit cpufreq-dt related file, just to make its
own cpufreq driver work.

I find that ugly somehow :)

-- 
viresh

Re: [PATCH V1 Resend 2/3] cpufreq: dt: Add generic platform-device creation support

[Arnd Bergmann]

On Thursday 07 April 2016 14:00:36 Viresh Kumar wrote:
> On 01-04-16, 09:15, Rob Herring wrote:
> > On Fri, Apr 1, 2016 at 5:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > >
> > >
> > > And the cpufreq-dt driver can match /cpus node's compatible string against
> > > "operating-points-v2" and create a device at runtime ?
> > >
> > > @Rob: Will that be acceptable to you? We are discussing (again) about how to
> > > probe cpufreq-dt driver automatically for platforms 
> > 
> > No, I don't think that belongs in /cpus.
> > 
> > Part of the problem is this requires a DT change if you switch between
> > a platform-specific driver and generic driver.
> 
> Right.

How likely is that to happen for future platforms though?

I'd rather see a whitelist for the existing platforms (as started
by Viresh), because we don't know what other out-of-tree platforms 
use the "operating-points-v2" binding that are in fact not compatible
with today's driver.

If we add one way to identify machines that are known to follow
the binding to the degree necessary to make them work with the
Linux driver (whatever way that would be), then the only remaining
worry is about machines suddenly breaking because of a change in
the Linux driver, and we can work around that in the kernel if
necessary (or revert whatever broke the platform).

> > I don't understand the issue having a little bit of code to parse the
> > DT and create the device.
> 
> I am fine with that, we were just re-evaluating our options 
> 
> > If you are worried about having a long list
> > of platforms,
> 
> At least I am not.

The length of the list is not the issue here, it's the requirement
to edit the list for each new SoC that gets added. That causes conflicts
and constant churn.

> > you could instead check the tree for operating-points-v2
> > property in the cpu node and create the device unless the platform is
> > black-listed.
> 
> I don't really like the black-list idea much. It forces a Non
> cpufreq-dt platform to edit cpufreq-dt related file, just to make its
> own cpufreq driver work.
> 
> I find that ugly somehow 

Agreed.

Sorry for the late reply, I'm still catching up with email from two
weeks of travelling.

	Arnd