[v2] OPP: decouple dt properties in opp_parse_supplies()

  The opp-microwatt property was added with the intention of providing
platforms a way to specify a precise value for the power consumption
of a device at a given OPP to enable better energy-aware scheduling
decisions by informing the kernel of the total static and dynamic
power of a device at a given OPP, removing the reliance on the EM
subsystem's often flawed estimations. This property is parsed by
opp_parse_supplies(), which creates a hard dependency on the
opp-microvolt property.

Some platforms, such as Apple Silicon, do not describe their devices'
voltage regulators in the DT as they cannot be controlled by the kernel
and/or rely on opaque firmware algorithms to control their voltage and
current characteristics at runtime. We can, however, experimentally
determine the power consumption of a given device at a given OPP, taking
advantage of opp-microwatt to provide EAS on such devices as was initially
intended.

Allow platforms to specify and consume any subset of opp-microvolt,
opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt
to enable this functionality on such platforms.

Fixes: 4f9a7a1dc2a2 ("OPP: Add "opp-microwatt" supporting code")
Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
---
Changes since v1:
Fixed bad reference (opp to opp_table)

 drivers/opp/of.c | 198 +++++++++++++++++++++++++----------------------
 1 file changed, 104 insertions(+), 94 deletions(-)
  

On 30-10-22, 20:15, James Calligeros wrote:
> The opp-microwatt property was added with the intention of providing
> platforms a way to specify a precise value for the power consumption
> of a device at a given OPP to enable better energy-aware scheduling
> decisions by informing the kernel of the total static and dynamic
> power of a device at a given OPP, removing the reliance on the EM
> subsystem's often flawed estimations. This property is parsed by
> opp_parse_supplies(), which creates a hard dependency on the
> opp-microvolt property.
> 
> Some platforms, such as Apple Silicon, do not describe their devices'
> voltage regulators in the DT as they cannot be controlled by the kernel
> and/or rely on opaque firmware algorithms to control their voltage and
> current characteristics at runtime. We can, however, experimentally
> determine the power consumption of a given device at a given OPP, taking
> advantage of opp-microwatt to provide EAS on such devices as was initially
> intended.

Do you supply a regulator to the OPP core for your platform ?

> Allow platforms to specify and consume any subset of opp-microvolt,
> opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt
> to enable this functionality on such platforms.
> 
> Fixes: 4f9a7a1dc2a2 ("OPP: Add "opp-microwatt" supporting code")

I won't call it a fix, we are trying to use this information in a
different way here, that's all.

> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> Changes since v1:
> Fixed bad reference (opp to opp_table)
> 
>  drivers/opp/of.c | 198 +++++++++++++++++++++++++----------------------
>  1 file changed, 104 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 605d68673f92..0fa25c3a959e 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -581,166 +581,176 @@ 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, *microwatt = NULL;
> +	u32 *microvolt = NULL, *microamp = NULL, *microwatt = NULL;
>  	int supplies = opp_table->regulator_count;
>  	int vcount, icount, pcount, ret, i, j;
> -	struct property *prop = NULL;
> +	struct property *prop_mv = NULL, *prop_ma = NULL, *prop_mw = NULL;
>  	char name[NAME_MAX];
>  
>  	/* Search for "opp-microvolt-<name>" */
>  	if (opp_table->prop_name) {
>  		snprintf(name, sizeof(name), "opp-microvolt-%s",
>  			 opp_table->prop_name);
> -		prop = of_find_property(opp->np, name, NULL);
> +		prop_mv = of_find_property(opp->np, name, NULL);
>  	}
>  
> -	if (!prop) {
> +	if (!prop_mv) {
>  		/* Search for "opp-microvolt" */
>  		sprintf(name, "opp-microvolt");
> -		prop = of_find_property(opp->np, name, NULL);
> -
> -		/* Missing property isn't a problem, but an invalid entry is */
> -		if (!prop) {
> -			if (unlikely(supplies == -1)) {
> -				/* Initialize regulator_count */
> -				opp_table->regulator_count = 0;
> -				return 0;
> -			}
> +		prop_mv = of_find_property(opp->np, name, NULL);
>  
> -			if (!supplies)
> -				return 0;
> -
> -			dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
> -				__func__);

Catching such errors are important and so the opp-microvolt property
was made compulsory earlier.

If there is a regulator, then we must have microvolt property.
amps/watts are optional.

> -			return -EINVAL;
> -		}
>  	}
>  
> -	if (unlikely(supplies == -1)) {
> -		/* Initialize regulator_count */
> -		supplies = opp_table->regulator_count = 1;
> -	} else if (unlikely(!supplies)) {
> -		dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__);
> -		return -EINVAL;
> +	if (prop_mv) {
> +		vcount = of_property_count_u32_elems(opp->np, name);
> +		if (unlikely(supplies == -1))
> +			supplies = opp_table->regulator_count = vcount;

This is wrong. There can be one or three entries per regulator here.
Target or min/max/target. If the supplies value is -1, we can only
support one regulator, i.e. one or three entries total.

I didn't look at rest of the patch yet. Lets discuss this a bit first.
  

>> The opp-microwatt property was added with the intention of providing
>> platforms a way to specify a precise value for the power consumption
>> of a device at a given OPP to enable better energy-aware scheduling
>> decisions by informing the kernel of the total static and dynamic
>> power of a device at a given OPP, removing the reliance on the EM
>> subsystem's often flawed estimations. This property is parsed by
>> opp_parse_supplies(), which creates a hard dependency on the
>> opp-microvolt property.
>> 
>> Some platforms, such as Apple Silicon, do not describe their devices'
>> voltage regulators in the DT as they cannot be controlled by the kernel
>> and/or rely on opaque firmware algorithms to control their voltage and
>> current characteristics at runtime. We can, however, experimentally
>> determine the power consumption of a given device at a given OPP, taking
>> advantage of opp-microwatt to provide EAS on such devices as was initially
>> intended.

>Do you supply a regulator to the OPP core for your platform ?

On Apple SoCs, all the regulators are controlled transparently
by the hardware/firmware. The CPUfreq driver requests a pstate as
described in the OPP table by setting some bits in some registers, and
the platform handles everything else.

Not only is there no use for the voltage and current information from
the kernel's perspective since there's nothing to control, we don't
even really have a way to reliably model their behaviour.

What we can do, however, is use energy counter registers in the core
clusters to determine energy consumption at a given pstate and register
that with the OPP core using the opp-microwatt property. Given that its
stated purpose is to enable such behaviour, requiring it to be coupled
to opp-microvolt is a major design deficiency.

>> Fixes: 4f9a7a1dc2a2 ("OPP: Add "opp-microwatt" supporting code")

>I won't call it a fix, we are trying to use this information in a
>different way here, that's all.

My interpretation of the commit message for 32bf8bc9a077 is that this
is the way in which the property was intended to be used, and that it not
working like this is a bug. Quoting the commit message, "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." 

There is probably a bigger discussion to be had on whether or not parsing
opp-microwatt for the purpose of EM registration should be entangled in
opp_parse_supplies() at all, but that's by the by.

>If there is a regulator, then we must have microvolt property.
>amps/watts are optional.

I noticed no adverse effects from not having opp-microvolt in my testing of
this patch, possibly because the data is not used to actually puppeteer any
supplies. This goes back to the greater discussion, though. If we're going
to use opp_parse_supplies() to do EM registration stuff then having the
check for a valid representation of an actual VRM should probably happen
elsewhere, where a valid VRM is actually a hard requirement of the code being
run.

>> -			return -EINVAL;
>> -		}
>>  	}
>>  
>> -	if (unlikely(supplies == -1)) {
>> -		/* Initialize regulator_count */
>> -		supplies = opp_table->regulator_count = 1;
>> -	} else if (unlikely(!supplies)) {
>> -		dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__);
>> -		return -EINVAL;
>> +	if (prop_mv) {
>> +		vcount = of_property_count_u32_elems(opp->np, name);
>> +		if (unlikely(supplies == -1))
>> +			supplies = opp_table->regulator_count = vcount;

>This is wrong. There can be one or three entries per regulator here.
>Target or min/max/target. If the supplies value is -1, we can only
>support one regulator, i.e. one or three entries total.

Ack.

- James
  

On 02-11-22, 20:20, James Calligeros wrote:
> On Apple SoCs, all the regulators are controlled transparently
> by the hardware/firmware. The CPUfreq driver requests a pstate as
> described in the OPP table by setting some bits in some registers, and
> the platform handles everything else.
> 
> Not only is there no use for the voltage and current information from
> the kernel's perspective since there's nothing to control, we don't
> even really have a way to reliably model their behaviour.
> 
> What we can do, however, is use energy counter registers in the core
> clusters to determine energy consumption at a given pstate and register
> that with the OPP core using the opp-microwatt property. Given that its
> stated purpose is to enable such behaviour, requiring it to be coupled
> to opp-microvolt is a major design deficiency.

I wasn't asking this. I was rather asking if some code for your
platform calls dev_pm_opp_set_regulators(). I assume No based on what
you provided.

> >I won't call it a fix, we are trying to use this information in a
> >different way here, that's all.
> 
> My interpretation of the commit message for 32bf8bc9a077 is that this
> is the way in which the property was intended to be used, and that it not
> working like this is a bug. Quoting the commit message, "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." 
> 
> There is probably a bigger discussion to be had on whether or not parsing
> opp-microwatt for the purpose of EM registration should be entangled in
> opp_parse_supplies() at all, but that's by the by.

I get it. I will still skip the Fixes tag for now as that may make
people backport this to older kernels, which we may not want as there
are no broken users currently in mainline.

> I noticed no adverse effects from not having opp-microvolt in my testing of
> this patch, possibly because the data is not used to actually puppeteer any
> supplies. This goes back to the greater discussion, though. If we're going
> to use opp_parse_supplies() to do EM registration stuff then having the
> check for a valid representation of an actual VRM should probably happen
> elsewhere, where a valid VRM is actually a hard requirement of the code being
> run.

I figured out that you had to make a lot of unrelated changes for this
simple change, just because of the layout of the routine.

I have included your patch in my series now, cc'd you. Please give
that a try and see if it works or not. Thanks.

I have kept your Authorship and Signed-off, hope that is fine.