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:   Tue, 22 Feb 2022 06:58:24 -0800
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     linux-kernel@vger.kernel.org, dietmar.eggemann@arm.com,
        viresh.kumar@linaro.org, rafael@kernel.org,
        daniel.lezcano@linaro.org, nm@ti.com, sboyd@kernel.org,
        dianders@chromium.org, robh+dt@kernel.org,
        devicetree@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [[PATCH v2 2/2] OPP: Add 'opp-microwatt' parsing for advanced EM
 registration
Message-ID: <YhT6EBzSE/7S3QqT@google.com>
References: <20220222140746.12293-1-lukasz.luba@arm.com>
 <20220222140746.12293-3-lukasz.luba@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220222140746.12293-3-lukasz.luba@arm.com>
Precedence: bulk

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
>