[PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[Thomas Abraham]

Changes since v5:
- Fixes for checkpatch warnings with --strict option as suggested by
  Nishanth Menon.

Changes since v4:
- Code improvements suggested by Viresh Kumar.

Changes since v3:
- Minor changes as suggested in the last version.

Changes since v2:
- Reworked based on the "PM / OPP: move cpufreq specific helpers out of OPP 
  layer" patch series posted by Nishanth Menon.

Changes since v1:
- Boost mode frequencies are specfied as a set of frequencies instead of
  specifying them as OPPs. Thanks to Nishanth, Lukasz and Rob for the
  feedback.

This series has been acked by authors and maintainers of the code
portions changed in this series. The new DT binding introduced in this
patch needs an ack from device tree binding maintainers as well.

Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
support for CPU boost mode for CPUfreq drivers. To use the new boost
mode, CPUfreq drivers have to specify the boost mode frequency and
voltage within the CPUfreq driver, which is the case for Exynos4x12
CPUfreq driver.

But for CPUfreq drivers which obtain the OPPs from cpus node, this
patch series adds support to specify boost mode frequencies in the
cpu device tree node. This requirement came up when Lukasz pointed
out the regression caused by the Exynos CPUfreq driver consolidation
patches.

[PATCH v6 1/2] cpufreq / OPP: Allow boost frequency to be looked up from device tree

[Thomas Abraham]

From: Thomas Abraham <thomas.ab@samsung.com>

Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
support for CPU boost mode. This patch adds support for finding available
boost frequencies from device tree and marking them as usable in boost mode.

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
Acked-by: Nishanth Menon <nm@ti.com>
Acked-by: Lukasz Majewski <l.majewski@samsung.com>
---
 drivers/cpufreq/cpufreq_opp.c |   45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c
index c0c6f4a..a13019a 100644
--- a/drivers/cpufreq/cpufreq_opp.c
+++ b/drivers/cpufreq/cpufreq_opp.c
@@ -19,6 +19,7 @@
 #include <linux/pm_opp.h>
 #include <linux/rcupdate.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 /**
  * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device
@@ -51,6 +52,11 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
 	struct cpufreq_frequency_table *freq_table = NULL;
 	int i, max_opps, ret = 0;
 	unsigned long rate;
+#ifdef CONFIG_CPU_FREQ_BOOST_SW
+	struct cpufreq_frequency_table *ft;
+	int len, count;
+	u32 *boost_freqs = NULL;
+#endif
 
 	rcu_read_lock();
 
@@ -82,6 +88,45 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
 
 	*table = &freq_table[0];
 
+#ifdef CONFIG_CPU_FREQ_BOOST_SW
+	if (!of_find_property(dev->of_node, "boost-frequencies", &len))
+		goto out;
+
+	if (!len || !IS_ALIGNED(len, sizeof(u32))) {
+		dev_err(dev, "%s: invalid boost frequency\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	boost_freqs = kmalloc(len, GFP_KERNEL);
+	if (!boost_freqs) {
+		dev_err(dev, "%s: no memory for boost freq table\n", __func__);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	count = len / sizeof(u32);
+	of_property_read_u32_array(dev->of_node, "boost-frequencies",
+				   boost_freqs, count);
+
+	for (i = 0; i < count; i++) {
+		cpufreq_for_each_valid_entry(ft, *table) {
+			if (boost_freqs[i] == ft->frequency) {
+				ft->flags |= CPUFREQ_BOOST_FREQ;
+				pr_debug("%s: marked %d as boost frequency\n",
+					 __func__, boost_freqs[i]);
+				break;
+			}
+		}
+
+		if (ft->frequency == CPUFREQ_TABLE_END)
+			dev_err(dev, "%s: invalid boost frequency %d\n",
+				__func__, boost_freqs[i]);
+	}
+
+	kfree(boost_freqs);
+#endif
+
 out:
 	rcu_read_unlock();
 	if (ret)
-- 
1.7.9.5

[PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Thomas Abraham]

From: Thomas Abraham <thomas.ab@samsung.com>

Add a new optional boost-frequency binding for specifying the frequencies
usable in boost mode.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Nishanth Menon <nm@ti.com>
Acked-by: Lukasz Majewski <l.majewski@samsung.com>
---
 .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
new file mode 100644
index 0000000..63ed0fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
@@ -0,0 +1,38 @@
+* Device tree binding for CPU boost frequency (aka over-clocking)
+
+Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
+overclocking) in which the CPU can operate at frequencies which are not
+specified by the manufacturer as CPU's operating frequency.
+
+Optional Properties:
+- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
+  This list should be a subset of frequencies listed in "operating-points"
+  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
+  details about "operating-points" property.
+
+Example:
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a9";
+			reg = <0>;
+
+			operating-points = <
+				1500000 1350000
+				1400000 1287500
+				1300000 1250000
+				1200000 1187500
+				1100000 1137500
+				1000000 1087500
+			>;
+			boost-frequencies = <1500000 1400000>;
+		};
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a9";
+			reg = <1>;
+		};
+	};
-- 
1.7.9.5

Re: [PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[Rafael J. Wysocki]

On Friday, May 30, 2014 02:31:15 PM Thomas Abraham wrote:
> Changes since v5:
> - Fixes for checkpatch warnings with --strict option as suggested by
>   Nishanth Menon.
> 
> Changes since v4:
> - Code improvements suggested by Viresh Kumar.
> 
> Changes since v3:
> - Minor changes as suggested in the last version.
> 
> Changes since v2:
> - Reworked based on the "PM / OPP: move cpufreq specific helpers out of OPP 
>   layer" patch series posted by Nishanth Menon.
> 
> Changes since v1:
> - Boost mode frequencies are specfied as a set of frequencies instead of
>   specifying them as OPPs. Thanks to Nishanth, Lukasz and Rob for the
>   feedback.
> 
> This series has been acked by authors and maintainers of the code
> portions changed in this series. The new DT binding introduced in this
> patch needs an ack from device tree binding maintainers as well.
> 
> Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
> support for CPU boost mode for CPUfreq drivers. To use the new boost
> mode, CPUfreq drivers have to specify the boost mode frequency and
> voltage within the CPUfreq driver, which is the case for Exynos4x12
> CPUfreq driver.
> 
> But for CPUfreq drivers which obtain the OPPs from cpus node, this
> patch series adds support to specify boost mode frequencies in the
> cpu device tree node. This requirement came up when Lukasz pointed
> out the regression caused by the Exynos CPUfreq driver consolidation
> patches.

Both patches queued up for 3.16 in the linux-pm tree.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Mark Rutland]

Hi,

Apologies for being somewhat late w.r.t. review on this.

On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> Add a new optional boost-frequency binding for specifying the frequencies
> usable in boost mode.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Nishanth Menon <nm@ti.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> new file mode 100644
> index 0000000..63ed0fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> @@ -0,0 +1,38 @@
> +* Device tree binding for CPU boost frequency (aka over-clocking)
> +
> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as

Nit: CPUs (we're not greengrocers [1])

> +overclocking) in which the CPU can operate at frequencies which are not
> +specified by the manufacturer as CPU's operating frequency.
> +
> +Optional Properties:
> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
> +  This list should be a subset of frequencies listed in "operating-points"
> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
> +  details about "operating-points" property.

What is 'boost-mode'?

What are the limitations on boost frequencies? When is a CPU expected to
go to these frequencies and for now long? When should I as a dt author
place elements in boost-frequencies?

Why are these in both operating-points and boost-frequencies? It'll be
really easy to accidentally forget to mark something as a
boost-frequency this way. Why not have a boost-points instead?

> +
> +Example:
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a9";
> +			reg = <0>;
> +
> +			operating-points = <
> +				1500000 1350000
> +				1400000 1287500
> +				1300000 1250000
> +				1200000 1187500
> +				1100000 1137500
> +				1000000 1087500
> +			>;
> +			boost-frequencies = <1500000 1400000>;

This is more of a general issue, but I hate the whole cpufreq-cpu0 way
of assuming that all CPUs mirror CPU0.

It would be nicer if either this were dropped in /cpus or repeated
per-cpu.

Cheers,
Mark.

> +		};
> +		cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a9";
> +			reg = <1>;
> +		};
> +	};
> -- 
> 1.7.9.5
> 
> 

[1] http://en.wikipedia.org/wiki/Apostrophe#Superfluous_apostrophes_.28.22greengrocers.27_apostrophes.22.29

Re: [PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[Rafael J. Wysocki]

On Friday, May 30, 2014 02:19:06 PM Rafael J. Wysocki wrote:
> On Friday, May 30, 2014 02:31:15 PM Thomas Abraham wrote:
> > Changes since v5:
> > - Fixes for checkpatch warnings with --strict option as suggested by
> >   Nishanth Menon.
> > 
> > Changes since v4:
> > - Code improvements suggested by Viresh Kumar.
> > 
> > Changes since v3:
> > - Minor changes as suggested in the last version.
> > 
> > Changes since v2:
> > - Reworked based on the "PM / OPP: move cpufreq specific helpers out of OPP 
> >   layer" patch series posted by Nishanth Menon.
> > 
> > Changes since v1:
> > - Boost mode frequencies are specfied as a set of frequencies instead of
> >   specifying them as OPPs. Thanks to Nishanth, Lukasz and Rob for the
> >   feedback.
> > 
> > This series has been acked by authors and maintainers of the code
> > portions changed in this series. The new DT binding introduced in this
> > patch needs an ack from device tree binding maintainers as well.
> > 
> > Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
> > support for CPU boost mode for CPUfreq drivers. To use the new boost
> > mode, CPUfreq drivers have to specify the boost mode frequency and
> > voltage within the CPUfreq driver, which is the case for Exynos4x12
> > CPUfreq driver.
> > 
> > But for CPUfreq drivers which obtain the OPPs from cpus node, this
> > patch series adds support to specify boost mode frequencies in the
> > cpu device tree node. This requirement came up when Lukasz pointed
> > out the regression caused by the Exynos CPUfreq driver consolidation
> > patches.
> 
> Both patches queued up for 3.16 in the linux-pm tree.

But please address the Mark's comments or I'll drop them.

Rafael

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Thomas Abraham]

Hi Mark,

On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> Apologies for being somewhat late w.r.t. review on this.
>
> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> Add a new optional boost-frequency binding for specifying the frequencies
>> usable in boost mode.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> Acked-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>> ---
>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>> new file mode 100644
>> index 0000000..63ed0fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>> @@ -0,0 +1,38 @@
>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>> +
>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>
> Nit: CPUs (we're not greengrocers [1])
>
>> +overclocking) in which the CPU can operate at frequencies which are not
>> +specified by the manufacturer as CPU's operating frequency.
>> +
>> +Optional Properties:
>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>> +  This list should be a subset of frequencies listed in "operating-points"
>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>> +  details about "operating-points" property.
>
> What is 'boost-mode'?

boost-mode activates additional one or more cpu clock speeds (which
are not specified as operating frequency of the cpu by the
manufacturer). The cpu is allowed to operate in these boost mode
speeds when the boost mode is active. The boost mode speeds are
usually undocumented. Some of the chip samples could be clocked in
boost mode speeds and only such samples can be safely operated in
boost mode.

The mechanism of entry into and exit out of boost mode is outside the
scope of this documentation.

>
> What are the limitations on boost frequencies? When is a CPU expected to
> go to these frequencies and for now long? When should I as a dt author
> place elements in boost-frequencies?

I will add these details in the next revision of this patch.

>
> Why are these in both operating-points and boost-frequencies? It'll be
> really easy to accidentally forget to mark something as a
> boost-frequency this way. Why not have a boost-points instead?

Does boost-points mean a set of clock speeds which are not listed as
part of operating-points property? If yes, that also is a possible
implementation (it was implemented in one of the earlier version of
this series). Could you confirm that you want the boost mode speeds to
be exclusive of the speeds listed in operating-points?

Thanks,
Thomas.

>
>> +
>> +Example:
>> +
>> +     cpus {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             cpu@0 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a9";
>> +                     reg = <0>;
>> +
>> +                     operating-points = <
>> +                             1500000 1350000
>> +                             1400000 1287500
>> +                             1300000 1250000
>> +                             1200000 1187500
>> +                             1100000 1137500
>> +                             1000000 1087500
>> +                     >;
>> +                     boost-frequencies = <1500000 1400000>;
>
> This is more of a general issue, but I hate the whole cpufreq-cpu0 way
> of assuming that all CPUs mirror CPU0.
>
> It would be nicer if either this were dropped in /cpus or repeated
> per-cpu.
>
> Cheers,
> Mark.
>
>> +             };
>> +             cpu@1 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a9";
>> +                     reg = <1>;
>> +             };
>> +     };
>> --
>> 1.7.9.5
>>
>>
>
> [1] http://en.wikipedia.org/wiki/Apostrophe#Superfluous_apostrophes_.28.22greengrocers.27_apostrophes.22.29
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[Thomas Abraham]

On Fri, May 30, 2014 at 8:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, May 30, 2014 02:19:06 PM Rafael J. Wysocki wrote:
>> On Friday, May 30, 2014 02:31:15 PM Thomas Abraham wrote:
>> > Changes since v5:
>> > - Fixes for checkpatch warnings with --strict option as suggested by
>> >   Nishanth Menon.
>> >
>> > Changes since v4:
>> > - Code improvements suggested by Viresh Kumar.
>> >
>> > Changes since v3:
>> > - Minor changes as suggested in the last version.
>> >
>> > Changes since v2:
>> > - Reworked based on the "PM / OPP: move cpufreq specific helpers out of OPP
>> >   layer" patch series posted by Nishanth Menon.
>> >
>> > Changes since v1:
>> > - Boost mode frequencies are specfied as a set of frequencies instead of
>> >   specifying them as OPPs. Thanks to Nishanth, Lukasz and Rob for the
>> >   feedback.
>> >
>> > This series has been acked by authors and maintainers of the code
>> > portions changed in this series. The new DT binding introduced in this
>> > patch needs an ack from device tree binding maintainers as well.
>> >
>> > Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
>> > support for CPU boost mode for CPUfreq drivers. To use the new boost
>> > mode, CPUfreq drivers have to specify the boost mode frequency and
>> > voltage within the CPUfreq driver, which is the case for Exynos4x12
>> > CPUfreq driver.
>> >
>> > But for CPUfreq drivers which obtain the OPPs from cpus node, this
>> > patch series adds support to specify boost mode frequencies in the
>> > cpu device tree node. This requirement came up when Lukasz pointed
>> > out the regression caused by the Exynos CPUfreq driver consolidation
>> > patches.
>>
>> Both patches queued up for 3.16 in the linux-pm tree.
>
> But please address the Mark's comments or I'll drop them.

Hi Rafael,

Please drop this series for now. Both the patches have to reworked
based on Mark's comments.

Thanks,
Thomas.

>
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 20:05, Thomas Abraham wrote:
> Hi Mark,
> 
> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi,
>>
>> Apologies for being somewhat late w.r.t. review on this.
>>
>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>
>>> Add a new optional boost-frequency binding for specifying the frequencies
>>> usable in boost mode.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> Cc: Kumar Gala <galak@codeaurora.org>
>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> Acked-by: Nishanth Menon <nm@ti.com>
>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>> ---
>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>> new file mode 100644
>>> index 0000000..63ed0fc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>> @@ -0,0 +1,38 @@
>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>> +
>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>
>> Nit: CPUs (we're not greengrocers [1])
>>
>>> +overclocking) in which the CPU can operate at frequencies which are not
>>> +specified by the manufacturer as CPU's operating frequency.
>>> +
>>> +Optional Properties:
>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>> +  This list should be a subset of frequencies listed in "operating-points"
>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>> +  details about "operating-points" property.
>>
>> What is 'boost-mode'?
> 
> boost-mode activates additional one or more cpu clock speeds (which
> are not specified as operating frequency of the cpu by the
> manufacturer). The cpu is allowed to operate in these boost mode
> speeds when the boost mode is active. The boost mode speeds are
> usually undocumented. Some of the chip samples could be clocked in
> boost mode speeds and only such samples can be safely operated in
> boost mode.
> 

IMHO the most important part that I believe should be stated in the
documentation is that CPU usually can operate in boost mode for limited
amount of time, which depends on thermal conditions, which makes the
boost operating points separate from normal ones, which can be used at
any time.

> The mechanism of entry into and exit out of boost mode is outside the
> scope of this documentation.
> 
>>
>> What are the limitations on boost frequencies? When is a CPU expected to
>> go to these frequencies and for now long? When should I as a dt author
>> place elements in boost-frequencies?
> 
> I will add these details in the next revision of this patch.
> 
>>
>> Why are these in both operating-points and boost-frequencies? It'll be
>> really easy to accidentally forget to mark something as a
>> boost-frequency this way. Why not have a boost-points instead?
> 
> Does boost-points mean a set of clock speeds which are not listed as
> part of operating-points property? If yes, that also is a possible
> implementation (it was implemented in one of the earlier version of
> this series). Could you confirm that you want the boost mode speeds to
> be exclusive of the speeds listed in operating-points?

It seems reasonable to have boost operating points completely separate
from normal ones, so that a kernel without support for boost mode will
not use them. Also considering my comment above, logically boost
operating points are not considered normal operating points, due to
various constraints that need to be met to use them (i.e. mostly thermal
conditions).

> 
> Thanks,
> Thomas.
> 
>>
>>> +
>>> +Example:
>>> +
>>> +     cpus {
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>> +             cpu@0 {
>>> +                     device_type = "cpu";
>>> +                     compatible = "arm,cortex-a9";
>>> +                     reg = <0>;
>>> +
>>> +                     operating-points = <
>>> +                             1500000 1350000
>>> +                             1400000 1287500
>>> +                             1300000 1250000
>>> +                             1200000 1187500
>>> +                             1100000 1137500
>>> +                             1000000 1087500
>>> +                     >;
>>> +                     boost-frequencies = <1500000 1400000>;
>>
>> This is more of a general issue, but I hate the whole cpufreq-cpu0 way
>> of assuming that all CPUs mirror CPU0.
>>
>> It would be nicer if either this were dropped in /cpus or repeated
>> per-cpu.

Would it make any difference?

IMHO, as long as the binding is well defined and doesn't cause
significant issues, I don't think there is a reason to redesign it and
add a necessity to support both new and old variants.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Thomas Abraham]

On Fri, May 30, 2014 at 11:45 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
>
> On 30.05.2014 20:05, Thomas Abraham wrote:
>> Hi Mark,
>>
>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> Apologies for being somewhat late w.r.t. review on this.
>>>
>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>
>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>> usable in boost mode.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> new file mode 100644
>>>> index 0000000..63ed0fc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> @@ -0,0 +1,38 @@
>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>> +
>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>
>>> Nit: CPUs (we're not greengrocers [1])
>>>
>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>> +specified by the manufacturer as CPU's operating frequency.
>>>> +
>>>> +Optional Properties:
>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>> +  details about "operating-points" property.
>>>
>>> What is 'boost-mode'?
>>
>> boost-mode activates additional one or more cpu clock speeds (which
>> are not specified as operating frequency of the cpu by the
>> manufacturer). The cpu is allowed to operate in these boost mode
>> speeds when the boost mode is active. The boost mode speeds are
>> usually undocumented. Some of the chip samples could be clocked in
>> boost mode speeds and only such samples can be safely operated in
>> boost mode.
>>
>
> IMHO the most important part that I believe should be stated in the
> documentation is that CPU usually can operate in boost mode for limited
> amount of time, which depends on thermal conditions, which makes the
> boost operating points separate from normal ones, which can be used at
> any time.

The factors that allow a CPU to operate in boost mode and the duration
the CPU can operate in boost mode clock speed is a CPU specific
description. This binding does not describe such CPU specific
behavior. This binding only states the additional clock speeds CPU is
allowed to operate in boost mode. The boost mode entry and exit
conditions are implementation specific and not part of the scope of
this binding and documentation.

>
>> The mechanism of entry into and exit out of boost mode is outside the
>> scope of this documentation.
>>
>>>
>>> What are the limitations on boost frequencies? When is a CPU expected to
>>> go to these frequencies and for now long? When should I as a dt author
>>> place elements in boost-frequencies?
>>
>> I will add these details in the next revision of this patch.
>>
>>>
>>> Why are these in both operating-points and boost-frequencies? It'll be
>>> really easy to accidentally forget to mark something as a
>>> boost-frequency this way. Why not have a boost-points instead?
>>
>> Does boost-points mean a set of clock speeds which are not listed as
>> part of operating-points property? If yes, that also is a possible
>> implementation (it was implemented in one of the earlier version of
>> this series). Could you confirm that you want the boost mode speeds to
>> be exclusive of the speeds listed in operating-points?
>
> It seems reasonable to have boost operating points completely separate
> from normal ones, so that a kernel without support for boost mode will
> not use them. Also considering my comment above, logically boost
> operating points are not considered normal operating points, due to
> various constraints that need to be met to use them (i.e. mostly thermal
> conditions).
>
>>
>> Thanks,
>> Thomas.
>>
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +     cpus {
>>>> +             #address-cells = <1>;
>>>> +             #size-cells = <0>;
>>>> +             cpu@0 {
>>>> +                     device_type = "cpu";
>>>> +                     compatible = "arm,cortex-a9";
>>>> +                     reg = <0>;
>>>> +
>>>> +                     operating-points = <
>>>> +                             1500000 1350000
>>>> +                             1400000 1287500
>>>> +                             1300000 1250000
>>>> +                             1200000 1187500
>>>> +                             1100000 1137500
>>>> +                             1000000 1087500
>>>> +                     >;
>>>> +                     boost-frequencies = <1500000 1400000>;
>>>
>>> This is more of a general issue, but I hate the whole cpufreq-cpu0 way
>>> of assuming that all CPUs mirror CPU0.
>>>
>>> It would be nicer if either this were dropped in /cpus or repeated
>>> per-cpu.
>
> Would it make any difference?
>
> IMHO, as long as the binding is well defined and doesn't cause
> significant issues, I don't think there is a reason to redesign it and
> add a necessity to support both new and old variants.
>
> Best regards,
> Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 20:33, Thomas Abraham wrote:
> On Fri, May 30, 2014 at 11:45 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>
>>
>> On 30.05.2014 20:05, Thomas Abraham wrote:
>>> Hi Mark,
>>>
>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> Hi,
>>>>
>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>
>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>
>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>> usable in boost mode.
>>>>>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>> ---
>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>  1 file changed, 38 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>> new file mode 100644
>>>>> index 0000000..63ed0fc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>> @@ -0,0 +1,38 @@
>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>> +
>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>
>>>> Nit: CPUs (we're not greengrocers [1])
>>>>
>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>> +
>>>>> +Optional Properties:
>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>> +  details about "operating-points" property.
>>>>
>>>> What is 'boost-mode'?
>>>
>>> boost-mode activates additional one or more cpu clock speeds (which
>>> are not specified as operating frequency of the cpu by the
>>> manufacturer). The cpu is allowed to operate in these boost mode
>>> speeds when the boost mode is active. The boost mode speeds are
>>> usually undocumented. Some of the chip samples could be clocked in
>>> boost mode speeds and only such samples can be safely operated in
>>> boost mode.
>>>
>>
>> IMHO the most important part that I believe should be stated in the
>> documentation is that CPU usually can operate in boost mode for limited
>> amount of time, which depends on thermal conditions, which makes the
>> boost operating points separate from normal ones, which can be used at
>> any time.
> 
> The factors that allow a CPU to operate in boost mode and the duration
> the CPU can operate in boost mode clock speed is a CPU specific
> description. This binding does not describe such CPU specific
> behavior. This binding only states the additional clock speeds CPU is
> allowed to operate in boost mode. The boost mode entry and exit
> conditions are implementation specific and not part of the scope of
> this binding and documentation.

The binding documentation describes properties related to boost mode and
so it must explain what boost mode is, otherwise those properties will
not make any sense. I'm not saying that specific constraints must be
listed, but the text should mention that those operating points might
need some.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Sudeep Holla]

On 30/05/14 19:15, Tomasz Figa wrote:
>
>
> On 30.05.2014 20:05, Thomas Abraham wrote:
>> Hi Mark,
>>
>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> Apologies for being somewhat late w.r.t. review on this.
>>>
>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>
>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>> usable in boost mode.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>>   .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>   1 file changed, 38 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> new file mode 100644
>>>> index 0000000..63ed0fc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> @@ -0,0 +1,38 @@
>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>> +
>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>
>>> Nit: CPUs (we're not greengrocers [1])
>>>
>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>> +specified by the manufacturer as CPU's operating frequency.
>>>> +
>>>> +Optional Properties:
>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>> +  details about "operating-points" property.
>>>
>>> What is 'boost-mode'?
>>
>> boost-mode activates additional one or more cpu clock speeds (which
>> are not specified as operating frequency of the cpu by the
>> manufacturer). The cpu is allowed to operate in these boost mode
>> speeds when the boost mode is active. The boost mode speeds are
>> usually undocumented. Some of the chip samples could be clocked in
>> boost mode speeds and only such samples can be safely operated in
>> boost mode.
>>
>
> IMHO the most important part that I believe should be stated in the
> documentation is that CPU usually can operate in boost mode for limited
> amount of time, which depends on thermal conditions, which makes the
> boost operating points separate from normal ones, which can be used at
> any time.
>

Yes exactly what I mentioned couple of times on previous version of this
patch set[1][2]

>> The mechanism of entry into and exit out of boost mode is outside the
>> scope of this documentation.
>>
>>>
>>> What are the limitations on boost frequencies? When is a CPU expected to
>>> go to these frequencies and for now long? When should I as a dt author
>>> place elements in boost-frequencies?
>>
>> I will add these details in the next revision of this patch.
>>
>>>
>>> Why are these in both operating-points and boost-frequencies? It'll be
>>> really easy to accidentally forget to mark something as a
>>> boost-frequency this way. Why not have a boost-points instead?
>>

I was told that index is not preferred based on the previous discussions
when the OPP bindings were designed. In addition the OPP binding doesn't
enforce any ordering. There are thermal bindings that assume otherwise and
is broken. So boost-points is not feasible.

>> Does boost-points mean a set of clock speeds which are not listed as
>> part of operating-points property? If yes, that also is a possible
>> implementation (it was implemented in one of the earlier version of
>> this series). Could you confirm that you want the boost mode speeds to
>> be exclusive of the speeds listed in operating-points?
>
> It seems reasonable to have boost operating points completely separate
> from normal ones, so that a kernel without support for boost mode will
> not use them. Also considering my comment above, logically boost
> operating points are not considered normal operating points, due to
> various constraints that need to be met to use them (i.e. mostly thermal
> conditions).
>

IMO, at-least the existing OPP binding can't distinguish between under-,
nominal- and over-drive OPP points. So my suggestion was to have a property
that provides the beginning of these 3 points on the OPP curve.

Regards,
Sudeep

[1] https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg26250.html
[2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/31552

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 20:38, Sudeep Holla wrote:
> On 30/05/14 19:15, Tomasz Figa wrote:
>> On 30.05.2014 20:05, Thomas Abraham wrote:
>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com>
>>> wrote:

[snip]

>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>> really easy to accidentally forget to mark something as a
>>>> boost-frequency this way. Why not have a boost-points instead?
>>>
> 
> I was told that index is not preferred based on the previous discussions
> when the OPP bindings were designed. In addition the OPP binding doesn't
> enforce any ordering. There are thermal bindings that assume otherwise and
> is broken. So boost-points is not feasible.
> 

My understanding of Mark's comment was that the boost-points property
would use the same format as operating-points and parsing code would
just concatenate operating points with boost points after making the
latter with necessary flag or whatever.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Sudeep Holla]

On 30/05/14 19:41, Tomasz Figa wrote:
> On 30.05.2014 20:38, Sudeep Holla wrote:
>> On 30/05/14 19:15, Tomasz Figa wrote:
>>> On 30.05.2014 20:05, Thomas Abraham wrote:
>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com>
>>>> wrote:
>
> [snip]
>
>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>> really easy to accidentally forget to mark something as a
>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>
>>
>> I was told that index is not preferred based on the previous discussions
>> when the OPP bindings were designed. In addition the OPP binding doesn't
>> enforce any ordering. There are thermal bindings that assume otherwise and
>> is broken. So boost-points is not feasible.
>>
>
> My understanding of Mark's comment was that the boost-points property
> would use the same format as operating-points and parsing code would
> just concatenate operating points with boost points after making the
> latter with necessary flag or whatever.
>

Ah, I misunderstood that. That should be fine as it avoids duplication.

Regards,
Sudeep

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Mark Rutland]

On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
> Hi Mark,
> 
> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > Apologies for being somewhat late w.r.t. review on this.
> >
> > On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
> >> From: Thomas Abraham <thomas.ab@samsung.com>
> >>
> >> Add a new optional boost-frequency binding for specifying the frequencies
> >> usable in boost mode.
> >>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Pawel Moll <pawel.moll@arm.com>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> >> Cc: Kumar Gala <galak@codeaurora.org>
> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> Acked-by: Nishanth Menon <nm@ti.com>
> >> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> >> ---
> >>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> >> new file mode 100644
> >> index 0000000..63ed0fc
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> >> @@ -0,0 +1,38 @@
> >> +* Device tree binding for CPU boost frequency (aka over-clocking)
> >> +
> >> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
> >
> > Nit: CPUs (we're not greengrocers [1])
> >
> >> +overclocking) in which the CPU can operate at frequencies which are not
> >> +specified by the manufacturer as CPU's operating frequency.
> >> +
> >> +Optional Properties:
> >> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
> >> +  This list should be a subset of frequencies listed in "operating-points"
> >> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
> >> +  details about "operating-points" property.
> >
> > What is 'boost-mode'?
> 
> boost-mode activates additional one or more cpu clock speeds (which
> are not specified as operating frequency of the cpu by the
> manufacturer). The cpu is allowed to operate in these boost mode
> speeds when the boost mode is active. The boost mode speeds are
> usually undocumented. Some of the chip samples could be clocked in
> boost mode speeds and only such samples can be safely operated in
> boost mode.
> 
> The mechanism of entry into and exit out of boost mode is outside the
> scope of this documentation.
> 
> >
> > What are the limitations on boost frequencies? When is a CPU expected to
> > go to these frequencies and for now long? When should I as a dt author
> > place elements in boost-frequencies?
> 
> I will add these details in the next revision of this patch.

Cheers.

> >
> > Why are these in both operating-points and boost-frequencies? It'll be
> > really easy to accidentally forget to mark something as a
> > boost-frequency this way. Why not have a boost-points instead?
> 
> Does boost-points mean a set of clock speeds which are not listed as
> part of operating-points property? If yes, that also is a possible
> implementation (it was implemented in one of the earlier version of
> this series). Could you confirm that you want the boost mode speeds to
> be exclusive of the speeds listed in operating-points?

If these boost mode operating points are not generally advisable for use
as the other operating-points are, then they should IMO been in an
entirely separate property exclusive of (but in the same format as) the
operating-points property, e.g.

operating points = <A B>, <C D>;
boost-points = <E F>;

Otherwise, without boost-mode support we have to parse the boost mode
table to figure out which points to avoid. Or if someone typos a value
in either table we might go into a boost mode when we didn't want to!

Cheers,
Mark.

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Nishanth Menon]

On 05/30/2014 01:55 PM, Mark Rutland wrote:
> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>> Hi Mark,
>>
>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> Apologies for being somewhat late w.r.t. review on this.
>>>
>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>
>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>> usable in boost mode.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> new file mode 100644
>>>> index 0000000..63ed0fc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>> @@ -0,0 +1,38 @@
>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>> +
>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>
>>> Nit: CPUs (we're not greengrocers [1])
>>>
>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>> +specified by the manufacturer as CPU's operating frequency.
>>>> +
>>>> +Optional Properties:
>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>> +  details about "operating-points" property.
>>>
>>> What is 'boost-mode'?
>>
>> boost-mode activates additional one or more cpu clock speeds (which
>> are not specified as operating frequency of the cpu by the
>> manufacturer). The cpu is allowed to operate in these boost mode
>> speeds when the boost mode is active. The boost mode speeds are
>> usually undocumented. Some of the chip samples could be clocked in
>> boost mode speeds and only such samples can be safely operated in
>> boost mode.
>>
>> The mechanism of entry into and exit out of boost mode is outside the
>> scope of this documentation.
>>
>>>
>>> What are the limitations on boost frequencies? When is a CPU expected to
>>> go to these frequencies and for now long? When should I as a dt author
>>> place elements in boost-frequencies?
>>
>> I will add these details in the next revision of this patch.
> 
> Cheers.
> 
>>>
>>> Why are these in both operating-points and boost-frequencies? It'll be
>>> really easy to accidentally forget to mark something as a
>>> boost-frequency this way. Why not have a boost-points instead?
>>
>> Does boost-points mean a set of clock speeds which are not listed as
>> part of operating-points property? If yes, that also is a possible
>> implementation (it was implemented in one of the earlier version of
>> this series). Could you confirm that you want the boost mode speeds to
>> be exclusive of the speeds listed in operating-points?
> 
> If these boost mode operating points are not generally advisable for use
> as the other operating-points are, then they should IMO been in an
> entirely separate property exclusive of (but in the same format as) the
> operating-points property, e.g.
> 
> operating points = <A B>, <C D>;
> boost-points = <E F>;

you are asking boost frequencies to remain for ever tied down to OPP
style description.

What we are trying to describe? "What are my SoC's overclocked
frequencies"? That description can be used even in a system that does
not use OPP style table (say ACPI based OPP tables or whatever
acronymned table).

Tying it down to operating points just because we have it today as a
convenient description, is limiting.

Further, if we decide to educate boost-frequencies to also indicate
how long is it safe? That does indeed belong to boost-frequency
description and not OPP description. Or if we decide to change
operating-points description[1] in the future has an impact on
"boost-points" description, when it should not have.

> 
> Otherwise, without boost-mode support we have to parse the boost mode
> table to figure out which points to avoid. Or if someone typos a value
That is OS usage of h/w description - yeah - in anycase, if OS has no
ability to deal with boost-frequencies, it should skip it for sure.

> in either table we might go into a boost mode when we didn't want to!
> 
There are other ways to screw up device with dts typo. you could give
a wrong voltage(extra 0?) and ensure you never use the chip ever
again.. typos are dt bugs, we can do the best to write robust code to
report them.


[1] http://marc.info/?t=140059618500008&r=1&w=2
-- 
Regards,
Nishanth Menon

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 21:50, Nishanth Menon wrote:
> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>> Hi Mark,
>>>
>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> Hi,
>>>>
>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>
>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>
>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>> usable in boost mode.
>>>>>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>> ---
>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>  1 file changed, 38 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>> new file mode 100644
>>>>> index 0000000..63ed0fc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>> @@ -0,0 +1,38 @@
>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>> +
>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>
>>>> Nit: CPUs (we're not greengrocers [1])
>>>>
>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>> +
>>>>> +Optional Properties:
>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>> +  details about "operating-points" property.
>>>>
>>>> What is 'boost-mode'?
>>>
>>> boost-mode activates additional one or more cpu clock speeds (which
>>> are not specified as operating frequency of the cpu by the
>>> manufacturer). The cpu is allowed to operate in these boost mode
>>> speeds when the boost mode is active. The boost mode speeds are
>>> usually undocumented. Some of the chip samples could be clocked in
>>> boost mode speeds and only such samples can be safely operated in
>>> boost mode.
>>>
>>> The mechanism of entry into and exit out of boost mode is outside the
>>> scope of this documentation.
>>>
>>>>
>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>> go to these frequencies and for now long? When should I as a dt author
>>>> place elements in boost-frequencies?
>>>
>>> I will add these details in the next revision of this patch.
>>
>> Cheers.
>>
>>>>
>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>> really easy to accidentally forget to mark something as a
>>>> boost-frequency this way. Why not have a boost-points instead?
>>>
>>> Does boost-points mean a set of clock speeds which are not listed as
>>> part of operating-points property? If yes, that also is a possible
>>> implementation (it was implemented in one of the earlier version of
>>> this series). Could you confirm that you want the boost mode speeds to
>>> be exclusive of the speeds listed in operating-points?
>>
>> If these boost mode operating points are not generally advisable for use
>> as the other operating-points are, then they should IMO been in an
>> entirely separate property exclusive of (but in the same format as) the
>> operating-points property, e.g.
>>
>> operating points = <A B>, <C D>;
>> boost-points = <E F>;
> 
> you are asking boost frequencies to remain for ever tied down to OPP
> style description.
> 
> What we are trying to describe? "What are my SoC's overclocked
> frequencies"? That description can be used even in a system that does
> not use OPP style table (say ACPI based OPP tables or whatever
> acronymned table).
> 
> Tying it down to operating points just because we have it today as a
> convenient description, is limiting.
> 
> Further, if we decide to educate boost-frequencies to also indicate
> how long is it safe? That does indeed belong to boost-frequency
> description and not OPP description. Or if we decide to change
> operating-points description[1] in the future has an impact on
> "boost-points" description, when it should not have.
> 
>>
>> Otherwise, without boost-mode support we have to parse the boost mode
>> table to figure out which points to avoid. Or if someone typos a value
> That is OS usage of h/w description - yeah - in anycase, if OS has no
> ability to deal with boost-frequencies, it should skip it for sure.
> 
>> in either table we might go into a boost mode when we didn't want to!
>>
> There are other ways to screw up device with dts typo. you could give
> a wrong voltage(extra 0?) and ensure you never use the chip ever
> again.. typos are dt bugs, we can do the best to write robust code to
> report them.
> 

Typos are not the primary thing to worry about here. Adding boost
frequencies to the list of primary operating points is flawed, because
an OS that has no idea of boost mode will use them as normal operating
points and this is not desired.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Nishanth Menon]

On 05/30/2014 03:02 PM, Tomasz Figa wrote:
> On 30.05.2014 21:50, Nishanth Menon wrote:
>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>> Hi Mark,
>>>>
>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>> Hi,
>>>>>
>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>
>>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>
>>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>>> usable in boost mode.
>>>>>>
>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>>  1 file changed, 38 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..63ed0fc
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>> @@ -0,0 +1,38 @@
>>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>>> +
>>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>>
>>>>> Nit: CPUs (we're not greengrocers [1])
>>>>>
>>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>>> +
>>>>>> +Optional Properties:
>>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>>> +  details about "operating-points" property.
>>>>>
>>>>> What is 'boost-mode'?
>>>>
>>>> boost-mode activates additional one or more cpu clock speeds (which
>>>> are not specified as operating frequency of the cpu by the
>>>> manufacturer). The cpu is allowed to operate in these boost mode
>>>> speeds when the boost mode is active. The boost mode speeds are
>>>> usually undocumented. Some of the chip samples could be clocked in
>>>> boost mode speeds and only such samples can be safely operated in
>>>> boost mode.
>>>>
>>>> The mechanism of entry into and exit out of boost mode is outside the
>>>> scope of this documentation.
>>>>
>>>>>
>>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>>> go to these frequencies and for now long? When should I as a dt author
>>>>> place elements in boost-frequencies?
>>>>
>>>> I will add these details in the next revision of this patch.
>>>
>>> Cheers.
>>>
>>>>>
>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>> really easy to accidentally forget to mark something as a
>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>
>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>> part of operating-points property? If yes, that also is a possible
>>>> implementation (it was implemented in one of the earlier version of
>>>> this series). Could you confirm that you want the boost mode speeds to
>>>> be exclusive of the speeds listed in operating-points?
>>>
>>> If these boost mode operating points are not generally advisable for use
>>> as the other operating-points are, then they should IMO been in an
>>> entirely separate property exclusive of (but in the same format as) the
>>> operating-points property, e.g.
>>>
>>> operating points = <A B>, <C D>;
>>> boost-points = <E F>;
>>
>> you are asking boost frequencies to remain for ever tied down to OPP
>> style description.
>>
>> What we are trying to describe? "What are my SoC's overclocked
>> frequencies"? That description can be used even in a system that does
>> not use OPP style table (say ACPI based OPP tables or whatever
>> acronymned table).
>>
>> Tying it down to operating points just because we have it today as a
>> convenient description, is limiting.
>>
>> Further, if we decide to educate boost-frequencies to also indicate
>> how long is it safe? That does indeed belong to boost-frequency
>> description and not OPP description. Or if we decide to change
>> operating-points description[1] in the future has an impact on
>> "boost-points" description, when it should not have.
>>
>>>
>>> Otherwise, without boost-mode support we have to parse the boost mode
>>> table to figure out which points to avoid. Or if someone typos a value
>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>> ability to deal with boost-frequencies, it should skip it for sure.
>>
>>> in either table we might go into a boost mode when we didn't want to!
>>>
>> There are other ways to screw up device with dts typo. you could give
>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>> again.. typos are dt bugs, we can do the best to write robust code to
>> report them.
>>
> 
> Typos are not the primary thing to worry about here. Adding boost
> frequencies to the list of primary operating points is flawed, because
> an OS that has no idea of boost mode will use them as normal operating
> points and this is not desired.

That means we have an implementation bug in OS since it does not
consider the complete hardware description that device tree is
providing. An analogy will be a regulator compatible match being used
but regulator-min-microvolt and regulator-max-microvolt being ignored
by OS.

We never said that "operating points" are "primary operating points".
all we said is they are "operating points" for the device - we dont
associate meanings to it. You may add to it[1] in platform code, as we
decided to.

boost-frequencies are describing "overclocked frequencies" - should it
matter if the description of that comes from platform code OR existing
opp tables or what ever? I dont think defining them as "operating
point" style as the only way of describing overclocked frequencies are
the right approach to describing it.


[1] http://marc.info/?t=139482540400003&r=1&w=2
-- 
Regards,
Nishanth Menon

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 22:13, Nishanth Menon wrote:
> On 05/30/2014 03:02 PM, Tomasz Figa wrote:
>> On 30.05.2014 21:50, Nishanth Menon wrote:
>>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>>> Hi Mark,
>>>>>
>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>>
>>>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>
>>>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>>>> usable in boost mode.
>>>>>>>
>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..63ed0fc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>> @@ -0,0 +1,38 @@
>>>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>>>> +
>>>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>>>
>>>>>> Nit: CPUs (we're not greengrocers [1])
>>>>>>
>>>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>>>> +
>>>>>>> +Optional Properties:
>>>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>>>> +  details about "operating-points" property.
>>>>>>
>>>>>> What is 'boost-mode'?
>>>>>
>>>>> boost-mode activates additional one or more cpu clock speeds (which
>>>>> are not specified as operating frequency of the cpu by the
>>>>> manufacturer). The cpu is allowed to operate in these boost mode
>>>>> speeds when the boost mode is active. The boost mode speeds are
>>>>> usually undocumented. Some of the chip samples could be clocked in
>>>>> boost mode speeds and only such samples can be safely operated in
>>>>> boost mode.
>>>>>
>>>>> The mechanism of entry into and exit out of boost mode is outside the
>>>>> scope of this documentation.
>>>>>
>>>>>>
>>>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>>>> go to these frequencies and for now long? When should I as a dt author
>>>>>> place elements in boost-frequencies?
>>>>>
>>>>> I will add these details in the next revision of this patch.
>>>>
>>>> Cheers.
>>>>
>>>>>>
>>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>>> really easy to accidentally forget to mark something as a
>>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>>
>>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>>> part of operating-points property? If yes, that also is a possible
>>>>> implementation (it was implemented in one of the earlier version of
>>>>> this series). Could you confirm that you want the boost mode speeds to
>>>>> be exclusive of the speeds listed in operating-points?
>>>>
>>>> If these boost mode operating points are not generally advisable for use
>>>> as the other operating-points are, then they should IMO been in an
>>>> entirely separate property exclusive of (but in the same format as) the
>>>> operating-points property, e.g.
>>>>
>>>> operating points = <A B>, <C D>;
>>>> boost-points = <E F>;
>>>
>>> you are asking boost frequencies to remain for ever tied down to OPP
>>> style description.
>>>
>>> What we are trying to describe? "What are my SoC's overclocked
>>> frequencies"? That description can be used even in a system that does
>>> not use OPP style table (say ACPI based OPP tables or whatever
>>> acronymned table).
>>>
>>> Tying it down to operating points just because we have it today as a
>>> convenient description, is limiting.
>>>
>>> Further, if we decide to educate boost-frequencies to also indicate
>>> how long is it safe? That does indeed belong to boost-frequency
>>> description and not OPP description. Or if we decide to change
>>> operating-points description[1] in the future has an impact on
>>> "boost-points" description, when it should not have.
>>>
>>>>
>>>> Otherwise, without boost-mode support we have to parse the boost mode
>>>> table to figure out which points to avoid. Or if someone typos a value
>>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>>> ability to deal with boost-frequencies, it should skip it for sure.
>>>
>>>> in either table we might go into a boost mode when we didn't want to!
>>>>
>>> There are other ways to screw up device with dts typo. you could give
>>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>>> again.. typos are dt bugs, we can do the best to write robust code to
>>> report them.
>>>
>>
>> Typos are not the primary thing to worry about here. Adding boost
>> frequencies to the list of primary operating points is flawed, because
>> an OS that has no idea of boost mode will use them as normal operating
>> points and this is not desired.
> 
> That means we have an implementation bug in OS since it does not
> consider the complete hardware description that device tree is
> providing. An analogy will be a regulator compatible match being used
> but regulator-min-microvolt and regulator-max-microvolt being ignored
> by OS.

No. The operating points bindings were defined far before this
boost-frequency and so there is no requirement to support the latter.

> 
> We never said that "operating points" are "primary operating points".
> all we said is they are "operating points" for the device - we dont
> associate meanings to it. You may add to it[1] in platform code, as we
> decided to.

Maybe generic OPP bindings don't state that, but I believe that at least
cpufreq-cpu0 bindings have been defined (and the driver implemented)
this way and changing the semantics now would be breaking DT ABI
compatibility.

> 
> boost-frequencies are describing "overclocked frequencies" - should it
> matter if the description of that comes from platform code OR existing
> opp tables or what ever? I dont think defining them as "operating
> point" style as the only way of describing overclocked frequencies are
> the right approach to describing it.

Nobody said that it is the only way. The whole point here is that it
should be separate from the main operating-points property, as boost
operating points are not normal operating points and the OS must be
specifically aware how to handle them.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Nishanth Menon]

On 05/30/2014 03:19 PM, Tomasz Figa wrote:
> On 30.05.2014 22:13, Nishanth Menon wrote:
>> On 05/30/2014 03:02 PM, Tomasz Figa wrote:
>>> On 30.05.2014 21:50, Nishanth Menon wrote:
>>>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>>>> Hi Mark,
>>>>>>
>>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>>>
>>>>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>>
>>>>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>>>>> usable in boost mode.
>>>>>>>>
>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>> ---
>>>>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..63ed0fc
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>>>>> +
>>>>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>>>>
>>>>>>> Nit: CPUs (we're not greengrocers [1])
>>>>>>>
>>>>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>>>>> +
>>>>>>>> +Optional Properties:
>>>>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>>>>> +  details about "operating-points" property.
>>>>>>>
>>>>>>> What is 'boost-mode'?
>>>>>>
>>>>>> boost-mode activates additional one or more cpu clock speeds (which
>>>>>> are not specified as operating frequency of the cpu by the
>>>>>> manufacturer). The cpu is allowed to operate in these boost mode
>>>>>> speeds when the boost mode is active. The boost mode speeds are
>>>>>> usually undocumented. Some of the chip samples could be clocked in
>>>>>> boost mode speeds and only such samples can be safely operated in
>>>>>> boost mode.
>>>>>>
>>>>>> The mechanism of entry into and exit out of boost mode is outside the
>>>>>> scope of this documentation.
>>>>>>
>>>>>>>
>>>>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>>>>> go to these frequencies and for now long? When should I as a dt author
>>>>>>> place elements in boost-frequencies?
>>>>>>
>>>>>> I will add these details in the next revision of this patch.
>>>>>
>>>>> Cheers.
>>>>>
>>>>>>>
>>>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>>>> really easy to accidentally forget to mark something as a
>>>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>>>
>>>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>>>> part of operating-points property? If yes, that also is a possible
>>>>>> implementation (it was implemented in one of the earlier version of
>>>>>> this series). Could you confirm that you want the boost mode speeds to
>>>>>> be exclusive of the speeds listed in operating-points?
>>>>>
>>>>> If these boost mode operating points are not generally advisable for use
>>>>> as the other operating-points are, then they should IMO been in an
>>>>> entirely separate property exclusive of (but in the same format as) the
>>>>> operating-points property, e.g.
>>>>>
>>>>> operating points = <A B>, <C D>;
>>>>> boost-points = <E F>;
>>>>
>>>> you are asking boost frequencies to remain for ever tied down to OPP
>>>> style description.
>>>>
>>>> What we are trying to describe? "What are my SoC's overclocked
>>>> frequencies"? That description can be used even in a system that does
>>>> not use OPP style table (say ACPI based OPP tables or whatever
>>>> acronymned table).
>>>>
>>>> Tying it down to operating points just because we have it today as a
>>>> convenient description, is limiting.
>>>>
>>>> Further, if we decide to educate boost-frequencies to also indicate
>>>> how long is it safe? That does indeed belong to boost-frequency
>>>> description and not OPP description. Or if we decide to change
>>>> operating-points description[1] in the future has an impact on
>>>> "boost-points" description, when it should not have.
>>>>
>>>>>
>>>>> Otherwise, without boost-mode support we have to parse the boost mode
>>>>> table to figure out which points to avoid. Or if someone typos a value
>>>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>>>> ability to deal with boost-frequencies, it should skip it for sure.
>>>>
>>>>> in either table we might go into a boost mode when we didn't want to!
>>>>>
>>>> There are other ways to screw up device with dts typo. you could give
>>>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>>>> again.. typos are dt bugs, we can do the best to write robust code to
>>>> report them.
>>>>
>>>
>>> Typos are not the primary thing to worry about here. Adding boost
>>> frequencies to the list of primary operating points is flawed, because
>>> an OS that has no idea of boost mode will use them as normal operating
>>> points and this is not desired.
>>
>> That means we have an implementation bug in OS since it does not
>> consider the complete hardware description that device tree is
>> providing. An analogy will be a regulator compatible match being used
>> but regulator-min-microvolt and regulator-max-microvolt being ignored
>> by OS.
> 
> No. The operating points bindings were defined far before this
> boost-frequency and so there is no requirement to support the latter.

So, we dont add any new bindings ever again? /me blinks. *IF* we add a
new property in the future, do we expect the new description to be
supported in older kernel(which could not have known about it)? How
far are we taking this ABI thing?
Documentation/devicetree/bindings/ABI.txt states:
3) Bindings can be augmented, but the driver shouldn't break when given
   the old binding. ie. add additional properties, but don't change the
   meaning of an existing property. For drivers, default to the original
   behaviour when a newly added property is missing.

we are not changing the meaning of existing property, we are
augumenting it.

In my opinion, *IF* we are concerned about polluting operating-points
description, why dont we enforce that the boost operating points
should NOT be defined in the current "operating-points" description -
and - just follow what Rob suggested and iMx already does - add such
operating points from platform code.


>> We never said that "operating points" are "primary operating points".
>> all we said is they are "operating points" for the device - we dont
>> associate meanings to it. You may add to it[1] in platform code, as we
>> decided to.
> 
> Maybe generic OPP bindings don't state that, but I believe that at least
> cpufreq-cpu0 bindings have been defined (and the driver implemented)
> this way and changing the semantics now would be breaking DT ABI
> compatibility.
Did you mean
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt which
points to the generic bindings? Quote:
- operating-points: Refer to
Documentation/devicetree/bindings/power/opp.txt
  for details

>> boost-frequencies are describing "overclocked frequencies" - should it
>> matter if the description of that comes from platform code OR existing
>> opp tables or what ever? I dont think defining them as "operating
>> point" style as the only way of describing overclocked frequencies are
>> the right approach to describing it.
> 
> Nobody said that it is the only way. The whole point here is that it
> should be separate from the main operating-points property, as boost
> operating points are not normal operating points and the OS must be
> specifically aware how to handle them.
They are descriptions - I repeat myself when I state that they are
"overclocked frequencies" that happen to map to operating points on
the platforms of concern, but may not necessarily be OPP based on
other platforms which also be able to support "overclocked frequencies".


-- 
Regards,
Nishanth Menon

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Tomasz Figa]

On 30.05.2014 22:33, Nishanth Menon wrote:
> On 05/30/2014 03:19 PM, Tomasz Figa wrote:
>> On 30.05.2014 22:13, Nishanth Menon wrote:
>>> On 05/30/2014 03:02 PM, Tomasz Figa wrote:
>>>> On 30.05.2014 21:50, Nishanth Menon wrote:
>>>>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>>>>> Hi Mark,
>>>>>>>
>>>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>>>>
>>>>>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>>>
>>>>>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>>>>>> usable in boost mode.
>>>>>>>>>
>>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..63ed0fc
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>>>>>> +
>>>>>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>>>>>
>>>>>>>> Nit: CPUs (we're not greengrocers [1])
>>>>>>>>
>>>>>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>>>>>> +
>>>>>>>>> +Optional Properties:
>>>>>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>>>>>> +  details about "operating-points" property.
>>>>>>>>
>>>>>>>> What is 'boost-mode'?
>>>>>>>
>>>>>>> boost-mode activates additional one or more cpu clock speeds (which
>>>>>>> are not specified as operating frequency of the cpu by the
>>>>>>> manufacturer). The cpu is allowed to operate in these boost mode
>>>>>>> speeds when the boost mode is active. The boost mode speeds are
>>>>>>> usually undocumented. Some of the chip samples could be clocked in
>>>>>>> boost mode speeds and only such samples can be safely operated in
>>>>>>> boost mode.
>>>>>>>
>>>>>>> The mechanism of entry into and exit out of boost mode is outside the
>>>>>>> scope of this documentation.
>>>>>>>
>>>>>>>>
>>>>>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>>>>>> go to these frequencies and for now long? When should I as a dt author
>>>>>>>> place elements in boost-frequencies?
>>>>>>>
>>>>>>> I will add these details in the next revision of this patch.
>>>>>>
>>>>>> Cheers.
>>>>>>
>>>>>>>>
>>>>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>>>>> really easy to accidentally forget to mark something as a
>>>>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>>>>
>>>>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>>>>> part of operating-points property? If yes, that also is a possible
>>>>>>> implementation (it was implemented in one of the earlier version of
>>>>>>> this series). Could you confirm that you want the boost mode speeds to
>>>>>>> be exclusive of the speeds listed in operating-points?
>>>>>>
>>>>>> If these boost mode operating points are not generally advisable for use
>>>>>> as the other operating-points are, then they should IMO been in an
>>>>>> entirely separate property exclusive of (but in the same format as) the
>>>>>> operating-points property, e.g.
>>>>>>
>>>>>> operating points = <A B>, <C D>;
>>>>>> boost-points = <E F>;
>>>>>
>>>>> you are asking boost frequencies to remain for ever tied down to OPP
>>>>> style description.
>>>>>
>>>>> What we are trying to describe? "What are my SoC's overclocked
>>>>> frequencies"? That description can be used even in a system that does
>>>>> not use OPP style table (say ACPI based OPP tables or whatever
>>>>> acronymned table).
>>>>>
>>>>> Tying it down to operating points just because we have it today as a
>>>>> convenient description, is limiting.
>>>>>
>>>>> Further, if we decide to educate boost-frequencies to also indicate
>>>>> how long is it safe? That does indeed belong to boost-frequency
>>>>> description and not OPP description. Or if we decide to change
>>>>> operating-points description[1] in the future has an impact on
>>>>> "boost-points" description, when it should not have.
>>>>>
>>>>>>
>>>>>> Otherwise, without boost-mode support we have to parse the boost mode
>>>>>> table to figure out which points to avoid. Or if someone typos a value
>>>>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>>>>> ability to deal with boost-frequencies, it should skip it for sure.
>>>>>
>>>>>> in either table we might go into a boost mode when we didn't want to!
>>>>>>
>>>>> There are other ways to screw up device with dts typo. you could give
>>>>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>>>>> again.. typos are dt bugs, we can do the best to write robust code to
>>>>> report them.
>>>>>
>>>>
>>>> Typos are not the primary thing to worry about here. Adding boost
>>>> frequencies to the list of primary operating points is flawed, because
>>>> an OS that has no idea of boost mode will use them as normal operating
>>>> points and this is not desired.
>>>
>>> That means we have an implementation bug in OS since it does not
>>> consider the complete hardware description that device tree is
>>> providing. An analogy will be a regulator compatible match being used
>>> but regulator-min-microvolt and regulator-max-microvolt being ignored
>>> by OS.
>>
>> No. The operating points bindings were defined far before this
>> boost-frequency and so there is no requirement to support the latter.
> 
> So, we dont add any new bindings ever again? /me blinks. *IF* we add a
> new property in the future, do we expect the new description to be
> supported in older kernel(which could not have known about it)? How
> far are we taking this ABI thing?
> Documentation/devicetree/bindings/ABI.txt states:
> 3) Bindings can be augmented, but the driver shouldn't break when given
>    the old binding. ie. add additional properties, but don't change the
>    meaning of an existing property. For drivers, default to the original
>    behaviour when a newly added property is missing.
> 
> we are not changing the meaning of existing property, we are
> augumenting it.
> 
> In my opinion, *IF* we are concerned about polluting operating-points
> description, why dont we enforce that the boost operating points
> should NOT be defined in the current "operating-points" description -
> and - just follow what Rob suggested and iMx already does - add such
> operating points from platform code.
> 
> 
>>> We never said that "operating points" are "primary operating points".
>>> all we said is they are "operating points" for the device - we dont
>>> associate meanings to it. You may add to it[1] in platform code, as we
>>> decided to.
>>
>> Maybe generic OPP bindings don't state that, but I believe that at least
>> cpufreq-cpu0 bindings have been defined (and the driver implemented)
>> this way and changing the semantics now would be breaking DT ABI
>> compatibility.
> Did you mean
> Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt which
> points to the generic bindings? Quote:
> - operating-points: Refer to
> Documentation/devicetree/bindings/power/opp.txt
>   for details
> 
>>> boost-frequencies are describing "overclocked frequencies" - should it
>>> matter if the description of that comes from platform code OR existing
>>> opp tables or what ever? I dont think defining them as "operating
>>> point" style as the only way of describing overclocked frequencies are
>>> the right approach to describing it.
>>
>> Nobody said that it is the only way. The whole point here is that it
>> should be separate from the main operating-points property, as boost
>> operating points are not normal operating points and the OS must be
>> specifically aware how to handle them.
> They are descriptions - I repeat myself when I state that they are
> "overclocked frequencies" that happen to map to operating points on
> the platforms of concern, but may not necessarily be OPP based on
> other platforms which also be able to support "overclocked frequencies".
> 
> 

OK, so you add overclocked frequencies to operating-points property,
boost-frequency property, build a dtb, use it with a kernel that doesn't
support boost-frequency and nicely overheat (and likely destroy) your
board. I don't think this makes too much sense, sorry.

Best regards,
Tomasz

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Rob Herring]

On Fri, May 30, 2014 at 3:33 PM, Nishanth Menon <nm@ti.com> wrote:
> On 05/30/2014 03:19 PM, Tomasz Figa wrote:
>> On 30.05.2014 22:13, Nishanth Menon wrote:
>>> On 05/30/2014 03:02 PM, Tomasz Figa wrote:
>>>> On 30.05.2014 21:50, Nishanth Menon wrote:
>>>>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>>>>> Hi Mark,
>>>>>>>
>>>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>>>>
>>>>>>>> On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:
>>>>>>>>> From: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>>>
>>>>>>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>>>>>>> usable in boost mode.
>>>>>>>>>
>>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>>>>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>>>> Acked-by: Nishanth Menon <nm@ti.com>
>>>>>>>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
>>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..63ed0fc
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>>> +* Device tree binding for CPU boost frequency (aka over-clocking)
>>>>>>>>> +
>>>>>>>>> +Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as
>>>>>>>>
>>>>>>>> Nit: CPUs (we're not greengrocers [1])
>>>>>>>>
>>>>>>>>> +overclocking) in which the CPU can operate at frequencies which are not
>>>>>>>>> +specified by the manufacturer as CPU's operating frequency.
>>>>>>>>> +
>>>>>>>>> +Optional Properties:
>>>>>>>>> +- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
>>>>>>>>> +  This list should be a subset of frequencies listed in "operating-points"
>>>>>>>>> +  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
>>>>>>>>> +  details about "operating-points" property.
>>>>>>>>
>>>>>>>> What is 'boost-mode'?
>>>>>>>
>>>>>>> boost-mode activates additional one or more cpu clock speeds (which
>>>>>>> are not specified as operating frequency of the cpu by the
>>>>>>> manufacturer). The cpu is allowed to operate in these boost mode
>>>>>>> speeds when the boost mode is active. The boost mode speeds are
>>>>>>> usually undocumented. Some of the chip samples could be clocked in
>>>>>>> boost mode speeds and only such samples can be safely operated in
>>>>>>> boost mode.
>>>>>>>
>>>>>>> The mechanism of entry into and exit out of boost mode is outside the
>>>>>>> scope of this documentation.
>>>>>>>
>>>>>>>>
>>>>>>>> What are the limitations on boost frequencies? When is a CPU expected to
>>>>>>>> go to these frequencies and for now long? When should I as a dt author
>>>>>>>> place elements in boost-frequencies?
>>>>>>>
>>>>>>> I will add these details in the next revision of this patch.
>>>>>>
>>>>>> Cheers.
>>>>>>
>>>>>>>>
>>>>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>>>>> really easy to accidentally forget to mark something as a
>>>>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>>>>
>>>>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>>>>> part of operating-points property? If yes, that also is a possible
>>>>>>> implementation (it was implemented in one of the earlier version of
>>>>>>> this series). Could you confirm that you want the boost mode speeds to
>>>>>>> be exclusive of the speeds listed in operating-points?
>>>>>>
>>>>>> If these boost mode operating points are not generally advisable for use
>>>>>> as the other operating-points are, then they should IMO been in an
>>>>>> entirely separate property exclusive of (but in the same format as) the
>>>>>> operating-points property, e.g.
>>>>>>
>>>>>> operating points = <A B>, <C D>;
>>>>>> boost-points = <E F>;
>>>>>
>>>>> you are asking boost frequencies to remain for ever tied down to OPP
>>>>> style description.
>>>>>
>>>>> What we are trying to describe? "What are my SoC's overclocked
>>>>> frequencies"? That description can be used even in a system that does
>>>>> not use OPP style table (say ACPI based OPP tables or whatever
>>>>> acronymned table).
>>>>>
>>>>> Tying it down to operating points just because we have it today as a
>>>>> convenient description, is limiting.
>>>>>
>>>>> Further, if we decide to educate boost-frequencies to also indicate
>>>>> how long is it safe? That does indeed belong to boost-frequency
>>>>> description and not OPP description. Or if we decide to change
>>>>> operating-points description[1] in the future has an impact on
>>>>> "boost-points" description, when it should not have.
>>>>>
>>>>>>
>>>>>> Otherwise, without boost-mode support we have to parse the boost mode
>>>>>> table to figure out which points to avoid. Or if someone typos a value
>>>>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>>>>> ability to deal with boost-frequencies, it should skip it for sure.
>>>>>
>>>>>> in either table we might go into a boost mode when we didn't want to!
>>>>>>
>>>>> There are other ways to screw up device with dts typo. you could give
>>>>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>>>>> again.. typos are dt bugs, we can do the best to write robust code to
>>>>> report them.
>>>>>
>>>>
>>>> Typos are not the primary thing to worry about here. Adding boost
>>>> frequencies to the list of primary operating points is flawed, because
>>>> an OS that has no idea of boost mode will use them as normal operating
>>>> points and this is not desired.
>>>
>>> That means we have an implementation bug in OS since it does not
>>> consider the complete hardware description that device tree is
>>> providing. An analogy will be a regulator compatible match being used
>>> but regulator-min-microvolt and regulator-max-microvolt being ignored
>>> by OS.
>>
>> No. The operating points bindings were defined far before this
>> boost-frequency and so there is no requirement to support the latter.
>
> So, we dont add any new bindings ever again? /me blinks. *IF* we add a
> new property in the future, do we expect the new description to be
> supported in older kernel(which could not have known about it)? How
> far are we taking this ABI thing?
> Documentation/devicetree/bindings/ABI.txt states:
> 3) Bindings can be augmented, but the driver shouldn't break when given
>    the old binding. ie. add additional properties, but don't change the
>    meaning of an existing property. For drivers, default to the original
>    behaviour when a newly added property is missing.
>
> we are not changing the meaning of existing property, we are
> augumenting it.

You are changing the meaning of entries in that they can have
additional data which changes their properties.

If we accept the DT changes (as DT maintainers) and reject the kernel
changes (as kernel maintainers), you would be left with a broken
system.

> In my opinion, *IF* we are concerned about polluting operating-points
> description, why dont we enforce that the boost operating points
> should NOT be defined in the current "operating-points" description -
> and - just follow what Rob suggested and iMx already does - add such
> operating points from platform code.

I believe I also said on this and other attempts to bandage up the
existing OPP binding, that we should not change/append it, but define
a new binding for OPPs that addresses this and other issues.
Otherwise, I'm going to just NAK every incomplete OPP binding bandaid.

Rob

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Nishanth Menon]

On 05/30/2014 03:45 PM, Rob Herring wrote:
> On Fri, May 30, 2014 at 3:33 PM, Nishanth Menon <nm@ti.com> wrote:
>> On 05/30/2014 03:19 PM, Tomasz Figa wrote:
>>> On 30.05.2014 22:13, Nishanth Menon wrote:
>>>> On 05/30/2014 03:02 PM, Tomasz Figa wrote:
>>>>> On 30.05.2014 21:50, Nishanth Menon wrote:
>>>>>> On 05/30/2014 01:55 PM, Mark Rutland wrote:
>>>>>>> On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:
>>>>>>>> Hi Mark,
>>>>>>>>
>>>>>>>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Apologies for being somewhat late w.r.t. review on this.
>>>>>>>>>
[...]
>>>>>>>>> Why are these in both operating-points and boost-frequencies? It'll be
>>>>>>>>> really easy to accidentally forget to mark something as a
>>>>>>>>> boost-frequency this way. Why not have a boost-points instead?
>>>>>>>>
>>>>>>>> Does boost-points mean a set of clock speeds which are not listed as
>>>>>>>> part of operating-points property? If yes, that also is a possible
>>>>>>>> implementation (it was implemented in one of the earlier version of
>>>>>>>> this series). Could you confirm that you want the boost mode speeds to
>>>>>>>> be exclusive of the speeds listed in operating-points?
>>>>>>>
>>>>>>> If these boost mode operating points are not generally advisable for use
>>>>>>> as the other operating-points are, then they should IMO been in an
>>>>>>> entirely separate property exclusive of (but in the same format as) the
>>>>>>> operating-points property, e.g.
>>>>>>>
>>>>>>> operating points = <A B>, <C D>;
>>>>>>> boost-points = <E F>;
>>>>>>
>>>>>> you are asking boost frequencies to remain for ever tied down to OPP
>>>>>> style description.
>>>>>>
>>>>>> What we are trying to describe? "What are my SoC's overclocked
>>>>>> frequencies"? That description can be used even in a system that does
>>>>>> not use OPP style table (say ACPI based OPP tables or whatever
>>>>>> acronymned table).
>>>>>>
>>>>>> Tying it down to operating points just because we have it today as a
>>>>>> convenient description, is limiting.
>>>>>>
>>>>>> Further, if we decide to educate boost-frequencies to also indicate
>>>>>> how long is it safe? That does indeed belong to boost-frequency
>>>>>> description and not OPP description. Or if we decide to change
>>>>>> operating-points description[1] in the future has an impact on
>>>>>> "boost-points" description, when it should not have.
>>>>>>
>>>>>>>
>>>>>>> Otherwise, without boost-mode support we have to parse the boost mode
>>>>>>> table to figure out which points to avoid. Or if someone typos a value
>>>>>> That is OS usage of h/w description - yeah - in anycase, if OS has no
>>>>>> ability to deal with boost-frequencies, it should skip it for sure.
>>>>>>
>>>>>>> in either table we might go into a boost mode when we didn't want to!
>>>>>>>
>>>>>> There are other ways to screw up device with dts typo. you could give
>>>>>> a wrong voltage(extra 0?) and ensure you never use the chip ever
>>>>>> again.. typos are dt bugs, we can do the best to write robust code to
>>>>>> report them.
>>>>>>
>>>>>
>>>>> Typos are not the primary thing to worry about here. Adding boost
>>>>> frequencies to the list of primary operating points is flawed, because
>>>>> an OS that has no idea of boost mode will use them as normal operating
>>>>> points and this is not desired.
>>>>
>>>> That means we have an implementation bug in OS since it does not
>>>> consider the complete hardware description that device tree is
>>>> providing. An analogy will be a regulator compatible match being used
>>>> but regulator-min-microvolt and regulator-max-microvolt being ignored
>>>> by OS.
>>>
>>> No. The operating points bindings were defined far before this
>>> boost-frequency and so there is no requirement to support the latter.
>>
>> So, we dont add any new bindings ever again? /me blinks. *IF* we add a
>> new property in the future, do we expect the new description to be
>> supported in older kernel(which could not have known about it)? How
>> far are we taking this ABI thing?
>> Documentation/devicetree/bindings/ABI.txt states:
>> 3) Bindings can be augmented, but the driver shouldn't break when given
>>    the old binding. ie. add additional properties, but don't change the
>>    meaning of an existing property. For drivers, default to the original
>>    behaviour when a newly added property is missing.
>>
>> we are not changing the meaning of existing property, we are
>> augumenting it.
> 
> You are changing the meaning of entries in that they can have
> additional data which changes their properties.
> 
> If we accept the DT changes (as DT maintainers) and reject the kernel
> changes (as kernel maintainers), you would be left with a broken
> system.

That is true and I agree.

> 
>> In my opinion, *IF* we are concerned about polluting operating-points
>> description, why dont we enforce that the boost operating points
>> should NOT be defined in the current "operating-points" description -
>> and - just follow what Rob suggested and iMx already does - add such
>> operating points from platform code.
> 
> I believe I also said on this and other attempts to bandage up the
> existing OPP binding, that we should not change/append it, but define
> a new binding for OPPs that addresses this and other issues.
> Otherwise, I'm going to just NAK every incomplete OPP binding bandaid.

Are we open to creating a completely mutually-exclusive binding and
maintain the legacy one as "legacy support without any modification"
while we debate the new binding? I dont deny that we have more bandage
discussions that we'd like on these bindings, So, would like views if
we want to define a better one from scratch rather than continue to
figure out how to live with existing one?

Here are the options I can think of.
1. New binding: Lets say each OPP as phandles? -> that sounds like the
closest I have heard as something that remotely makes sense to scale
properties?
2. Remove OPP bindings entirely - only way to add/modify OPP is from
platform code.

Anyone has better suggestions what we can do?

-- 
Regards,
Nishanth Menon

Re: [PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Nishanth Menon]

On 05/30/2014 03:43 PM, Tomasz Figa wrote:
[...]
> OK, so you add overclocked frequencies to operating-points property,
> boost-frequency property, build a dtb, use it with a kernel that doesn't
> support boost-frequency and nicely overheat (and likely destroy) your
> board. I don't think this makes too much sense, sorry.
Yes, that is unfortunately true as well :( Sigh.

-- 
Regards,
Nishanth Menon

Re: [PATCH v6 0/2] cpufreq: opp: Add device tree based lookup of boost mode frequency

[Rafael J. Wysocki]

On Friday, May 30, 2014 11:37:58 PM Thomas Abraham wrote:
> On Fri, May 30, 2014 at 8:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, May 30, 2014 02:19:06 PM Rafael J. Wysocki wrote:
> >> On Friday, May 30, 2014 02:31:15 PM Thomas Abraham wrote:
> >> > Changes since v5:
> >> > - Fixes for checkpatch warnings with --strict option as suggested by
> >> >   Nishanth Menon.
> >> >
> >> > Changes since v4:
> >> > - Code improvements suggested by Viresh Kumar.
> >> >
> >> > Changes since v3:
> >> > - Minor changes as suggested in the last version.
> >> >
> >> > Changes since v2:
> >> > - Reworked based on the "PM / OPP: move cpufreq specific helpers out of OPP
> >> >   layer" patch series posted by Nishanth Menon.
> >> >
> >> > Changes since v1:
> >> > - Boost mode frequencies are specfied as a set of frequencies instead of
> >> >   specifying them as OPPs. Thanks to Nishanth, Lukasz and Rob for the
> >> >   feedback.
> >> >
> >> > This series has been acked by authors and maintainers of the code
> >> > portions changed in this series. The new DT binding introduced in this
> >> > patch needs an ack from device tree binding maintainers as well.
> >> >
> >> > Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds
> >> > support for CPU boost mode for CPUfreq drivers. To use the new boost
> >> > mode, CPUfreq drivers have to specify the boost mode frequency and
> >> > voltage within the CPUfreq driver, which is the case for Exynos4x12
> >> > CPUfreq driver.
> >> >
> >> > But for CPUfreq drivers which obtain the OPPs from cpus node, this
> >> > patch series adds support to specify boost mode frequencies in the
> >> > cpu device tree node. This requirement came up when Lukasz pointed
> >> > out the regression caused by the Exynos CPUfreq driver consolidation
> >> > patches.
> >>
> >> Both patches queued up for 3.16 in the linux-pm tree.
> >
> > But please address the Mark's comments or I'll drop them.
> 
> Hi Rafael,
> 
> Please drop this series for now. Both the patches have to reworked
> based on Mark's comments.

OK, dropped.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.