| Message ID | 5acd93462bff6c108d65802fd39f6002dfadd1a0.1667473008.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: Allow power/current values without voltage | expand |
On Thursday, 3 November 2022 9:01:08 PM AEST Viresh Kumar wrote: > @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > bool triplet; > > microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); > - if (IS_ERR_OR_NULL(microvolt)) > + if (IS_ERR(microvolt)) > return PTR_ERR(microvolt); Erroring out here will still block EM registration on platforms without the opp-microvolt prop so we're back to square one, which means the patch does not do what the description says it does. It behaves almost identically to the current code. I assume this is an intentional choice given the comments in opp_parse_microvolt(), so the commit message should drop references to fixing such platforms. If this is a hard sticking point and opp_parse_supplies() must return a regulator with opp-microvolt, then I am of the opinion that the parsing of opp-microwatt should be detangled entirely from the regulator infrastructure. Otherwise for the whole series, Tested-by: James Calligeros <jcalligeros99@gmail.com> - James
On 03-11-22, 22:24, James Calligeros wrote: > On Thursday, 3 November 2022 9:01:08 PM AEST Viresh Kumar wrote: > > > @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > > bool triplet; > > > > microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); > > - if (IS_ERR_OR_NULL(microvolt)) > > + if (IS_ERR(microvolt)) > > return PTR_ERR(microvolt); > > Erroring out here will still block EM registration on platforms without > the opp-microvolt prop so we're back to square one, which means the > patch does not do what the description says it does. It behaves > almost identically to the current code. I am confused. With the current code, the following will work: - all three available - microvolt available and nothing else. - only microamp available - only microwatt available - both microamp and microwatt available but no microvolt - and other combinations Isn't this all we want ? We error out here when there is a problem with DT entries, i.e. incorrect entries, etc. Else we will get NULL here and we continue as we don't check for IS_ERR_OR_NULL() anymore after this patch. > I assume this is an intentional choice given the comments in > opp_parse_microvolt(), so the commit message should drop > references to fixing such platforms. > > If this is a hard sticking point and opp_parse_supplies() must return > a regulator with opp-microvolt, then I am of the opinion that the parsing > of opp-microwatt should be detangled entirely from the regulator > infrastructure. > > Otherwise for the whole series, > > Tested-by: James Calligeros <jcalligeros99@gmail.com> What did you test exactly ? As I thought you will be testing the only microwatt case here and you say it won't work. Sorry, I just got a little bit confused :(
On Thursday, 3 November 2022 11:10:51 PM AEST Viresh Kumar wrote: > On 03-11-22, 22:24, James Calligeros wrote: > > On Thursday, 3 November 2022 9:01:08 PM AEST Viresh Kumar wrote: > > > > > @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, > > > bool triplet; > > > > > > microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); > > > - if (IS_ERR_OR_NULL(microvolt)) > > > + if (IS_ERR(microvolt)) > > > return PTR_ERR(microvolt); > > > > Erroring out here will still block EM registration on platforms without > > the opp-microvolt prop so we're back to square one, which means the > > patch does not do what the description says it does. It behaves > > almost identically to the current code. > > I am confused. > > With the current code, the following will work: > - all three available > - microvolt available and nothing else. > - only microamp available > - only microwatt available > - both microamp and microwatt available but no microvolt > - and other combinations > > Isn't this all we want ? You're right, I misinterpreted an error. Details as below. > What did you test exactly ? As I thought you will be testing the only microwatt > case here and you say it won't work. > > Sorry, I just got a little bit confused :( > I did test on the Apple machine with only opp-microwatt, but I misinterpreted the error. We end up here upon parsing the second OPP: >if (list_empty(&opp_table->opp_list) && > opp_table->regulator_count > 0) { > dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", > __func__); > return ERR_PTR(-EINVAL); >} When this path is removed, things work as expected. Could it be that opp_list has not been updated by the time we're parsing the next OPP? Seems unlikely, but at the same time if we're ending up taking this branch then there's not much else that could have gone wrong. - James
On 04-11-22, 01:23, James Calligeros wrote: > >if (list_empty(&opp_table->opp_list) && > > opp_table->regulator_count > 0) { I did test this and it should have worked for you as well. > > dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", > > __func__); > > return ERR_PTR(-EINVAL); > >} > > When this path is removed, things work as expected. Could it be that opp_list has > not been updated by the time we're parsing the next OPP? Seems unlikely, but > at the same time if we're ending up taking this branch then there's not much else > that could have gone wrong. It should happen sequentially. Ahh, I looked code for sometime before I wrote this line. I know what's going on. Its a bug in the patchset that I fixed yesterday and pushed. Initialize "ret = 0" in opp_parse_supplies(). Or pick patches from: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next Sorry for the trouble.
On 03-11-22, 16:31, Viresh Kumar wrote: > From: James Calligeros <jcalligeros99@gmail.com> > > 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 device's > 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. > > 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. > > Signed-off-by: James Calligeros <jcalligeros99@gmail.com> > Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V2: Rewritten by Viresh on top of his changes. > > drivers/opp/of.c | 36 +++++++++++++++++++++++++----------- > 1 file changed, 25 insertions(+), 11 deletions(-) Plus this fix: diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 273fa9c0e1c0..e55c6095adf0 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -673,7 +673,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) { u32 *microvolt, *microamp, *microwatt; - int ret, i, j; + int ret = 0, i, j; bool triplet; microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
On Friday, 4 November 2022 3:07:52 PM AEST Viresh Kumar wrote: > It should happen sequentially. I noticed the mutexes after I got some sleep :) > Initialize "ret = 0" in opp_parse_supplies(). > > Or pick patches from: > > git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next > It didn't even cross my mind that opp_parse_supplies() returning ret uninitialised was causing the failure further up the chain. Applied and tested, everything's working as expected now. > Sorry for the trouble. This is on me, I should have heeded the compiler warning. - James
On 04-11-22, 15:25, James Calligeros wrote: > On Friday, 4 November 2022 3:07:52 PM AEST Viresh Kumar wrote: > > It should happen sequentially. > > I noticed the mutexes after I got some sleep :) > > > Initialize "ret = 0" in opp_parse_supplies(). > > > > Or pick patches from: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next > > > > It didn't even cross my mind that opp_parse_supplies() returning > ret uninitialised was causing the failure further up the chain. Applied > and tested, everything's working as expected now. > > > Sorry for the trouble. > > This is on me, I should have heeded the compiler warning. Thanks James for giving this a try. Really appreciate it. The changes are applied and pushed for linux-next along with your Tested-by.
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index e51c43495e21..273fa9c0e1c0 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -654,9 +654,12 @@ static u32 *opp_parse_microvolt(struct dev_pm_opp *opp, struct device *dev, /* * Missing property isn't a problem, but an invalid * entry is. This property isn't optional if regulator - * information is provided. + * information is provided. Check only for the first OPP, as + * regulator_count may get initialized after that to a valid + * value. */ - if (opp_table->regulator_count > 0) { + if (list_empty(&opp_table->opp_list) && + opp_table->regulator_count > 0) { dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", __func__); return ERR_PTR(-EINVAL); @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, bool triplet; microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); - if (IS_ERR_OR_NULL(microvolt)) + if (IS_ERR(microvolt)) return PTR_ERR(microvolt); microamp = _parse_named_prop(opp, dev, opp_table, "microamp", NULL); @@ -689,15 +692,26 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, goto free_microamp; } - for (i = 0, j = 0; i < opp_table->regulator_count; i++) { - opp->supplies[i].u_volt = microvolt[j++]; + /* + * Initialize regulator_count if it is uninitialized and no properties + * are found. + */ + if (unlikely(opp_table->regulator_count == -1)) { + opp_table->regulator_count = 0; + return 0; + } - if (triplet) { - opp->supplies[i].u_volt_min = microvolt[j++]; - opp->supplies[i].u_volt_max = microvolt[j++]; - } else { - opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; - opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + for (i = 0, j = 0; i < opp_table->regulator_count; i++) { + if (microvolt) { + opp->supplies[i].u_volt = microvolt[j++]; + + if (triplet) { + opp->supplies[i].u_volt_min = microvolt[j++]; + opp->supplies[i].u_volt_max = microvolt[j++]; + } else { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + } } if (microamp)