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

[PATCH v3 0/4] Introduce "opp-microwatt" and 'advanced' 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:
v3:
- added generic support in the OPP for "opp-microwatt"
- renamed _of_has_opp_microwatt_property() and return bool from it
- simplified _of_has_opp_microwatt_property() just checking 1st opp's power
- squashed the return ret; into return em_dev_register_perf_domain();
- added EM documentation update patch
v2 [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/20220222140746.12293-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  |  15 +++
 Documentation/power/energy-model.rst          |  10 ++
 drivers/opp/core.c                            |  20 ++++
 drivers/opp/of.c                              | 105 +++++++++++++++++-
 include/linux/pm_opp.h                        |  12 +-
 5 files changed, 159 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v3 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      | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 15a76bcd6d42..433164186a98 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.
-- 
2.17.1

[PATCH v3 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     | 20 ++++++++++++++++++
 drivers/opp/of.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/pm_opp.h | 12 ++++++++++-
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3057beabd370..5e0cce924ba4 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -113,6 +113,26 @@ 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)
+{
+	if (IS_ERR_OR_NULL(opp)) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return 0;
+	}
+
+	return opp->supplies[0].u_watt;
+}
+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/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 v3 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 | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7bff30f27dc1..af3488779867 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_opp_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,14 @@ 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)) {
+		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power);
+
+		return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
+						   cpus, true);
+	}
+
 	np = of_node_get(dev->of_node);
 	if (!np) {
 		ret = -EINVAL;
-- 
2.17.1

[PATCH v3 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 'advanced' 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..719253fd5215 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 'advanced' EM using DT
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The 'advanced' 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 v3 1/4] dt-bindings: opp: Add "opp-microwatt" entry in the OPP

[Viresh Kumar]

On 24-02-22, 08:11, Lukasz Luba wrote:
> 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      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Also need to update patternProperties section I think.

-- 
viresh

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

[Viresh Kumar]

On 24-02-22, 08:11, Lukasz Luba wrote:
> +/**
> + * 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)
> +{
> +	if (IS_ERR_OR_NULL(opp)) {
> +		pr_err("%s: Invalid parameters\n", __func__);
> +		return 0;
> +	}
> +
> +	return opp->supplies[0].u_watt;

What about returning the total of all u_watts for this OPP here ?
Surely it doesn't make sense to do the same for voltage and current,
but power is different.

-- 
viresh

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

[Viresh Kumar]

On 24-02-22, 08:11, Lukasz Luba wrote:
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>  
> +/*
> + * 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_opp_power(unsigned long *mW, unsigned long *kHz, struct device *dev)

Lets call it _get_dt_opp_power() or _get_dt_power() ?

> +{
> +	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);

As I said in 2/4, this should really give the total instead.

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

What if this particular frequency has 0 power mentioned for some
reason :)

Instead of this heavy stuff, just pick the first OPP from the opp
table and see its power-value.

> +		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,14 @@ 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)) {
> +		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power);
> +
> +		return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
> +						   cpus, true);
> +	}
> +

We have another em_dev_register_perf_domain() down the line, lets keep
only one such call and get it a callback that should be set in an
if/else loop.

>  	np = of_node_get(dev->of_node);
>  	if (!np) {
>  		ret = -EINVAL;
> -- 
> 2.17.1

-- 
viresh

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

[Lukasz Luba]

On 2/24/22 09:01, Viresh Kumar wrote:
> On 24-02-22, 08:11, Lukasz Luba wrote:
>> 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      | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
> 
> Also need to update patternProperties section I think.
> 

You're right. If that would be the only thing for the patch set,
I will just resend this patch 1/4 below this thread.

Thank you for having a look at it.

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

[Viresh Kumar]

On 24-02-22, 08:11, Lukasz Luba wrote:
> 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     | 20 ++++++++++++++++++
>  drivers/opp/of.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pm_opp.h | 12 ++++++++++-
>  3 files changed, 76 insertions(+), 3 deletions(-)

Also update drivers/opp/debugfs.c please.

-- 
viresh

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

[Viresh Kumar]

On 24-02-22, 08:11, Lukasz Luba wrote:
> 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 'advanced' 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..719253fd5215 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 'advanced' EM using DT

I am not really sure if this should be called *Advanced*. Just call it
registration of EM using DT.

> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The 'advanced' 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

-- 
viresh

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

[Lukasz Luba]

On 2/24/22 09:15, Viresh Kumar wrote:
> On 24-02-22, 08:11, Lukasz Luba wrote:
>> 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 'advanced' 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..719253fd5215 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 'advanced' EM using DT
> 
> I am not really sure if this should be called *Advanced*. Just call it
> registration of EM using DT.

Our partners had a lot of issues with EM+EAS, because they were not
aware of the internals of EM and limitations.

We've started to name two types of EM: 'advanced' and 'simple'.
The 'simple' is the one which causes issues. Now when we contact with
partners we ask if they use 'simple' EM and see some issues in EAS.
This is a needed clarification and naming convention that we use.

Here the paragraph name is stressing the fact explicitly that
from today we have the option to provide real power measurements using
DT and it will be the 'advanced' EM.

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

[Lukasz Luba]

On 2/24/22 09:13, Viresh Kumar wrote:
> On 24-02-22, 08:11, Lukasz Luba wrote:
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>>   
>> +/*
>> + * 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_opp_power(unsigned long *mW, unsigned long *kHz, struct device *dev)
> 
> Lets call it _get_dt_opp_power() or _get_dt_power() ?

OK, I'll return to _get_dt_power()

> 
>> +{
>> +	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);
> 
> As I said in 2/4, this should really give the total instead.

make sense

> 
>> +	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)
> 
> What if this particular frequency has 0 power mentioned for some
> reason :)

If that power 0 would be allowed here, then in next step when EM
calls active_power() we check !power and report dev_err().
IPA design would also not accept the power=0

> 
> Instead of this heavy stuff, just pick the first OPP from the opp
> table and see its power-value.

It is the first opp: freq=0.
You mean by parsing the the DT node instead, like I had in v2 version?

> 
>> +		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,14 @@ 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)) {
>> +		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power);
>> +
>> +		return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
>> +						   cpus, true);
>> +	}
>> +
> 
> We have another em_dev_register_perf_domain() down the line, lets keep
> only one such call and get it a callback that should be set in an
> if/else loop.

OK

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

[Lukasz Luba]

On 2/24/22 09:14, Viresh Kumar wrote:
> On 24-02-22, 08:11, Lukasz Luba wrote:
>> 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     | 20 ++++++++++++++++++
>>   drivers/opp/of.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/pm_opp.h | 12 ++++++++++-
>>   3 files changed, 76 insertions(+), 3 deletions(-)
> 
> Also update drivers/opp/debugfs.c please.
> 

OK

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

[Lukasz Luba]

On 2/24/22 09:03, Viresh Kumar wrote:
> On 24-02-22, 08:11, Lukasz Luba wrote:
>> +/**
>> + * 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)
>> +{
>> +	if (IS_ERR_OR_NULL(opp)) {
>> +		pr_err("%s: Invalid parameters\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	return opp->supplies[0].u_watt;
> 
> What about returning the total of all u_watts for this OPP here ?
> Surely it doesn't make sense to do the same for voltage and current,
> but power is different.
> 

OK, I'll do the summation of all used supplies' power here.

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

[Viresh Kumar]

On 24-02-22, 09:25, Lukasz Luba wrote:
> Our partners had a lot of issues with EM+EAS, because they were not
> aware of the internals of EM and limitations.
> 
> We've started to name two types of EM: 'advanced' and 'simple'.
> The 'simple' is the one which causes issues. Now when we contact with
> partners we ask if they use 'simple' EM and see some issues in EAS.
> This is a needed clarification and naming convention that we use.
> 
> Here the paragraph name is stressing the fact explicitly that
> from today we have the option to provide real power measurements using
> DT and it will be the 'advanced' EM.

I understand the background now, and since I am part of the same
community I can appreciate that. But being a maintainer, I have to say
that when we look at something from Upstream's point of view, we may
have to neglect/ignore the terminology used in downstream.

From what I can see, there is no advancement here, as of now. This is
a very small change where we are getting pre-evaluated power values
from DT, instead of calculating them at runtime. The data may be more
correct, but the EM doesn't get advanced because of that. And so using
such terminology is only going to harm further. If EM gets a
"advanced" algorithm later on, which can improve things, then yes we
can call it advanced, but for now there is nothing.

-- 
viresh

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

[Lukasz Luba]

On 2/24/22 09:37, Viresh Kumar wrote:
> On 24-02-22, 09:25, Lukasz Luba wrote:
>> Our partners had a lot of issues with EM+EAS, because they were not
>> aware of the internals of EM and limitations.
>>
>> We've started to name two types of EM: 'advanced' and 'simple'.
>> The 'simple' is the one which causes issues. Now when we contact with
>> partners we ask if they use 'simple' EM and see some issues in EAS.
>> This is a needed clarification and naming convention that we use.
>>
>> Here the paragraph name is stressing the fact explicitly that
>> from today we have the option to provide real power measurements using
>> DT and it will be the 'advanced' EM.
> 
> I understand the background now, and since I am part of the same
> community I can appreciate that. But being a maintainer, I have to say
> that when we look at something from Upstream's point of view, we may
> have to neglect/ignore the terminology used in downstream.

I understand your upstream point of view.

> 
>  From what I can see, there is no advancement here, as of now. This is
> a very small change where we are getting pre-evaluated power values
> from DT, instead of calculating them at runtime. The data may be more
> correct, but the EM doesn't get advanced because of that. And so using
> such terminology is only going to harm further. If EM gets a
> "advanced" algorithm later on, which can improve things, then yes we
> can call it advanced, but for now there is nothing.
> 

Fair enough, I'll drop this word from the paragraph name.

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

[Viresh Kumar]

On 24-02-22, 09:33, Lukasz Luba wrote:
> On 2/24/22 09:13, Viresh Kumar wrote:
> > On 24-02-22, 08:11, Lukasz Luba wrote:
> > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c

> > > +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)
> > 
> > Instead of this heavy stuff, just pick the first OPP from the opp
> > table and see its power-value.
> 
> It is the first opp: freq=0.
> You mean by parsing the the DT node instead, like I had in v2 version?

No, I was thinking if you can simply do:

opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);

But that requires locking, etc as well. So maybe this is fine.
 
-- 
viresh

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

[Lukasz Luba]

On 2/24/22 09:42, Viresh Kumar wrote:
> On 24-02-22, 09:33, Lukasz Luba wrote:
>> On 2/24/22 09:13, Viresh Kumar wrote:
>>> On 24-02-22, 08:11, Lukasz Luba wrote:
>>>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> 
>>>> +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)
>>>
>>> Instead of this heavy stuff, just pick the first OPP from the opp
>>> table and see its power-value.
>>
>> It is the first opp: freq=0.
>> You mean by parsing the the DT node instead, like I had in v2 version?
> 
> No, I was thinking if you can simply do:
> 
> opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);
> 
> But that requires locking, etc as well. So maybe this is fine.
>   

OK, so I'll leave it as is now.

Thank you for the comments. I'll work on v4