| Message ID | 20220224081131.27282-4-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | Introduce "opp-microwatt" and 'advanced' Energy Model from DT | expand |
On 24-02-22, 08:11, Lukasz Luba wrote: > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > > +/* > + * Callback function provided to the Energy Model framework upon registration. > + * It provides the power used 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_opp_power(unsigned long *mW, unsigned long *kHz, struct device *dev) Lets call it _get_dt_opp_power() or _get_dt_power() ? > +{ > + struct dev_pm_opp *opp; > + unsigned long opp_freq, opp_power; > + > + /* 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; > + > + opp_power = dev_pm_opp_get_power(opp); As I said in 2/4, this should really give the total instead. > + dev_pm_opp_put(opp); > + if (!opp_power) > + 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 > @@ -1488,6 +1520,24 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz, > return 0; > } > > +static bool _of_has_opp_microwatt_property(struct device *dev) > +{ > + unsigned long power, freq = 0; > + struct dev_pm_opp *opp; > + > + /* Check if at least one OPP has needed property */ > + opp = dev_pm_opp_find_freq_ceil(dev, &freq); > + if (IS_ERR(opp)) > + return false; > + > + power = dev_pm_opp_get_power(opp); > + dev_pm_opp_put(opp); > + if (!power) What if this particular frequency has 0 power mentioned for some reason :) Instead of this heavy stuff, just pick the first OPP from the opp table and see its power-value. > + return false; > + > + return true; > +} > + > /** > * dev_pm_opp_of_register_em() - Attempt to register an Energy Model > * @dev : Device for which an Energy Model has to be registered > @@ -1517,6 +1567,14 @@ 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_has_opp_microwatt_property(dev)) { > + struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power); > + > + return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb, > + cpus, true); > + } > + We have another em_dev_register_perf_domain() down the line, lets keep only one such call and get it a callback that should be set in an if/else loop. > np = of_node_get(dev->of_node); > if (!np) { > ret = -EINVAL; > -- > 2.17.1
On 2/24/22 09:13, Viresh Kumar wrote: > On 24-02-22, 08:11, Lukasz Luba wrote: >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c >> >> +/* >> + * Callback function provided to the Energy Model framework upon registration. >> + * It provides the power used 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_opp_power(unsigned long *mW, unsigned long *kHz, struct device *dev) > > Lets call it _get_dt_opp_power() or _get_dt_power() ? OK, I'll return to _get_dt_power() > >> +{ >> + struct dev_pm_opp *opp; >> + unsigned long opp_freq, opp_power; >> + >> + /* 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; >> + >> + opp_power = dev_pm_opp_get_power(opp); > > As I said in 2/4, this should really give the total instead. make sense > >> + dev_pm_opp_put(opp); >> + if (!opp_power) >> + 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 >> @@ -1488,6 +1520,24 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz, >> return 0; >> } >> >> +static bool _of_has_opp_microwatt_property(struct device *dev) >> +{ >> + unsigned long power, freq = 0; >> + struct dev_pm_opp *opp; >> + >> + /* Check if at least one OPP has needed property */ >> + opp = dev_pm_opp_find_freq_ceil(dev, &freq); >> + if (IS_ERR(opp)) >> + return false; >> + >> + power = dev_pm_opp_get_power(opp); >> + dev_pm_opp_put(opp); >> + if (!power) > > What if this particular frequency has 0 power mentioned for some > reason :) If that power 0 would be allowed here, then in next step when EM calls active_power() we check !power and report dev_err(). IPA design would also not accept the power=0 > > Instead of this heavy stuff, just pick the first OPP from the opp > table and see its power-value. It is the first opp: freq=0. You mean by parsing the the DT node instead, like I had in v2 version? > >> + return false; >> + >> + return true; >> +} >> + >> /** >> * dev_pm_opp_of_register_em() - Attempt to register an Energy Model >> * @dev : Device for which an Energy Model has to be registered >> @@ -1517,6 +1567,14 @@ 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_has_opp_microwatt_property(dev)) { >> + struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power); >> + >> + return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb, >> + cpus, true); >> + } >> + > > We have another em_dev_register_perf_domain() down the line, lets keep > only one such call and get it a callback that should be set in an > if/else loop. OK
On 24-02-22, 09:33, Lukasz Luba wrote: > On 2/24/22 09:13, Viresh Kumar wrote: > > On 24-02-22, 08:11, Lukasz Luba wrote: > > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > > > +static bool _of_has_opp_microwatt_property(struct device *dev) > > > +{ > > > + unsigned long power, freq = 0; > > > + struct dev_pm_opp *opp; > > > + > > > + /* Check if at least one OPP has needed property */ > > > + opp = dev_pm_opp_find_freq_ceil(dev, &freq); > > > + if (IS_ERR(opp)) > > > + return false; > > > + > > > + power = dev_pm_opp_get_power(opp); > > > + dev_pm_opp_put(opp); > > > + if (!power) > > > > Instead of this heavy stuff, just pick the first OPP from the opp > > table and see its power-value. > > It is the first opp: freq=0. > You mean by parsing the the DT node instead, like I had in v2 version? No, I was thinking if you can simply do: opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node); But that requires locking, etc as well. So maybe this is fine.
On 2/24/22 09:42, Viresh Kumar wrote: > On 24-02-22, 09:33, Lukasz Luba wrote: >> On 2/24/22 09:13, Viresh Kumar wrote: >>> On 24-02-22, 08:11, Lukasz Luba wrote: >>>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c > >>>> +static bool _of_has_opp_microwatt_property(struct device *dev) >>>> +{ >>>> + unsigned long power, freq = 0; >>>> + struct dev_pm_opp *opp; >>>> + >>>> + /* Check if at least one OPP has needed property */ >>>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq); >>>> + if (IS_ERR(opp)) >>>> + return false; >>>> + >>>> + power = dev_pm_opp_get_power(opp); >>>> + dev_pm_opp_put(opp); >>>> + if (!power) >>> >>> Instead of this heavy stuff, just pick the first OPP from the opp >>> table and see its power-value. >> >> It is the first opp: freq=0. >> You mean by parsing the the DT node instead, like I had in v2 version? > > No, I was thinking if you can simply do: > > opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node); > > But that requires locking, etc as well. So maybe this is fine. > OK, so I'll leave it as is now. Thank you for the comments. I'll work on v4
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 7bff30f27dc1..af3488779867 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -1438,6 +1438,38 @@ 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 used 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_opp_power(unsigned long *mW, unsigned long *kHz, struct device *dev) +{ + struct dev_pm_opp *opp; + unsigned long opp_freq, opp_power; + + /* 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; + + opp_power = dev_pm_opp_get_power(opp); + dev_pm_opp_put(opp); + if (!opp_power) + 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 @@ -1488,6 +1520,24 @@ static int __maybe_unused _get_power(unsigned long *mW, unsigned long *kHz, return 0; } +static bool _of_has_opp_microwatt_property(struct device *dev) +{ + unsigned long power, freq = 0; + struct dev_pm_opp *opp; + + /* Check if at least one OPP has needed property */ + opp = dev_pm_opp_find_freq_ceil(dev, &freq); + if (IS_ERR(opp)) + return false; + + power = dev_pm_opp_get_power(opp); + dev_pm_opp_put(opp); + if (!power) + return false; + + return true; +} + /** * dev_pm_opp_of_register_em() - Attempt to register an Energy Model * @dev : Device for which an Energy Model has to be registered @@ -1517,6 +1567,14 @@ 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_has_opp_microwatt_property(dev)) { + struct em_data_callback em_dt_cb = EM_DATA_CB(_get_opp_power); + + return em_dev_register_perf_domain(dev, nr_opp, &em_dt_cb, + cpus, true); + } + np = of_node_get(dev->of_node); if (!np) { ret = -EINVAL;
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 relies on the new OPP feature allowing to get power (which is coming from "opp-microwatt" DT property) expressed in micro-Watts. The patch also opens new opportunity to better support platforms, which have a decent static power. It allows to register 'advanced' EM (based on real power measurements) which models total power (static + dynamic), so better reflects real HW. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/opp/of.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)