CPUFreq: create platform-dev for DT based cpufreq drivers

DT based cpufreq drivers doesn't require much support from platform code now a days as most of the stuff is moved behind generic APIs. Like clk APIs for changing clock rates, regulator APIs for changing voltages, etc.

One of the bottleneck still left was how to select which cpufreq driver to probe for a given platform as there might be multiple drivers available.

Traditionally, we used to create platform devices from machine specific code which binds with a cpufreq driver. And while we moved towards DT based device creation, these devices stayed as is.

The problem is getting worse now as we have architectures now with Zero platform specific code. Forcefully these platforms have to create a new file in drivers/cpufreq/ to just add these platform devices in order to use the generic drivers like cpufreq-dt.c.

This has been discussed again and again, but with no solution yet. Last it was discussed here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256154.html

This patch is an attempt towards getting the bindings.

We only need to have cpufreq drivers name string present in "compatible" property for the root node.. If a cpufreq driver with DT support exists with that name, then cpufreq core will create a platform device for that.

The first patch presents the new binding, second one creates another file responsible for creating platform devices for all DT based cpufreq drivers.

And later patches update platforms to migrate to it one by one.

A BLACKLIST of platforms already supported by these drivers is also created for backward compatibility of newer kernels with older DTs. And so for such platforms DT files aren't updated.

An initial RFC that presented the idea was discussed here:
https://www.mail-archive.com/devicetree at vger.kernel.org/msg52509.html

Tested-ON: Exynos5250. (The last patch for exynos depends on some commits to be upstreamed in 3.19, presented here just for testing).

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

3. the compatible property of the /cpus node
4. a top-level device node that gets turned into a platform device
5. a new property in the / node
6. a new property in the /chosen node
7. the compatible property in the / node

- Implementation-wise, I don't think it's helpful to have a global function that registers a platform device to be consumed by the device driver. I'd rather just see a module_init function in each driver that rather than

[PATCH, RFC] cpufreq: dt: simplify and generalize probing

We should not have to create a platform device from platform specific code in order to use the completely generic cpufreq-dt driver. This adds a simpler method by creating a new standard property of the "/cpus" node to look for, with a fallback for existing users. The list of existing users needs to be completed, and the same change done for the other DT based drivers.

Signed-off-by Arnd Bergmann <[email protected]>

drivers/cpufreq/cpufreq-dt.c +14 -1

index 6f5f5615fbf1..697b4dc47715 100644
@@ -345,13 +345,50 @@ static struct cpufreq_driver dt_cpufreq_driver = {
        .attr = cpufreq_generic_attr,
 };
 
-static int dt_cpufreq_probe(struct platform_device *pdev)
+/*
+ * these machines are using the driver but provide no standard
+ * probing method, only for old machines with existing dtbs.
+ */
+static struct of_device_id legacy_machines = {
+       { .compatible = "calxeda,highbank" },
+       { .compatible = "renesas,sh7372" },
+       { .compatible = "renesas,sh73a0"  },
+       { .compatible = "samsung,exynos5250" },
+       { .compatible = "samsung,exynos4210" },
+       { .compatible = "xlnx,zynq-7000" },
+};
+
+static bool dt_cpufreq_available(void)

drivers/cpufreq/cpufreq-dt.c +14 -0

+       of_node_put(node);
+       if (ret)
+               return 1;
+
+       node = of_find_node_by_path("/");
+       ret = of_match_device(&legacy_machines, node);
+       of_node_put(node);
+
+       return ret;
+}
+
+static int __init dt_cpufreq_probe(void)
 {
        struct device *cpu_dev;
        struct regulator *cpu_reg;
        struct clk *cpu_clk;
        int ret;
 
+       if (!dt_cpufreq_available())
+               return -ENXIO;
        /*
         * All per-cluster (CPUs sharing clock/voltages) initialization is done
         * from ->init(). In probe(), we just need to make sure that clk and

drivers/cpufreq/cpufreq-dt.c +3 -11

        ret = cpufreq_register_driver(&dt_cpufreq_driver);
        if (ret)
                dev_err(cpu_dev, "failed register driver: %d\n", ret);
 
        return ret;
 }
+module_init(dt_cpufreq_probe);
 
-static int dt_cpufreq_remove(struct platform_device *pdev)
+static void __exit dt_cpufreq_remove(void)
 {
        cpufreq_unregister_driver(&dt_cpufreq_driver);
-       return 0;
 }
-
-static struct platform_driver dt_cpufreq_platdrv = {
-       .driver = {
-               .name   = "cpufreq-dt",
-       },
-       .probe          = dt_cpufreq_probe,
-       .remove         = dt_cpufreq_remove,
-};
-module_platform_driver(dt_cpufreq_platdrv);
+module_exit(dt_cpufreq_remove);
 
 MODULE_AUTHOR("Viresh Kumar <[email protected]>");
 MODULE_AUTHOR("Shawn Guo <[email protected]>");

Quoted 1 line · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

:)

Quoted 11 lines

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Are you fine with the name here? "dvfs-method"

Quoted 1 line · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

:)

Quoted 11 lines

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Are you fine with the name here? "dvfs-method"

Quoted 7 lines

drivers/cpufreq/cpufreq-dt.c +7 -0

+static bool dt_cpufreq_available(void)
+{
+       struct device_node *node;
+       bool ret;
+
+       node = of_find_node_by_path("/cpus");
+       if (!node)

drivers/cpufreq/cpufreq-dt.c static struct cpufreq_driver dt_cpufreq_driver = { · line 371

Quoted 4 lines

drivers/cpufreq/cpufreq-dt.c +4 -0

+               return 0;
+
+       /* the specific property needs to be debated */
+       ret = of_property_read_bool("linux,cpu-frequency-scaling");

How can this be a bool? We need to differentiate on which binding wants to go for arm-bl or cupfreq-dt or any other driver. So we surely need a string ?

drivers/cpufreq/cpufreq-dt.c static int dt_cpufreq_probe(struct platform_device *pdev) · line 371

Quoted 6 lines

drivers/cpufreq/cpufreq-dt.c +0 -2

@@ -367,29 +404,19 @@ static int dt_cpufreq_probe(struct platform_device *pdev)
        if (!IS_ERR(cpu_reg))
                regulator_put(cpu_reg);
 
-       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
-

We still need this, and its about how clocks are shared between CPUs.

Quoted 21 lines · Viresh Kumar

Quoted 8 lines · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

Quoted 7 lines

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Quoted 2 lines

Are you fine with the name here? "dvfs-method"

That's right, I don't like driver specific method in the cpu node as you initially did. But if it's a property in the chosen node (where we usually put the Linux specific properties), I am fine with that as Arnd has illustrated in his patch.

I guess a string would be better here, the idea here was to have a different bool property per driver, which would also work.

Quoted 8 lines

drivers/cpufreq/cpufreq-dt.c static int dt_cpufreq_probe(struct platform_device *pdev) · line 371

Quoted 6 lines

drivers/cpufreq/cpufreq-dt.c +0 -2

@@ -367,29 +404,19 @@ static int dt_cpufreq_probe(struct platform_device *pdev)
        if (!IS_ERR(cpu_reg))
                regulator_put(cpu_reg);
 
-       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
-

We still need this, and its about how clocks are shared between CPUs.

I didn't see where this comes from. Who is setting up this platform data?

I would prefer the /cpus node over the /chosen node because the former describes the hardware while the latter is supposed to be user-settable (on real open-firmware at least). But I think either one is better than using the / node compatible string.

How about a "linux,cpu-dvfs-method" property in the root node? Would that work better for you than a "linux,dvfs-method" in the "/cpus" node?

Quoted 3 lines · Arnd Bergmann

I guess a string would be better here, the idea here was to have a different bool property per driver, which would also work.

Hmm, I will prefer string as we don't need to define any more bindings then for new drivers.

Quoted 9 lines

Quoted 8 lines

drivers/cpufreq/cpufreq-dt.c static int dt_cpufreq_probe(struct platform_device *pdev) · line 371

Quoted 6 lines

drivers/cpufreq/cpufreq-dt.c +0 -2

@@ -367,29 +404,19 @@ static int dt_cpufreq_probe(struct platform_device *pdev)
        if (!IS_ERR(cpu_reg))
                regulator_put(cpu_reg);
 
-       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
-

We still need this, and its about how clocks are shared between CPUs.

Quoted 2 lines

I didn't see where this comes from. Who is setting up this platform data?

Mvebu is using it to communicate that all CPUs have separate clock lines.

Quoted 22 lines · On Monday 01 December 2014 13:35:25 Sudeep Holla wrote:

Quoted 21 lines · Viresh Kumar

Quoted 8 lines · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

Quoted 7 lines

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Quoted 7 lines

Quoted 2 lines

Are you fine with the name here? "dvfs-method"

That's right, I don't like driver specific method in the cpu node as you initially did. But if it's a property in the chosen node (where we usually put the Linux specific properties), I am fine with that as Arnd has illustrated in his patch.

I would prefer the /cpus node over the /chosen node because the former describes the hardware while the latter is supposed to be user-settable (on real open-firmware at least). But I think either one is better than using the / node compatible string.

How about a "linux,cpu-dvfs-method" property in the root node? Would that work better for you than a "linux,dvfs-method" in the "/cpus" node?

Quoted 31 lines · Arnd Bergmann

Quoted 22 lines · On Monday 01 December 2014 13:35:25 Sudeep Holla wrote:

Quoted 21 lines · Viresh Kumar

Quoted 8 lines · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

Quoted 7 lines

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Quoted 7 lines

Quoted 2 lines

Are you fine with the name here? "dvfs-method"

That's right, I don't like driver specific method in the cpu node as you initially did. But if it's a property in the chosen node (where we usually put the Linux specific properties), I am fine with that as Arnd has illustrated in his patch.

Quoted 5 lines

I would prefer the /cpus node over the /chosen node because the former describes the hardware while the latter is supposed to be user-settable (on real open-firmware at least). But I think either one is better than using the / node compatible string.

Agreed, the main reason for objection was that in the initial proposal it was more a Linux configuration rather than hardware property.

<dvfs-method> = "cpufreq-dt";

I don't see anything hardware feature presented with that. On the other hand, we could represent the some thing like whether the cpu share clock or are they independently clocked as a hardware property in the cpu nodes.

Quoted 4 lines

How about a "linux,cpu-dvfs-method" property in the root node? Would that work better for you than a "linux,dvfs-method" in the "/cpus" node?

I will leave that to the DT maintainers for specific details though my preference is still chosen node as it's more related to Linux and it's driver choice and unlikely to be used by other OSes. IMO we just need single entry in the DT, so I am fine with either of your choice above.

Quoted 41 lines · On Monday 01 December 2014 15:07:15 Sudeep Holla wrote:

Quoted 31 lines · Arnd Bergmann

Quoted 22 lines · On Monday 01 December 2014 13:35:25 Sudeep Holla wrote:

Quoted 21 lines · Viresh Kumar

Quoted 8 lines · Arnd Bergmann

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

Quoted 7 lines

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.

I did it this way earlier and named it dvfs-method but probably putting this into the /cpus/ node is far better. But then Sudeep asked to utilize compatible property only..

Quoted 7 lines

Quoted 2 lines

Are you fine with the name here? "dvfs-method"

That's right, I don't like driver specific method in the cpu node as you initially did. But if it's a property in the chosen node (where we usually put the Linux specific properties), I am fine with that as Arnd has illustrated in his patch.

Quoted 5 lines

I would prefer the /cpus node over the /chosen node because the former describes the hardware while the latter is supposed to be user-settable (on real open-firmware at least). But I think either one is better than using the / node compatible string.

Agreed, the main reason for objection was that in the initial proposal it was more a Linux configuration rather than hardware property.

Quoted 6 lines

<dvfs-method> = "cpufreq-dt";

I don't see anything hardware feature presented with that. On the other hand, we could represent the some thing like whether the cpu share clock or are they independently clocked as a hardware property in the cpu nodes.

My interpretation of the dvfs-method property is that the string states how dvfs works. In particular, it would be used to tell the difference between machines that just rely on the clocks and regulators properties of the CPU nodes as defined in bindings/cpufreq/cpufreq-dt.txt compared to those that need platform-specific properties beyond that. The value of the string should indeed not be "cpufreq-dt", as that would be a linux implementation detail and inappropriate here.

Quoted 40 lines · On Monday 01 December 2014 17:11:21 Viresh Kumar wrote:

DT based cpufreq drivers doesn't require much support from platform code now a days as most of the stuff is moved behind generic APIs. Like clk APIs for changing clock rates, regulator APIs for changing voltages, etc.

One of the bottleneck still left was how to select which cpufreq driver to probe for a given platform as there might be multiple drivers available.

Traditionally, we used to create platform devices from machine specific code which binds with a cpufreq driver. And while we moved towards DT based device creation, these devices stayed as is.

The problem is getting worse now as we have architectures now with Zero platform specific code. Forcefully these platforms have to create a new file in drivers/cpufreq/ to just add these platform devices in order to use the generic drivers like cpufreq-dt.c.

This has been discussed again and again, but with no solution yet. Last it was discussed here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256154.html

This patch is an attempt towards getting the bindings.

We only need to have cpufreq drivers name string present in "compatible" property for the root node.. If a cpufreq driver with DT support exists with that name, then cpufreq core will create a platform device for that.

The first patch presents the new binding, second one creates another file responsible for creating platform devices for all DT based cpufreq drivers.

And later patches update platforms to migrate to it one by one.

A BLACKLIST of platforms already supported by these drivers is also created for backward compatibility of newer kernels with older DTs. And so for such platforms DT files aren't updated.

An initial RFC that presented the idea was discussed here:
https://www.mail-archive.com/devicetree at vger.kernel.org/msg52509.html

Quoted 2 lines

Tested-ON: Exynos5250. (The last patch for exynos depends on some commits to be upstreamed in 3.19, presented here just for testing).

Thanks a lot for working on this, we really need to figure it out one day!

Your patches seem well-implemented, so if everybody thinks the general approach is the best solution, we should do that. From my point of view, there are two things I would do differently:

- In the DT binding, I would strongly prefer anything but the root compatible property as the key for the new platforms. Clearly we have to keep using it for the backwards-compatibility case, as you do, but I think there are more appropriate places to put it. Sorting from most favorite to least

favorite, my list would be:
1. a new property in /cpus/
2. a new property each /cpus/cpu at ... node.
3. the compatible property of the /cpus node
4. a top-level device node that gets turned into a platform device
5. a new property in the / node
6. a new property in the /chosen node
7. the compatible property in the / node

- Implementation-wise, I don't think it's helpful to have a global function that registers a platform device to be consumed by the device driver. I'd rather just see a module_init function in each driver that rather than registering a platform_driver scans the DT properties. This is only really necessary when not using DT (omap2, omap3, davinci, loongson) or when passing additional resources or platform_data (kirkwood, but that can look up the "marvell,orion-system-controller" node if necessary). My preferred solution would be to eventually remove the platform_device registration for all DT users. If a driver needs a platform device pointer internally, it can use platform_create_bundle(), but that's probably not even necessary.

I still don't see where it does that. All I see for mvebu is

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

without any platform data. I see this patch
http://lists.linaro.org/pipermail/linaro-kernel/2014-September/017693.html
on the mailing list, but it's not in linux-next, and it obviously would not work any more with the patch I proposed. Instead I suppose you would use a different string to match against for the case of separate clocks.