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

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

[Thomas Abraham]

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

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.

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.

Thomas Abraham (2):
  cpufreq / OPP: Allow boost frequency to be looked up from device tree
  Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

 .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   11 ++++++
 drivers/cpufreq/cpufreq_opp.c                      |   39 ++++++++++++++++++++
 2 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt

-- 
1.7.4.4

[PATCH v3 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.

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

diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c
index c0c6f4a..e3c97f3 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,10 @@ 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
+	int j, len;
+	u32 *boost_freqs = NULL;
+#endif
 
 	rcu_read_lock();
 
@@ -82,6 +87,40 @@ 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-frequency", &len)) {
+		if (len == 0 || (len & (sizeof(u32) - 1)) != 0) {
+			dev_err(dev, "%s: invalid boost frequency\n", __func__);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		boost_freqs = kzalloc(len, GFP_KERNEL);
+		if (!boost_freqs) {
+			dev_warn(dev, "%s: no memory for boost freq table\n",
+					__func__);
+			ret = -ENOMEM;
+			goto out;
+		}
+		of_property_read_u32_array(dev->of_node, "boost-frequency",
+			boost_freqs, len / sizeof(u32));
+	}
+
+	for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) {
+		for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
+			if (boost_freqs[j] == freq_table[i].frequency) {
+				freq_table[i].flags |= CPUFREQ_BOOST_FREQ;
+				break;
+			}
+		}
+		if (freq_table[i].frequency == CPUFREQ_TABLE_END)
+			pr_err("%s: invalid boost frequency %d\n",
+				__func__, boost_freqs[j]);
+	}
+
+	kfree(boost_freqs);
+#endif
+
 out:
 	rcu_read_unlock();
 	if (ret)
-- 
1.7.4.4

[PATCH v3 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: Nishanth Menon <nm@ti.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
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>
---
 .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
 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..d925e38
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
@@ -0,0 +1,11 @@
+* 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 in frequencies beyond the normal
+operating conditions.
+
+Optional Properties:
+- boost-frequency: 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.
-- 
1.7.4.4

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

[Nishanth Menon]

On Tue, May 13, 2014 at 8:03 PM, Thomas Abraham <ta.omasab@gmail.com> wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Add a new optional boost-frequency binding for specifying the frequencies
> usable in boost mode.
>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> 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>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>  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..d925e38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> @@ -0,0 +1,11 @@
> +* 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 in frequencies beyond the normal

"operate at"?

> +operating conditions.

"normal operating conditions" probably need a little bit of an
expansion here perhaps?

> +
> +Optional Properties:
> +- boost-frequency: list of frequencies in KHz to be used only in boost mode.

probably boost-frequencies?

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


an example is expected here.

personally, I think I understand the intent here, but as a hardware
description, will let folks comment if it is acceptable.

--
Regards,
Nishanth Menon

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

[Viresh Kumar]

On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote:
> 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.
>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/cpufreq/cpufreq_opp.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c
> index c0c6f4a..e3c97f3 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,10 @@ 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
> +       int j, len;
> +       u32 *boost_freqs = NULL;
> +#endif
>
>         rcu_read_lock();
>
> @@ -82,6 +87,40 @@ 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-frequency", &len)) {

Does this mean another block inside the cpu node ? Like this: ?

cpu@0 {
        compatible = "arm,cortex-a9";
        reg = <0>;
        next-level-cache = <&L2>;
        operating-points = <
                /* kHz    uV */
                792000  1100000
                396000  950000
                198000  850000
        >;
        boost-frequency = <
                792000
                198000
        >;
};

I think it we might better add another field in the opp block as these
OPPs are rather boost one..

@Rob/Rafael: Opinion please ..

> +               if (len == 0 || (len & (sizeof(u32) - 1)) != 0) {
> +                       dev_err(dev, "%s: invalid boost frequency\n", __func__);
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               boost_freqs = kzalloc(len, GFP_KERNEL);
> +               if (!boost_freqs) {
> +                       dev_warn(dev, "%s: no memory for boost freq table\n",
> +                                       __func__);
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +               of_property_read_u32_array(dev->of_node, "boost-frequency",
> +                       boost_freqs, len / sizeof(u32));
> +       }
> +
> +       for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) {

Why is this present outside of above if {} ? as boost_freqs is guaranteed to
be NULL without that.

> +               for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> +                       if (boost_freqs[j] == freq_table[i].frequency) {

Use cpufreq_frequency_table_get_index() instead.

> +                               freq_table[i].flags |= CPUFREQ_BOOST_FREQ;
> +                               break;
> +                       }
> +               }
> +               if (freq_table[i].frequency == CPUFREQ_TABLE_END)
> +                       pr_err("%s: invalid boost frequency %d\n",
> +                               __func__, boost_freqs[j]);
> +       }
> +
> +       kfree(boost_freqs);
> +#endif
> +
>  out:
>         rcu_read_unlock();
>         if (ret)
> --
> 1.7.4.4
>

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

[Nishanth Menon]

On Tue, May 13, 2014 at 10:40 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote:
[...]
>> +#ifdef CONFIG_CPU_FREQ_BOOST_SW
>> +       if (of_find_property(dev->of_node, "boost-frequency", &len)) {
>
> Does this mean another block inside the cpu node ? Like this: ?
>
> cpu@0 {
>         compatible = "arm,cortex-a9";
>         reg = <0>;
>         next-level-cache = <&L2>;
>         operating-points = <
>                 /* kHz    uV */
>                 792000  1100000
>                 396000  950000
>                 198000  850000
>         >;
>         boost-frequency = <
>                 792000
>                 198000
>         >;
> };
>
> I think it we might better add another field in the opp block as these
> OPPs are rather boost one..

If we change the meaning to be:
         operating-points = <
                 /* kHz    uV          boost? */
                 792000  1100000  1
                 396000  950000    0

We are adding a modifier to the OPP definition here and does imply
every platform which may or maynot require "boost" need to indicate
that - basically fails at least my "least common" description for a
generic definition. As I had indicated in other threads -> we are back
to the discussion of "additional properties" to an OPP..
Option 1:
  - describe modifiers separately (as in boost-frequencies) - that
operate based on data from opp table.
Option 2:
  - keep adding to the description of opp as time goes by - every SoC
has it's own set of quirks that are needed for an OPP - I know that at
TI, we have certain OPPs that can only be enabled *if* "custom
hardware driver" is enabled, and similar stories. - yet another
example is enable the OPP if efuse X, bit Y is set.

Personally, looking at the various descriptions and bL, cpu-idle
states dependencies on OPPs, etc.. Option 2 is a non-scalable
approach.

[...]

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

[Lukasz Majewski]

Hi Nishanth, Viresh

If I may add my 2 cents.

> On Tue, May 13, 2014 at 10:40 PM, Viresh Kumar
> <viresh.kumar@linaro.org> wrote:
> > On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote:
> [...]
> >> +#ifdef CONFIG_CPU_FREQ_BOOST_SW
> >> +       if (of_find_property(dev->of_node, "boost-frequency",
> >> &len)) {
> >
> > Does this mean another block inside the cpu node ? Like this: ?
> >
> > cpu@0 {
> >         compatible = "arm,cortex-a9";
> >         reg = <0>;
> >         next-level-cache = <&L2>;
> >         operating-points = <
> >                 /* kHz    uV */
> >                 792000  1100000
> >                 396000  950000
> >                 198000  850000
> >         >;
> >         boost-frequency = <
> >                 792000
> >                 198000
> >         >;

I think that the above approach is more appealing [*].

> > };
> >
> > I think it we might better add another field in the opp block as
> > these OPPs are rather boost one..
> 
> If we change the meaning to be:
>          operating-points = <
>                  /* kHz    uV          boost? */
>                  792000  1100000  1
>                  396000  950000    0
> 
> We are adding a modifier to the OPP definition here and does imply
> every platform which may or maynot require "boost" need to indicate
> that - basically fails at least my "least common" description for a
> generic definition. As I had indicated in other threads -> we are back
> to the discussion of "additional properties" to an OPP..
> Option 1:
>   - describe modifiers separately (as in boost-frequencies) - that
> operate based on data from opp table.
> Option 2:
>   - keep adding to the description of opp as time goes by - every SoC
> has it's own set of quirks that are needed for an OPP - I know that at
> TI, we have certain OPPs that can only be enabled *if* "custom
> hardware driver" is enabled, and similar stories. - yet another
> example is enable the OPP if efuse X, bit Y is set.
> 
> Personally, looking at the various descriptions and bL, cpu-idle
> states dependencies on OPPs, etc.. Option 2 is a non-scalable
> approach.

I agree with Nishanth here, that point 1 (as described by Viresh at
[*]) is a more scalable approach.

> 
> [...]



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

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

[Viresh Kumar]

On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
> I agree with Nishanth here, that point 1 (as described by Viresh at
> [*]) is a more scalable approach.

The only reason why I wanted all that to be done at OPP level was to
ensure if somebody else also needs it apart from cpufreq, they don't have
to duplicate code and find it.. As it is present at a central place..

But if no other code is going to look for that, it may just be fine as is..

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

[Thomas Abraham]

On Wed, May 14, 2014 at 7:32 AM, Nishanth Menon <nm@ti.com> wrote:
> On Tue, May 13, 2014 at 8:03 PM, Thomas Abraham <ta.omasab@gmail.com> wrote:
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> Add a new optional boost-frequency binding for specifying the frequencies
>> usable in boost mode.
>>
>> Cc: Nishanth Menon <nm@ti.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> 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>
>> ---
>>  .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>  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..d925e38
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
>> @@ -0,0 +1,11 @@
>> +* 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 in frequencies beyond the normal
>
> "operate at"?
>
>> +operating conditions.
>
> "normal operating conditions" probably need a little bit of an
> expansion here perhaps?
>
>> +
>> +Optional Properties:
>> +- boost-frequency: list of frequencies in KHz to be used only in boost mode.
>
> probably boost-frequencies?
>
>> +  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.
>
>
> an example is expected here.
>
> personally, I think I understand the intent here, but as a hardware
> description, will let folks comment if it is acceptable.

Thanks for your review Nishanth. I will do the changes you have suggested.

Regards,
Thomas.

>
> --
> Regards,
> Nishanth Menon

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

[Thomas Abraham]

On Wed, May 14, 2014 at 9:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote:
>> 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.
>>
>> Cc: Nishanth Menon <nm@ti.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  drivers/cpufreq/cpufreq_opp.c |   39 +++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 39 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c
>> index c0c6f4a..e3c97f3 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,10 @@ 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
>> +       int j, len;
>> +       u32 *boost_freqs = NULL;
>> +#endif
>>
>>         rcu_read_lock();
>>
>> @@ -82,6 +87,40 @@ 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-frequency", &len)) {
>
> Does this mean another block inside the cpu node ? Like this: ?
>
> cpu@0 {
>         compatible = "arm,cortex-a9";
>         reg = <0>;
>         next-level-cache = <&L2>;
>         operating-points = <
>                 /* kHz    uV */
>                 792000  1100000
>                 396000  950000
>                 198000  850000
>         >;
>         boost-frequency = <
>                 792000
>                 198000
>         >;
> };
>
> I think it we might better add another field in the opp block as these
> OPPs are rather boost one..
>
> @Rob/Rafael: Opinion please ..
>
>> +               if (len == 0 || (len & (sizeof(u32) - 1)) != 0) {
>> +                       dev_err(dev, "%s: invalid boost frequency\n", __func__);
>> +                       ret = -EINVAL;
>> +                       goto out;
>> +               }
>> +
>> +               boost_freqs = kzalloc(len, GFP_KERNEL);
>> +               if (!boost_freqs) {
>> +                       dev_warn(dev, "%s: no memory for boost freq table\n",
>> +                                       __func__);
>> +                       ret = -ENOMEM;
>> +                       goto out;
>> +               }
>> +               of_property_read_u32_array(dev->of_node, "boost-frequency",
>> +                       boost_freqs, len / sizeof(u32));
>> +       }
>> +
>> +       for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) {
>
> Why is this present outside of above if {} ? as boost_freqs is guaranteed to
> be NULL without that.

Just to reduce indentation by one tab. No technical reasons. The code
had to wrap at 80 column was becoming unreadable.

>
>> +               for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
>> +                       if (boost_freqs[j] == freq_table[i].frequency) {
>
> Use cpufreq_frequency_table_get_index() instead.

Okay. Thanks for pointing out.

>
>> +                               freq_table[i].flags |= CPUFREQ_BOOST_FREQ;
>> +                               break;
>> +                       }
>> +               }
>> +               if (freq_table[i].frequency == CPUFREQ_TABLE_END)
>> +                       pr_err("%s: invalid boost frequency %d\n",
>> +                               __func__, boost_freqs[j]);
>> +       }
>> +
>> +       kfree(boost_freqs);
>> +#endif
>> +
>>  out:
>>         rcu_read_unlock();
>>         if (ret)
>> --
>> 1.7.4.4
>>

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

[Thomas Abraham]

On Wed, May 14, 2014 at 11:54 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> I agree with Nishanth here, that point 1 (as described by Viresh at
>> [*]) is a more scalable approach.
>
> The only reason why I wanted all that to be done at OPP level was to
> ensure if somebody else also needs it apart from cpufreq, they don't have
> to duplicate code and find it.. As it is present at a central place..
>
> But if no other code is going to look for that, it may just be fine as is..

Yes, I agree with Nishanth and Lukasz for a separate binding.

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

[Viresh Kumar]

On 14 May 2014 19:16, Thomas Abraham <ta.omasab@gmail.com> wrote:
> On Wed, May 14, 2014 at 9:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

>> Why is this present outside of above if {} ? as boost_freqs is guaranteed to
>> be NULL without that.
>
> Just to reduce indentation by one tab. No technical reasons. The code
> had to wrap at 80 column was becoming unreadable.

That's bad :), you have added an extra comparison just for that :(
Instead kill the below indentation by doing a "goto out" with a ! of
below expression:

>> +       if (of_find_property(dev->of_node, "boost-frequency", &len)) {

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

[Nishanth Menon]

On 05/14/2014 01:24 AM, Viresh Kumar wrote:
> On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> I agree with Nishanth here, that point 1 (as described by Viresh at
>> [*]) is a more scalable approach.
> 
> The only reason why I wanted all that to be done at OPP level was to
> ensure if somebody else also needs it apart from cpufreq, they don't have
> to duplicate code and find it.. As it is present at a central place..
> 
> But if no other code is going to look for that, it may just be fine as is..
> 
If we eventually have a need beyond cpufreq (say devfreq) with similar
instances, then it makes sense to move it out to a generic place.
Either way, code implementation/duplication is a OS problem - and
should be looked at independent of the description in dts. If we feel
the description is valid hardware description (which, personally, I
do), then lets go to the next discussion point of where to put it -
generic or cpufreq specific (here, I have no preference), and finally
decide the implementation as necessary as a result of the description.

-- 
Regards,
Nishanth Menon

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

[Sudeep Holla]

On 14/05/14 02:03, 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: Nishanth Menon <nm@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> 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>
> ---
>   .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>   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..d925e38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
> @@ -0,0 +1,11 @@
> +* 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 in frequencies beyond the normal
> +operating conditions.
> +
> +Optional Properties:
> +- boost-frequency: 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.
>

Though I understand the need of it, I think the binding is designed to suffice
the need of the boost frequency support in cpufreq.

Typically SoC would provide characteristics like under-drive(hits performance
but most energy efficient), nominal(highest sustained performance w/o any
external constraint like power, thermal) and over-drive(maximum performance but
not sustainable for long periods)

IMO the binding could represent these unique points on the curve instead.

Regards,
Sudeep