* [PATCH resend] cpufreq: dt: disable unsupported OPPs
@ 2014-09-30 15:29 Lucas Stach
2014-09-30 19:53 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-09-30 15:29 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar; +Cc: Mark Brown, linux-pm
If the regulator connected to the CPU voltage plane doesn't
support an OPP specified voltage with the acceptable tolerance
it's better to just disable the OPP instead of constantly
failing the voltage scaling later on.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
resend:
- no functional change, split out from the imx5 cpufreq series
v2:
- rebase on top of pm/linux-next
---
drivers/cpufreq/cpufreq-cpu0.c | 66 +++++++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index a5f8c5f5f4f4..5d73efbd0240 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -185,6 +185,7 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
struct device *cpu_dev;
struct regulator *cpu_reg;
struct clk *cpu_clk;
+ unsigned long min_uV = ~0, max_uV = 0;
unsigned int transition_latency;
int ret;
@@ -204,16 +205,10 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
/* OPPs might be populated at runtime, don't check for error here */
of_init_opp_table(cpu_dev);
- ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
- if (ret) {
- dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
- goto out_put_node;
- }
-
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
- goto out_free_table;
+ goto out_put_node;
}
of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
@@ -222,30 +217,49 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
transition_latency = CPUFREQ_ETERNAL;
if (!IS_ERR(cpu_reg)) {
- struct dev_pm_opp *opp;
- unsigned long min_uV, max_uV;
- int i;
-
/*
- * OPP is maintained in order of increasing frequency, and
- * freq_table initialised from OPP is therefore sorted in the
- * same order.
+ * Disable any OPPs where the connected regulator isn't able to
+ * provide the specified voltage and record minimum and maximum
+ * voltage levels.
*/
- for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++)
- ;
- rcu_read_lock();
- opp = dev_pm_opp_find_freq_exact(cpu_dev,
- freq_table[0].frequency * 1000, true);
- min_uV = dev_pm_opp_get_voltage(opp);
- opp = dev_pm_opp_find_freq_exact(cpu_dev,
- freq_table[i-1].frequency * 1000, true);
- max_uV = dev_pm_opp_get_voltage(opp);
- rcu_read_unlock();
+ while (1) {
+ struct dev_pm_opp *opp;
+ unsigned long opp_freq = 0, opp_uV, tol_uV;
+
+ rcu_read_lock();
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
+ if (IS_ERR(opp)) {
+ rcu_read_unlock();
+ break;
+ }
+ opp_uV = dev_pm_opp_get_voltage(opp);
+ rcu_read_unlock();
+
+ tol_uV = opp_uV * priv->voltage_tolerance / 100;
+ if (regulator_is_supported_voltage(cpu_reg, opp_uV,
+ opp_uV + tol_uV)) {
+ if (opp_uV < min_uV)
+ min_uV = opp_uV;
+ if (opp_uV > max_uV)
+ max_uV = opp_uV;
+ } else {
+ dev_pm_opp_disable(cpu_dev, opp_freq);
+ }
+
+ opp_freq++;
+ }
+
ret = regulator_set_voltage_time(cpu_reg, min_uV, max_uV);
if (ret > 0)
transition_latency += ret * 1000;
}
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
+ if (ret) {
+ pr_err("failed to init cpufreq table: %d\n", ret);
+ goto out_free_priv;
+ }
+
/*
* For now, just loading the cooling device;
* thermal DT code takes care of matching them.
@@ -274,9 +288,9 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
out_cooling_unregister:
cpufreq_cooling_unregister(priv->cdev);
- kfree(priv);
-out_free_table:
dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_free_priv:
+ kfree(priv);
out_put_node:
of_node_put(np);
out_put_reg_clk:
--
2.1.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-09-30 15:29 [PATCH resend] cpufreq: dt: disable unsupported OPPs Lucas Stach
@ 2014-09-30 19:53 ` Rafael J. Wysocki
2014-10-01 3:29 ` Viresh Kumar
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-09-30 19:53 UTC (permalink / raw)
To: Lucas Stach; +Cc: Viresh Kumar, Mark Brown, linux-pm
On Tuesday, September 30, 2014 05:29:10 PM Lucas Stach wrote:
> If the regulator connected to the CPU voltage plane doesn't
> support an OPP specified voltage with the acceptable tolerance
> it's better to just disable the OPP instead of constantly
> failing the voltage scaling later on.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> resend:
> - no functional change, split out from the imx5 cpufreq series
> v2:
> - rebase on top of pm/linux-next
This makes sense to me, but I need ACKs from people who are more familiar
with OPPs in general.
> ---
> drivers/cpufreq/cpufreq-cpu0.c | 66 +++++++++++++++++++++++++-----------------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> index a5f8c5f5f4f4..5d73efbd0240 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -185,6 +185,7 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> struct device *cpu_dev;
> struct regulator *cpu_reg;
> struct clk *cpu_clk;
> + unsigned long min_uV = ~0, max_uV = 0;
> unsigned int transition_latency;
> int ret;
>
> @@ -204,16 +205,10 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> /* OPPs might be populated at runtime, don't check for error here */
> of_init_opp_table(cpu_dev);
>
> - ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> - if (ret) {
> - dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> - goto out_put_node;
> - }
> -
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> - goto out_free_table;
> + goto out_put_node;
> }
>
> of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
> @@ -222,30 +217,49 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> transition_latency = CPUFREQ_ETERNAL;
>
> if (!IS_ERR(cpu_reg)) {
> - struct dev_pm_opp *opp;
> - unsigned long min_uV, max_uV;
> - int i;
> -
> /*
> - * OPP is maintained in order of increasing frequency, and
> - * freq_table initialised from OPP is therefore sorted in the
> - * same order.
> + * Disable any OPPs where the connected regulator isn't able to
> + * provide the specified voltage and record minimum and maximum
> + * voltage levels.
> */
> - for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++)
> - ;
> - rcu_read_lock();
> - opp = dev_pm_opp_find_freq_exact(cpu_dev,
> - freq_table[0].frequency * 1000, true);
> - min_uV = dev_pm_opp_get_voltage(opp);
> - opp = dev_pm_opp_find_freq_exact(cpu_dev,
> - freq_table[i-1].frequency * 1000, true);
> - max_uV = dev_pm_opp_get_voltage(opp);
> - rcu_read_unlock();
> + while (1) {
> + struct dev_pm_opp *opp;
> + unsigned long opp_freq = 0, opp_uV, tol_uV;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + break;
> + }
> + opp_uV = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + tol_uV = opp_uV * priv->voltage_tolerance / 100;
> + if (regulator_is_supported_voltage(cpu_reg, opp_uV,
> + opp_uV + tol_uV)) {
> + if (opp_uV < min_uV)
> + min_uV = opp_uV;
> + if (opp_uV > max_uV)
> + max_uV = opp_uV;
> + } else {
> + dev_pm_opp_disable(cpu_dev, opp_freq);
> + }
> +
> + opp_freq++;
> + }
> +
> ret = regulator_set_voltage_time(cpu_reg, min_uV, max_uV);
> if (ret > 0)
> transition_latency += ret * 1000;
> }
>
> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> + if (ret) {
> + pr_err("failed to init cpufreq table: %d\n", ret);
> + goto out_free_priv;
> + }
> +
> /*
> * For now, just loading the cooling device;
> * thermal DT code takes care of matching them.
> @@ -274,9 +288,9 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
>
> out_cooling_unregister:
> cpufreq_cooling_unregister(priv->cdev);
> - kfree(priv);
> -out_free_table:
> dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_free_priv:
> + kfree(priv);
> out_put_node:
> of_node_put(np);
> out_put_reg_clk:
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-09-30 19:53 ` Rafael J. Wysocki
@ 2014-10-01 3:29 ` Viresh Kumar
2014-10-01 20:39 ` Rafael J. Wysocki
2014-10-08 22:36 ` Rafael J. Wysocki
0 siblings, 2 replies; 22+ messages in thread
From: Viresh Kumar @ 2014-10-01 3:29 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On 1 October 2014 01:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, September 30, 2014 05:29:10 PM Lucas Stach wrote:
>> If the regulator connected to the CPU voltage plane doesn't
>> support an OPP specified voltage with the acceptable tolerance
>> it's better to just disable the OPP instead of constantly
>> failing the voltage scaling later on.
>>
>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> ---
>> resend:
>> - no functional change, split out from the imx5 cpufreq series
>> v2:
>> - rebase on top of pm/linux-next
>
> This makes sense to me, but I need ACKs from people who are more familiar
> with OPPs in general.
@Rafael: Where is this gone?
http://permalink.gmane.org/gmane.linux.power-management.general/49384
I am quite sure you applied this.. Have you dropped this while playing with
branches ?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-01 3:29 ` Viresh Kumar
@ 2014-10-01 20:39 ` Rafael J. Wysocki
2014-10-02 5:24 ` Viresh Kumar
2014-10-08 22:36 ` Rafael J. Wysocki
1 sibling, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-01 20:39 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On Wednesday, October 01, 2014 08:59:17 AM Viresh Kumar wrote:
> On 1 October 2014 01:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 30, 2014 05:29:10 PM Lucas Stach wrote:
> >> If the regulator connected to the CPU voltage plane doesn't
> >> support an OPP specified voltage with the acceptable tolerance
> >> it's better to just disable the OPP instead of constantly
> >> failing the voltage scaling later on.
> >>
> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> ---
> >> resend:
> >> - no functional change, split out from the imx5 cpufreq series
> >> v2:
> >> - rebase on top of pm/linux-next
> >
> > This makes sense to me, but I need ACKs from people who are more familiar
> > with OPPs in general.
>
> @Rafael: Where is this gone?
>
> http://permalink.gmane.org/gmane.linux.power-management.general/49384
>
> I am quite sure you applied this..
Yes, I did.
> Have you dropped this while playing with branches ?
I dropped it temporarily, because I wanted to fold
https://patchwork.kernel.org/patch/4983431/
into it, but then forgot to re-apply it.
Would you mind sending it again with the fix folded in for me?
Rafael
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-01 20:39 ` Rafael J. Wysocki
@ 2014-10-02 5:24 ` Viresh Kumar
2014-10-02 11:57 ` Lucas Stach
0 siblings, 1 reply; 22+ messages in thread
From: Viresh Kumar @ 2014-10-02 5:24 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On 2 October 2014 02:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Would you mind sending it again with the fix folded in for me?
Done.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-02 5:24 ` Viresh Kumar
@ 2014-10-02 11:57 ` Lucas Stach
2014-10-02 17:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-02 11:57 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael J. Wysocki, Mark Brown, linux-pm@vger.kernel.org
Am Donnerstag, den 02.10.2014, 10:54 +0530 schrieb Viresh Kumar:
> On 2 October 2014 02:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > Would you mind sending it again with the fix folded in for me?
>
> Done.
I don't know how it makes sense to fold the fix into this patch. It has
nothing to do with the rename.
If you want to fold the fix in the relevant patch this would be
"cpufreq: cpu0: Move per-cluster initialization code to ->init()"
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-02 11:57 ` Lucas Stach
@ 2014-10-02 17:33 ` Rafael J. Wysocki
0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-02 17:33 UTC (permalink / raw)
To: Lucas Stach; +Cc: Viresh Kumar, Mark Brown, linux-pm@vger.kernel.org
On Thursday, October 02, 2014 01:57:55 PM Lucas Stach wrote:
> Am Donnerstag, den 02.10.2014, 10:54 +0530 schrieb Viresh Kumar:
> > On 2 October 2014 02:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > Would you mind sending it again with the fix folded in for me?
> >
> > Done.
>
> I don't know how it makes sense to fold the fix into this patch. It has
> nothing to do with the rename.
> If you want to fold the fix in the relevant patch this would be
> "cpufreq: cpu0: Move per-cluster initialization code to ->init()"
OK, my confusion, sorry about that.
I'll take the original version of the Viresh's patch and the fix on top
of that separately, then.
That said, it would help if the information about when things broke was there
in the fix' chanelog.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-01 3:29 ` Viresh Kumar
2014-10-01 20:39 ` Rafael J. Wysocki
@ 2014-10-08 22:36 ` Rafael J. Wysocki
2014-10-09 3:43 ` Viresh Kumar
1 sibling, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-08 22:36 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On Wednesday, October 01, 2014 08:59:17 AM Viresh Kumar wrote:
> On 1 October 2014 01:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 30, 2014 05:29:10 PM Lucas Stach wrote:
> >> If the regulator connected to the CPU voltage plane doesn't
> >> support an OPP specified voltage with the acceptable tolerance
> >> it's better to just disable the OPP instead of constantly
> >> failing the voltage scaling later on.
> >>
> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> ---
> >> resend:
> >> - no functional change, split out from the imx5 cpufreq series
> >> v2:
> >> - rebase on top of pm/linux-next
> >
> > This makes sense to me, but I need ACKs from people who are more familiar
> > with OPPs in general.
>
> @Rafael: Where is this gone?
>
> http://permalink.gmane.org/gmane.linux.power-management.general/49384
OK, since this has been restored, what am I supposed to do with the
$subject patch?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-08 22:36 ` Rafael J. Wysocki
@ 2014-10-09 3:43 ` Viresh Kumar
2014-10-12 20:27 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: Viresh Kumar @ 2014-10-09 3:43 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On 9 October 2014 04:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> OK, since this has been restored, what am I supposed to do with the
> $subject patch?
Yeah, so the patch looked fine to me. If this can still be applied without
Lucas required to resend it, please add my Ack and apply it :)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH resend] cpufreq: dt: disable unsupported OPPs
2014-10-09 3:43 ` Viresh Kumar
@ 2014-10-12 20:27 ` Rafael J. Wysocki
2014-10-16 10:08 ` [RESEND 2] " Lucas Stach
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-12 20:27 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Lucas Stach, Mark Brown, linux-pm@vger.kernel.org
On Thursday, October 09, 2014 09:13:54 AM Viresh Kumar wrote:
> On 9 October 2014 04:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > OK, since this has been restored, what am I supposed to do with the
> > $subject patch?
>
> Yeah, so the patch looked fine to me. If this can still be applied without
> Lucas required to resend it, please add my Ack and apply it :)
No, it didn't apply for me cleanly.
Lucas, can you please rebase this on top of the current Linus' tree and resend?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-12 20:27 ` Rafael J. Wysocki
@ 2014-10-16 10:08 ` Lucas Stach
2014-10-21 14:19 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-16 10:08 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Viresh Kumar, linux-pm
If the regulator connected to the CPU voltage plane doesn't
support an OPP specified voltage with the acceptable tolerance
it's better to just disable the OPP instead of constantly
failing the voltage scaling later on.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
resend 2:
- no functional change, rebase against latest Linus tree
- added Viresh's ack
resend:
- no functional change, split out from the imx5 cpufreq series
v2:
- rebase on top of pm/linux-next
---
drivers/cpufreq/cpufreq-dt.c | 66 +++++++++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 6bbb8b913446..4485c8eccdc2 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -185,6 +185,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
struct device *cpu_dev;
struct regulator *cpu_reg;
struct clk *cpu_clk;
+ unsigned long min_uV = ~0, max_uV = 0;
unsigned int transition_latency;
int ret;
@@ -204,16 +205,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
/* OPPs might be populated at runtime, don't check for error here */
of_init_opp_table(cpu_dev);
- ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
- if (ret) {
- dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
- goto out_put_node;
- }
-
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
- goto out_free_table;
+ goto out_put_node;
}
of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
@@ -222,30 +217,49 @@ static int cpufreq_init(struct cpufreq_policy *policy)
transition_latency = CPUFREQ_ETERNAL;
if (!IS_ERR(cpu_reg)) {
- struct dev_pm_opp *opp;
- unsigned long min_uV, max_uV;
- int i;
-
/*
- * OPP is maintained in order of increasing frequency, and
- * freq_table initialised from OPP is therefore sorted in the
- * same order.
+ * Disable any OPPs where the connected regulator isn't able to
+ * provide the specified voltage and record minimum and maximum
+ * voltage levels.
*/
- for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++)
- ;
- rcu_read_lock();
- opp = dev_pm_opp_find_freq_exact(cpu_dev,
- freq_table[0].frequency * 1000, true);
- min_uV = dev_pm_opp_get_voltage(opp);
- opp = dev_pm_opp_find_freq_exact(cpu_dev,
- freq_table[i-1].frequency * 1000, true);
- max_uV = dev_pm_opp_get_voltage(opp);
- rcu_read_unlock();
+ while (1) {
+ struct dev_pm_opp *opp;
+ unsigned long opp_freq = 0, opp_uV, tol_uV;
+
+ rcu_read_lock();
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
+ if (IS_ERR(opp)) {
+ rcu_read_unlock();
+ break;
+ }
+ opp_uV = dev_pm_opp_get_voltage(opp);
+ rcu_read_unlock();
+
+ tol_uV = opp_uV * priv->voltage_tolerance / 100;
+ if (regulator_is_supported_voltage(cpu_reg, opp_uV,
+ opp_uV + tol_uV)) {
+ if (opp_uV < min_uV)
+ min_uV = opp_uV;
+ if (opp_uV > max_uV)
+ max_uV = opp_uV;
+ } else {
+ dev_pm_opp_disable(cpu_dev, opp_freq);
+ }
+
+ opp_freq++;
+ }
+
ret = regulator_set_voltage_time(cpu_reg, min_uV, max_uV);
if (ret > 0)
transition_latency += ret * 1000;
}
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
+ if (ret) {
+ pr_err("failed to init cpufreq table: %d\n", ret);
+ goto out_free_priv;
+ }
+
/*
* For now, just loading the cooling device;
* thermal DT code takes care of matching them.
@@ -275,9 +289,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
out_cooling_unregister:
cpufreq_cooling_unregister(priv->cdev);
- kfree(priv);
-out_free_table:
dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_free_priv:
+ kfree(priv);
out_put_node:
of_node_put(np);
out_put_reg_clk:
--
2.1.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-16 10:08 ` [RESEND 2] " Lucas Stach
@ 2014-10-21 14:19 ` Rafael J. Wysocki
2014-10-23 9:19 ` Geert Uytterhoeven
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-21 14:19 UTC (permalink / raw)
To: Lucas Stach; +Cc: Viresh Kumar, linux-pm
On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> If the regulator connected to the CPU voltage plane doesn't
> support an OPP specified voltage with the acceptable tolerance
> it's better to just disable the OPP instead of constantly
> failing the voltage scaling later on.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Applied, thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-21 14:19 ` Rafael J. Wysocki
@ 2014-10-23 9:19 ` Geert Uytterhoeven
2014-10-23 14:10 ` Lucas Stach
0 siblings, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2014-10-23 9:19 UTC (permalink / raw)
To: Rafael J. Wysocki, Lucas Stach; +Cc: Viresh Kumar, Linux PM list, Linux-sh list
Hi Rafael, Lucas,
On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
>> If the regulator connected to the CPU voltage plane doesn't
>> support an OPP specified voltage with the acceptable tolerance
>> it's better to just disable the OPP instead of constantly
>> failing the voltage scaling later on.
>>
>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Applied, thanks!
This commit
(http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
causes a boot regression on r8a7791/koelsch. It hangs after:
TCP: cubic registered
Initializing XFRM netlink socket
NET: Registered protocol family 17
NET: Registered protocol family 15
ata1: link resume succeeded after 1 retries
ata1: SATA link down (SStatus 0 SControl 300)
random: nonblocking pool is initialized
With more debugging, it seems to end up in an infinite loop
calling runtime_{suspend,resume}():
cpufreq-dt cpufreq-dt: pm_clk_notify() 4
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
MSTP i2c6 ON
i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
MSTP i2c6 OFF
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
MSTP i2c6 ON
i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
MSTP i2c6 OFF
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
MSTP i2c6 ON
i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
MSTP i2c6 OFF
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
MSTP i2c6 ON
i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
MSTP i2c6 OFF
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
...
Reverting this commit fixes the issue, and makes the boot continue with:
cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
changed to: 1312500 KHz
cpu cpu1: failed to get cpu-2 clock: 1
cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 9:19 ` Geert Uytterhoeven
@ 2014-10-23 14:10 ` Lucas Stach
2014-10-23 14:43 ` Geert Uytterhoeven
0 siblings, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-23 14:10 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Hi Geert,
Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> Hi Rafael, Lucas,
>
> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> >> If the regulator connected to the CPU voltage plane doesn't
> >> support an OPP specified voltage with the acceptable tolerance
> >> it's better to just disable the OPP instead of constantly
> >> failing the voltage scaling later on.
> >>
> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > Applied, thanks!
>
> This commit
> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> causes a boot regression on r8a7791/koelsch. It hangs after:
>
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> NET: Registered protocol family 15
> ata1: link resume succeeded after 1 retries
> ata1: SATA link down (SStatus 0 SControl 300)
> random: nonblocking pool is initialized
>
> With more debugging, it seems to end up in an infinite loop
> calling runtime_{suspend,resume}():
>
> cpufreq-dt cpufreq-dt: pm_clk_notify() 4
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> MSTP i2c6 ON
> i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
> MSTP i2c6 OFF
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> MSTP i2c6 ON
> i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
> MSTP i2c6 OFF
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> MSTP i2c6 ON
> i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
> MSTP i2c6 OFF
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> MSTP i2c6 ON
> i2c-sh_mobile e60b0000.i2c: pm_clk_suspend()
> MSTP i2c6 OFF
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> ...
>
> Reverting this commit fixes the issue, and makes the boot continue with:
>
> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> changed to: 1312500 KHz
> cpu cpu1: failed to get cpu-2 clock: 1
> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
>
Urgh, thanks for the report. Am I right that for koelsch you do
reference a regulator supply for the cpu, but don't actually have a
driver for it, so a dummy regulator gets plugged in there?
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 14:10 ` Lucas Stach
@ 2014-10-23 14:43 ` Geert Uytterhoeven
2014-10-23 15:13 ` Lucas Stach
2014-10-24 10:19 ` Lucas Stach
0 siblings, 2 replies; 22+ messages in thread
From: Geert Uytterhoeven @ 2014-10-23 14:43 UTC (permalink / raw)
To: Lucas Stach; +Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Hi Lucas,
On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
>> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
>> >> If the regulator connected to the CPU voltage plane doesn't
>> >> support an OPP specified voltage with the acceptable tolerance
>> >> it's better to just disable the OPP instead of constantly
>> >> failing the voltage scaling later on.
>> >>
>> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >
>> > Applied, thanks!
>>
>> This commit
>> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
>> causes a boot regression on r8a7791/koelsch. It hangs after:
>>
>> TCP: cubic registered
>> Initializing XFRM netlink socket
>> NET: Registered protocol family 17
>> NET: Registered protocol family 15
>> ata1: link resume succeeded after 1 retries
>> ata1: SATA link down (SStatus 0 SControl 300)
>> random: nonblocking pool is initialized
>> Reverting this commit fixes the issue, and makes the boot continue with:
>>
>> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
>> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
>> changed to: 1312500 KHz
>> cpu cpu1: failed to get cpu-2 clock: 1
>> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
>>
>
> Urgh, thanks for the report. Am I right that for koelsch you do
> reference a regulator supply for the cpu, but don't actually have a
> driver for it, so a dummy regulator gets plugged in there?
arch/arm/boot/dts/r8a7791-koelsch.dts has:
&cpu0 {
cpu0-supply = <&vdd_dvfs>;
};
&i2c6 {
status = "okay";
clock-frequency = <100000>;
vdd_dvfs: regulator@68 {
compatible = "dlg,da9210";
reg = <0x68>;
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1000000>;
regulator-boot-on;
regulator-always-on;
};
};
CONFIG_REGULATOR_DA9210=y
and the driver does seem to run:
DA9210: 1000 mV at 4600 mA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 14:43 ` Geert Uytterhoeven
@ 2014-10-23 15:13 ` Lucas Stach
2014-10-23 21:26 ` Rafael J. Wysocki
2014-10-24 10:19 ` Lucas Stach
1 sibling, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-23 15:13 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> Hi Lucas,
>
> On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> >> >> If the regulator connected to the CPU voltage plane doesn't
> >> >> support an OPP specified voltage with the acceptable tolerance
> >> >> it's better to just disable the OPP instead of constantly
> >> >> failing the voltage scaling later on.
> >> >>
> >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> >
> >> > Applied, thanks!
> >>
> >> This commit
> >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> >> causes a boot regression on r8a7791/koelsch. It hangs after:
> >>
> >> TCP: cubic registered
> >> Initializing XFRM netlink socket
> >> NET: Registered protocol family 17
> >> NET: Registered protocol family 15
> >> ata1: link resume succeeded after 1 retries
> >> ata1: SATA link down (SStatus 0 SControl 300)
> >> random: nonblocking pool is initialized
>
> >> Reverting this commit fixes the issue, and makes the boot continue with:
> >>
> >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> >> changed to: 1312500 KHz
> >> cpu cpu1: failed to get cpu-2 clock: 1
> >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
> >>
> >
> > Urgh, thanks for the report. Am I right that for koelsch you do
> > reference a regulator supply for the cpu, but don't actually have a
> > driver for it, so a dummy regulator gets plugged in there?
>
> arch/arm/boot/dts/r8a7791-koelsch.dts has:
>
> &cpu0 {
> cpu0-supply = <&vdd_dvfs>;
> };
>
> &i2c6 {
> status = "okay";
> clock-frequency = <100000>;
>
> vdd_dvfs: regulator@68 {
> compatible = "dlg,da9210";
> reg = <0x68>;
>
> regulator-min-microvolt = <1000000>;
> regulator-max-microvolt = <1000000>;
> regulator-boot-on;
> regulator-always-on;
> };
> };
>
> CONFIG_REGULATOR_DA9210=y
>
> and the driver does seem to run:
>
> DA9210: 1000 mV at 4600 mA
>
Ah right, I misread your initial report. So the issue doesn't seem to be
directly related to the cpufreq-dt driver but seems to be located
somewhere deeper down the chain. The fact that this change triggers the
bug may be a hint here. The only thing which is new in the change is
that it tries to get the supported voltage from the regulator. As the
regulator can not change its voltage (min_uV == max_uV) this translates
directly to a get_voltage() which in turn only tries to read a register
of the i2c chip via regmap.
So it seems that somehow the i2c transaction fails and things spin
indefinitely there. Maybe this gives a clue on how to debug this
further.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 15:13 ` Lucas Stach
@ 2014-10-23 21:26 ` Rafael J. Wysocki
2014-10-24 0:26 ` Khiem Nguyen
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-23 21:26 UTC (permalink / raw)
To: Lucas Stach
Cc: Geert Uytterhoeven, Viresh Kumar, Linux PM list, Linux-sh list
On Thursday, October 23, 2014 05:13:02 PM Lucas Stach wrote:
> Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> > Hi Lucas,
> >
> > On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> > >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> > >> >> If the regulator connected to the CPU voltage plane doesn't
> > >> >> support an OPP specified voltage with the acceptable tolerance
> > >> >> it's better to just disable the OPP instead of constantly
> > >> >> failing the voltage scaling later on.
> > >> >>
> > >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >> >
> > >> > Applied, thanks!
> > >>
> > >> This commit
> > >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> > >> causes a boot regression on r8a7791/koelsch. It hangs after:
> > >>
> > >> TCP: cubic registered
> > >> Initializing XFRM netlink socket
> > >> NET: Registered protocol family 17
> > >> NET: Registered protocol family 15
> > >> ata1: link resume succeeded after 1 retries
> > >> ata1: SATA link down (SStatus 0 SControl 300)
> > >> random: nonblocking pool is initialized
> >
> > >> Reverting this commit fixes the issue, and makes the boot continue with:
> > >>
> > >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> > >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> > >> changed to: 1312500 KHz
> > >> cpu cpu1: failed to get cpu-2 clock: 1
> > >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
> > >>
> > >
> > > Urgh, thanks for the report. Am I right that for koelsch you do
> > > reference a regulator supply for the cpu, but don't actually have a
> > > driver for it, so a dummy regulator gets plugged in there?
> >
> > arch/arm/boot/dts/r8a7791-koelsch.dts has:
> >
> > &cpu0 {
> > cpu0-supply = <&vdd_dvfs>;
> > };
> >
> > &i2c6 {
> > status = "okay";
> > clock-frequency = <100000>;
> >
> > vdd_dvfs: regulator@68 {
> > compatible = "dlg,da9210";
> > reg = <0x68>;
> >
> > regulator-min-microvolt = <1000000>;
> > regulator-max-microvolt = <1000000>;
> > regulator-boot-on;
> > regulator-always-on;
> > };
> > };
> >
> > CONFIG_REGULATOR_DA9210=y
> >
> > and the driver does seem to run:
> >
> > DA9210: 1000 mV at 4600 mA
> >
> Ah right, I misread your initial report. So the issue doesn't seem to be
> directly related to the cpufreq-dt driver but seems to be located
> somewhere deeper down the chain. The fact that this change triggers the
> bug may be a hint here. The only thing which is new in the change is
> that it tries to get the supported voltage from the regulator. As the
> regulator can not change its voltage (min_uV == max_uV) this translates
> directly to a get_voltage() which in turn only tries to read a register
> of the i2c chip via regmap.
>
> So it seems that somehow the i2c transaction fails and things spin
> indefinitely there. Maybe this gives a clue on how to debug this
> further.
Well, I've dropped the patch for now. The underlying issue needs to be fixed
before we can apply it again.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 21:26 ` Rafael J. Wysocki
@ 2014-10-24 0:26 ` Khiem Nguyen
0 siblings, 0 replies; 22+ messages in thread
From: Khiem Nguyen @ 2014-10-24 0:26 UTC (permalink / raw)
To: Rafael J. Wysocki, Lucas Stach
Cc: khiem.nguyen.xt, Geert Uytterhoeven, Viresh Kumar, Linux PM list,
Linux-sh list, Gaku Inami
++Inami-san, author of recent CPUFreq patches.
On 10/24/2014 6:26 AM, Rafael J. Wysocki wrote:
> On Thursday, October 23, 2014 05:13:02 PM Lucas Stach wrote:
>> Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
>>> Hi Lucas,
>>>
>>> On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>>>> Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
>>>>> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>> On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
>>>>>>> If the regulator connected to the CPU voltage plane doesn't
>>>>>>> support an OPP specified voltage with the acceptable tolerance
>>>>>>> it's better to just disable the OPP instead of constantly
>>>>>>> failing the voltage scaling later on.
>>>>>>>
>>>>>>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>
>>>>>> Applied, thanks!
>>>>>
>>>>> This commit
>>>>> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
>>>>> causes a boot regression on r8a7791/koelsch. It hangs after:
>>>>>
>>>>> TCP: cubic registered
>>>>> Initializing XFRM netlink socket
>>>>> NET: Registered protocol family 17
>>>>> NET: Registered protocol family 15
>>>>> ata1: link resume succeeded after 1 retries
>>>>> ata1: SATA link down (SStatus 0 SControl 300)
>>>>> random: nonblocking pool is initialized
>>>
>>>>> Reverting this commit fixes the issue, and makes the boot continue with:
>>>>>
>>>>> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
>>>>> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
>>>>> changed to: 1312500 KHz
>>>>> cpu cpu1: failed to get cpu-2 clock: 1
>>>>> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
>>>>>
>>>>
>>>> Urgh, thanks for the report. Am I right that for koelsch you do
>>>> reference a regulator supply for the cpu, but don't actually have a
>>>> driver for it, so a dummy regulator gets plugged in there?
>>>
>>> arch/arm/boot/dts/r8a7791-koelsch.dts has:
>>>
>>> &cpu0 {
>>> cpu0-supply = <&vdd_dvfs>;
>>> };
>>>
>>> &i2c6 {
>>> status = "okay";
>>> clock-frequency = <100000>;
>>>
>>> vdd_dvfs: regulator@68 {
>>> compatible = "dlg,da9210";
>>> reg = <0x68>;
>>>
>>> regulator-min-microvolt = <1000000>;
>>> regulator-max-microvolt = <1000000>;
>>> regulator-boot-on;
>>> regulator-always-on;
>>> };
>>> };
>>>
>>> CONFIG_REGULATOR_DA9210=y
>>>
>>> and the driver does seem to run:
>>>
>>> DA9210: 1000 mV at 4600 mA
>>>
>> Ah right, I misread your initial report. So the issue doesn't seem to be
>> directly related to the cpufreq-dt driver but seems to be located
>> somewhere deeper down the chain. The fact that this change triggers the
>> bug may be a hint here. The only thing which is new in the change is
>> that it tries to get the supported voltage from the regulator. As the
>> regulator can not change its voltage (min_uV == max_uV) this translates
>> directly to a get_voltage() which in turn only tries to read a register
>> of the i2c chip via regmap.
>>
>> So it seems that somehow the i2c transaction fails and things spin
>> indefinitely there. Maybe this gives a clue on how to debug this
>> further.
>
> Well, I've dropped the patch for now. The underlying issue needs to be fixed
> before we can apply it again.
>
>
--
Best regards,
KHIEM Nguyen
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-23 14:43 ` Geert Uytterhoeven
2014-10-23 15:13 ` Lucas Stach
@ 2014-10-24 10:19 ` Lucas Stach
2014-10-24 12:30 ` Geert Uytterhoeven
1 sibling, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-24 10:19 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Geert,
Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> Hi Lucas,
>
> On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> >> >> If the regulator connected to the CPU voltage plane doesn't
> >> >> support an OPP specified voltage with the acceptable tolerance
> >> >> it's better to just disable the OPP instead of constantly
> >> >> failing the voltage scaling later on.
> >> >>
> >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> >
> >> > Applied, thanks!
> >>
> >> This commit
> >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> >> causes a boot regression on r8a7791/koelsch. It hangs after:
> >>
> >> TCP: cubic registered
> >> Initializing XFRM netlink socket
> >> NET: Registered protocol family 17
> >> NET: Registered protocol family 15
> >> ata1: link resume succeeded after 1 retries
> >> ata1: SATA link down (SStatus 0 SControl 300)
> >> random: nonblocking pool is initialized
>
> >> Reverting this commit fixes the issue, and makes the boot continue with:
> >>
> >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> >> changed to: 1312500 KHz
> >> cpu cpu1: failed to get cpu-2 clock: 1
> >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
> >>
> >
I thought a bit more about about this to make sure this isn't a fault on
my side, but can't seem to make any sense out of this. Can you please
print out the value of opp_freq in each iteration of the while loop and
also the return value of regulator_is_supported_voltage()? This would
help me a lot to understand what's happening here.
Thanks,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-24 10:19 ` Lucas Stach
@ 2014-10-24 12:30 ` Geert Uytterhoeven
2014-10-24 12:39 ` Lucas Stach
0 siblings, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2014-10-24 12:30 UTC (permalink / raw)
To: Lucas Stach; +Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Hi Lucas,
On Fri, 24 Oct 2014, Lucas Stach wrote:
> Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> > On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> > >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> > >> >> If the regulator connected to the CPU voltage plane doesn't
> > >> >> support an OPP specified voltage with the acceptable tolerance
> > >> >> it's better to just disable the OPP instead of constantly
> > >> >> failing the voltage scaling later on.
> > >> >>
> > >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >> >
> > >> > Applied, thanks!
> > >>
> > >> This commit
> > >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> > >> causes a boot regression on r8a7791/koelsch. It hangs after:
> > >>
> > >> TCP: cubic registered
> > >> Initializing XFRM netlink socket
> > >> NET: Registered protocol family 17
> > >> NET: Registered protocol family 15
> > >> ata1: link resume succeeded after 1 retries
> > >> ata1: SATA link down (SStatus 0 SControl 300)
> > >> random: nonblocking pool is initialized
> >
> > >> Reverting this commit fixes the issue, and makes the boot continue with:
> > >>
> > >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> > >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> > >> changed to: 1312500 KHz
> > >> cpu cpu1: failed to get cpu-2 clock: 1
> > >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
>
> I thought a bit more about about this to make sure this isn't a fault on
> my side, but can't seem to make any sense out of this. Can you please
> print out the value of opp_freq in each iteration of the while loop and
> also the return value of regulator_is_supported_voltage()? This would
> help me a lot to understand what's happening here.
It gets into an infinite loop doing:
cpufreq_dt: cpufreq_init:232: opp_freq = 0
cpufreq_dt: cpufreq_init:240: opp_freq = 375000000
cpufreq_dt: cpufreq_init:247: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:258: opp_freq = 375000001
...
The loop is only aborted if dev_pm_opp_find_freq_ceil() returns an error,
but that never happens.
I think it should try the next frequency on each subsequent iteration, right?
So I came up with the fix below. After that it behaves better:
cpufreq_dt: cpufreq_init:233: opp_freq = 0
cpufreq_dt: cpufreq_init:241: opp_freq = 375000000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 375000001
cpufreq_dt: cpufreq_init:233: opp_freq = 375000001
cpufreq_dt: cpufreq_init:241: opp_freq = 750000000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 750000001
cpufreq_dt: cpufreq_init:233: opp_freq = 750000001
cpufreq_dt: cpufreq_init:241: opp_freq = 937500000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 937500001
cpufreq_dt: cpufreq_init:233: opp_freq = 937500001
cpufreq_dt: cpufreq_init:241: opp_freq = 1125000000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 1125000001
cpufreq_dt: cpufreq_init:233: opp_freq = 1125000001
cpufreq_dt: cpufreq_init:241: opp_freq = 1312500000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 1312500001
cpufreq_dt: cpufreq_init:233: opp_freq = 1312500001
cpufreq_dt: cpufreq_init:241: opp_freq = 1500000000
cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
cpufreq_dt: cpufreq_init:259: opp_freq = 1500000001
cpufreq_dt: cpufreq_init:233: opp_freq = 1500000001
cpufreq_dt: cpufreq_init:237: Error -34 => break
From e5f401eee8b316587fc385cac7acdd92b33adec7 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Fri, 24 Oct 2014 14:23:14 +0200
Subject: [PATCH] cpufreq: dt: Don't reset opp_freq in subsequent loop
iterations
opp_freq is incremented at the end of the loop, to find the next
supported frequency, but this is overturned by its re-initialization
in next iteration, causing an infinite loop.
Move the initialization of opp_freq outside the loop to fix this.
Fixes: d7bbd4cd0359d781 ("cpufreq: dt: disable unsupported OPPs")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/cpufreq/cpufreq-dt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index f6e39a2e324ffc87..2f25958053778ad1 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -219,6 +219,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
transition_latency = CPUFREQ_ETERNAL;
if (!IS_ERR(cpu_reg)) {
+ unsigned long opp_freq = 0;
+
/*
* Disable any OPPs where the connected regulator isn't able to
* provide the specified voltage and record minimum and maximum
@@ -226,7 +228,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
*/
while (1) {
struct dev_pm_opp *opp;
- unsigned long opp_freq = 0, opp_uV, tol_uV;
+ unsigned long opp_uV, tol_uV;
rcu_read_lock();
opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
--
1.9.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-24 12:30 ` Geert Uytterhoeven
@ 2014-10-24 12:39 ` Lucas Stach
2014-10-24 13:01 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: Lucas Stach @ 2014-10-24 12:39 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM list, Linux-sh list
Am Freitag, den 24.10.2014, 14:30 +0200 schrieb Geert Uytterhoeven:
> Hi Lucas,
>
> On Fri, 24 Oct 2014, Lucas Stach wrote:
> > Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> > > On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> > > >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> > > >> >> If the regulator connected to the CPU voltage plane doesn't
> > > >> >> support an OPP specified voltage with the acceptable tolerance
> > > >> >> it's better to just disable the OPP instead of constantly
> > > >> >> failing the voltage scaling later on.
> > > >> >>
> > > >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > >> >
> > > >> > Applied, thanks!
> > > >>
> > > >> This commit
> > > >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> > > >> causes a boot regression on r8a7791/koelsch. It hangs after:
> > > >>
> > > >> TCP: cubic registered
> > > >> Initializing XFRM netlink socket
> > > >> NET: Registered protocol family 17
> > > >> NET: Registered protocol family 15
> > > >> ata1: link resume succeeded after 1 retries
> > > >> ata1: SATA link down (SStatus 0 SControl 300)
> > > >> random: nonblocking pool is initialized
> > >
> > > >> Reverting this commit fixes the issue, and makes the boot continue with:
> > > >>
> > > >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> > > >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> > > >> changed to: 1312500 KHz
> > > >> cpu cpu1: failed to get cpu-2 clock: 1
> > > >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
> >
> > I thought a bit more about about this to make sure this isn't a fault on
> > my side, but can't seem to make any sense out of this. Can you please
> > print out the value of opp_freq in each iteration of the while loop and
> > also the return value of regulator_is_supported_voltage()? This would
> > help me a lot to understand what's happening here.
>
> It gets into an infinite loop doing:
>
> cpufreq_dt: cpufreq_init:232: opp_freq = 0
> cpufreq_dt: cpufreq_init:240: opp_freq = 375000000
> cpufreq_dt: cpufreq_init:247: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:258: opp_freq = 375000001
> ...
>
> The loop is only aborted if dev_pm_opp_find_freq_ceil() returns an error,
> but that never happens.
>
> I think it should try the next frequency on each subsequent iteration, right?
> So I came up with the fix below. After that it behaves better:
>
> cpufreq_dt: cpufreq_init:233: opp_freq = 0
> cpufreq_dt: cpufreq_init:241: opp_freq = 375000000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 375000001
> cpufreq_dt: cpufreq_init:233: opp_freq = 375000001
> cpufreq_dt: cpufreq_init:241: opp_freq = 750000000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 750000001
> cpufreq_dt: cpufreq_init:233: opp_freq = 750000001
> cpufreq_dt: cpufreq_init:241: opp_freq = 937500000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 937500001
> cpufreq_dt: cpufreq_init:233: opp_freq = 937500001
> cpufreq_dt: cpufreq_init:241: opp_freq = 1125000000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 1125000001
> cpufreq_dt: cpufreq_init:233: opp_freq = 1125000001
> cpufreq_dt: cpufreq_init:241: opp_freq = 1312500000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 1312500001
> cpufreq_dt: cpufreq_init:233: opp_freq = 1312500001
> cpufreq_dt: cpufreq_init:241: opp_freq = 1500000000
> cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> cpufreq_dt: cpufreq_init:259: opp_freq = 1500000001
> cpufreq_dt: cpufreq_init:233: opp_freq = 1500000001
> cpufreq_dt: cpufreq_init:237: Error -34 => break
>
> From e5f401eee8b316587fc385cac7acdd92b33adec7 Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> Date: Fri, 24 Oct 2014 14:23:14 +0200
> Subject: [PATCH] cpufreq: dt: Don't reset opp_freq in subsequent loop
> iterations
>
> opp_freq is incremented at the end of the loop, to find the next
> supported frequency, but this is overturned by its re-initialization
> in next iteration, causing an infinite loop.
>
> Move the initialization of opp_freq outside the loop to fix this.
>
> Fixes: d7bbd4cd0359d781 ("cpufreq: dt: disable unsupported OPPs")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> drivers/cpufreq/cpufreq-dt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index f6e39a2e324ffc87..2f25958053778ad1 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -219,6 +219,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> transition_latency = CPUFREQ_ETERNAL;
>
> if (!IS_ERR(cpu_reg)) {
> + unsigned long opp_freq = 0;
> +
> /*
> * Disable any OPPs where the connected regulator isn't able to
> * provide the specified voltage and record minimum and maximum
> @@ -226,7 +228,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> */
> while (1) {
> struct dev_pm_opp *opp;
> - unsigned long opp_freq = 0, opp_uV, tol_uV;
> + unsigned long opp_uV, tol_uV;
>
> rcu_read_lock();
> opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
> --
> 1.9.1
>
*puts on brown paper bag*
Right, the fixed behavior with your patch was clearly the intention,
which may be the cause it didn't see when staring at the code.
I remember moving the variable initialization at one point, but somehow
this slipped through my testing. Thanks for hunting it down.
Rafael, how do we handle this? Are you going to reapply the patch
together with this fix, or want me to send an updated version with this
fix included?
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND 2] cpufreq: dt: disable unsupported OPPs
2014-10-24 12:39 ` Lucas Stach
@ 2014-10-24 13:01 ` Rafael J. Wysocki
0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2014-10-24 13:01 UTC (permalink / raw)
To: Lucas Stach
Cc: Geert Uytterhoeven, Viresh Kumar, Linux PM list, Linux-sh list
On Friday, October 24, 2014 02:39:11 PM Lucas Stach wrote:
> Am Freitag, den 24.10.2014, 14:30 +0200 schrieb Geert Uytterhoeven:
> > Hi Lucas,
> >
> > On Fri, 24 Oct 2014, Lucas Stach wrote:
> > > Am Donnerstag, den 23.10.2014, 16:43 +0200 schrieb Geert Uytterhoeven:
> > > > On Thu, Oct 23, 2014 at 4:10 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > Am Donnerstag, den 23.10.2014, 11:19 +0200 schrieb Geert Uytterhoeven:
> > > > >> On Tue, Oct 21, 2014 at 4:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >> > On Thursday, October 16, 2014 12:08:20 PM Lucas Stach wrote:
> > > > >> >> If the regulator connected to the CPU voltage plane doesn't
> > > > >> >> support an OPP specified voltage with the acceptable tolerance
> > > > >> >> it's better to just disable the OPP instead of constantly
> > > > >> >> failing the voltage scaling later on.
> > > > >> >>
> > > > >> >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > >> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > >> >
> > > > >> > Applied, thanks!
> > > > >>
> > > > >> This commit
> > > > >> (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=d7bbd4cd0359d781b67c9e621d4bbfd1bb2f3783)
> > > > >> causes a boot regression on r8a7791/koelsch. It hangs after:
> > > > >>
> > > > >> TCP: cubic registered
> > > > >> Initializing XFRM netlink socket
> > > > >> NET: Registered protocol family 17
> > > > >> NET: Registered protocol family 15
> > > > >> ata1: link resume succeeded after 1 retries
> > > > >> ata1: SATA link down (SStatus 0 SControl 300)
> > > > >> random: nonblocking pool is initialized
> > > >
> > > > >> Reverting this commit fixes the issue, and makes the boot continue with:
> > > > >>
> > > > >> cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 1300000 KHz
> > > > >> cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency
> > > > >> changed to: 1312500 KHz
> > > > >> cpu cpu1: failed to get cpu-2 clock: 1
> > > > >> cpufreq_dt: cpufreq_init: Failed to allocate resources: -2
> > >
> > > I thought a bit more about about this to make sure this isn't a fault on
> > > my side, but can't seem to make any sense out of this. Can you please
> > > print out the value of opp_freq in each iteration of the while loop and
> > > also the return value of regulator_is_supported_voltage()? This would
> > > help me a lot to understand what's happening here.
> >
> > It gets into an infinite loop doing:
> >
> > cpufreq_dt: cpufreq_init:232: opp_freq = 0
> > cpufreq_dt: cpufreq_init:240: opp_freq = 375000000
> > cpufreq_dt: cpufreq_init:247: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:258: opp_freq = 375000001
> > ...
> >
> > The loop is only aborted if dev_pm_opp_find_freq_ceil() returns an error,
> > but that never happens.
> >
> > I think it should try the next frequency on each subsequent iteration, right?
> > So I came up with the fix below. After that it behaves better:
> >
> > cpufreq_dt: cpufreq_init:233: opp_freq = 0
> > cpufreq_dt: cpufreq_init:241: opp_freq = 375000000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 375000001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 375000001
> > cpufreq_dt: cpufreq_init:241: opp_freq = 750000000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 750000001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 750000001
> > cpufreq_dt: cpufreq_init:241: opp_freq = 937500000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 937500001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 937500001
> > cpufreq_dt: cpufreq_init:241: opp_freq = 1125000000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 1125000001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 1125000001
> > cpufreq_dt: cpufreq_init:241: opp_freq = 1312500000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 1312500001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 1312500001
> > cpufreq_dt: cpufreq_init:241: opp_freq = 1500000000
> > cpufreq_dt: cpufreq_init:248: [ 1000000, 1010000 ] is supported
> > cpufreq_dt: cpufreq_init:259: opp_freq = 1500000001
> > cpufreq_dt: cpufreq_init:233: opp_freq = 1500000001
> > cpufreq_dt: cpufreq_init:237: Error -34 => break
> >
> > From e5f401eee8b316587fc385cac7acdd92b33adec7 Mon Sep 17 00:00:00 2001
> > From: Geert Uytterhoeven <geert+renesas@glider.be>
> > Date: Fri, 24 Oct 2014 14:23:14 +0200
> > Subject: [PATCH] cpufreq: dt: Don't reset opp_freq in subsequent loop
> > iterations
> >
> > opp_freq is incremented at the end of the loop, to find the next
> > supported frequency, but this is overturned by its re-initialization
> > in next iteration, causing an infinite loop.
> >
> > Move the initialization of opp_freq outside the loop to fix this.
> >
> > Fixes: d7bbd4cd0359d781 ("cpufreq: dt: disable unsupported OPPs")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > drivers/cpufreq/cpufreq-dt.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> > index f6e39a2e324ffc87..2f25958053778ad1 100644
> > --- a/drivers/cpufreq/cpufreq-dt.c
> > +++ b/drivers/cpufreq/cpufreq-dt.c
> > @@ -219,6 +219,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> > transition_latency = CPUFREQ_ETERNAL;
> >
> > if (!IS_ERR(cpu_reg)) {
> > + unsigned long opp_freq = 0;
> > +
> > /*
> > * Disable any OPPs where the connected regulator isn't able to
> > * provide the specified voltage and record minimum and maximum
> > @@ -226,7 +228,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> > */
> > while (1) {
> > struct dev_pm_opp *opp;
> > - unsigned long opp_freq = 0, opp_uV, tol_uV;
> > + unsigned long opp_uV, tol_uV;
> >
> > rcu_read_lock();
> > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
>
> *puts on brown paper bag*
>
> Right, the fixed behavior with your patch was clearly the intention,
> which may be the cause it didn't see when staring at the code.
> I remember moving the variable initialization at one point, but somehow
> this slipped through my testing. Thanks for hunting it down.
>
> Rafael, how do we handle this? Are you going to reapply the patch
> together with this fix, or want me to send an updated version with this
> fix included?
An update, please. Plus say in the changelog that this includes a fix from
Geert.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-10-24 13:01 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-30 15:29 [PATCH resend] cpufreq: dt: disable unsupported OPPs Lucas Stach
2014-09-30 19:53 ` Rafael J. Wysocki
2014-10-01 3:29 ` Viresh Kumar
2014-10-01 20:39 ` Rafael J. Wysocki
2014-10-02 5:24 ` Viresh Kumar
2014-10-02 11:57 ` Lucas Stach
2014-10-02 17:33 ` Rafael J. Wysocki
2014-10-08 22:36 ` Rafael J. Wysocki
2014-10-09 3:43 ` Viresh Kumar
2014-10-12 20:27 ` Rafael J. Wysocki
2014-10-16 10:08 ` [RESEND 2] " Lucas Stach
2014-10-21 14:19 ` Rafael J. Wysocki
2014-10-23 9:19 ` Geert Uytterhoeven
2014-10-23 14:10 ` Lucas Stach
2014-10-23 14:43 ` Geert Uytterhoeven
2014-10-23 15:13 ` Lucas Stach
2014-10-23 21:26 ` Rafael J. Wysocki
2014-10-24 0:26 ` Khiem Nguyen
2014-10-24 10:19 ` Lucas Stach
2014-10-24 12:30 ` Geert Uytterhoeven
2014-10-24 12:39 ` Lucas Stach
2014-10-24 13:01 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).