[PATCH 0/2] Add device tree based lookup of boost mode OPPs

[PATCH 0/2] Add device tree based lookup of boost mode OPPs

[Thomas Abraham]

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 OPPs in dt node. This
requirement came up when Lukasz pointed out the regression caused by
the Exynos CPUfreq driver consolidation patches.

Thomas Abraham (2):
  PM / OPP: Add support for 'boost' mode OPP
  Documentation: devicetree: Add boost-opp binding to list boost mode OPPs

 Documentation/devicetree/bindings/power/opp.txt |    9 +++
 drivers/base/power/opp.c                        |   69 ++++++++++++++++++-----
 2 files changed, 65 insertions(+), 13 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] PM / OPP: Add support for 'boost' mode OPP

[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 OPPs 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/base/power/opp.c |   69 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2553867..de4d52d 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -62,6 +62,7 @@ struct dev_pm_opp {
 	struct list_head node;
 
 	bool available;
+	bool boost;
 	unsigned long rate;
 	unsigned long u_volt;
 
@@ -380,10 +381,12 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
- * dev_pm_opp_add()  - Add an OPP table from a table definitions
+ * dev_pm_opp_add_flags()  - Add an OPP to device OPP list with flags
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
  * @u_volt:	Voltage in uVolts for this OPP
+ * @available:	initial availability of the OPP with adding it to the list.
+ * @boost:	availability of this opp in controller's boost operating mode.
  *
  * This function adds an opp definition to the opp list and returns status.
  * The opp is made available by default and it can be controlled using
@@ -395,7 +398,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+static int dev_pm_opp_add_flags(struct device *dev, unsigned long freq,
+			unsigned long u_volt, bool available, bool boost)
 {
 	struct device_opp *dev_opp = NULL;
 	struct dev_pm_opp *opp, *new_opp;
@@ -441,7 +445,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	new_opp->dev_opp = dev_opp;
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
-	new_opp->available = true;
+	new_opp->available = available;
+	new_opp->boost = boost;
 
 	/* Insert new OPP in order of increasing frequency */
 	head = &dev_opp->opp_list;
@@ -462,6 +467,27 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
 	return 0;
 }
+
+/**
+ * dev_pm_opp_add()  - Add an OPP table from a table definitions
+ * @dev:	device for which we do this operation
+ * @freq:	Frequency in Hz for this OPP
+ * @u_volt:	Voltage in uVolts for this OPP
+ *
+ * This function adds an opp definition to the opp list and returns status.
+ * The opp is made available by default and it can be controlled using
+ * dev_pm_opp_enable/disable functions.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+{
+	return dev_pm_opp_add_flags(dev, freq, u_volt, true, false);
+}
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
@@ -651,7 +677,8 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
 
 	list_for_each_entry(opp, &dev_opp->opp_list, node) {
 		if (opp->available) {
-			freq_table[i].driver_data = i;
+			freq_table[i].driver_data =
+				opp->boost ? CPUFREQ_BOOST_FREQ : i;
 			freq_table[i].frequency = opp->rate / 1000;
 			i++;
 		}
@@ -701,19 +728,14 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
 }
 
 #ifdef CONFIG_OF
-/**
- * of_init_opp_table() - Initialize opp table from device tree
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Register the initial OPP table with the OPP library for given device.
- */
-int of_init_opp_table(struct device *dev)
+static int of_parse_opp_table(struct device *dev, const char *prop_name,
+					bool boost)
 {
 	const struct property *prop;
 	const __be32 *val;
 	int nr;
 
-	prop = of_find_property(dev->of_node, "operating-points", NULL);
+	prop = of_find_property(dev->of_node, prop_name, NULL);
 	if (!prop)
 		return -ENODEV;
 	if (!prop->value)
@@ -734,7 +756,7 @@ int of_init_opp_table(struct device *dev)
 		unsigned long freq = be32_to_cpup(val++) * 1000;
 		unsigned long volt = be32_to_cpup(val++);
 
-		if (dev_pm_opp_add(dev, freq, volt)) {
+		if (dev_pm_opp_add_flags(dev, freq, volt, true, boost)) {
 			dev_warn(dev, "%s: Failed to add OPP %ld\n",
 				 __func__, freq);
 			continue;
@@ -744,5 +766,26 @@ int of_init_opp_table(struct device *dev)
 
 	return 0;
 }
+
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ * Additional "boost" operating points of the controller, if any, are
+ * specified with "boost-opp" property.
+ */
+int of_init_opp_table(struct device *dev)
+{
+	int ret;
+
+	ret = of_parse_opp_table(dev, "operating-points", false);
+	if (!ret) {
+		ret = of_parse_opp_table(dev, "boost-opp", true);
+		if (ret == -ENODEV)
+			ret = 0;
+	}
+	return ret;
+}
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
-- 
1.7.10.4

Re: [PATCH 1/2] PM / OPP: Add support for 'boost' mode OPP

[Nishanth Menon]

On 02/04/2014 03:41 AM, Thomas Abraham 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 OPPs 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>
> ---

Why is a cpufreq feature being pushed on to OPP library? you can very
well have a property boot-frequencies = < a b c > and be done with the
job.

I agree with Rob's comment that this is something we have to discuss
in wider "add features to an OPP" discussion[1].


[1] http://marc.info/?t=139108946400001&r=1&w=2

>  drivers/base/power/opp.c |   69 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..de4d52d 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -62,6 +62,7 @@ struct dev_pm_opp {
>  	struct list_head node;
>  
>  	bool available;
> +	bool boost;
>  	unsigned long rate;
>  	unsigned long u_volt;
>  
> @@ -380,10 +381,12 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>  
>  /**
> - * dev_pm_opp_add()  - Add an OPP table from a table definitions
> + * dev_pm_opp_add_flags()  - Add an OPP to device OPP list with flags
>   * @dev:	device for which we do this operation
>   * @freq:	Frequency in Hz for this OPP
>   * @u_volt:	Voltage in uVolts for this OPP
> + * @available:	initial availability of the OPP with adding it to the list.
> + * @boost:	availability of this opp in controller's boost operating mode.
>   *
>   * This function adds an opp definition to the opp list and returns status.
>   * The opp is made available by default and it can be controlled using
> @@ -395,7 +398,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>   * that this function is *NOT* called under RCU protection or in contexts where
>   * mutex cannot be locked.
>   */
> -int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> +static int dev_pm_opp_add_flags(struct device *dev, unsigned long freq,
> +			unsigned long u_volt, bool available, bool boost)
>  {
>  	struct device_opp *dev_opp = NULL;
>  	struct dev_pm_opp *opp, *new_opp;
> @@ -441,7 +445,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>  	new_opp->dev_opp = dev_opp;
>  	new_opp->rate = freq;
>  	new_opp->u_volt = u_volt;
> -	new_opp->available = true;
> +	new_opp->available = available;
> +	new_opp->boost = boost;
>  
>  	/* Insert new OPP in order of increasing frequency */
>  	head = &dev_opp->opp_list;
> @@ -462,6 +467,27 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>  	srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
>  	return 0;
>  }
> +
> +/**
> + * dev_pm_opp_add()  - Add an OPP table from a table definitions
> + * @dev:	device for which we do this operation
> + * @freq:	Frequency in Hz for this OPP
> + * @u_volt:	Voltage in uVolts for this OPP
> + *
> + * This function adds an opp definition to the opp list and returns status.
> + * The opp is made available by default and it can be controlled using
> + * dev_pm_opp_enable/disable functions.
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks
> + * to keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex cannot be locked.
> + */
> +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> +{
> +	return dev_pm_opp_add_flags(dev, freq, u_volt, true, false);
> +}
>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
>  
>  /**
> @@ -651,7 +677,8 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
>  
>  	list_for_each_entry(opp, &dev_opp->opp_list, node) {
>  		if (opp->available) {
> -			freq_table[i].driver_data = i;
> +			freq_table[i].driver_data =
> +				opp->boost ? CPUFREQ_BOOST_FREQ : i;
>  			freq_table[i].frequency = opp->rate / 1000;
>  			i++;
>  		}
> @@ -701,19 +728,14 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
>  }
>  
>  #ifdef CONFIG_OF
> -/**
> - * of_init_opp_table() - Initialize opp table from device tree
> - * @dev:	device pointer used to lookup device OPPs.
> - *
> - * Register the initial OPP table with the OPP library for given device.
> - */
> -int of_init_opp_table(struct device *dev)
> +static int of_parse_opp_table(struct device *dev, const char *prop_name,
> +					bool boost)
>  {
>  	const struct property *prop;
>  	const __be32 *val;
>  	int nr;
>  
> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
> +	prop = of_find_property(dev->of_node, prop_name, NULL);
>  	if (!prop)
>  		return -ENODEV;
>  	if (!prop->value)
> @@ -734,7 +756,7 @@ int of_init_opp_table(struct device *dev)
>  		unsigned long freq = be32_to_cpup(val++) * 1000;
>  		unsigned long volt = be32_to_cpup(val++);
>  
> -		if (dev_pm_opp_add(dev, freq, volt)) {
> +		if (dev_pm_opp_add_flags(dev, freq, volt, true, boost)) {
>  			dev_warn(dev, "%s: Failed to add OPP %ld\n",
>  				 __func__, freq);
>  			continue;
> @@ -744,5 +766,26 @@ int of_init_opp_table(struct device *dev)
>  
>  	return 0;
>  }
> +
> +/**
> + * of_init_opp_table() - Initialize opp table from device tree
> + * @dev:	device pointer used to lookup device OPPs.
> + *
> + * Register the initial OPP table with the OPP library for given device.
> + * Additional "boost" operating points of the controller, if any, are
> + * specified with "boost-opp" property.
> + */
> +int of_init_opp_table(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = of_parse_opp_table(dev, "operating-points", false);
> +	if (!ret) {
> +		ret = of_parse_opp_table(dev, "boost-opp", true);
> +		if (ret == -ENODEV)
> +			ret = 0;
> +	}
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>  #endif
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/2] PM / OPP: Add support for 'boost' mode OPP

[Thomas Abraham]

On Tue, Feb 4, 2014 at 8:45 PM, Nishanth Menon <nm@ti.com> wrote:
> On 02/04/2014 03:41 AM, Thomas Abraham 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 OPPs 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>
>> ---
>
> Why is a cpufreq feature being pushed on to OPP library? you can very
> well have a property boot-frequencies = < a b c > and be done with the
> job.

The boost-opp was not limited to be a cpu/cpufreq only feature. Any
device (such as a bus) which has OPPs and if it can support optional
boost OPPs, can utilize this feature. The boost OPPs also require a
voltage to be associated with the frequency and hence the binding
boost-frequencies would not be suffice. The code changes in this patch
also do not have anything that is cpufreq specific.

>
> I agree with Rob's comment that this is something we have to discuss
> in wider "add features to an OPP" discussion[1].

Okay. I have read through the discussion in [1]. Thanks for the link.
Assuming that the current OPP tuple format will not change, I do not
feel the code changes in this patch will be hinder the outcome of the
discussion in [1].

Regards,
Thomas.

>
>
> [1] http://marc.info/?t=139108946400001&r=1&w=2
>
>>  drivers/base/power/opp.c |   69 +++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index 2553867..de4d52d 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -62,6 +62,7 @@ struct dev_pm_opp {
>>       struct list_head node;
>>
>>       bool available;
>> +     bool boost;
>>       unsigned long rate;
>>       unsigned long u_volt;
>>
>> @@ -380,10 +381,12 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>>
>>  /**
>> - * dev_pm_opp_add()  - Add an OPP table from a table definitions
>> + * dev_pm_opp_add_flags()  - Add an OPP to device OPP list with flags
>>   * @dev:     device for which we do this operation
>>   * @freq:    Frequency in Hz for this OPP
>>   * @u_volt:  Voltage in uVolts for this OPP
>> + * @available:       initial availability of the OPP with adding it to the list.
>> + * @boost:   availability of this opp in controller's boost operating mode.
>>   *
>>   * This function adds an opp definition to the opp list and returns status.
>>   * The opp is made available by default and it can be controlled using
>> @@ -395,7 +398,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>>   * that this function is *NOT* called under RCU protection or in contexts where
>>   * mutex cannot be locked.
>>   */
>> -int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>> +static int dev_pm_opp_add_flags(struct device *dev, unsigned long freq,
>> +                     unsigned long u_volt, bool available, bool boost)
>>  {
>>       struct device_opp *dev_opp = NULL;
>>       struct dev_pm_opp *opp, *new_opp;
>> @@ -441,7 +445,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>       new_opp->dev_opp = dev_opp;
>>       new_opp->rate = freq;
>>       new_opp->u_volt = u_volt;
>> -     new_opp->available = true;
>> +     new_opp->available = available;
>> +     new_opp->boost = boost;
>>
>>       /* Insert new OPP in order of increasing frequency */
>>       head = &dev_opp->opp_list;
>> @@ -462,6 +467,27 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>       srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
>>       return 0;
>>  }
>> +
>> +/**
>> + * dev_pm_opp_add()  - Add an OPP table from a table definitions
>> + * @dev:     device for which we do this operation
>> + * @freq:    Frequency in Hz for this OPP
>> + * @u_volt:  Voltage in uVolts for this OPP
>> + *
>> + * This function adds an opp definition to the opp list and returns status.
>> + * The opp is made available by default and it can be controlled using
>> + * dev_pm_opp_enable/disable functions.
>> + *
>> + * Locking: The internal device_opp and opp structures are RCU protected.
>> + * Hence this function internally uses RCU updater strategy with mutex locks
>> + * to keep the integrity of the internal data structures. Callers should ensure
>> + * that this function is *NOT* called under RCU protection or in contexts where
>> + * mutex cannot be locked.
>> + */
>> +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>> +{
>> +     return dev_pm_opp_add_flags(dev, freq, u_volt, true, false);
>> +}
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
>>
>>  /**
>> @@ -651,7 +677,8 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
>>
>>       list_for_each_entry(opp, &dev_opp->opp_list, node) {
>>               if (opp->available) {
>> -                     freq_table[i].driver_data = i;
>> +                     freq_table[i].driver_data =
>> +                             opp->boost ? CPUFREQ_BOOST_FREQ : i;
>>                       freq_table[i].frequency = opp->rate / 1000;
>>                       i++;
>>               }
>> @@ -701,19 +728,14 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
>>  }
>>
>>  #ifdef CONFIG_OF
>> -/**
>> - * of_init_opp_table() - Initialize opp table from device tree
>> - * @dev:     device pointer used to lookup device OPPs.
>> - *
>> - * Register the initial OPP table with the OPP library for given device.
>> - */
>> -int of_init_opp_table(struct device *dev)
>> +static int of_parse_opp_table(struct device *dev, const char *prop_name,
>> +                                     bool boost)
>>  {
>>       const struct property *prop;
>>       const __be32 *val;
>>       int nr;
>>
>> -     prop = of_find_property(dev->of_node, "operating-points", NULL);
>> +     prop = of_find_property(dev->of_node, prop_name, NULL);
>>       if (!prop)
>>               return -ENODEV;
>>       if (!prop->value)
>> @@ -734,7 +756,7 @@ int of_init_opp_table(struct device *dev)
>>               unsigned long freq = be32_to_cpup(val++) * 1000;
>>               unsigned long volt = be32_to_cpup(val++);
>>
>> -             if (dev_pm_opp_add(dev, freq, volt)) {
>> +             if (dev_pm_opp_add_flags(dev, freq, volt, true, boost)) {
>>                       dev_warn(dev, "%s: Failed to add OPP %ld\n",
>>                                __func__, freq);
>>                       continue;
>> @@ -744,5 +766,26 @@ int of_init_opp_table(struct device *dev)
>>
>>       return 0;
>>  }
>> +
>> +/**
>> + * of_init_opp_table() - Initialize opp table from device tree
>> + * @dev:     device pointer used to lookup device OPPs.
>> + *
>> + * Register the initial OPP table with the OPP library for given device.
>> + * Additional "boost" operating points of the controller, if any, are
>> + * specified with "boost-opp" property.
>> + */
>> +int of_init_opp_table(struct device *dev)
>> +{
>> +     int ret;
>> +
>> +     ret = of_parse_opp_table(dev, "operating-points", false);
>> +     if (!ret) {
>> +             ret = of_parse_opp_table(dev, "boost-opp", true);
>> +             if (ret == -ENODEV)
>> +                     ret = 0;
>> +     }
>> +     return ret;
>> +}
>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>  #endif
>>
>
>
> --
> Regards,
> Nishanth Menon

Re: [PATCH 1/2] PM / OPP: Add support for 'boost' mode OPP

[Nishanth Menon]

On 02/04/2014 09:59 AM, Thomas Abraham wrote:
> On Tue, Feb 4, 2014 at 8:45 PM, Nishanth Menon <nm@ti.com> wrote:
>> On 02/04/2014 03:41 AM, Thomas Abraham 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 OPPs 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>
>>> ---
>>
>> Why is a cpufreq feature being pushed on to OPP library? you can very
>> well have a property boot-frequencies = < a b c > and be done with the
>> job.
> 
> The boost-opp was not limited to be a cpu/cpufreq only feature. Any
> device (such as a bus) which has OPPs and if it can support optional
> boost OPPs, can utilize this feature. The boost OPPs also require a
> voltage to be associated with the frequency and hence the binding
> boost-frequencies would not be suffice. The code changes in this patch
> also do not have anything that is cpufreq specific.
> 
if we have
operating-points = < Fa Va
	Fb Vb
	Fc Vc
	Fd Vd
	>;
boost-frequencies = <Fc Fd>;
you can easily pick up the voltages from the table.

The point being - there does not seem to be a need to modify the
existing definitions to introduce new OPP definitions.

a way to flip this over is to consider iMX6(see
arch/arm/mach-imx/mach-imx6q.) where OPP tuple <Fd Vd> can only be
enabled *iff* efuse register x has bit y set.

Would we want to introduce efuse offsets into OPP definitions? into
something like additional field definition 'efuse_controlled'?


>>
>> I agree with Rob's comment that this is something we have to discuss
>> in wider "add features to an OPP" discussion[1].
> 
> Okay. I have read through the discussion in [1]. Thanks for the link.
> Assuming that the current OPP tuple format will not change, I do not
> feel the code changes in this patch will be hinder the outcome of the
> discussion in [1].

The context of that discussion is to consider what is the data form we
consider OPP as? should we consider OPP as a data that is extensible
(as in phandle with properties that we add on) OR should we consider
key, value pair which provides a key (frequency) into another table
for other data (like efuse bit map, boost set etc..).

Both approaches I mentioned will work, and will take additional code
to make it happen. But having custom properties like this limits
extensibility - that is not scalable for other property definitions
we'd like to make in the future.

> 
> Regards,
> Thomas.
> 
>>
>>
>> [1] http://marc.info/?t=139108946400001&r=1&w=2
>>
>>>  drivers/base/power/opp.c |   69 +++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>> index 2553867..de4d52d 100644
>>> --- a/drivers/base/power/opp.c
>>> +++ b/drivers/base/power/opp.c
>>> @@ -62,6 +62,7 @@ struct dev_pm_opp {
>>>       struct list_head node;
>>>
>>>       bool available;
>>> +     bool boost;
>>>       unsigned long rate;
>>>       unsigned long u_volt;
>>>
>>> @@ -380,10 +381,12 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>>>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>>>
>>>  /**
>>> - * dev_pm_opp_add()  - Add an OPP table from a table definitions
>>> + * dev_pm_opp_add_flags()  - Add an OPP to device OPP list with flags
>>>   * @dev:     device for which we do this operation
>>>   * @freq:    Frequency in Hz for this OPP
>>>   * @u_volt:  Voltage in uVolts for this OPP
>>> + * @available:       initial availability of the OPP with adding it to the list.
>>> + * @boost:   availability of this opp in controller's boost operating mode.
>>>   *
>>>   * This function adds an opp definition to the opp list and returns status.
>>>   * The opp is made available by default and it can be controlled using
>>> @@ -395,7 +398,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>>>   * that this function is *NOT* called under RCU protection or in contexts where
>>>   * mutex cannot be locked.
>>>   */
>>> -int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>> +static int dev_pm_opp_add_flags(struct device *dev, unsigned long freq,
>>> +                     unsigned long u_volt, bool available, bool boost)
>>>  {
>>>       struct device_opp *dev_opp = NULL;
>>>       struct dev_pm_opp *opp, *new_opp;
>>> @@ -441,7 +445,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>>       new_opp->dev_opp = dev_opp;
>>>       new_opp->rate = freq;
>>>       new_opp->u_volt = u_volt;
>>> -     new_opp->available = true;
>>> +     new_opp->available = available;
>>> +     new_opp->boost = boost;
>>>
>>>       /* Insert new OPP in order of increasing frequency */
>>>       head = &dev_opp->opp_list;
>>> @@ -462,6 +467,27 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>>       srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
>>>       return 0;
>>>  }
>>> +
>>> +/**
>>> + * dev_pm_opp_add()  - Add an OPP table from a table definitions
>>> + * @dev:     device for which we do this operation
>>> + * @freq:    Frequency in Hz for this OPP
>>> + * @u_volt:  Voltage in uVolts for this OPP
>>> + *
>>> + * This function adds an opp definition to the opp list and returns status.
>>> + * The opp is made available by default and it can be controlled using
>>> + * dev_pm_opp_enable/disable functions.
>>> + *
>>> + * Locking: The internal device_opp and opp structures are RCU protected.
>>> + * Hence this function internally uses RCU updater strategy with mutex locks
>>> + * to keep the integrity of the internal data structures. Callers should ensure
>>> + * that this function is *NOT* called under RCU protection or in contexts where
>>> + * mutex cannot be locked.
>>> + */
>>> +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>> +{
>>> +     return dev_pm_opp_add_flags(dev, freq, u_volt, true, false);
>>> +}
>>>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
>>>
>>>  /**
>>> @@ -651,7 +677,8 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
>>>
>>>       list_for_each_entry(opp, &dev_opp->opp_list, node) {
>>>               if (opp->available) {
>>> -                     freq_table[i].driver_data = i;
>>> +                     freq_table[i].driver_data =
>>> +                             opp->boost ? CPUFREQ_BOOST_FREQ : i;
>>>                       freq_table[i].frequency = opp->rate / 1000;
>>>                       i++;
>>>               }
>>> @@ -701,19 +728,14 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
>>>  }
>>>
>>>  #ifdef CONFIG_OF
>>> -/**
>>> - * of_init_opp_table() - Initialize opp table from device tree
>>> - * @dev:     device pointer used to lookup device OPPs.
>>> - *
>>> - * Register the initial OPP table with the OPP library for given device.
>>> - */
>>> -int of_init_opp_table(struct device *dev)
>>> +static int of_parse_opp_table(struct device *dev, const char *prop_name,
>>> +                                     bool boost)
>>>  {
>>>       const struct property *prop;
>>>       const __be32 *val;
>>>       int nr;
>>>
>>> -     prop = of_find_property(dev->of_node, "operating-points", NULL);
>>> +     prop = of_find_property(dev->of_node, prop_name, NULL);
>>>       if (!prop)
>>>               return -ENODEV;
>>>       if (!prop->value)
>>> @@ -734,7 +756,7 @@ int of_init_opp_table(struct device *dev)
>>>               unsigned long freq = be32_to_cpup(val++) * 1000;
>>>               unsigned long volt = be32_to_cpup(val++);
>>>
>>> -             if (dev_pm_opp_add(dev, freq, volt)) {
>>> +             if (dev_pm_opp_add_flags(dev, freq, volt, true, boost)) {
>>>                       dev_warn(dev, "%s: Failed to add OPP %ld\n",
>>>                                __func__, freq);
>>>                       continue;
>>> @@ -744,5 +766,26 @@ int of_init_opp_table(struct device *dev)
>>>
>>>       return 0;
>>>  }
>>> +
>>> +/**
>>> + * of_init_opp_table() - Initialize opp table from device tree
>>> + * @dev:     device pointer used to lookup device OPPs.
>>> + *
>>> + * Register the initial OPP table with the OPP library for given device.
>>> + * Additional "boost" operating points of the controller, if any, are
>>> + * specified with "boost-opp" property.
>>> + */
>>> +int of_init_opp_table(struct device *dev)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = of_parse_opp_table(dev, "operating-points", false);
>>> +     if (!ret) {
>>> +             ret = of_parse_opp_table(dev, "boost-opp", true);
>>> +             if (ret == -ENODEV)
>>> +                     ret = 0;
>>> +     }
>>> +     return ret;
>>> +}
>>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>>  #endif
>>>
>>
>>
>> --
>> Regards,
>> Nishanth Menon


-- 
Regards,
Nishanth Menon

[PATCH 2/2] Documentation: devicetree: Add boost-opp binding to list boost mode OPPs

[Thomas Abraham]

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

Certain CPUs or devices can support optional boost operating modes. Add a new
binding to list OPPs to be additionally made available in boost operating modes.

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>
---
 Documentation/devicetree/bindings/power/opp.txt |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5..4df5cca 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -10,6 +10,10 @@ Properties:
 	freq: clock frequency in kHz
 	vol: voltage in microvolt
 
+Optional Properties:
+- boost-opp: Similar to "operating-points" property but usable only in
+  optional boost operating modes.
+
 Examples:
 
 cpu@0 {
@@ -22,4 +26,9 @@ cpu@0 {
 		396000  950000
 		198000  850000
 	>;
+	boost-opp = <
+		/* kHz     uV */
+		1500000 1350000
+		1400000 1285000
+	>;
 };
-- 
1.7.10.4

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

[Rob Herring]

On Tue, Feb 4, 2014 at 3:41 AM, Thomas Abraham <ta.omasab@gmail.com> wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Certain CPUs or devices can support optional boost operating modes. Add a new
> binding to list OPPs to be additionally made available in boost operating modes.
>
> 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>
> ---
>  Documentation/devicetree/bindings/power/opp.txt |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5..4df5cca 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -10,6 +10,10 @@ Properties:
>         freq: clock frequency in kHz
>         vol: voltage in microvolt
>
> +Optional Properties:
> +- boost-opp: Similar to "operating-points" property but usable only in
> +  optional boost operating modes.
> +
>  Examples:
>
>  cpu@0 {
> @@ -22,4 +26,9 @@ cpu@0 {
>                 396000  950000
>                 198000  850000
>         >;
> +       boost-opp = <
> +               /* kHz     uV */
> +               1500000 1350000
> +               1400000 1285000
> +       >;

This looks like an example of needing to add more properties to the
OPP table. There are ongoing discussions on how to extend OPP table
and map to C states. This should be part of that discussion.

Rob