cpufreq: dt: Create platform device from generic code

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 <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
 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

drivers/cpufreq/Kconfig +1 -0

index a7f45853c103..08573d54105b 100644
@@ -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.

drivers/cpufreq/Kconfig config CPUFREQ_DT · line 205

Quoted 3 lines · On Tuesday 29 March 2016 12:09:48 Viresh Kumar wrote:

drivers/cpufreq/Kconfig +3 -0

+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.

Quoted 8 lines · Arnd Bergmann

drivers/cpufreq/Kconfig config CPUFREQ_DT · line 205

Quoted 3 lines · On Tuesday 29 March 2016 12:09:48 Viresh Kumar wrote:

drivers/cpufreq/Kconfig +3 -0

+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.

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

Quoted 2 lines · Arnd Bergmann

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.

Cc'ing Rob and Mason.

Quoted 4 lines · Arnd Bergmann

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 ?

@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.

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?

Quoted 5 lines

@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?

Cc'ing Rob and Mason.

Quoted 4 lines · Arnd Bergmann

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 :)

Quoted 36 lines · Viresh Kumar

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 ?

Quoted 2 lines

@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.

Quoted 10 lines · Rob Herring

Quoted 7 lines · Viresh Kumar

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

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 2 lines

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

At least I am not.

Quoted 3 lines

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

if (of_device_match(of_root, compat))

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

Quoted 1 line · Arnd Bergmann

if (of_device_match(of_root, compat))

You meant of_match_node() here, right?

Quoted 9 lines · On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:

arch/arm/mach-exynos/exynos.c +0 -9

-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;

arch/arm/mach-exynos/exynos.c static void __init exynos_init_irq(void) · line 241

Quoted 4 lines

arch/arm/mach-exynos/exynos.c +0 -4

-       }
-
-       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.

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Tuesday 29 March 2016 12:09:49 Viresh Kumar wrote:

arch/arm/mach-exynos/exynos.c +0 -9

-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;

Quoted 8 lines

arch/arm/mach-exynos/exynos.c static void __init exynos_init_irq(void) · line 241

Quoted 4 lines

arch/arm/mach-exynos/exynos.c +0 -4

-       }
-
-       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.