Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-25 Thread Viresh Kumar
On 25-10-16, 16:13, Dave Gerlach wrote:
> I think what you have shared below is a good safety check but if I rename
> the regulator properties in the DT for the cpu (to vdd and vbb, meaning
> cpufreq detects no regulator) and do *not* call dev_pm_opp_set_regulators
> before cpufreq-dt probes we fail before we even get to that point:
> 
> [16.946] cpu cpu0: opp_parse_supplies: Invalid number of elements in
> opp-microvolt property (6) with supplies (1)
> [16.967] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
> [16.982] cpu cpu0: dev_pm_opp_get_opp_count: OPP table not found (-19)
> [16.982] cpu cpu0: OPP table is not ready, deferring probe
> 
> This failure is because opp_parse_supplies assumes a count of 1 regulator if
> no regulators at all are present and then hard fails if too many voltages
> have been passed for each OPP.

Exactly. And yes this is intentional.

> It seems we need a check much earlier similar
> to what you suggested below to allow us to defer if an OPP has supplied
> voltages but no regulator has been registered with the system. I think this
> is reasonable even for the 1 regulator case, no?

No.

OPP core needs to know about regulators only if the user drivers want it to
manage DVFS. It is still possible for cpufreq drivers to use OPP framework for
managing the tables, but do the real DVFS stuff themselves. That's why it is not
compulsory in the code to set regulator names.

And its only wrong if dev_pm_opp_set_rate() is called without first setting the
regulators..

> cpufreq-dt won't handle this properly as is, but now that the opp core is
> evolving perhaps it makes sense to modify the resources_available check
> slightly to rely on the OPP core rather than just a dummy
> regulator_get_optional to see if the regulator is ready.

I am not sure yet on what to change there. You mean regarding multiple
regulators?

-- 
viresh


Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-25 Thread Dave Gerlach

Hi,
On 10/23/2016 11:26 PM, Viresh Kumar wrote:

On 23-10-16, 20:08, Dave Gerlach wrote:

Overall this series looks good to me apart from a few small things. Most
importantly I was able to get a working implementation using two regulators
on ti dra7xx platform with proper sequencing built on top of this series. We
have cpu regulator and Adaptive body bias (abb) regulator that must be
scaled in a certain order before or after clock scaling and I was able to
implement a rough custom set_rate to perform this and ran some dvfs stress
tests that all worked fine.


Thanks for testing it buddy.


First comment, I think the platform specific set_rate is a good place to
hook in for adaptive voltage scaling as well. I was able to implement TI
Class0 AVS in the same code by using the requested transition voltage as a
reference and programming AVS voltage using that, along with scaling the
additional regulators in sequence (the original multi regulator
functionality).


Hmm, interesting..


I would think some people would want to use this even with
single regulator platforms, no?


Maybe, but I would like to see such user code first. It may be possible to
handle much of AVS stuff in core so that everyone isn't required to do it.


Ok, I think it would be a logical next step to look at once this series 
gets accepted. For now, the particular implementation I did just looks 
up an optimized value for the requested voltage from a register and 
programs the optimal value instead of the requested voltage.





This raises some concerns about dependencies/probe sequencing. Right now we
just need to make sure the cpufreq-dt driver probes after we have called
_set_regulators, but if our platform code fails cpufreq-dt currently will
treat this as no regulator needed for the platform and operate without one,
which will likely hang the system. Is there a good way to to guarantee this
doesn't happen? My main concern is that if we plan to provide a platform
specific set-rate function, we should have a way to indicate this and
prevent things from progressing if it isn't yet ready.

Again, overall I think it solves the multi regulator problem, and it works
well for AVS as well. For the series:

Tested-by: Dave Gerlach 


Thanks.

For the concern you shared about, does the below patch fix it ? I will include
that in V3 then.


I think what you have shared below is a good safety check but if I 
rename the regulator properties in the DT for the cpu (to vdd and vbb, 
meaning cpufreq detects no regulator) and do *not* call 
dev_pm_opp_set_regulators before cpufreq-dt probes we fail before we 
even get to that point:


[16.946] cpu cpu0: opp_parse_supplies: Invalid number of elements in 
opp-microvolt property (6) with supplies (1)

[16.967] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
[16.982] cpu cpu0: dev_pm_opp_get_opp_count: OPP table not found (-19)
[16.982] cpu cpu0: OPP table is not ready, deferring probe

This failure is because opp_parse_supplies assumes a count of 1 
regulator if no regulators at all are present and then hard fails if too 
many voltages have been passed for each OPP. It seems we need a check 
much earlier similar to what you suggested below to allow us to defer if 
an OPP has supplied voltages but no regulator has been registered with 
the system. I think this is reasonable even for the 1 regulator case, 
no? If we have passed voltages then we presumably are hoping to use them 
with a regulator, and if no regulators are present, OPP framework should 
defer.


cpufreq-dt won't handle this properly as is, but now that the opp core 
is evolving perhaps it makes sense to modify the resources_available 
check slightly to rely on the OPP core rather than just a dummy 
regulator_get_optional to see if the regulator is ready.


Regards,
Dave



-8<-

From: Viresh Kumar 
Date: Mon, 24 Oct 2016 09:45:30 +0530
Subject: [PATCH] PM / OPP: Don't assume platform doesn't have regulators

If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.

If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.

Handle that properly by making sure that the DT didn't had any entries
for supply voltages as well.

Signed-off-by: Viresh Kumar 
---
 drivers/base/power/opp/core.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b69908b74ed6..fb4250532180 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -737,7 +737,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long 
target_freq)

/* Only frequency scaling */
if (!regulators) {
-   rcu_read_unlock();
+   /*
+* DT contain

Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-23 Thread Viresh Kumar
On 23-10-16, 20:08, Dave Gerlach wrote:
> Overall this series looks good to me apart from a few small things. Most
> importantly I was able to get a working implementation using two regulators
> on ti dra7xx platform with proper sequencing built on top of this series. We
> have cpu regulator and Adaptive body bias (abb) regulator that must be
> scaled in a certain order before or after clock scaling and I was able to
> implement a rough custom set_rate to perform this and ran some dvfs stress
> tests that all worked fine.

Thanks for testing it buddy.

> First comment, I think the platform specific set_rate is a good place to
> hook in for adaptive voltage scaling as well. I was able to implement TI
> Class0 AVS in the same code by using the requested transition voltage as a
> reference and programming AVS voltage using that, along with scaling the
> additional regulators in sequence (the original multi regulator
> functionality). 

Hmm, interesting..

> I would think some people would want to use this even with
> single regulator platforms, no?

Maybe, but I would like to see such user code first. It may be possible to
handle much of AVS stuff in core so that everyone isn't required to do it.

> This raises some concerns about dependencies/probe sequencing. Right now we
> just need to make sure the cpufreq-dt driver probes after we have called
> _set_regulators, but if our platform code fails cpufreq-dt currently will
> treat this as no regulator needed for the platform and operate without one,
> which will likely hang the system. Is there a good way to to guarantee this
> doesn't happen? My main concern is that if we plan to provide a platform
> specific set-rate function, we should have a way to indicate this and
> prevent things from progressing if it isn't yet ready.
> 
> Again, overall I think it solves the multi regulator problem, and it works
> well for AVS as well. For the series:
> 
> Tested-by: Dave Gerlach 

Thanks.

For the concern you shared about, does the below patch fix it ? I will include
that in V3 then.

-8<-

From: Viresh Kumar 
Date: Mon, 24 Oct 2016 09:45:30 +0530
Subject: [PATCH] PM / OPP: Don't assume platform doesn't have regulators

If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.

If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.

Handle that properly by making sure that the DT didn't had any entries
for supply voltages as well.

Signed-off-by: Viresh Kumar 
---
 drivers/base/power/opp/core.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b69908b74ed6..fb4250532180 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -737,7 +737,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long 
target_freq)
 
/* Only frequency scaling */
if (!regulators) {
-   rcu_read_unlock();
+   /*
+* DT contained supply ratings? Consider platform failed to set
+* regulators.
+*/
+   if (unlikely(opp->supplies[0].u_volt)) {
+   rcu_read_unlock();
+   dev_err(dev, "%s: Regulator not registered with OPP 
core\n",
+   __func__);
+   return -EINVAL;
+   }
+
return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
}
 
-- 
viresh


Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-23 Thread Dave Gerlach

Hi,
On 10/21/2016 10:40 AM, Viresh Kumar wrote:

On 21 October 2016 at 19:09, Rafael J. Wysocki  wrote:

On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar  wrote:

Hi,

Some platforms (like TI) have complex DVFS configuration for CPU
devices, where multiple regulators are required to be configured to
change DVFS state of the device. This was explained well by Nishanth
earlier [1].

Some thoughts went into it few months back but then it all got lost. I
am trying to get that back on track with this thread.

One of the major complaints around multiple regulators case was that the
DT isn't responsible in any way to represent the ordering in which
multiple supplies need to be programmed, before or after frequency
change. It was considered in this patch and such information is left to
the platform specific OPP driver now, which can register its own
opp_set_rate() callback with the OPP core and the OPP core will then
call it during DVFS.

The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
and code to pass values for multiple regulators and verified that they
are all properly read by the kernel (using debugfs interface).

Though more testing on real (TI) platforms would be useful.

This is rebased over: linux-next branch in the PM tree.

V1->V2:
- Ack from Rob for 1st patch
- Moved the supplies structure to pm_opp.h (Dave)
- Fixed an compilation warning.


I need somebody from the OPP camp to review patches [2-8/8] for me.


Sure, I have already asked Stephen yesterday to do that.


Overall this series looks good to me apart from a few small things. Most 
importantly I was able to get a working implementation using two 
regulators on ti dra7xx platform with proper sequencing built on top of 
this series. We have cpu regulator and Adaptive body bias (abb) 
regulator that must be scaled in a certain order before or after clock 
scaling and I was able to implement a rough custom set_rate to perform 
this and ran some dvfs stress tests that all worked fine.


First comment, I think the platform specific set_rate is a good place to 
hook in for adaptive voltage scaling as well. I was able to implement TI 
Class0 AVS in the same code by using the requested transition voltage as 
a reference and programming AVS voltage using that, along with scaling 
the additional regulators in sequence (the original multi regulator 
functionality). I would think some people would want to use this even 
with single regulator platforms, no? cpufreq-dt works as is for that, we 
just swap out the regulators.


This raises some concerns about dependencies/probe sequencing. Right now 
we just need to make sure the cpufreq-dt driver probes after we have 
called _set_regulators, but if our platform code fails cpufreq-dt 
currently will treat this as no regulator needed for the platform and 
operate without one, which will likely hang the system. Is there a good 
way to to guarantee this doesn't happen? My main concern is that if we 
plan to provide a platform specific set-rate function, we should have a 
way to indicate this and prevent things from progressing if it isn't yet 
ready.


Again, overall I think it solves the multi regulator problem, and it 
works well for AVS as well. For the series:


Tested-by: Dave Gerlach 

Regards,
Dave



--
viresh





Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-21 Thread Viresh Kumar
On 21 October 2016 at 19:09, Rafael J. Wysocki  wrote:
> On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar  
> wrote:
>> Hi,
>>
>> Some platforms (like TI) have complex DVFS configuration for CPU
>> devices, where multiple regulators are required to be configured to
>> change DVFS state of the device. This was explained well by Nishanth
>> earlier [1].
>>
>> Some thoughts went into it few months back but then it all got lost. I
>> am trying to get that back on track with this thread.
>>
>> One of the major complaints around multiple regulators case was that the
>> DT isn't responsible in any way to represent the ordering in which
>> multiple supplies need to be programmed, before or after frequency
>> change. It was considered in this patch and such information is left to
>> the platform specific OPP driver now, which can register its own
>> opp_set_rate() callback with the OPP core and the OPP core will then
>> call it during DVFS.
>>
>> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
>> and code to pass values for multiple regulators and verified that they
>> are all properly read by the kernel (using debugfs interface).
>>
>> Though more testing on real (TI) platforms would be useful.
>>
>> This is rebased over: linux-next branch in the PM tree.
>>
>> V1->V2:
>> - Ack from Rob for 1st patch
>> - Moved the supplies structure to pm_opp.h (Dave)
>> - Fixed an compilation warning.
>
> I need somebody from the OPP camp to review patches [2-8/8] for me.

Sure, I have already asked Stephen yesterday to do that.

--
viresh


Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-21 Thread Rafael J. Wysocki
On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar  wrote:
> Hi,
>
> Some platforms (like TI) have complex DVFS configuration for CPU
> devices, where multiple regulators are required to be configured to
> change DVFS state of the device. This was explained well by Nishanth
> earlier [1].
>
> Some thoughts went into it few months back but then it all got lost. I
> am trying to get that back on track with this thread.
>
> One of the major complaints around multiple regulators case was that the
> DT isn't responsible in any way to represent the ordering in which
> multiple supplies need to be programmed, before or after frequency
> change. It was considered in this patch and such information is left to
> the platform specific OPP driver now, which can register its own
> opp_set_rate() callback with the OPP core and the OPP core will then
> call it during DVFS.
>
> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> and code to pass values for multiple regulators and verified that they
> are all properly read by the kernel (using debugfs interface).
>
> Though more testing on real (TI) platforms would be useful.
>
> This is rebased over: linux-next branch in the PM tree.
>
> V1->V2:
> - Ack from Rob for 1st patch
> - Moved the supplies structure to pm_opp.h (Dave)
> - Fixed an compilation warning.

I need somebody from the OPP camp to review patches [2-8/8] for me.

Thanks,
Rafael


[PATCH V2 0/8] PM / OPP: Multiple regulator support

2016-10-20 Thread Viresh Kumar
Hi,

Some platforms (like TI) have complex DVFS configuration for CPU
devices, where multiple regulators are required to be configured to
change DVFS state of the device. This was explained well by Nishanth
earlier [1].

Some thoughts went into it few months back but then it all got lost. I
am trying to get that back on track with this thread.

One of the major complaints around multiple regulators case was that the
DT isn't responsible in any way to represent the ordering in which
multiple supplies need to be programmed, before or after frequency
change. It was considered in this patch and such information is left to
the platform specific OPP driver now, which can register its own
opp_set_rate() callback with the OPP core and the OPP core will then
call it during DVFS.

The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
and code to pass values for multiple regulators and verified that they
are all properly read by the kernel (using debugfs interface).

Though more testing on real (TI) platforms would be useful.

This is rebased over: linux-next branch in the PM tree.

V1->V2:
- Ack from Rob for 1st patch
- Moved the supplies structure to pm_opp.h (Dave)
- Fixed an compilation warning.

--
viresh

[1] https://marc.info/?l=linux-pm&m=145684495832764&w=2

Viresh Kumar (8):
  PM / OPP: Reword binding supporting multiple regulators per device
  PM / OPP: Don't use OPP structure outside of rcu protected section
  PM / OPP: Manage supply's voltage/current in a separate structure
  PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
  PM / OPP: Add infrastructure to manage multiple regulators
  PM / OPP: Separate out _generic_opp_set_rate()
  PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()

 Documentation/devicetree/bindings/opp/opp.txt |  25 +-
 drivers/base/power/opp/core.c | 501 --
 drivers/base/power/opp/debugfs.c  |  48 ++-
 drivers/base/power/opp/of.c   | 104 --
 drivers/base/power/opp/opp.h  |  20 +-
 drivers/cpufreq/cpufreq-dt.c  |   9 +-
 include/linux/pm_opp.h|  49 ++-
 7 files changed, 574 insertions(+), 182 deletions(-)

-- 
2.7.1.410.g6faf27b