lkml.org 
[lkml]   [2022]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-11-02 10:04    [W:0.072 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site