[RFC PATCH] PM / OPP: Always expose one supply in debugfs

[RFC PATCH] PM / OPP: Always expose one supply in debugfs

[Quentin Perret]

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(-)

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

Re: [RFC PATCH] PM / OPP: Always expose one supply in debugfs

[Quentin Perret]

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
> 

Re: [RFC PATCH] PM / OPP: Always expose one supply in debugfs

[Viresh Kumar]

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.

-- 
viresh

Re: [RFC PATCH] PM / OPP: Always expose one supply in debugfs

[Quentin Perret]

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

Re: [RFC PATCH] PM / OPP: Always expose one supply in debugfs

[Viresh Kumar]

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.

-- 
viresh