| Message ID | 20180116165119.15168-2-quentin.perret@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 16-01-18, 16:51, Quentin Perret wrote: > +/** > + * dev_pm_opp_has_power() - Get the status of power values for OPPs > + * @cpu_dev: CPU device for which we do this operation > + * > + * Return: True if the OPPs hold power estimates for the CPU > + */ > +bool dev_pm_opp_has_power(struct device *cpu_dev) > +{ > + struct opp_table *opp_table; > + bool has_power; > + > + opp_table = _find_opp_table(cpu_dev); > + if (IS_ERR(opp_table)) > + return false; > + > + has_power = opp_table->capacitance; > + > + dev_pm_opp_put_opp_table(opp_table); > + > + return has_power; > +} > +EXPORT_SYMBOL_GPL(dev_pm_opp_has_power); I am not sure about the worthiness of this routine right now. Users can just ask for the power and that will be 0 if it is not available. For the thermal stuff we can just check if the right property is present or not for the device (like we used to do), if we want to avoid running extra code there. The point is I am not sure if this will be useful to multiple users of the power numbers or not and would like to wait to see more users before adding this API. > /** > * dev_pm_opp_get_max_clock_latency() - Get max clock latency in nanoseconds > * @dev: device for which we do this operation > @@ -1112,6 +1153,8 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev, > goto free_opp; > } > > + _opp_estimate_power(new_opp); > + Call this from _opp_add(), right after the call to ->get_pstate(). > /* > * Notify the changes in the availability of the operable > * frequency/voltage list. > @@ -1728,6 +1771,26 @@ static int _opp_set_availability(struct device *dev, unsigned long freq, > return r; > } > > +/** > + * _opp_estimate_power() -- helper to estimate power of an OPP > + * @opp: OPP for which we want to do this > + * > + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's > + * capacitance, V the OPP's voltage and f the OPP's frequency. > + */ Don't need doc style comments for internal routines. > +void _opp_estimate_power(struct dev_pm_opp *opp) > +{ > + unsigned long capacitance = opp->opp_table->capacitance; > + unsigned long mV, KHz, uW; > + > + if (capacitance) { Maybe just remove the if block, the result will anyway be zero here and that may be better than having an additional conditional statement. > + mV = opp->supplies[0].u_volt / 1000; > + KHz = opp->rate / 1000; > + uW = capacitance * mV * mV * KHz / 1000000000; > + opp->power_estimate_uw = uW; > + } > +} > + > /** > * dev_pm_opp_enable() - Enable a specific OPP > * @dev: device for which we do this operation > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index cb716aa2f44b..607566c4fc9a 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -55,14 +55,17 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev) > { > struct device_node *np; > > - /* > - * Only required for backward compatibility with v1 bindings, but isn't > - * harmful for other cases. And so we do it unconditionally. > - */ > np = of_node_get(dev->of_node); > if (np) { > u32 val; > > + of_property_read_u32(np, "dynamic-power-coefficient", > + &opp_table->capacitance); > + > + /* > + * Required for backward compatibility with v1 bindings, but > + * isn't harmful for other cases so we do it unconditionally. > + */ > if (!of_property_read_u32(np, "clock-latency", &val)) > opp_table->clock_latency_ns_max = val; > of_property_read_u32(np, "voltage-tolerance", > @@ -336,6 +339,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, > goto free_opp; > } > > + _opp_estimate_power(new_opp); > + This wouldn't be required after you move it to _opp_add(). > /* OPP to select on device suspend */ > if (of_property_read_bool(np, "opp-suspend")) { > if (opp_table->suspend_opp) { > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > index 4d00061648a3..cb331f9a565b 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -84,6 +84,8 @@ struct dev_pm_opp { > > unsigned long clock_latency_ns; > > + unsigned long power_estimate_uw; > + > struct opp_table *opp_table; > > struct device_node *np; > @@ -165,6 +167,8 @@ struct opp_table { > /* For backward compatibility with v1 bindings */ > unsigned int voltage_tolerance_v1; > > + unsigned int capacitance; > + > enum opp_table_access shared_opp; > struct dev_pm_opp *suspend_opp; There is documentation above the structure that you need to update.
Hi Viresh, Thanks for the feedback ! On Wednesday 17 Jan 2018 at 11:07:24 (+0530), Viresh Kumar wrote: > On 16-01-18, 16:51, Quentin Perret wrote: > > +/** > > + * dev_pm_opp_has_power() - Get the status of power values for OPPs > > + * @cpu_dev: CPU device for which we do this operation > > + * > > + * Return: True if the OPPs hold power estimates for the CPU > > + */ > > +bool dev_pm_opp_has_power(struct device *cpu_dev) > > +{ > > + struct opp_table *opp_table; > > + bool has_power; > > + > > + opp_table = _find_opp_table(cpu_dev); > > + if (IS_ERR(opp_table)) > > + return false; > > + > > + has_power = opp_table->capacitance; > > + > > + dev_pm_opp_put_opp_table(opp_table); > > + > > + return has_power; > > +} > > +EXPORT_SYMBOL_GPL(dev_pm_opp_has_power); > > I am not sure about the worthiness of this routine right now. Users > can just ask for the power and that will be 0 if it is not available. > > For the thermal stuff we can just check if the right property is > present or not for the device (like we used to do), if we want to > avoid running extra code there. > > The point is I am not sure if this will be useful to multiple users of > the power numbers or not and would like to wait to see more users > before adding this API. Right, I see your point. I'm happy to remove this but I'm just not sure about calling of_find_property() in the thermal subsystem. The point of this series is to make the thermal subsystem use a more abstract API so checking if dynamic-power-coefficient is present is kind of against the idea. What I suggest is to call __cpufreq_cooling_register() with has_power=true in of_cpufreq_cooling_register() and bail out from update_freq_table() cleanly whenever dev_pm_opp_get_power() returns 0 (and in this case we fallback on the has_power=false path). Does that make sense ? > > /** > > * dev_pm_opp_get_max_clock_latency() - Get max clock latency in nanoseconds > > * @dev: device for which we do this operation > > @@ -1112,6 +1153,8 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev, > > goto free_opp; > > } > > > > + _opp_estimate_power(new_opp); > > + > > Call this from _opp_add(), right after the call to ->get_pstate(). Ok. > > /* > > * Notify the changes in the availability of the operable > > * frequency/voltage list. > > @@ -1728,6 +1771,26 @@ static int _opp_set_availability(struct device *dev, unsigned long freq, > > return r; > > } > > > > +/** > > + * _opp_estimate_power() -- helper to estimate power of an OPP > > + * @opp: OPP for which we want to do this > > + * > > + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's > > + * capacitance, V the OPP's voltage and f the OPP's frequency. > > + */ > > Don't need doc style comments for internal routines. Some of them do have doc style comments (_opp_set_availaility() above this function for ex) so I just wanted to stay consistent. > > +void _opp_estimate_power(struct dev_pm_opp *opp) > > +{ > > + unsigned long capacitance = opp->opp_table->capacitance; > > + unsigned long mV, KHz, uW; > > + > > + if (capacitance) { > > Maybe just remove the if block, the result will anyway be zero here > and that may be better than having an additional conditional > statement. Ok. > > > + mV = opp->supplies[0].u_volt / 1000; > > + KHz = opp->rate / 1000; > > + uW = capacitance * mV * mV * KHz / 1000000000; > > + opp->power_estimate_uw = uW; > > + } > > +} > > + > > /** > > * dev_pm_opp_enable() - Enable a specific OPP > > * @dev: device for which we do this operation > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > > index cb716aa2f44b..607566c4fc9a 100644 > > --- a/drivers/opp/of.c > > +++ b/drivers/opp/of.c > > @@ -55,14 +55,17 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev) > > { > > struct device_node *np; > > > > - /* > > - * Only required for backward compatibility with v1 bindings, but isn't > > - * harmful for other cases. And so we do it unconditionally. > > - */ > > np = of_node_get(dev->of_node); > > if (np) { > > u32 val; > > > > + of_property_read_u32(np, "dynamic-power-coefficient", > > + &opp_table->capacitance); > > + > > + /* > > + * Required for backward compatibility with v1 bindings, but > > + * isn't harmful for other cases so we do it unconditionally. > > + */ > > if (!of_property_read_u32(np, "clock-latency", &val)) > > opp_table->clock_latency_ns_max = val; > > of_property_read_u32(np, "voltage-tolerance", > > @@ -336,6 +339,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, > > goto free_opp; > > } > > > > + _opp_estimate_power(new_opp); > > + > > This wouldn't be required after you move it to _opp_add(). > > > /* OPP to select on device suspend */ > > if (of_property_read_bool(np, "opp-suspend")) { > > if (opp_table->suspend_opp) { > > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > > index 4d00061648a3..cb331f9a565b 100644 > > --- a/drivers/opp/opp.h > > +++ b/drivers/opp/opp.h > > @@ -84,6 +84,8 @@ struct dev_pm_opp { > > > > unsigned long clock_latency_ns; > > > > + unsigned long power_estimate_uw; > > + > > struct opp_table *opp_table; > > > > struct device_node *np; > > @@ -165,6 +167,8 @@ struct opp_table { > > /* For backward compatibility with v1 bindings */ > > unsigned int voltage_tolerance_v1; > > > > + unsigned int capacitance; > > + > > enum opp_table_access shared_opp; > > struct dev_pm_opp *suspend_opp; > > There is documentation above the structure that you need to update. Right, I will update it. Thank you very much, Quentin
On 17-01-18, 10:27, Quentin Perret wrote: > What I suggest is to call __cpufreq_cooling_register() with > has_power=true in of_cpufreq_cooling_register() and bail out from > update_freq_table() cleanly whenever dev_pm_opp_get_power() returns 0 > (and in this case we fallback on the has_power=false path). Does that > make sense ? Maybe just remove the has_power thing completely and always call update_freq_table() and let it update power to 0 ? Its anyway a one time initialization thing, wouldn't matter much.
On Wednesday 17 Jan 2018 at 16:11:03 (+0530), Viresh Kumar wrote: > On 17-01-18, 10:27, Quentin Perret wrote: > > What I suggest is to call __cpufreq_cooling_register() with > > has_power=true in of_cpufreq_cooling_register() and bail out from > > update_freq_table() cleanly whenever dev_pm_opp_get_power() returns 0 > > (and in this case we fallback on the has_power=false path). Does that > > make sense ? > > Maybe just remove the has_power thing completely and always call > update_freq_table() and let it update power to 0 ? Its anyway a one time > initialization thing, wouldn't matter much. Hmmm I just need to make sure to fallback on the right cooling_ops if update_freq_table() updated the power to 0, but yes, that should work. I'll do the change in v3. Cheers, Quentin
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 92fa94a6dcc1..ca088b6230ca 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -127,6 +127,24 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp) } EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq); +/** + * dev_pm_opp_get_power() - Gets the estimated power corresponding to an opp + * @opp: opp for which power has to be returned for + * + * Return: estimated power in mirco-watts corresponding to the opp, else + * return 0 + */ +unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp) +{ + if (IS_ERR_OR_NULL(opp)) { + pr_err("%s: Invalid parameters\n", __func__); + return 0; + } + + return opp->power_estimate_uw; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_get_power); + /** * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not * @opp: opp for which turbo mode is being verified @@ -148,6 +166,29 @@ bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp) } EXPORT_SYMBOL_GPL(dev_pm_opp_is_turbo); +/** + * dev_pm_opp_has_power() - Get the status of power values for OPPs + * @cpu_dev: CPU device for which we do this operation + * + * Return: True if the OPPs hold power estimates for the CPU + */ +bool dev_pm_opp_has_power(struct device *cpu_dev) +{ + struct opp_table *opp_table; + bool has_power; + + opp_table = _find_opp_table(cpu_dev); + if (IS_ERR(opp_table)) + return false; + + has_power = opp_table->capacitance; + + dev_pm_opp_put_opp_table(opp_table); + + return has_power; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_has_power); + /** * dev_pm_opp_get_max_clock_latency() - Get max clock latency in nanoseconds * @dev: device for which we do this operation @@ -1112,6 +1153,8 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev, goto free_opp; } + _opp_estimate_power(new_opp); + /* * Notify the changes in the availability of the operable * frequency/voltage list. @@ -1728,6 +1771,26 @@ static int _opp_set_availability(struct device *dev, unsigned long freq, return r; } +/** + * _opp_estimate_power() -- helper to estimate power of an OPP + * @opp: OPP for which we want to do this + * + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's + * capacitance, V the OPP's voltage and f the OPP's frequency. + */ +void _opp_estimate_power(struct dev_pm_opp *opp) +{ + unsigned long capacitance = opp->opp_table->capacitance; + unsigned long mV, KHz, uW; + + if (capacitance) { + mV = opp->supplies[0].u_volt / 1000; + KHz = opp->rate / 1000; + uW = capacitance * mV * mV * KHz / 1000000000; + opp->power_estimate_uw = uW; + } +} + /** * dev_pm_opp_enable() - Enable a specific OPP * @dev: device for which we do this operation diff --git a/drivers/opp/of.c b/drivers/opp/of.c index cb716aa2f44b..607566c4fc9a 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -55,14 +55,17 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev) { struct device_node *np; - /* - * Only required for backward compatibility with v1 bindings, but isn't - * harmful for other cases. And so we do it unconditionally. - */ np = of_node_get(dev->of_node); if (np) { u32 val; + of_property_read_u32(np, "dynamic-power-coefficient", + &opp_table->capacitance); + + /* + * Required for backward compatibility with v1 bindings, but + * isn't harmful for other cases so we do it unconditionally. + */ if (!of_property_read_u32(np, "clock-latency", &val)) opp_table->clock_latency_ns_max = val; of_property_read_u32(np, "voltage-tolerance", @@ -336,6 +339,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, goto free_opp; } + _opp_estimate_power(new_opp); + /* OPP to select on device suspend */ if (of_property_read_bool(np, "opp-suspend")) { if (opp_table->suspend_opp) { diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index 4d00061648a3..cb331f9a565b 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -84,6 +84,8 @@ struct dev_pm_opp { unsigned long clock_latency_ns; + unsigned long power_estimate_uw; + struct opp_table *opp_table; struct device_node *np; @@ -165,6 +167,8 @@ struct opp_table { /* For backward compatibility with v1 bindings */ unsigned int voltage_tolerance_v1; + unsigned int capacitance; + enum opp_table_access shared_opp; struct dev_pm_opp *suspend_opp; @@ -198,6 +202,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *o int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic); void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of); struct opp_table *_add_opp_table(struct device *dev); +void _opp_estimate_power(struct dev_pm_opp *opp); #ifdef CONFIG_OF void _of_init_opp_table(struct opp_table *opp_table, struct device *dev); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 6c2d2e88f066..ad50203b7ea9 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -85,8 +85,12 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp); unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp); +unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp); + bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp); +bool dev_pm_opp_has_power(struct device *cpu_dev); + int dev_pm_opp_get_opp_count(struct device *dev); unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev); unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev); @@ -150,11 +154,21 @@ static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp) return 0; } +static inline unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp) +{ + return 0; +} + static inline bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp) { return false; } +static inline bool dev_pm_opp_has_power(struct device *cpu_dev) +{ + return false; +} + static inline int dev_pm_opp_get_opp_count(struct device *dev) { return 0;
The power dissipated by a CPU at a specific OPP is currently estimated by the thermal subsystem as P = C * V^2 * f, with P the power, C the CPU's capacitance, V the OPP's voltage and f the OPP's frequency. As this feature can be useful for other clients requiring energy models of CPUs, this commit introduces an equivalent power estimator directly into the OPP library, hence enabling code re-use. Signed-off-by: Quentin Perret <quentin.perret@arm.com> --- drivers/opp/core.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/opp/of.c | 13 +++++++---- drivers/opp/opp.h | 5 ++++ include/linux/pm_opp.h | 14 +++++++++++ 4 files changed, 91 insertions(+), 4 deletions(-)