[PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[PATCH v2 0/2] Introduce 'advanced' Energy Model in 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:
It adds a new callback in OPP framework to parse the OPP node entry and
read the "opp-microwatt". It's going to only work with OPP-v2, but it's
agreed to be OK.

Comments, suggestions are very welcome.

changelog:
v2:
- implemented Viresh idea to add "opp-microwatt" into the OPP node entry in DT
v1 [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/20220221225131.15836-1-lukasz.luba@arm.com/

Lukasz Luba (2):
  dt-bindings: opp: Add 'opp-microwatt' entry in the OPP
  OPP: Add 'opp-microwatt' parsing for advanced EM registration

 .../devicetree/bindings/opp/opp-v2-base.yaml  |  7 ++
 drivers/opp/of.c                              | 70 +++++++++++++++++++
 2 files changed, 77 insertions(+)

-- 
2.17.1

[[PATCH v2 1/2] 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>
---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 15a76bcd6d42..3f07a279ed2a 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -93,6 +93,13 @@ patternProperties:
         minItems: 1
         maxItems: 8   # Should be enough regulators
 
+      opp-microwatt:
+        description:
+          Power for the OPP
+
+          A value representing power for the OPP in micro-Watts.
+        $ref: /schemas/types.yaml#/definitions/uint32
+
       opp-level:
         description:
           A value representing the performance level of the device.
-- 
2.17.1

[[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing 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 parses OPP node in DT which
contains the power expressed in micro-Watts. It also allows to register
'advanced' EM, which models total power (static + dynamic), so better
reflects real HW.

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

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 2f40afa4e65c..94059408fa39 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1395,6 +1395,40 @@ 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 based on DT 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;
+	u32 opp_power;
+	int ret;
+
+	/* 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;
+
+	ret = of_property_read_u32(opp->np, "opp-microwatt", &opp_power);
+	dev_pm_opp_put(opp);
+	if (ret)
+		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
@@ -1445,6 +1479,33 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz,
 	return 0;
 }
 
+static int _of_find_opp_microwatt_property(struct device *dev)
+{
+	unsigned long freq = 0;
+	struct dev_pm_opp *opp;
+	struct device_node *np;
+	struct property *prop;
+
+	/* We only support "operating-points-v2" */
+	np = dev_pm_opp_of_get_opp_desc_node(dev);
+	if (!np)
+		return -EINVAL;
+
+	of_node_put(np);
+
+	/* Check if an OPP has needed property */
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+	if (IS_ERR(opp))
+		return -EINVAL;
+
+	prop = of_find_property(opp->np, "opp-microwatt", NULL);
+	dev_pm_opp_put(opp);
+	if (!prop)
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
  * @dev		: Device for which an Energy Model has to be registered
@@ -1474,6 +1535,15 @@ 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_find_opp_microwatt_property(dev)) {
+		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_dt_power);
+
+		ret = em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
+						  cpus, true);
+		return ret;
+	}
+
 	np = of_node_get(dev->of_node);
 	if (!np) {
 		ret = -EINVAL;
-- 
2.17.1

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Matthias Kaehlcke]

On Tue, Feb 22, 2022 at 02:07:46PM +0000, 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 parses OPP node in DT which
> contains the power expressed in micro-Watts. It also allows to register
> 'advanced' EM, which models total power (static + dynamic), so better
> reflects real HW.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/opp/of.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 2f40afa4e65c..94059408fa39 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1395,6 +1395,40 @@ 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 based on DT 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)

nit: the device is usually the first parameter. It's also the only true input
parameter of this function, most code puts input parameters first.

> +{
> +	struct dev_pm_opp *opp;
> +	unsigned long opp_freq;
> +	u32 opp_power;
> +	int ret;
> +
> +	/* 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;
> +
> +	ret = of_property_read_u32(opp->np, "opp-microwatt", &opp_power);
> +	dev_pm_opp_put(opp);
> +	if (ret)
> +		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
> @@ -1445,6 +1479,33 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz,
>  	return 0;
>  }
>  
> +static int _of_find_opp_microwatt_property(struct device *dev)

this function doesn't retrurn the property like of_find_property() does,
_of_has_opp_microwatt_property() would be a be a better name IMO. I'd
also suggest to change the return type to bool, since callers don't
really care about the specific error (which with the current code is
-EINVAL) in all cases.


> +{
> +	unsigned long freq = 0;

Does the compiler complain when the initialization is skipped? The
value of the variable is never read, only it's address is passed to
dev_pm_opp_find_freq_ceil().

> +	struct dev_pm_opp *opp;
> +	struct device_node *np;
> +	struct property *prop;
> +
> +	/* We only support "operating-points-v2" */
> +	np = dev_pm_opp_of_get_opp_desc_node(dev);
> +	if (!np)
> +		return -EINVAL;
> +
> +	of_node_put(np);
> +
> +	/* Check if an OPP has needed property */

The comment doesn't add much value IMO

> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +	if (IS_ERR(opp))
> +		return -EINVAL;
> +
> +	prop = of_find_property(opp->np, "opp-microwatt", NULL);
> +	dev_pm_opp_put(opp);
> +	if (!prop)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /**
>   * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
>   * @dev		: Device for which an Energy Model has to be registered
> @@ -1474,6 +1535,15 @@ 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_find_opp_microwatt_property(dev)) {
> +		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_dt_power);
> +
> +		ret = em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
> +						  cpus, true);
> +		return ret;

just 'return em_dev_register_perf_domain(...);'?

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

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Lukasz Luba]

Hi Matthias,

On 2/22/22 14:58, Matthias Kaehlcke wrote:
> On Tue, Feb 22, 2022 at 02:07:46PM +0000, Lukasz Luba wrote:

[snip]

>> +static int __maybe_unused
>> +_get_dt_power(unsigned long *mW, unsigned long *kHz, struct device *dev)
> 
> nit: the device is usually the first parameter. It's also the only true input
> parameter of this function, most code puts input parameters first.

Good point. I have internal patch set under review changing this. It's
going to be changed and the 'dev' would be the 1st arg. I'll send this
patch set as soon as this one gets queued into pm tree.

> 
>> +{
>> +	struct dev_pm_opp *opp;
>> +	unsigned long opp_freq;
>> +	u32 opp_power;
>> +	int ret;
>> +
>> +	/* 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;
>> +
>> +	ret = of_property_read_u32(opp->np, "opp-microwatt", &opp_power);
>> +	dev_pm_opp_put(opp);
>> +	if (ret)
>> +		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
>> @@ -1445,6 +1479,33 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz,
>>   	return 0;
>>   }
>>   
>> +static int _of_find_opp_microwatt_property(struct device *dev)
> 
> this function doesn't retrurn the property like of_find_property() does,
> _of_has_opp_microwatt_property() would be a be a better name IMO. I'd
> also suggest to change the return type to bool, since callers don't
> really care about the specific error (which with the current code is
> -EINVAL) in all cases.

Agree, I'll change the name and return type.

> 
> 
>> +{
>> +	unsigned long freq = 0;
> 
> Does the compiler complain when the initialization is skipped? The
> value of the variable is never read, only it's address is passed to
> dev_pm_opp_find_freq_ceil().

It has to be 0, since under the hood the dev_pm_opp_find_freq_ceil()
is going to find first freq which is equal or bigger than this one.
We actually use that ptr value in the _find_freq_ceil().

> 
>> +	struct dev_pm_opp *opp;
>> +	struct device_node *np;
>> +	struct property *prop;
>> +
>> +	/* We only support "operating-points-v2" */
>> +	np = dev_pm_opp_of_get_opp_desc_node(dev);
>> +	if (!np)
>> +		return -EINVAL;
>> +
>> +	of_node_put(np);
>> +
>> +	/* Check if an OPP has needed property */
> 
> The comment doesn't add much value IMO

Well, it just stress the 'an' as in this case it's the 1st OPP,
due to the fact freq = 0 and finding the 'ceiling' on it.
I'll remove it.

> 
>> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> +	if (IS_ERR(opp))
>> +		return -EINVAL;
>> +
>> +	prop = of_find_property(opp->np, "opp-microwatt", NULL);
>> +	dev_pm_opp_put(opp);
>> +	if (!prop)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
>>    * @dev		: Device for which an Energy Model has to be registered
>> @@ -1474,6 +1535,15 @@ 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_find_opp_microwatt_property(dev)) {
>> +		struct em_data_callback em_dt_cb = EM_DATA_CB(_get_dt_power);
>> +
>> +		ret = em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb,
>> +						  cpus, true);
>> +		return ret;
> 
> just 'return em_dev_register_perf_domain(...);'?

true

Thanks for the review! I'll address these comments in v3 if Viresh
agrees with this new approach.

Regards,
Lukasz

Re: [[PATCH v2 1/2] dt-bindings: opp: Add 'opp-microwatt' entry in the OPP

[Viresh Kumar]

On 22-02-22, 14:07, 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>
> ---
>  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> index 15a76bcd6d42..3f07a279ed2a 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> @@ -93,6 +93,13 @@ patternProperties:
>          minItems: 1
>          maxItems: 8   # Should be enough regulators
>  
> +      opp-microwatt:
> +        description:
> +          Power for the OPP
> +
> +          A value representing power for the OPP in micro-Watts.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +

I was expecting a much larger change here. Look at how opp-microvolt and
opp-microamp is defined in this file.

Should this value be made per-supply/regulator, just like voltage/current ?

>        opp-level:
>          description:
>            A value representing the performance level of the device.

-- 
viresh

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Viresh Kumar]

On 22-02-22, 14:07, Lukasz Luba wrote:
> +static int _of_find_opp_microwatt_property(struct device *dev)
> +{
> +	unsigned long freq = 0;
> +	struct dev_pm_opp *opp;
> +	struct device_node *np;
> +	struct property *prop;
> +
> +	/* We only support "operating-points-v2" */
> +	np = dev_pm_opp_of_get_opp_desc_node(dev);
> +	if (!np)
> +		return -EINVAL;
> +
> +	of_node_put(np);
> +
> +	/* Check if an OPP has needed property */
> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +	if (IS_ERR(opp))
> +		return -EINVAL;
> +
> +	prop = of_find_property(opp->np, "opp-microwatt", NULL);
> +	dev_pm_opp_put(opp);
> +	if (!prop)
> +		return -EINVAL;
> +
> +	return 0;
> +}

Please follow everything just like opp-microvolt is defined. Create a new field
in the struct dev_pm_opp, initialize it only once when the OPP is created, that
field should be used here instead of parsing the DT here again. There also needs
to be a debug file in debugfs for this new field.

Search for "supply" and "microvolt" in the OPP core, you will see all the places
that need it.

-- 
viresh

Re: [[PATCH v2 1/2] dt-bindings: opp: Add 'opp-microwatt' entry in the OPP

[Lukasz Luba]

On 2/23/22 05:50, Viresh Kumar wrote:
> On 22-02-22, 14:07, 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>
>> ---
>>   Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
>> index 15a76bcd6d42..3f07a279ed2a 100644
>> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
>> @@ -93,6 +93,13 @@ patternProperties:
>>           minItems: 1
>>           maxItems: 8   # Should be enough regulators
>>   
>> +      opp-microwatt:
>> +        description:
>> +          Power for the OPP
>> +
>> +          A value representing power for the OPP in micro-Watts.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
> 
> I was expecting a much larger change here. Look at how opp-microvolt and
> opp-microamp is defined in this file.
> 
> Should this value be made per-supply/regulator, just like voltage/current ?
> 

For the EM we need only one value. If there would be some other users
of this field in future we might add the multiple power values support.
Currently there is no need I would say, unless it's a hard requirement
to be aligned with opp-microvolt.

Re: [[PATCH v2 1/2] dt-bindings: opp: Add 'opp-microwatt' entry in the OPP

[Viresh Kumar]

On 23-02-22, 08:39, Lukasz Luba wrote:
> For the EM we need only one value. If there would be some other users
> of this field in future we might add the multiple power values support.
> Currently there is no need I would say, unless it's a hard requirement
> to be aligned with opp-microvolt.

This isn't really up to what the software wants to do (to some level yes it is).
The DT describes the hardware and should do so in an unambiguous way and it
shouldn't be required to update the bindings again and again. It should be done
just once, and in the right way possible.

If the power is actually per-regulator, then it should be present in that form.
The EM can just sum the up later on.

-- 
viresh

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Lukasz Luba]

On 2/23/22 05:53, Viresh Kumar wrote:
> On 22-02-22, 14:07, Lukasz Luba wrote:
>> +static int _of_find_opp_microwatt_property(struct device *dev)
>> +{
>> +	unsigned long freq = 0;
>> +	struct dev_pm_opp *opp;
>> +	struct device_node *np;
>> +	struct property *prop;
>> +
>> +	/* We only support "operating-points-v2" */
>> +	np = dev_pm_opp_of_get_opp_desc_node(dev);
>> +	if (!np)
>> +		return -EINVAL;
>> +
>> +	of_node_put(np);
>> +
>> +	/* Check if an OPP has needed property */
>> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> +	if (IS_ERR(opp))
>> +		return -EINVAL;
>> +
>> +	prop = of_find_property(opp->np, "opp-microwatt", NULL);
>> +	dev_pm_opp_put(opp);
>> +	if (!prop)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
> 
> Please follow everything just like opp-microvolt is defined. Create a new field
> in the struct dev_pm_opp, initialize it only once when the OPP is created, that
> field should be used here instead of parsing the DT here again. There also needs
> to be a debug file in debugfs for this new field.
> 
> Search for "supply" and "microvolt" in the OPP core, you will see all the places
> that need it.
> 

OK, so you want to have this available for the whole system. I can do
that. I would go for one value of power and try to fit into the
opp_parse_supplies() code. As far as I can see in the
dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt
is used. I would go for similar solution for u_watt.
There is even a single u_amp and no _max, _min variants, so should be
good..

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Viresh Kumar]

On 23-02-22, 08:59, Lukasz Luba wrote:
> OK, so you want to have this available for the whole system. I can do
> that. I would go for one value of power 

One value per supply, right ?

> and try to fit into the
> opp_parse_supplies() code.

Correct.

> As far as I can see in the
> dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt
> is used. I would go for similar solution for u_watt.
> There is even a single u_amp and no _max, _min variants, so should be
> good..

Yes, I don't think we need min/max/target kind of naming here. Just a single
value per supply is enough.

-- 
viresh

Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM registration

[Lukasz Luba]

On 2/23/22 09:10, Viresh Kumar wrote:
> On 23-02-22, 08:59, Lukasz Luba wrote:
>> OK, so you want to have this available for the whole system. I can do
>> that. I would go for one value of power
> 
> One value per supply, right ?

yes, only the u_watt, no _min, _max variants.

> 
>> and try to fit into the
>> opp_parse_supplies() code.
> 
> Correct.
> 
>> As far as I can see in the
>> dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt
>> is used. I would go for similar solution for u_watt.
>> There is even a single u_amp and no _max, _min variants, so should be
>> good..
> 
> Yes, I don't think we need min/max/target kind of naming here. Just a single
> value per supply is enough.
> 

Good, thanks for comments. Let me craft the v3 then.

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Daniel Lezcano]

Hi Lukasz,

why not extend the energy model to any kind of devices?

The changes are shyly proposing a new entry in the OPP table like that 
is the only place where power management can happen.

Is the approach to describe by small pieces here and there, specific 
attributes and let the kernel create an energy model from that soap?

I prefer the RFC approach where the energy model is described clearly 
but, IMHO, it should be more abstracted, without reference to frequency 
or whatever but index <-> power (t-uple or equation)

By this way, it could be possible to describe the battery with the 
different charges, the LCD bright light, etc ...


On 22/02/2022 15:07, 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:
> It adds a new callback in OPP framework to parse the OPP node entry and
> read the "opp-microwatt". It's going to only work with OPP-v2, but it's
> agreed to be OK.
> 
> Comments, suggestions are very welcome.
> 
> changelog:
> v2:
> - implemented Viresh idea to add "opp-microwatt" into the OPP node entry in DT
> v1 [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/20220221225131.15836-1-lukasz.luba@arm.com/
> 
> Lukasz Luba (2):
>    dt-bindings: opp: Add 'opp-microwatt' entry in the OPP
>    OPP: Add 'opp-microwatt' parsing for advanced EM registration
> 
>   .../devicetree/bindings/opp/opp-v2-base.yaml  |  7 ++
>   drivers/opp/of.c                              | 70 +++++++++++++++++++
>   2 files changed, 77 insertions(+)
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Lukasz Luba]

Hi Daniel,

On 2/23/22 09:52, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> why not extend the energy model to any kind of devices?
> 
> The changes are shyly proposing a new entry in the OPP table like that 
> is the only place where power management can happen.

It was Viresh proposal to make it in the OPP v2 table. I've checked
the code and this u_watt fits perfectly there. New power value looks
natural there. We also have the interconnect info in the opp, so even
this kind of extensions are there.
It is a clean solution which meats the GPU requirements.

> 
> Is the approach to describe by small pieces here and there, specific 
> attributes and let the kernel create an energy model from that soap?
> 
> I prefer the RFC approach where the energy model is described clearly 
> but, IMHO, it should be more abstracted, without reference to frequency 
> or whatever but index <-> power (t-uple or equation)
> 
> By this way, it could be possible to describe the battery with the 
> different charges, the LCD bright light, etc ...

I can see your need, but I would focus on existing issues and devices.
This change was motivated by existing mainline platform which wants
to have EM in GPU (Chromebook) from DT. The GPU has OPP table, thus it
meets this requirement. The requirements are clear for the GPU (and
similar like DSP/ISP/etc which all have OPP table).

This is a clean, small step forward with the OPP approach and it
doesn't block your future needs and requirements. IMO it's orthogonal,
devices which have OPP table naturally might provide power there.
Devices which wouldn't have OPP table, but wanted to register EM
via DT - it's a different story (not the existing Chromebook's GPU).

This future story can be addressed in some next step. We need real
devices and examples to figure out the requirements and craft something.

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Viresh Kumar]

On 23-02-22, 10:52, Daniel Lezcano wrote:
> why not extend the energy model to any kind of devices?

FWIW, the OPP core supports a wide range of devices now, not just CPUs.

-- 
viresh

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Lukasz Luba]

On 2/23/22 10:43, Viresh Kumar wrote:
> On 23-02-22, 10:52, Daniel Lezcano wrote:
>> why not extend the energy model to any kind of devices?
> 
> FWIW, the OPP core supports a wide range of devices now, not just CPUs.
> 

Is that the "opp-level" thing which would allow that?
I can see some DT files with regulators(?) using it e.g. [1].
It looks flexible, the opp-hz is not hard requirement,
the opp-level can be used instead IIUC.

It might be a next step which might meet Daniel's needs.
If that 'level' can be any number and frequency is not available
then EM must have 'level' filed in the struct em_perf_state
for this kind of new devices. I'm open for such change.
We can discuss this as a next step. We would need to find some examples
how this new thing would be used.

[1] 
https://elixir.bootlin.com/linux/v5.17-rc5/source/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi#L4

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Viresh Kumar]

On 23-02-22, 11:22, Lukasz Luba wrote:
> On 2/23/22 10:43, Viresh Kumar wrote:
> > On 23-02-22, 10:52, Daniel Lezcano wrote:
> > > why not extend the energy model to any kind of devices?
> > 
> > FWIW, the OPP core supports a wide range of devices now, not just CPUs.

There are many other devices which still use Freq.

> Is that the "opp-level" thing which would allow that?

For power supplies/regulators, we don't have freq and they use level, right.

Also for interconnect we use bandwidth, in a similar way.

> I can see some DT files with regulators(?) using it e.g. [1].
> It looks flexible, the opp-hz is not hard requirement,
> the opp-level can be used instead IIUC.

Right.

> It might be a next step which might meet Daniel's needs.
> If that 'level' can be any number and frequency is not available
> then EM must have 'level' filed in the struct em_perf_state
> for this kind of new devices. I'm open for such change.
> We can discuss this as a next step. We would need to find some examples
> how this new thing would be used.
> 
> [1] https://elixir.bootlin.com/linux/v5.17-rc5/source/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi#L4

-- 
viresh

Re: [PATCH v2 0/2] Introduce 'advanced' Energy Model in DT

[Lukasz Luba]

On 2/23/22 11:27, Viresh Kumar wrote:
> On 23-02-22, 11:22, Lukasz Luba wrote:
>> On 2/23/22 10:43, Viresh Kumar wrote:
>>> On 23-02-22, 10:52, Daniel Lezcano wrote:
>>>> why not extend the energy model to any kind of devices?
>>>
>>> FWIW, the OPP core supports a wide range of devices now, not just CPUs.
> 
> There are many other devices which still use Freq.
> 
>> Is that the "opp-level" thing which would allow that?
> 
> For power supplies/regulators, we don't have freq and they use level, right.
> 
> Also for interconnect we use bandwidth, in a similar way.
> 
>> I can see some DT files with regulators(?) using it e.g. [1].
>> It looks flexible, the opp-hz is not hard requirement,
>> the opp-level can be used instead IIUC.
> 
> Right.
> 

Looks good. It also doesn't collide with this patch set.

We could have an opp entry like:

	opp_1: opp-1 {
		opp-level = <1>;
		opp-microwatt = <200000>;
	};

Daniel would that design make sense to you?


If yes, we could discuss this further after this first
step for fixing GPU in merged. I would need to re-think
the EM em_perf_state and maybe the new ::level there.