[RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

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 one entry in cpus@cpu0 node which will match with drivers
name.

We can then add another file drivers/cpufreq/device_dt.c, which will add a
platform device with the name it finds from cpus@cpu0 node and existing drivers
will work without any change. Or something else if somebody have a better
proposal. But lets fix the bindings first.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .../devicetree/bindings/cpufreq/drivers.txt        | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/drivers.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/drivers.txt b/Documentation/devicetree/bindings/cpufreq/drivers.txt
new file mode 100644
index 0000000..bd14917
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/drivers.txt
@@ -0,0 +1,53 @@
+Binding to select which cpufreq driver to register
+
+It is a generic DT binding for selecting which cpufreq-driver to register for
+any platform.
+
+The property  listed below must be defined under node /cpus/cpu@0 node. We don't
+support multiple CPUFreq driver currently for different cluster and so this
+information isn't required to be present in CPUs of all clusters.
+
+Required properties:
+- None
+
+Optional properties:
+- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
+  "cpufreq-dt", etc
+
+Examples:
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	cpu@0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		next-level-cache = <&L2>;
+		operating-points = <
+			/* kHz    uV */
+			792000  1100000
+			396000  950000
+			198000  850000
+		>;
+		dvfs-method = "cpufreq-dt";
+	};
+
+	cpu@1 {
+		compatible = "arm,cortex-a9";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@2 {
+		compatible = "arm,cortex-a9";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@3 {
+		compatible = "arm,cortex-a9";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+};
-- 
2.0.3.693.g996b0fd

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

Fixing Santosh's email id as he switched employer ..

On 26 November 2014 at 14:16, Viresh Kumar <viresh.kumar@linaro.org> 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 one entry in cpus@cpu0 node which will match with drivers
> name.
>
> We can then add another file drivers/cpufreq/device_dt.c, which will add a
> platform device with the name it finds from cpus@cpu0 node and existing drivers
> will work without any change. Or something else if somebody have a better
> proposal. But lets fix the bindings first.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/drivers.txt        | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/drivers.txt
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/drivers.txt b/Documentation/devicetree/bindings/cpufreq/drivers.txt
> new file mode 100644
> index 0000000..bd14917
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/drivers.txt
> @@ -0,0 +1,53 @@
> +Binding to select which cpufreq driver to register
> +
> +It is a generic DT binding for selecting which cpufreq-driver to register for
> +any platform.
> +
> +The property  listed below must be defined under node /cpus/cpu@0 node. We don't
> +support multiple CPUFreq driver currently for different cluster and so this
> +information isn't required to be present in CPUs of all clusters.
> +
> +Required properties:
> +- None
> +
> +Optional properties:
> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
> +  "cpufreq-dt", etc
> +
> +Examples:
> +
> +cpus {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       cpu@0 {
> +               compatible = "arm,cortex-a9";
> +               reg = <0>;
> +               next-level-cache = <&L2>;
> +               operating-points = <
> +                       /* kHz    uV */
> +                       792000  1100000
> +                       396000  950000
> +                       198000  850000
> +               >;
> +               dvfs-method = "cpufreq-dt";
> +       };
> +
> +       cpu@1 {
> +               compatible = "arm,cortex-a9";
> +               reg = <1>;
> +               next-level-cache = <&L2>;
> +       };
> +
> +       cpu@2 {
> +               compatible = "arm,cortex-a9";
> +               reg = <2>;
> +               next-level-cache = <&L2>;
> +       };
> +
> +       cpu@3 {
> +               compatible = "arm,cortex-a9";
> +               reg = <3>;
> +               next-level-cache = <&L2>;
> +       };
> +};
> --
> 2.0.3.693.g996b0fd
>

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[santosh shilimkar]

On 11/26/2014 12:49 AM, Viresh Kumar wrote:
> Fixing Santosh's email id as he switched employer ..
>
Yeah I did ;-)

> On 26 November 2014 at 14:16, Viresh Kumar <viresh.kumar@linaro.org> 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.
>>
We should definitely avoid that since that code is just pointless. Good
to see that you picked up that.

>> 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 one entry in cpus@cpu0 node which will match with drivers
>> name.
>>
>> We can then add another file drivers/cpufreq/device_dt.c, which will add a
>> platform device with the name it finds from cpus@cpu0 node and existing drivers
>> will work without any change. Or something else if somebody have a better
>> proposal. But lets fix the bindings first.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   .../devicetree/bindings/cpufreq/drivers.txt        | 53 ++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/cpufreq/drivers.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/drivers.txt b/Documentation/devicetree/bindings/cpufreq/drivers.txt
>> new file mode 100644
>> index 0000000..bd14917
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/drivers.txt
>> @@ -0,0 +1,53 @@
>> +Binding to select which cpufreq driver to register
>> +
>> +It is a generic DT binding for selecting which cpufreq-driver to register for
>> +any platform.
>> +
>> +The property  listed below must be defined under node /cpus/cpu@0 node. We don't
>> +support multiple CPUFreq driver currently for different cluster and so this
>> +information isn't required to be present in CPUs of all clusters.
>> +
>> +Required properties:
>> +- None
>> +
>> +Optional properties:
>> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
>> +  "cpufreq-dt", etc
>> +
>> +Examples:
>> +
>> +cpus {
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +
>> +       cpu@0 {
>> +               compatible = "arm,cortex-a9";
>> +               reg = <0>;
>> +               next-level-cache = <&L2>;
>> +               operating-points = <
>> +                       /* kHz    uV */
>> +                       792000  1100000
>> +                       396000  950000
>> +                       198000  850000
>> +               >;
>> +               dvfs-method = "cpufreq-dt";
>> +       };

Its really not 'dvfs-method' but really the actual driver which you
want to probe. Also we should just have one global way to parse
DT vs non-DT cpufreq drivers. In other words, instead of matching
multiple driver strings for different drivers, we should come up with
slightly generic binding. Probably 'cpufreq-dt' for all DT based probed
CPUFREQ drivers.

What you say ?

Regards,
Santosh

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

On 26 November 2014 at 22:04, santosh shilimkar
<santosh.shilimkar@oracle.com> wrote:
> Its really not 'dvfs-method' but really the actual driver which you
> want to probe. Also we should just have one global way to parse
> DT vs non-DT cpufreq drivers. In other words, instead of matching
> multiple driver strings for different drivers, we should come up with
> slightly generic binding. Probably 'cpufreq-dt' for all DT based probed
> CPUFREQ drivers.
>
> What you say ?

I got a bit confused. :(

The whole purpose of this binding is to select which cpufreq driver to
choose from the available ones.

Are you saying that we should have only one cpufreq driver that works
with DT ? I don't think that would be the right decision, but we should
try to reuse cpufreq-dt wherever possible.

Or are you saying that some common code should parse this DT information?
This is what I said in the patch, we can write another file which will
just create
a platform_device for us, depending on the string that came in.

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[santosh.shilimkar]

On 11/26/14 9:14 PM, Viresh Kumar wrote:
> On 26 November 2014 at 22:04, santosh shilimkar
> <santosh.shilimkar@oracle.com> wrote:
>> Its really not 'dvfs-method' but really the actual driver which you
>> want to probe. Also we should just have one global way to parse
>> DT vs non-DT cpufreq drivers. In other words, instead of matching
>> multiple driver strings for different drivers, we should come up with
>> slightly generic binding. Probably 'cpufreq-dt' for all DT based probed
>> CPUFREQ drivers.
>>
>> What you say ?
>
> I got a bit confused. :(
>
> The whole purpose of this binding is to select which cpufreq driver to
> choose from the available ones.
>
> Are you saying that we should have only one cpufreq driver that works
> with DT ? I don't think that would be the right decision, but we should
> try to reuse cpufreq-dt wherever possible.
>
I didn't mean one CPUFreq driver which works with DT.

> Or are you saying that some common code should parse this DT information?
Yes.

> This is what I said in the patch, we can write another file which will
> just create
> a platform_device for us, depending on the string that came in.
>
Sounds good.

Regards,
Santosh

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Sudeep Holla]

Hi Viresh,

On 26/11/14 08:46, 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 one entry in cpus@cpu0 node which will match with drivers
> name.
>

This seems fundamentally broken as the driver always needs to
unconditionally refer to cpu0. Furthermore the node need not be called
cpu0 as the name depends on its reg field.

> We can then add another file drivers/cpufreq/device_dt.c, which will add a
> platform device with the name it finds from cpus@cpu0 node and existing drivers
> will work without any change. Or something else if somebody have a better
> proposal. But lets fix the bindings first.

IIUC you will retain the existing list of cpufreq-dt platform device
creation as is. If not that breaks compatibility with old DT.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   .../devicetree/bindings/cpufreq/drivers.txt        | 53 ++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/cpufreq/drivers.txt
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/drivers.txt b/Documentation/devicetree/bindings/cpufreq/drivers.txt
> new file mode 100644
> index 0000000..bd14917
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/drivers.txt
> @@ -0,0 +1,53 @@
> +Binding to select which cpufreq driver to register
> +
> +It is a generic DT binding for selecting which cpufreq-driver to register for
> +any platform.
> +
> +The property  listed below must be defined under node /cpus/cpu@0 node. We don't
> +support multiple CPUFreq driver currently for different cluster and so this
> +information isn't required to be present in CPUs of all clusters.
> +
> +Required properties:
> +- None
> +
> +Optional properties:
> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
> +  "cpufreq-dt", etc
> +

You should manage this with compatible rather than a new property as
it's not a real hardware property. IMHO Rob's suggestion[1] should work
fine.

IIUC, you can have the driver which create this platform device if DT
has generic compatible unconditionally(e.g "cpufreq-dt" as you have
chosen above). For all existing DT you can create a blacklist of
compatibles to match(as it doesn't have the generic compatible) covering
all the existing platforms using cpufreq-dt driver, there by you can
even remove the platform device creating from multiple places.
IMO something like the patch below should work(not tested, also
late_initcall is used just to demonstrate the idea)

Rob, please correct me if my understanding is wrong.

Regards,
Sudeep

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256191.html

--->8

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index f657c571b18e..19a616e298e0 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -387,6 +387,32 @@ static struct platform_driver dt_cpufreq_platdrv = {
  };
  module_platform_driver(dt_cpufreq_platdrv);

+static const struct of_device_id compatible_machine_match[] = {
+       /* All new machines must have the below compatible to use this 
driver */
+       { .compatible = "cpufreq-generic-dt" },
+       /* BLACKLIST of existing users of cpufreq-dt below */
+       { .compatible = "samsung,exynos5420" },
+       { .compatible = "samsung,exynos5800" },
+       {},
+};
+
+static int cpufreq_generic_dt_init(void)
+{
+       struct device_node *root = of_find_node_by_path("/");
+       struct platform_device_info devinfo = { .name = "cpufreq-dt", };
+       /*
+        * Initialize the device for the platforms either
+        * blacklisted or compliant to generic compatible
+        */
+       if (!of_match_node(compatible_machine_match, root))
+               return -ENODEV;
+
+       /* Instantiate cpufreq-dt */
+       platform_device_register_full(&devinfo);
+       return 0;
+}
+late_initcall(cpufreq_generic_dt_init);
+

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Sudeep Holla]

On 26/11/14 17:00, Sudeep Holla wrote:
> Hi Viresh,
>

[...]

>> diff --git a/Documentation/devicetree/bindings/cpufreq/drivers.txt b/Documentation/devicetree/bindings/cpufreq/drivers.txt
>> new file mode 100644
>> index 0000000..bd14917
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/drivers.txt
>> @@ -0,0 +1,53 @@
>> +Binding to select which cpufreq driver to register
>> +
>> +It is a generic DT binding for selecting which cpufreq-driver to register for
>> +any platform.
>> +
>> +The property  listed below must be defined under node /cpus/cpu@0 node. We don't
>> +support multiple CPUFreq driver currently for different cluster and so this
>> +information isn't required to be present in CPUs of all clusters.
>> +
>> +Required properties:
>> +- None
>> +
>> +Optional properties:
>> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
>> +  "cpufreq-dt", etc
>> +
>
> You should manage this with compatible rather than a new property as
> it's not a real hardware property. IMHO Rob's suggestion[1] should work
> fine.
>
> IIUC, you can have the driver which create this platform device if DT
> has generic compatible unconditionally(e.g "cpufreq-dt" as you have
> chosen above). For all existing DT you can create a blacklist of
> compatibles to match(as it doesn't have the generic compatible) covering
> all the existing platforms using cpufreq-dt driver, there by you can
> even remove the platform device creating from multiple places.
> IMO something like the patch below should work(not tested, also
> late_initcall is used just to demonstrate the idea)
>
> Rob, please correct me if my understanding is wrong.
>
> Regards,
> Sudeep
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256191.html
>
> --->8
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index f657c571b18e..19a616e298e0 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -387,6 +387,32 @@ static struct platform_driver dt_cpufreq_platdrv = {
>    };
>    module_platform_driver(dt_cpufreq_platdrv);
>
> +static const struct of_device_id compatible_machine_match[] = {
> +       /* All new machines must have the below compatible to use this
> driver */
> +       { .compatible = "cpufreq-generic-dt" },
> +       /* BLACKLIST of existing users of cpufreq-dt below */
> +       { .compatible = "samsung,exynos5420" },
> +       { .compatible = "samsung,exynos5800" },

Please ignore the above 2 compatible values in this context, I just
chose randomly 2 values but I now realize that there are big-little
platforms and might not use this driver :(

Regards,
Sudeep

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

Hi Sudeep,

On 26 November 2014 at 22:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 26/11/14 08:46, Viresh Kumar wrote:
>> We only need to have one entry in cpus@cpu0 node which will match with
>> drivers
>> name.

> This seems fundamentally broken as the driver always needs to
> unconditionally refer to cpu0. Furthermore the node need not be called
> cpu0 as the name depends on its reg field.

I meant CPU0's node, whatever the name is. Its normally cpu0 now a days
but yes it can be something else as well :)

>> We can then add another file drivers/cpufreq/device_dt.c, which will add a
>> platform device with the name it finds from cpus@cpu0 node and existing
>> drivers
>> will work without any change. Or something else if somebody have a better
>> proposal. But lets fix the bindings first.
>
> IIUC you will retain the existing list of cpufreq-dt platform device
> creation as is. If not that breaks compatibility with old DT.

I couldn't get what exactly you understood :), but this is what I meant.
- Currently cpufreq drivers (like cpufreq-dt.c) are registering a
platform_driver
and platform specific code are registering a platform-device to match to this
driver.
- What will change is this platform specific code. The driver will stay as is.
- Now the devices wouldn't get created from platform-specific code, but
cpufreq core (may be a separate file for this: device_dt.c) will create platform
device based on the string it got from DT.

Makes sense?


>> +Optional properties:
>> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
>> +  "cpufreq-dt", etc
>
> You should manage this with compatible rather than a new property as
> it's not a real hardware property. IMHO Rob's suggestion[1] should work
> fine.
>
> IIUC, you can have the driver which create this platform device if DT
> has generic compatible unconditionally(e.g "cpufreq-dt" as you have
> chosen above). For all existing DT you can create a blacklist of
> compatibles to match(as it doesn't have the generic compatible) covering

I didn't get why you asked for a blacklist here. Why wouldn't "cpufreq-dt" in
compatible work for them ?

> all the existing platforms using cpufreq-dt driver, there by you can
> even remove the platform device creating from multiple places.

Yeah, we can remove all such occurrences for a single driver this way.
What I was suggesting earlier was to do this from a driver independent
file, so that its done once for all possible DT drivers. Like "cpufreq-dt",
"arm_big_little_dt". The same compatible property can be used there
too..

> IMO something like the patch below should work(not tested, also
> late_initcall is used just to demonstrate the idea)
>
> Rob, please correct me if my understanding is wrong.

> +static const struct of_device_id compatible_machine_match[] = {
> +       /* All new machines must have the below compatible to use this
> driver */
> +       { .compatible = "cpufreq-generic-dt" },
> +       /* BLACKLIST of existing users of cpufreq-dt below */
> +       { .compatible = "samsung,exynos5420" },
> +       { .compatible = "samsung,exynos5800" },
> +       {},
> +};
> +
> +static int cpufreq_generic_dt_init(void)
> +{
> +       struct device_node *root = of_find_node_by_path("/");
> +       struct platform_device_info devinfo = { .name = "cpufreq-dt", };
> +       /*
> +        * Initialize the device for the platforms either
> +        * blacklisted or compliant to generic compatible
> +        */
> +       if (!of_match_node(compatible_machine_match, root))
> +               return -ENODEV;
> +
> +       /* Instantiate cpufreq-dt */
> +       platform_device_register_full(&devinfo);
> +       return 0;
> +}
> +late_initcall(cpufreq_generic_dt_init);

Looks fine to me. Looks similar to what I had in mind for creating
devices from platform-independent code.

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Sudeep Holla]

Hi,

On 27/11/14 05:29, Viresh Kumar wrote:
> Hi Sudeep,
>
> On 26 November 2014 at 22:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 26/11/14 08:46, Viresh Kumar wrote:
>>> We only need to have one entry in cpus@cpu0 node which will match with
>>> drivers
>>> name.
>
>> This seems fundamentally broken as the driver always needs to
>> unconditionally refer to cpu0. Furthermore the node need not be called
>> cpu0 as the name depends on its reg field.
>
> I meant CPU0's node, whatever the name is. Its normally cpu0 now a days
> but yes it can be something else as well :)
>

Not really ;), have a look @ arch/arm/boot/dts/highbank.dts

>>> We can then add another file drivers/cpufreq/device_dt.c, which will add a
>>> platform device with the name it finds from cpus@cpu0 node and existing
>>> drivers
>>> will work without any change. Or something else if somebody have a better
>>> proposal. But lets fix the bindings first.
>>
>> IIUC you will retain the existing list of cpufreq-dt platform device
>> creation as is. If not that breaks compatibility with old DT.
>
> I couldn't get what exactly you understood :), but this is what I meant.
> - Currently cpufreq drivers (like cpufreq-dt.c) are registering a
> platform_driver
> and platform specific code are registering a platform-device to match to this
> driver.
> - What will change is this platform specific code. The driver will stay as is.
> - Now the devices wouldn't get created from platform-specific code, but
> cpufreq core (may be a separate file for this: device_dt.c) will create platform
> device based on the string it got from DT.
>
> Makes sense?
>

No that won't suffice. You can't modify the DTs of the platforms using
cpufreq-dt.c as of today. They should continue to work, so either you
retain all the existing platform device creation in platform code as is
or do something like below with the blacklist for those platforms.

>
>>> +Optional properties:
>>> +- dvfs-method: CPUFreq driver to probe. For example: "arm-bL-cpufreq-dt",
>>> +  "cpufreq-dt", etc
>>
>> You should manage this with compatible rather than a new property as
>> it's not a real hardware property. IMHO Rob's suggestion[1] should work
>> fine.
>>
>> IIUC, you can have the driver which create this platform device if DT
>> has generic compatible unconditionally(e.g "cpufreq-dt" as you have
>> chosen above). For all existing DT you can create a blacklist of
>> compatibles to match(as it doesn't have the generic compatible) covering
>
> I didn't get why you asked for a blacklist here. Why wouldn't "cpufreq-dt" in
> compatible work for them ?
>

Again for backward compatibility reasons :)

>> all the existing platforms using cpufreq-dt driver, there by you can
>> even remove the platform device creating from multiple places.
>
> Yeah, we can remove all such occurrences for a single driver this way.
> What I was suggesting earlier was to do this from a driver independent
> file, so that its done once for all possible DT drivers. Like "cpufreq-dt",
> "arm_big_little_dt". The same compatible property can be used there
> too..
>
>> IMO something like the patch below should work(not tested, also
>> late_initcall is used just to demonstrate the idea)
>>
>> Rob, please correct me if my understanding is wrong.
>
>> +static const struct of_device_id compatible_machine_match[] = {
>> +       /* All new machines must have the below compatible to use this
>> driver */
>> +       { .compatible = "cpufreq-generic-dt" },
>> +       /* BLACKLIST of existing users of cpufreq-dt below */
>> +       { .compatible = "samsung,exynos5420" },
>> +       { .compatible = "samsung,exynos5800" },
>> +       {},
>> +};
>> +
>> +static int cpufreq_generic_dt_init(void)
>> +{
>> +       struct device_node *root = of_find_node_by_path("/");
>> +       struct platform_device_info devinfo = { .name = "cpufreq-dt", };
>> +       /*
>> +        * Initialize the device for the platforms either
>> +        * blacklisted or compliant to generic compatible
>> +        */
>> +       if (!of_match_node(compatible_machine_match, root))
>> +               return -ENODEV;
>> +
>> +       /* Instantiate cpufreq-dt */
>> +       platform_device_register_full(&devinfo);
>> +       return 0;
>> +}
>> +late_initcall(cpufreq_generic_dt_init);
>
> Looks fine to me. Looks similar to what I had in mind for creating
> devices from platform-independent code.
>
OK, in this way you still continue to work on existing platforms with
*old DT*.

Regards,
Sudeep

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

On 27 November 2014 at 15:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> No that won't suffice. You can't modify the DTs of the platforms using
> cpufreq-dt.c as of today. They should continue to work, so either you
> retain all the existing platform device creation in platform code as is
> or do something like below with the blacklist for those platforms.

Is the meaning of "backward compatibility" for DTs mentioned somewhere?
I am not sure if we have to *always* follow the compatibility you are talking
about.

Probably we want new DTs to continue supporting older version of kernels.
Not that older DTs should work with new kernel changes.

So, even with another field in cpuX node, we wouldn't break older kernels
as they will anyway get the device from platform code.

@Rob?

> OK, in this way you still continue to work on existing platforms with
> *old DT*.

Probably not.

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Sudeep Holla]

On 27/11/14 10:22, Viresh Kumar wrote:
> On 27 November 2014 at 15:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> No that won't suffice. You can't modify the DTs of the platforms using
>> cpufreq-dt.c as of today. They should continue to work, so either you
>> retain all the existing platform device creation in platform code as is
>> or do something like below with the blacklist for those platforms.
>
> Is the meaning of "backward compatibility" for DTs mentioned somewhere?
> I am not sure if we have to *always* follow the compatibility you are talking
> about.
>

It's the general understanding, I am not sure if it's specified anywhere
in the kernel Documentation, but I could find the below excerpts from [1]:

"The compatibility rules say that new kernels must work with older
device trees. If changes are required, they should be put into new
properties; the old ones can then be deprecated but not removed. New
properties should be optional, so that device trees lacking those
properties continue to work. The device tree developers will provide a
set of guidelines for the creation of future-proof bindings."

> Probably we want new DTs to continue supporting older version of kernels.
> Not that older DTs should work with new kernel changes.
>

It's *exactly opposite* as DTs are considered as part of firmware that
gets shipped with the boards and any kernel should work with that DT if
it is compliance with the DT bindings(even old, as the DT bindings
should never get changed only gets extended)

> So, even with another field in cpuX node, we wouldn't break older kernels
> as they will anyway get the device from platform code.
>

Yes, but that's not what we want.

>> OK, in this way you still continue to work on existing platforms with
>> *old DT*.
>
> Probably not.
>

No, you *must* :). That's backward compatibility. Just consider a simple
case where the bootloader is generating DT and we don't want to upgrade it.

Regards,
Sudeep

[1] http://lwn.net/Articles/572114/

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

On 27 November 2014 at 16:45, Sudeep Holla <sudeep.holla@arm.com> wrote:
> It's the general understanding, I am not sure if it's specified anywhere
> in the kernel Documentation, but I could find the below excerpts from [1]:
>
> "The compatibility rules say that new kernels must work with older
> device trees. If changes are required, they should be put into new
> properties; the old ones can then be deprecated but not removed. New
> properties should be optional, so that device trees lacking those
> properties continue to work. The device tree developers will provide a
> set of guidelines for the creation of future-proof bindings."
>
> It's *exactly opposite* as DTs are considered as part of firmware that
> gets shipped with the boards and any kernel should work with that DT if
> it is compliance with the DT bindings(even old, as the DT bindings
> should never get changed only gets extended)

Okay, I was completely wrong. :)

> No, you *must* :). That's backward compatibility. Just consider a simple
> case where the bootloader is generating DT and we don't want to upgrade it.

Now these are the options we have for existing platforms:

- Update those platforms to check if DT has "compatible" string in CPU node.
If yes, don't create a device as this will be created by cpufreq-dt.

- Just remove the device creation from those paths if the Maintainers of
those platforms want to cleanup their code, accepting the loss of backward
compatibility..

--
viresh

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Sudeep Holla]

On 28/11/14 06:31, Viresh Kumar wrote:
> On 27 November 2014 at 16:45, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> It's the general understanding, I am not sure if it's specified anywhere
>> in the kernel Documentation, but I could find the below excerpts from [1]:
>>
>> "The compatibility rules say that new kernels must work with older
>> device trees. If changes are required, they should be put into new
>> properties; the old ones can then be deprecated but not removed. New
>> properties should be optional, so that device trees lacking those
>> properties continue to work. The device tree developers will provide a
>> set of guidelines for the creation of future-proof bindings."
>>
>> It's *exactly opposite* as DTs are considered as part of firmware that
>> gets shipped with the boards and any kernel should work with that DT if
>> it is compliance with the DT bindings(even old, as the DT bindings
>> should never get changed only gets extended)
>
> Okay, I was completely wrong. :)
>
>> No, you *must* :). That's backward compatibility. Just consider a simple
>> case where the bootloader is generating DT and we don't want to upgrade it.
>
> Now these are the options we have for existing platforms:
>
> - Update those platforms to check if DT has "compatible" string in CPU node.
> If yes, don't create a device as this will be created by cpufreq-dt.
>

This fixes how it will work with new DTs, how about old DTs ?
IMO we still need the blacklist kind of logic I sent before.
Do you see any issues with that ?

> - Just remove the device creation from those paths if the Maintainers of
> those platforms want to cleanup their code, accepting the loss of backward
> compatibility..
>

I don't think this is something acceptable, why will someone want to
lose a working feature with new kernel.

Regards,
Sudeep

Re: [RFC] cpufreq: Add "dvfs-method" binding to probe cpufreq drivers

[Viresh Kumar]

On 28 November 2014 at 17:21, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> - Update those platforms to check if DT has "compatible" string in CPU
>> node.
>> If yes, don't create a device as this will be created by cpufreq-dt.
>>
>
> This fixes how it will work with new DTs, how about old DTs ?
> IMO we still need the blacklist kind of logic I sent before.

Yes..

> I don't think this is something acceptable, why will someone want to
> lose a working feature with new kernel.

If some platform maintainers care for cleaner/simpler code instead
of backward compatibility ..

For example, exynos has just started to use cpufreq-dt. Probably
in 3.18 or may be 19 as well.. So, they might not care about
backward compatibility at all.. Let me see if I can come up with
a solution that looks reasonably clean.

--
viresh