| Message ID | 20220222140746.12293-3-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | Introduce 'advanced' Energy Model in DT | expand |
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 >
Hi Matthias, On 2/22/22 14:58, Matthias Kaehlcke wrote: > On Tue, Feb 22, 2022 at 02:07:46PM +0000, Lukasz Luba wrote: [snip] >> +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. Good point. I have internal patch set under review changing this. It's going to be changed and the 'dev' would be the 1st arg. I'll send this patch set as soon as this one gets queued into pm tree. > >> +{ >> + 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. Agree, I'll change the name and return type. > > >> +{ >> + 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(). It has to be 0, since under the hood the dev_pm_opp_find_freq_ceil() is going to find first freq which is equal or bigger than this one. We actually use that ptr value in the _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 Well, it just stress the 'an' as in this case it's the 1st OPP, due to the fact freq = 0 and finding the 'ceiling' on it. I'll remove it. > >> + 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(...);'? true Thanks for the review! I'll address these comments in v3 if Viresh agrees with this new approach. Regards, Lukasz
On 22-02-22, 14:07, Lukasz Luba wrote: > +static int _of_find_opp_microwatt_property(struct device *dev) > +{ > + unsigned long freq = 0; > + 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 */ > + 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; > +} Please follow everything just like opp-microvolt is defined. Create a new field in the struct dev_pm_opp, initialize it only once when the OPP is created, that field should be used here instead of parsing the DT here again. There also needs to be a debug file in debugfs for this new field. Search for "supply" and "microvolt" in the OPP core, you will see all the places that need it.
On 2/23/22 05:53, Viresh Kumar wrote: > On 22-02-22, 14:07, Lukasz Luba wrote: >> +static int _of_find_opp_microwatt_property(struct device *dev) >> +{ >> + unsigned long freq = 0; >> + 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 */ >> + 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; >> +} > > Please follow everything just like opp-microvolt is defined. Create a new field > in the struct dev_pm_opp, initialize it only once when the OPP is created, that > field should be used here instead of parsing the DT here again. There also needs > to be a debug file in debugfs for this new field. > > Search for "supply" and "microvolt" in the OPP core, you will see all the places > that need it. > OK, so you want to have this available for the whole system. I can do that. I would go for one value of power and try to fit into the opp_parse_supplies() code. As far as I can see in the dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt is used. I would go for similar solution for u_watt. There is even a single u_amp and no _max, _min variants, so should be good..
On 23-02-22, 08:59, Lukasz Luba wrote: > OK, so you want to have this available for the whole system. I can do > that. I would go for one value of power One value per supply, right ? > and try to fit into the > opp_parse_supplies() code. Correct. > As far as I can see in the > dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt > is used. I would go for similar solution for u_watt. > There is even a single u_amp and no _max, _min variants, so should be > good.. Yes, I don't think we need min/max/target kind of naming here. Just a single value per supply is enough.
On 2/23/22 09:10, Viresh Kumar wrote: > On 23-02-22, 08:59, Lukasz Luba wrote: >> OK, so you want to have this available for the whole system. I can do >> that. I would go for one value of power > > One value per supply, right ? yes, only the u_watt, no _min, _max variants. > >> and try to fit into the >> opp_parse_supplies() code. > > Correct. > >> As far as I can see in the >> dev_pm_opp_get_voltage() the simple solution: supplier[0] and u_volt >> is used. I would go for similar solution for u_watt. >> There is even a single u_amp and no _max, _min variants, so should be >> good.. > > Yes, I don't think we need min/max/target kind of naming here. Just a single > value per supply is enough. > Good, thanks for comments. Let me craft the v3 then.
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) +{ + 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) +{ + unsigned long freq = 0; + 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 */ + 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; + } + 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 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(+)