Messages in this thread Patch in this message | | | Date | Wed, 2 Nov 2022 14:32:28 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH v2] OPP: decouple dt properties in opp_parse_supplies() |
| |
On 30-10-22, 20:15, James Calligeros wrote: > The opp-microwatt property was added with the intention of providing > platforms a way to specify a precise value for the power consumption > of a device at a given OPP to enable better energy-aware scheduling > decisions by informing the kernel of the total static and dynamic > power of a device at a given OPP, removing the reliance on the EM > subsystem's often flawed estimations. This property is parsed by > opp_parse_supplies(), which creates a hard dependency on the > opp-microvolt property. > > Some platforms, such as Apple Silicon, do not describe their devices' > voltage regulators in the DT as they cannot be controlled by the kernel > and/or rely on opaque firmware algorithms to control their voltage and > current characteristics at runtime. We can, however, experimentally > determine the power consumption of a given device at a given OPP, taking > advantage of opp-microwatt to provide EAS on such devices as was initially > intended.
Do you supply a regulator to the OPP core for your platform ?
> Allow platforms to specify and consume any subset of opp-microvolt, > opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt > to enable this functionality on such platforms. > > Fixes: 4f9a7a1dc2a2 ("OPP: Add "opp-microwatt" supporting code")
I won't call it a fix, we are trying to use this information in a different way here, that's all.
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com> > --- > Changes since v1: > Fixed bad reference (opp to opp_table) > > drivers/opp/of.c | 198 +++++++++++++++++++++++++---------------------- > 1 file changed, 104 insertions(+), 94 deletions(-) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 605d68673f92..0fa25c3a959e 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -581,166 +581,176 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, > static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > struct opp_table *opp_table) > { > - u32 *microvolt, *microamp = NULL, *microwatt = NULL; > + u32 *microvolt = NULL, *microamp = NULL, *microwatt = NULL; > int supplies = opp_table->regulator_count; > int vcount, icount, pcount, ret, i, j; > - struct property *prop = NULL; > + struct property *prop_mv = NULL, *prop_ma = NULL, *prop_mw = NULL; > char name[NAME_MAX]; > > /* Search for "opp-microvolt-<name>" */ > if (opp_table->prop_name) { > snprintf(name, sizeof(name), "opp-microvolt-%s", > opp_table->prop_name); > - prop = of_find_property(opp->np, name, NULL); > + prop_mv = of_find_property(opp->np, name, NULL); > } > > - if (!prop) { > + if (!prop_mv) { > /* Search for "opp-microvolt" */ > sprintf(name, "opp-microvolt"); > - prop = of_find_property(opp->np, name, NULL); > - > - /* Missing property isn't a problem, but an invalid entry is */ > - if (!prop) { > - if (unlikely(supplies == -1)) { > - /* Initialize regulator_count */ > - opp_table->regulator_count = 0; > - return 0; > - } > + prop_mv = of_find_property(opp->np, name, NULL); > > - if (!supplies) > - return 0; > - > - dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", > - __func__);
Catching such errors are important and so the opp-microvolt property was made compulsory earlier.
If there is a regulator, then we must have microvolt property. amps/watts are optional.
> - return -EINVAL; > - } > } > > - if (unlikely(supplies == -1)) { > - /* Initialize regulator_count */ > - supplies = opp_table->regulator_count = 1; > - } else if (unlikely(!supplies)) { > - dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__); > - return -EINVAL; > + if (prop_mv) { > + vcount = of_property_count_u32_elems(opp->np, name); > + if (unlikely(supplies == -1)) > + supplies = opp_table->regulator_count = vcount;
This is wrong. There can be one or three entries per regulator here. Target or min/max/target. If the supplies value is -1, we can only support one regulator, i.e. one or three entries total.
I didn't look at rest of the patch yet. Lets discuss this a bit first.
-- viresh
| |