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;
Date:   Wed, 2 Nov 2022 14:32:28 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     James Calligeros <jcalligeros99@gmail.com>
Cc:     vireshk@kernel.org, sboyd@kernel.org, nm@ti.com,
        linux-pm@vger.kernel.org, asahi@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] OPP: decouple dt properties in opp_parse_supplies()
Message-ID: <20221102090228.odyt7ykdkcbdx4lw@vireshk-i7>
References: <20221030101546.29306-1-jcalligeros99@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221030101546.29306-1-jcalligeros99@gmail.com>
Precedence: bulk

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.

-- 
viresh