| Message ID | 20181210113247.11412-1-quentin.perret@arm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] PM / OPP: Always expose one supply in debugfs | expand |
On Monday 10 Dec 2018 at 11:32:47 (+0000), Quentin Perret wrote: > On some platforms, the opp_table->regulator_count field is kept at zero > even though opp->supplies is always allocated. However, the loop used to > display the supplies in the debugfs doesn't deal correctly with this, > which results in the supplies not being displayed in debugfs on those > platforms. > > Fix this by making sure to always display at least once supply in > debugfs. > > Signed-off-by: Quentin Perret <quentin.perret@arm.com> > > --- > > This has been observed on Juno r2 which uses SCPI and Hikey960 which > uses DT. I am not particularly familiar with that part of the code, so > I'm not sure if this is even remotely correct (hence the RFC tag). > > I first thought setting opp_table->regulator_count to 1 would be the > right fix but that causes other issues. This fix seems to work OK on > Juno and Hikey960, at least. > > Feedback is welcome :-) Hmm, so I just figured what I'm doing here is basically reverting: 1fae788ed640 ("PM / OPP: Don't create debugfs "supply-0" directory unnecessarily") Should I send a proper revert instead of this patch ? It _is_ handy to read voltage numbers when available. Thanks, Quentin > > Thanks, > Quentin > --- > drivers/opp/debugfs.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c > index e6828e5f81b0..2c14564575cb 100644 > --- a/drivers/opp/debugfs.c > +++ b/drivers/opp/debugfs.c > @@ -40,9 +40,9 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp, > struct dentry *pdentry) > { > struct dentry *d; > - int i; > + int i = 0; > > - for (i = 0; i < opp_table->regulator_count; i++) { > + do { > char name[15]; > > snprintf(name, sizeof(name), "supply-%d", i); > @@ -68,7 +68,9 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp, > if (!debugfs_create_ulong("u_amp", S_IRUGO, d, > &opp->supplies[i].u_amp)) > return false; > - } > + > + i++; > + } while (i < opp_table->regulator_count); > > return true; > } > -- > 2.19.2 >
On 10-12-18, 11:54, Quentin Perret wrote: > On Monday 10 Dec 2018 at 11:32:47 (+0000), Quentin Perret wrote: > > On some platforms, the opp_table->regulator_count field is kept at zero > > even though opp->supplies is always allocated. However, the loop used to > > display the supplies in the debugfs doesn't deal correctly with this, > > which results in the supplies not being displayed in debugfs on those > > platforms. > > > > Fix this by making sure to always display at least once supply in > > debugfs. > > > > Signed-off-by: Quentin Perret <quentin.perret@arm.com> > > > > --- > > > > This has been observed on Juno r2 which uses SCPI and Hikey960 which > > uses DT. I am not particularly familiar with that part of the code, so > > I'm not sure if this is even remotely correct (hence the RFC tag). > > > > I first thought setting opp_table->regulator_count to 1 would be the > > right fix but that causes other issues. This fix seems to work OK on > > Juno and Hikey960, at least. > > > > Feedback is welcome :-) > > Hmm, so I just figured what I'm doing here is basically reverting: > > 1fae788ed640 ("PM / OPP: Don't create debugfs "supply-0" directory unnecessarily") > > Should I send a proper revert instead of this patch ? It _is_ handy to > read voltage numbers when available. First of all I am really happy that someone apart from me is using this debug interface :) At least that patch was correct and if we have to fix something, it should be fixed somewhere else. I tried to reproduce the issue on hikey and straight away understood why it is behaving that way. There is no "cpu-supply" property defined for the CPUs. I looked at juno driver and couldn't see any references to voltage there. Are you trying to add that support ? Anyway, this should still be fixed irrespective of the "cpu-supply" property. The other fix you mentioned about setting opp_table->regulator_count to 1 is actually the right thing to do but the fix may require changes at multiple places. I can help writing a patch for that if you want, else I am open to you giving it a shot :) Thanks.
On Tuesday 11 Dec 2018 at 14:55:49 (+0530), Viresh Kumar wrote: > On 10-12-18, 11:54, Quentin Perret wrote: > > On Monday 10 Dec 2018 at 11:32:47 (+0000), Quentin Perret wrote: > > > On some platforms, the opp_table->regulator_count field is kept at zero > > > even though opp->supplies is always allocated. However, the loop used to > > > display the supplies in the debugfs doesn't deal correctly with this, > > > which results in the supplies not being displayed in debugfs on those > > > platforms. > > > > > > Fix this by making sure to always display at least once supply in > > > debugfs. > > > > > > Signed-off-by: Quentin Perret <quentin.perret@arm.com> > > > > > > --- > > > > > > This has been observed on Juno r2 which uses SCPI and Hikey960 which > > > uses DT. I am not particularly familiar with that part of the code, so > > > I'm not sure if this is even remotely correct (hence the RFC tag). > > > > > > I first thought setting opp_table->regulator_count to 1 would be the > > > right fix but that causes other issues. This fix seems to work OK on > > > Juno and Hikey960, at least. > > > > > > Feedback is welcome :-) > > > > Hmm, so I just figured what I'm doing here is basically reverting: > > > > 1fae788ed640 ("PM / OPP: Don't create debugfs "supply-0" directory unnecessarily") > > > > Should I send a proper revert instead of this patch ? It _is_ handy to > > read voltage numbers when available. > > First of all I am really happy that someone apart from me is using this debug > interface :) :-) My current use case is to compute the 'dynamic-power-coefficient' thing for IPA. With P=CV^2f, I can measure P and get f easily, but V is a bit trickier (as of now at least). PM_OPP has the information I need, so it'd be handy to expose it somewhere. > At least that patch was correct and if we have to fix something, it should be > fixed somewhere else. Understood. > I tried to reproduce the issue on hikey and straight away understood why it is > behaving that way. There is no "cpu-supply" property defined for the CPUs. Ah, right. The opp-microvolt binding is specified however, so I guess we could expose that too ... > I looked at juno driver and couldn't see any references to voltage there. Are > you trying to add that support ? So, on Juno we do get voltage numbers from firmware, and those get registered properly in PM_OPP, exactly like for Hikey960. So it's probably the same problem in both cases. FWIW, this is what I get on juno with my 'fix' applied: $ cat /sys/kernel/debug/opp/cpu0/*/supply-0/u_volt_target 820000 850000 900000 950000 1000000 > Anyway, this should still be fixed irrespective of the "cpu-supply" property. > The other fix you mentioned about setting opp_table->regulator_count to 1 is > actually the right thing to do but the fix may require changes at multiple > places. I can help writing a patch for that if you want, else I am open to you > giving it a shot :) You'll definitely know the code better than me so if you have a patch in mind, please don't hesitate. I'll be happy to test it on my two boards here :-) Thanks, Quentin
On 11-12-18, 09:49, Quentin Perret wrote: > So, on Juno we do get voltage numbers from firmware, and those get > registered properly in PM_OPP, exactly like for Hikey960. So it's > probably the same problem in both cases. > > FWIW, this is what I get on juno with my 'fix' applied: > > $ cat /sys/kernel/debug/opp/cpu0/*/supply-0/u_volt_target > 820000 > 850000 > 900000 > 950000 > 1000000 > > > Anyway, this should still be fixed irrespective of the "cpu-supply" property. > > The other fix you mentioned about setting opp_table->regulator_count to 1 is > > actually the right thing to do but the fix may require changes at multiple > > places. I can help writing a patch for that if you want, else I am open to you > > giving it a shot :) > > You'll definitely know the code better than me so if you have a patch in > mind, please don't hesitate. I'll be happy to test it on my two boards > here :-) Sent some patches, please give them a try.
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c index e6828e5f81b0..2c14564575cb 100644 --- a/drivers/opp/debugfs.c +++ b/drivers/opp/debugfs.c @@ -40,9 +40,9 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp, struct dentry *pdentry) { struct dentry *d; - int i; + int i = 0; - for (i = 0; i < opp_table->regulator_count; i++) { + do { char name[15]; snprintf(name, sizeof(name), "supply-%d", i); @@ -68,7 +68,9 @@ static bool opp_debug_create_supplies(struct dev_pm_opp *opp, if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supplies[i].u_amp)) return false; - } + + i++; + } while (i < opp_table->regulator_count); return true; }
On some platforms, the opp_table->regulator_count field is kept at zero even though opp->supplies is always allocated. However, the loop used to display the supplies in the debugfs doesn't deal correctly with this, which results in the supplies not being displayed in debugfs on those platforms. Fix this by making sure to always display at least once supply in debugfs. Signed-off-by: Quentin Perret <quentin.perret@arm.com> --- This has been observed on Juno r2 which uses SCPI and Hikey960 which uses DT. I am not particularly familiar with that part of the code, so I'm not sure if this is even remotely correct (hence the RFC tag). I first thought setting opp_table->regulator_count to 1 would be the right fix but that causes other issues. This fix seems to work OK on Juno and Hikey960, at least. Feedback is welcome :-) Thanks, Quentin --- drivers/opp/debugfs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)