Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   James Calligeros <jcalligeros99@gmail.com>
To:     viresh.kumar@linaro.org
Cc:     asahi@lists.linux.dev, jcalligeros99@gmail.com,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, nm@ti.com,
        sboyd@kernel.org, vireshk@kernel.org
Subject: Re: [PATCH v2] OPP: decouple dt properties in opp_parse_supplies()
Date:   Wed,  2 Nov 2022 20:20:13 +1000
Message-Id: <20221102102011.11623-1-jcalligeros99@gmail.com>
In-Reply-To: <20221102090228.odyt7ykdkcbdx4lw@vireshk-i7>
References: <20221102090228.odyt7ykdkcbdx4lw@vireshk-i7>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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