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