[PATCH v4 0/4] Introduce "opp-microwatt" and Energy Model from DT

[PATCH v4 0/4] Introduce "opp-microwatt" and Energy Model from DT

[Lukasz Luba]

Hi all,

This patch set solves a few issues:
1. It allows to register EM from DT, when the voltage information is not
   available. (Some background of the issues present on Chromebook devices
   can be checked at [1].)
2. It allows to register 'advanced' EM from the DT, which is more accurate
   and reflects total power (dynamic + static).

Implementation details:
Existing machinery in the OPP framework now handles "opp-microwatt", similarly
to "opp-microamp". It also has helper exported function to get power from OPP.
For the EM, it adds a new callback in OPP framework to use this new API and
read power while having an opp pointer. It's agreed to work with OPP-v2.

Comments, suggestions are very welcome.

changelog:
v4:
- changed calculation of power, summing add all supliers power for an opp
- added opp debugfs for u_watt
- added patternProperties in dt-bindings for this new opp-microwatt
- changed the EM opp registration flow and used one EM api registration call
- changed name of _get_opp_power() to _get_dt_power()
- removed 'advanced' word from the new paragraph and content in EM doc changes
v3 [2]

Regards,
Lukasz Luba

[1] https://lore.kernel.org/linux-pm/20220207073036.14901-2-lukasz.luba@arm.com/
[2] https://lore.kernel.org/linux-pm/20220224081131.27282-1-lukasz.luba@arm.com/

Lukasz Luba (4):
  dt-bindings: opp: Add "opp-microwatt" entry in the OPP
  OPP: Add "opp-microwatt" supporting code
  OPP: Add support of "opp-microwatt" for advanced EM registration
  Documentation: EM: Describe new registration method using DT

 .../devicetree/bindings/opp/opp-v2-base.yaml  |  23 ++++
 Documentation/power/energy-model.rst          |  10 ++
 drivers/opp/core.c                            |  25 +++++
 drivers/opp/debugfs.c                         |   3 +
 drivers/opp/of.c                              | 104 +++++++++++++++++-
 include/linux/pm_opp.h                        |  12 +-
 6 files changed, 174 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v4 1/4] dt-bindings: opp: Add "opp-microwatt" entry in the OPP

[Lukasz Luba]

Add new entry for the OPP which provides information about power
expressed in micro-Watts. It is useful for the Energy Model framework.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 .../devicetree/bindings/opp/opp-v2-base.yaml  | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 15a76bcd6d42..04a592c0f862 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -93,6 +93,21 @@ patternProperties:
         minItems: 1
         maxItems: 8   # Should be enough regulators
 
+      opp-microwatt:
+        description: |
+          The power for the OPP in micro-Watts.
+
+          Entries for multiple regulators shall be provided in the same field
+          separated by angular brackets <>. If current values aren't required
+          for a regulator, then it shall be filled with 0. If power values
+          aren't required for any of the regulators, then this field is not
+          required. The OPP binding doesn't provide any provisions to relate the
+          values to their power supplies or the order in which the supplies need
+          to be configured and that is left for the implementation specific
+          binding.
+        minItems: 1
+        maxItems: 8   # Should be enough regulators
+
       opp-level:
         description:
           A value representing the performance level of the device.
@@ -203,6 +218,14 @@ patternProperties:
         minItems: 1
         maxItems: 8   # Should be enough regulators
 
+      '^opp-microwatt':
+        description:
+          Named opp-microwatt property. Similar to opp-microamp property,
+          but for microwatt instead.
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        minItems: 1
+        maxItems: 8   # Should be enough regulators
+
     dependencies:
       opp-avg-kBps: [ opp-peak-kBps ]
 
-- 
2.17.1

[PATCH v4 2/4] OPP: Add "opp-microwatt" supporting code

[Lukasz Luba]

Add new property to the OPP: power value. The OPP entry in the DT can have
"opp-microwatt". Add the needed code to handle this new property in the
existing infrastructure.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/opp/core.c     | 25 ++++++++++++++++++++++
 drivers/opp/debugfs.c  |  3 +++
 drivers/opp/of.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/pm_opp.h | 12 ++++++++++-
 4 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3057beabd370..740407252298 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -113,6 +113,31 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage);
 
+/**
+ * dev_pm_opp_get_power() - Gets the power corresponding to an opp
+ * @opp:	opp for which power has to be returned for
+ *
+ * Return: power in micro watt corresponding to the opp, else
+ * return 0
+ *
+ * This is useful only for devices with single power supply.
+ */
+unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
+{
+	unsigned long opp_power = 0;
+	int i;
+
+	if (IS_ERR_OR_NULL(opp)) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return 0;
+	}
+	for (i = 0; i < opp->opp_table->regulator_count; i++)
+		opp_power += opp->supplies[i].u_watt;
+
+	return opp_power;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_power);
+
 /**
  * dev_pm_opp_get_freq() - Gets the frequency corresponding to an available opp
  * @opp:	opp for which frequency has to be returned for
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 596c185b5dda..45837f3c1765 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -99,6 +99,9 @@ static void opp_debug_create_supplies(struct dev_pm_opp *opp,
 
 		debugfs_create_ulong("u_amp", S_IRUGO, d,
 				     &opp->supplies[i].u_amp);
+
+		debugfs_create_ulong("u_watt", S_IRUGO, d,
+				     &opp->supplies[i].u_watt);
 	}
 }
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 2f40afa4e65c..7bff30f27dc1 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -575,8 +575,9 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 			      struct opp_table *opp_table)
 {
-	u32 *microvolt, *microamp = NULL;
-	int supplies = opp_table->regulator_count, vcount, icount, ret, i, j;
+	u32 *microvolt, *microamp = NULL, *microwatt = NULL;
+	int supplies = opp_table->regulator_count;
+	int vcount, icount, pcount, ret, i, j;
 	struct property *prop = NULL;
 	char name[NAME_MAX];
 
@@ -688,6 +689,43 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		}
 	}
 
+	/* Search for "opp-microwatt" */
+	sprintf(name, "opp-microwatt");
+	prop = of_find_property(opp->np, name, NULL);
+
+	if (prop) {
+		pcount = of_property_count_u32_elems(opp->np, name);
+		if (pcount < 0) {
+			dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
+				name, pcount);
+			ret = pcount;
+			goto free_microamp;
+		}
+
+		if (pcount != supplies) {
+			dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
+				__func__, name, pcount, supplies);
+			ret = -EINVAL;
+			goto free_microamp;
+		}
+
+		microwatt = kmalloc_array(pcount, sizeof(*microwatt),
+					  GFP_KERNEL);
+		if (!microwatt) {
+			ret = -EINVAL;
+			goto free_microamp;
+		}
+
+		ret = of_property_read_u32_array(opp->np, name, microwatt,
+						 pcount);
+		if (ret) {
+			dev_err(dev, "%s: error parsing %s: %d\n", __func__,
+				name, ret);
+			ret = -EINVAL;
+			goto free_microwatt;
+		}
+	}
+
 	for (i = 0, j = 0; i < supplies; i++) {
 		opp->supplies[i].u_volt = microvolt[j++];
 
@@ -701,8 +739,13 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 
 		if (microamp)
 			opp->supplies[i].u_amp = microamp[i];
+
+		if (microwatt)
+			opp->supplies[i].u_watt = microwatt[i];
 	}
 
+free_microwatt:
+	kfree(microwatt);
 free_microamp:
 	kfree(microamp);
 free_microvolt:
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 879c138c7b8e..0d85a63a1f78 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -32,14 +32,17 @@ enum dev_pm_opp_event {
  * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
  * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
  * @u_amp:	Maximum current drawn by the device in microamperes
+ * @u_watt:	Power used by the device in microwatts
  *
- * This structure stores the voltage/current values for a single power supply.
+ * This structure stores the voltage/current/power values for a single power
+ * supply.
  */
 struct dev_pm_opp_supply {
 	unsigned long u_volt;
 	unsigned long u_volt_min;
 	unsigned long u_volt_max;
 	unsigned long u_amp;
+	unsigned long u_watt;
 };
 
 /**
@@ -94,6 +97,8 @@ void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
 
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 
+unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp);
+
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
 
 unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp);
@@ -186,6 +191,11 @@ static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
+{
+	return 0;
+}
+
 static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 {
 	return 0;
-- 
2.17.1

[PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Lukasz Luba]

The Energy Model (EM) can be created based on DT entry:
'dynamic-power-coefficient'. It's a 'simple' EM which is limited to the
dynamic power. It has to fit into the math formula which requires also
information about voltage. Some of the platforms don't expose voltage
information, thus it's not possible to use EM registration using DT.

This patch aims to fix it. It introduces new implementation of the EM
registration callback. The new mechanism relies on the new OPP feature
allowing to get power (which is coming from "opp-microwatt" DT property)
expressed in micro-Watts.

The patch also opens new opportunity to better support platforms, which
have a decent static power. It allows to register 'advanced' EM (based
on real power measurements) which models total power (static + dynamic),
so better reflects real HW.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/opp/of.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7bff30f27dc1..9bae26577aca 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1438,6 +1438,38 @@ struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_of_node);
 
+/*
+ * Callback function provided to the Energy Model framework upon registration.
+ * It provides the power used by @dev at @kHz if it is the frequency of an
+ * existing OPP, or at the frequency of the first OPP above @kHz otherwise
+ * (see dev_pm_opp_find_freq_ceil()). This function updates @kHz to the ceiled
+ * frequency and @mW to the associated power.
+ *
+ * Returns 0 on success or a proper -EINVAL value in case of error.
+ */
+static int __maybe_unused
+_get_dt_power(unsigned long *mW, unsigned long *kHz, struct device *dev)
+{
+	struct dev_pm_opp *opp;
+	unsigned long opp_freq, opp_power;
+
+	/* Find the right frequency and related OPP */
+	opp_freq = *kHz * 1000;
+	opp = dev_pm_opp_find_freq_ceil(dev, &opp_freq);
+	if (IS_ERR(opp))
+		return -EINVAL;
+
+	opp_power = dev_pm_opp_get_power(opp);
+	dev_pm_opp_put(opp);
+	if (!opp_power)
+		return -EINVAL;
+
+	*kHz = opp_freq / 1000;
+	*mW = opp_power / 1000;
+
+	return 0;
+}
+
 /*
  * Callback function provided to the Energy Model framework upon registration.
  * This computes the power estimated by @dev at @kHz if it is the frequency
@@ -1488,6 +1520,24 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz,
 	return 0;
 }
 
+static bool _of_has_opp_microwatt_property(struct device *dev)
+{
+	unsigned long power, freq = 0;
+	struct dev_pm_opp *opp;
+
+	/* Check if at least one OPP has needed property */
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+	if (IS_ERR(opp))
+		return false;
+
+	power = dev_pm_opp_get_power(opp);
+	dev_pm_opp_put(opp);
+	if (!power)
+		return false;
+
+	return true;
+}
+
 /**
  * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
  * @dev		: Device for which an Energy Model has to be registered
@@ -1517,6 +1567,12 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
 		goto failed;
 	}
 
+	/* First, try to find more precised Energy Model in DT */
+	if (_of_has_opp_microwatt_property(dev)) {
+		em_cb.active_power = _get_dt_power;
+		goto register_em;
+	}
+
 	np = of_node_get(dev->of_node);
 	if (!np) {
 		ret = -EINVAL;
@@ -1538,6 +1594,7 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
 		goto failed;
 	}
 
+register_em:
 	ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
 	if (ret)
 		goto failed;
-- 
2.17.1

[PATCH v4 4/4] Documentation: EM: Describe new registration method using DT

[Lukasz Luba]

The new registration method allows to get power values from the DT OPP
definition. The new OPP entry property "opp-microwatt" contains total
power expressed in micro-Watts. Align the EM documentation with this
new possible registration method of EM.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 Documentation/power/energy-model.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/power/energy-model.rst b/Documentation/power/energy-model.rst
index 5ac62a7b4b7c..49549aab41b4 100644
--- a/Documentation/power/energy-model.rst
+++ b/Documentation/power/energy-model.rst
@@ -113,6 +113,16 @@ to: return warning/error, stop working or panic.
 See Section 3. for an example of driver implementing this
 callback, or Section 2.4 for further documentation on this API
 
+Registration of EM using DT
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The  EM can also be registered using OPP framework and information in DT
+"operating-points-v2". Each OPP entry in DT can be extended with a property
+"opp-microwatt" containing micro-Watts power value. This OPP DT property
+allows a platform to register EM power values which are reflecting total power
+(static + dynamic). These power values might be coming directly from
+experiments and measurements.
+
 Registration of 'simple' EM
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.17.1

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[kernel test robot]

Hi Lukasz,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linux/master rafael-pm/linux-next linus/master v5.17-rc6 next-20220301]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lukasz-Luba/Introduce-opp-microwatt-and-Energy-Model-from-DT/20220301-173700
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-randconfig-r014-20220301 (https://download.01.org/0day-ci/archive/20220302/202203020012.g7rYUP7M-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e1e83de9bf2f261cb3e5eb9f53bde3c83c6e0647
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Lukasz-Luba/Introduce-opp-microwatt-and-Energy-Model-from-DT/20220301-173700
        git checkout e1e83de9bf2f261cb3e5eb9f53bde3c83c6e0647
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/opp/of.c: In function 'dev_pm_opp_of_register_em':
>> drivers/opp/of.c:1572:22: error: 'struct em_data_callback' has no member named 'active_power'
    1572 |                 em_cb.active_power = _get_dt_power;
         |                      ^


vim +1572 drivers/opp/of.c

  1540	
  1541	/**
  1542	 * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
  1543	 * @dev		: Device for which an Energy Model has to be registered
  1544	 * @cpus	: CPUs for which an Energy Model has to be registered. For
  1545	 *		other type of devices it should be set to NULL.
  1546	 *
  1547	 * This checks whether the "dynamic-power-coefficient" devicetree property has
  1548	 * been specified, and tries to register an Energy Model with it if it has.
  1549	 * Having this property means the voltages are known for OPPs and the EM
  1550	 * might be calculated.
  1551	 */
  1552	int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
  1553	{
  1554		struct em_data_callback em_cb = EM_DATA_CB(_get_power);
  1555		struct device_node *np;
  1556		int ret, nr_opp;
  1557		u32 cap;
  1558	
  1559		if (IS_ERR_OR_NULL(dev)) {
  1560			ret = -EINVAL;
  1561			goto failed;
  1562		}
  1563	
  1564		nr_opp = dev_pm_opp_get_opp_count(dev);
  1565		if (nr_opp <= 0) {
  1566			ret = -EINVAL;
  1567			goto failed;
  1568		}
  1569	
  1570		/* First, try to find more precised Energy Model in DT */
  1571		if (_of_has_opp_microwatt_property(dev)) {
> 1572			em_cb.active_power = _get_dt_power;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Lukasz Luba]

On 3/1/22 16:58, kernel test robot wrote:
> Hi Lukasz,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on linux/master rafael-pm/linux-next linus/master v5.17-rc6 next-20220301]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Lukasz-Luba/Introduce-opp-microwatt-and-Energy-Model-from-DT/20220301-173700
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: arc-randconfig-r014-20220301 (https://download.01.org/0day-ci/archive/20220302/202203020012.g7rYUP7M-lkp@intel.com/config)
> compiler: arceb-elf-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/0day-ci/linux/commit/e1e83de9bf2f261cb3e5eb9f53bde3c83c6e0647
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Lukasz-Luba/Introduce-opp-microwatt-and-Energy-Model-from-DT/20220301-173700
>          git checkout e1e83de9bf2f261cb3e5eb9f53bde3c83c6e0647
>          # save the config file to linux build tree
>          mkdir build_dir
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     drivers/opp/of.c: In function 'dev_pm_opp_of_register_em':
>>> drivers/opp/of.c:1572:22: error: 'struct em_data_callback' has no member named 'active_power'
>      1572 |                 em_cb.active_power = _get_dt_power;
>           |                      ^
> 

Indeed, this structure is an empty definition for !EM kernels.
Let me create a fix for that.

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Viresh Kumar]

On 01-03-22, 09:35, Lukasz Luba wrote:
> The Energy Model (EM) can be created based on DT entry:
> 'dynamic-power-coefficient'. It's a 'simple' EM which is limited to the
> dynamic power. It has to fit into the math formula which requires also
> information about voltage. Some of the platforms don't expose voltage
> information, thus it's not possible to use EM registration using DT.
> 
> This patch aims to fix it. It introduces new implementation of the EM
> registration callback. The new mechanism relies on the new OPP feature
> allowing to get power (which is coming from "opp-microwatt" DT property)
> expressed in micro-Watts.
> 
> The patch also opens new opportunity to better support platforms, which
> have a decent static power. It allows to register 'advanced' EM (based
> on real power measurements) which models total power (static + dynamic),
> so better reflects real HW.

Advanced :(

-- 
viresh

Re: [PATCH v4 0/4] Introduce "opp-microwatt" and Energy Model from DT

[Viresh Kumar]

On 01-03-22, 09:35, Lukasz Luba wrote:
> Hi all,
> 
> This patch set solves a few issues:
> 1. It allows to register EM from DT, when the voltage information is not
>    available. (Some background of the issues present on Chromebook devices
>    can be checked at [1].)
> 2. It allows to register 'advanced' EM from the DT, which is more accurate
>    and reflects total power (dynamic + static).
> 
> Implementation details:
> Existing machinery in the OPP framework now handles "opp-microwatt", similarly
> to "opp-microamp". It also has helper exported function to get power from OPP.
> For the EM, it adds a new callback in OPP framework to use this new API and
> read power while having an opp pointer. It's agreed to work with OPP-v2.
> 
> Comments, suggestions are very welcome.
> 
> changelog:
> v4:
> - changed calculation of power, summing add all supliers power for an opp
> - added opp debugfs for u_watt
> - added patternProperties in dt-bindings for this new opp-microwatt
> - changed the EM opp registration flow and used one EM api registration call
> - changed name of _get_opp_power() to _get_dt_power()
> - removed 'advanced' word from the new paragraph and content in EM doc changes
> v3 [2]

Apart from one minor things, the patchset looks fine now. Thanks for
considering the review comments :)

-- 
viresh

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Lukasz Luba]

On 3/2/22 04:53, Viresh Kumar wrote:
> On 01-03-22, 09:35, Lukasz Luba wrote:
>> The Energy Model (EM) can be created based on DT entry:
>> 'dynamic-power-coefficient'. It's a 'simple' EM which is limited to the
>> dynamic power. It has to fit into the math formula which requires also
>> information about voltage. Some of the platforms don't expose voltage
>> information, thus it's not possible to use EM registration using DT.
>>
>> This patch aims to fix it. It introduces new implementation of the EM
>> registration callback. The new mechanism relies on the new OPP feature
>> allowing to get power (which is coming from "opp-microwatt" DT property)
>> expressed in micro-Watts.
>>
>> The patch also opens new opportunity to better support platforms, which
>> have a decent static power. It allows to register 'advanced' EM (based
>> on real power measurements) which models total power (static + dynamic),
>> so better reflects real HW.
> 
> Advanced :(
> 

You're right, here these words sneak in. I'm going to resent this patch
because it has this reported build issue for !EM. I'll update the words
there as well.

BTW, for the build fix, I'm going for the v3 approach:

------------8<--------------
         if (_of_has_opp_microwatt_property(dev)) {
                 struct em_data_callback em_cb_dt = 
EM_DATA_CB(_get_dt_power);

                 return em_dev_register_perf_domain(dev, nr_opp, &em_cb_dt,
                                                    cpus, true);
         }

----------->8---------------

I don't want to introduce another dual-macro mechanism, since
the type em_data_callback is not defined in !EM.

I hope you are OK with this approach, if not please let me know.

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Viresh Kumar]

On 01-03-22, 09:35, Lukasz Luba wrote:
>  /**
>   * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
>   * @dev		: Device for which an Energy Model has to be registered
> @@ -1517,6 +1567,12 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>  		goto failed;
>  	}
>  
> +	/* First, try to find more precised Energy Model in DT */
> +	if (_of_has_opp_microwatt_property(dev)) {
> +		em_cb.active_power = _get_dt_power;

You can also do (to fix the warning) this instead:

em_cb = EM_DATA_CB(_get_dt_power);

Similar for the else part.

> +		goto register_em;
> +	}
> +
>  	np = of_node_get(dev->of_node);
>  	if (!np) {
>  		ret = -EINVAL;
> @@ -1538,6 +1594,7 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>  		goto failed;
>  	}
>  
> +register_em:
>  	ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
>  	if (ret)
>  		goto failed;
> -- 
> 2.17.1

-- 
viresh

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Lukasz Luba]

On 3/2/22 07:45, Viresh Kumar wrote:
> On 01-03-22, 09:35, Lukasz Luba wrote:
>>   /**
>>    * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
>>    * @dev		: Device for which an Energy Model has to be registered
>> @@ -1517,6 +1567,12 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>>   		goto failed;
>>   	}
>>   
>> +	/* First, try to find more precised Energy Model in DT */
>> +	if (_of_has_opp_microwatt_property(dev)) {
>> +		em_cb.active_power = _get_dt_power;
> 
> You can also do (to fix the warning) this instead:
> 
> em_cb = EM_DATA_CB(_get_dt_power);
> 
> Similar for the else part.
> 

Unfortunately, not for this case. When we declare that em_cb
it can handle the brackets '{ }', but not later in the code no.
In both configs it won't fly:

EM:
drivers/opp/of.c: In function ‘dev_pm_opp_of_register_em’:
./include/linux/energy_model.h:118:38: error: expected expression before 
‘{’ token
  #define EM_DATA_CB(_active_power_cb) { .active_power = &_active_power_cb }
                                       ^
drivers/opp/of.c:1573:11: note: in expansion of macro ‘EM_DATA_CB’
    em_cb = EM_DATA_CB(_get_dt_power);
            ^~~~~~~~~~
./include/linux/energy_model.h:118:38: error: expected expression before 
‘{’ token
  #define EM_DATA_CB(_active_power_cb) { .active_power = &_active_power_cb }
                                       ^
drivers/opp/of.c:1600:10: note: in expansion of macro ‘EM_DATA_CB’
   em_cb = EM_DATA_CB(_get_power);
           ^~~~~~~~~~

!EM:
drivers/opp/of.c: In function ‘dev_pm_opp_of_register_em’:
./include/linux/energy_model.h:266:38: error: expected expression before 
‘{’ token
  #define EM_DATA_CB(_active_power_cb) { }
                                       ^
drivers/opp/of.c:1573:11: note: in expansion of macro ‘EM_DATA_CB’
    em_cb = EM_DATA_CB(_get_dt_power);
            ^~~~~~~~~~
./include/linux/energy_model.h:266:38: error: expected expression before 
‘{’ token
  #define EM_DATA_CB(_active_power_cb) { }
                                       ^
drivers/opp/of.c:1600:10: note: in expansion of macro ‘EM_DATA_CB’
   em_cb = EM_DATA_CB(_get_power);
           ^~~~~~~~~~


If you like, I can introduce new dual-macro implementation
in energy_modle.h which would sole this issue:

ifdef EM:
#define EM_SET_ACTIVE_POWER_CB(em_cb, cb) ((em_cb).active_power = cb)

ifndef EM:
#define EM_SET_ACTIVE_POWER_CB(em_cb, cb) do { } while (0)


Then we would keep the single call to the registration EM and
we would have:

         if (_of_has_opp_microwatt_property(dev)) {
                 EM_SET_ACTIVE_POWER_CB(em_cb, _get_dt_power);
                 goto register_em;
         }


	
         EM_SET_ACTIVE_POWER_CB(em_cb, _get_power);

register_em:
         ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);


I can do that, please let me know.

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Viresh Kumar]

On 02-03-22, 08:50, Lukasz Luba wrote:
> If you like, I can introduce new dual-macro implementation
> in energy_modle.h which would sole this issue:
> 
> ifdef EM:
> #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) ((em_cb).active_power = cb)
> 
> ifndef EM:
> #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) do { } while (0)
> 
> 
> Then we would keep the single call to the registration EM and
> we would have:
> 
>         if (_of_has_opp_microwatt_property(dev)) {
>                 EM_SET_ACTIVE_POWER_CB(em_cb, _get_dt_power);
>                 goto register_em;
>         }
> 
> 
> 	
>         EM_SET_ACTIVE_POWER_CB(em_cb, _get_power);
> 
> register_em:
>         ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
> 
> 
> I can do that, please let me know.

That will work as well.

-- 
viresh

Re: [PATCH v4 3/4] OPP: Add support of "opp-microwatt" for advanced EM registration

[Lukasz Luba]

On 3/2/22 09:30, Viresh Kumar wrote:
> On 02-03-22, 08:50, Lukasz Luba wrote:
>> If you like, I can introduce new dual-macro implementation
>> in energy_modle.h which would sole this issue:
>>
>> ifdef EM:
>> #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) ((em_cb).active_power = cb)
>>
>> ifndef EM:
>> #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) do { } while (0)
>>
>>
>> Then we would keep the single call to the registration EM and
>> we would have:
>>
>>          if (_of_has_opp_microwatt_property(dev)) {
>>                  EM_SET_ACTIVE_POWER_CB(em_cb, _get_dt_power);
>>                  goto register_em;
>>          }
>>
>>
>> 	
>>          EM_SET_ACTIVE_POWER_CB(em_cb, _get_power);
>>
>> register_em:
>>          ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
>>
>>
>> I can do that, please let me know.
> 
> That will work as well.
> 

Great. Thank you for your comments. I'll send v5.