From: Quentin Perret <quentin.perret@arm.com>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: viresh.kumar@linaro.org, sudeep.holla@arm.com, rjw@rjwysocki.net,
nm@ti.com, sboyd@kernel.org, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dietmar.eggemann@arm.com
Subject: Re: [PATCH v2 1/5] PM / OPP: Introduce a power estimation helper
Date: Thu, 31 Jan 2019 09:42:15 +0000 [thread overview]
Message-ID: <20190131094212.rjel77xcfvqj6tkr@queper01-lin> (raw)
In-Reply-To: <20190130190703.GM81583@google.com>
Hi Matthias,
On Wednesday 30 Jan 2019 at 11:07:03 (-0800), Matthias Kaehlcke wrote:
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index 06f0f632ec47..4c8bf172e9ed 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -20,6 +20,7 @@
> > #include <linux/pm_domain.h>
> > #include <linux/slab.h>
> > #include <linux/export.h>
> > +#include <linux/energy_model.h>
>
> nit: AFAIK typically alphabetical order is used for includes, though
> this file doesn't exactly adhere to it.
Yeah that's what I was thinking too. Since nothing is in order here I
figured there wasn't a best place to put it so I just stick it there. I
happy to re-order all of them if necessary.
>
> > #include "opp.h"
> >
> > @@ -1047,3 +1048,90 @@ struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
> > return of_node_get(opp->np);
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_opp_get_of_node);
> > +
> > +/*
> > + * Callback function provided to the Energy Model framework upon registration.
> > + * This computes the power estimated by @CPU at the first OPP above @kHz (ceil),
>
> that's not entirely correct, it could be the OPP at @kHz.
Right, I'll update that.
>
> > + * and updates @kHz and @mW accordingly. The power is estimated as
> > + * P = C * V^2 * f with C being the CPU's capacitance and V and f respectively
> > + * the voltage and frequency of the OPP.
> > + *
> > + * Returns -ENODEV if the CPU device cannot be found, -EINVAL if the power
> > + * calculation failed because of missing parameters, 0 otherwise.
> > + */
> > +static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
> > + int cpu)
>
> why __maybe_unused?
To avoid compiler warnings with CONFIG_ENERGY_MODEL=n, see my other
email ;-)
> > +{
> > + struct device *cpu_dev;
> > + struct dev_pm_opp *opp;
> > + struct device_node *np;
> > + unsigned long mV, Hz;
> > + u32 cap;
> > + u64 tmp;
> > + int ret;
> > +
> > + cpu_dev = get_cpu_device(cpu);
> > + if (!cpu_dev)
> > + return -ENODEV;
> > +
> > + np = of_node_get(cpu_dev->of_node);
> > + if (!np)
> > + return -EINVAL;
> > +
> > + ret = of_property_read_u32(np, "dynamic-power-coefficient", &cap);
> > + of_node_put(np);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + Hz = *kHz * 1000;
> > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &Hz);
> > + if (IS_ERR(opp))
> > + return -EINVAL;
> > +
> > + mV = dev_pm_opp_get_voltage(opp) / 1000;
> > + dev_pm_opp_put(opp);
> > + if (!mV)
> > + return -EINVAL;
> > +
> > + tmp = (u64)cap * mV * mV * (Hz / 1000000);
> > + do_div(tmp, 1000000000);
> > +
> > + *mW = (unsigned long)tmp;
> > + *kHz = Hz / 1000;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * dev_pm_opp_of_register_em() - Attempt to register an Energy Model
> > + * @cpus : CPUs for which an Energy Model has to be registered
> > + * @nr_opp : Number of OPPs to register in the Energy Model
> > + *
> > + * This checks whether the "dynamic-power-coefficient" devicetree binding has
>
> s/binding/property/ ?
Sounds good.
>
> > + * been specified, and tries to register an Energy Model with it if it has.
> > + */
> > +void dev_pm_opp_of_register_em(struct cpumask *cpus, int nr_opp)
>
> Is the nr_opp parameter really needed? The function looks up the CPU
> device and hence could determine the OPP count itself with
> dev_pm_opp_get_opp_count(). I see most cpufreq drivers call
> dev_pm_opp_get_opp_count() anyway, so passing the count as parameter
> can be considered a small optimization, not sure how relevant it is
> though, since dev_pm_opp_of_register_em() isn't called frequently.
Yeah, I figured since most callers of dev_pm_opp_of_register_em()
already counted the OPPs, I could as well use the data instead of
counting again. I mean, dev_pm_opp_get_count() has to traverse the whole
list every time, so there is no point in doing that twice. Not a huge
deal I guess.
>
> > +{
> > + struct em_data_callback em_cb = EM_DATA_CB(_get_cpu_power);
> > + int ret, cpu = cpumask_first(cpus);
> > + struct device *cpu_dev;
> > + struct device_node *np;
> > + u32 cap;
> > +
> > + cpu_dev = get_cpu_device(cpu);
> > + if (!cpu_dev)
> > + return;
> > +
> > + np = of_node_get(cpu_dev->of_node);
> > + if (!np)
> > + return;
> > +
> > + /* Don't register an EM without the right DT binding */
> > + ret = of_property_read_u32(np, "dynamic-power-coefficient", &cap);
> > + of_node_put(np);
> > + if (ret || !cap)
> > + return;
> > +
> > + em_register_perf_domain(cpus, nr_opp, &em_cb);
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > index b895f4e79868..58ae08b024bd 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -327,6 +327,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpuma
> > struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
> > struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
> > int of_get_required_opp_performance_state(struct device_node *np, int index);
> > +void dev_pm_opp_of_register_em(struct cpumask *cpus, int nr_opp);
> > #else
> > static inline int dev_pm_opp_of_add_table(struct device *dev)
> > {
> > @@ -365,6 +366,11 @@ static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
> > {
> > return NULL;
> > }
> > +
> > +static inline void dev_pm_opp_of_register_em(struct cpumask *cpus, int nr_opp)
> > +{
> > +}
> > +
> > static inline int of_get_required_opp_performance_state(struct device_node *np, int index)
> > {
> > return -ENOTSUPP;
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
Thank you very much !
Quentin
next prev parent reply other threads:[~2019-01-31 9:42 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-30 17:05 [PATCH v2 0/5] Register an Energy Model for Arm reference platforms Quentin Perret
2019-01-30 17:05 ` [PATCH v2 1/5] PM / OPP: Introduce a power estimation helper Quentin Perret
2019-01-30 19:07 ` Matthias Kaehlcke
2019-01-31 7:22 ` Viresh Kumar
2019-01-31 9:34 ` Quentin Perret
2019-01-31 9:37 ` Viresh Kumar
2019-01-31 9:42 ` Quentin Perret [this message]
2019-01-31 7:26 ` Viresh Kumar
2019-01-31 9:51 ` Quentin Perret
2019-01-30 17:05 ` [PATCH v2 2/5] cpufreq: dt: Register an Energy Model Quentin Perret
2019-01-30 17:05 ` [PATCH v2 3/5] cpufreq: scpi: " Quentin Perret
2019-01-30 17:05 ` [PATCH v2 4/5] cpufreq: arm_big_little: " Quentin Perret
2019-01-30 17:05 ` [PATCH v2 5/5] cpufreq: scmi: " Quentin Perret
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190131094212.rjel77xcfvqj6tkr@queper01-lin \
--to=quentin.perret@arm.com \
--cc=dietmar.eggemann@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mka@chromium.org \
--cc=nm@ti.com \
--cc=rjw@rjwysocki.net \
--cc=sboyd@kernel.org \
--cc=sudeep.holla@arm.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).