[04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings

Message ID 1f876a6d176784f9ab901aea7ff521d53522682d.1481015522.git.viresh.kumar@linaro.org
State Superseded
Headers show

Commit Message

Viresh Kumar Dec. 6, 2016, 9:15 a.m.
The code adding static OPPs for V2 bindings already does so. Make the V1
bindings specific code behave the same.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/base/power/opp/of.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.7.1.410.g6faf27b

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephen Boyd Dec. 7, 2016, 1:17 a.m. | #1
On 12/06, Viresh Kumar wrote:
> The code adding static OPPs for V2 bindings already does so. Make the V1

> bindings specific code behave the same.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---


Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>


It looks like that goes back to the original DT OPP binding
support written by Shawn Guo. Any idea why we kept trying to add
more OPPs if it failed to add one? Best effort or something?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 7, 2016, 3:25 a.m. | #2
On 06-12-16, 17:17, Stephen Boyd wrote:
> On 12/06, Viresh Kumar wrote:

> > The code adding static OPPs for V2 bindings already does so. Make the V1

> > bindings specific code behave the same.

> > 

> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > ---

> 

> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> 

> It looks like that goes back to the original DT OPP binding

> support written by Shawn Guo. Any idea why we kept trying to add

> more OPPs if it failed to add one? Best effort or something?


Not sure, maybe for handling duplicate OPPs.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Dec. 7, 2016, 9:13 p.m. | #3
On 12/07, Viresh Kumar wrote:
> On 06-12-16, 17:17, Stephen Boyd wrote:

> > On 12/06, Viresh Kumar wrote:

> > > The code adding static OPPs for V2 bindings already does so. Make the V1

> > > bindings specific code behave the same.

> > > 

> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > ---

> > 

> > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> > 

> > It looks like that goes back to the original DT OPP binding

> > support written by Shawn Guo. Any idea why we kept trying to add

> > more OPPs if it failed to add one? Best effort or something?

> 

> Not sure, maybe for handling duplicate OPPs.

> 


So perhaps we should Cc Shawn Guo who wrote the original code
then?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 8, 2016, 3:30 a.m. | #4
Hi Shawn,

On 07-12-16, 13:13, Stephen Boyd wrote:
> On 12/07, Viresh Kumar wrote:

> > On 06-12-16, 17:17, Stephen Boyd wrote:

> > > On 12/06, Viresh Kumar wrote:

> > > > The code adding static OPPs for V2 bindings already does so. Make the V1

> > > > bindings specific code behave the same.

> > > > 

> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > > ---

> > > 

> > > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> > > 

> > > It looks like that goes back to the original DT OPP binding

> > > support written by Shawn Guo. Any idea why we kept trying to add

> > > more OPPs if it failed to add one? Best effort or something?


Can you please answer this question from Stephen?

> > Not sure, maybe for handling duplicate OPPs.

> > 

> 

> So perhaps we should Cc Shawn Guo who wrote the original code

> then?


I wouldn't mind doing that, but I don't think it needs to be a blocker for now.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Dec. 8, 2016, 6:39 a.m. | #5
On Thu, Dec 08, 2016 at 09:00:59AM +0530, Viresh Kumar wrote:
> Hi Shawn,

> 

> On 07-12-16, 13:13, Stephen Boyd wrote:

> > On 12/07, Viresh Kumar wrote:

> > > On 06-12-16, 17:17, Stephen Boyd wrote:

> > > > On 12/06, Viresh Kumar wrote:

> > > > > The code adding static OPPs for V2 bindings already does so. Make the V1

> > > > > bindings specific code behave the same.

> > > > > 

> > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > > > ---

> > > > 

> > > > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> > > > 

> > > > It looks like that goes back to the original DT OPP binding

> > > > support written by Shawn Guo. Any idea why we kept trying to add

> > > > more OPPs if it failed to add one? Best effort or something?

> 

> Can you please answer this question from Stephen?


Yes, it's the best effort.  For example, a duplicated OPP shouldn't be a
stopper for the driver.

Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 8, 2016, 6:45 a.m. | #6
On 08-12-16, 14:39, Shawn Guo wrote:
> Yes, it's the best effort.


That may get a risky IMO. What if we fail to add all OPPs except the last one,
which selects the highest frequency? We will surely overheat most of the SoCs
and burn them out.

I think its better to fail and return in such cases.

> For example, a duplicated OPP shouldn't be a

> stopper for the driver.


That's a different case and is already supported properly, as we wouldn't fail
in that case.

Thanks for your inputs Shawn.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Dec. 8, 2016, 2:27 p.m. | #7
On Thu, Dec 08, 2016 at 12:15:13PM +0530, Viresh Kumar wrote:
> On 08-12-16, 14:39, Shawn Guo wrote:

> > Yes, it's the best effort.

> 

> That may get a risky IMO. What if we fail to add all OPPs except the last one,

> which selects the highest frequency?


That's the same case when Performance governor is selected, and should
be guaranteed safe, shouldn't it?

> We will surely overheat most of the SoCs

> and burn them out.


IMO, the problem is on OPP table.  Every entry in the table should be
safe for running for any time.

Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 8, 2016, 2:47 p.m. | #8
On 8 December 2016 at 19:57, Shawn Guo <shawnguo@kernel.org> wrote:
> That's the same case when Performance governor is selected, and should

> be guaranteed safe, shouldn't it?


Depends on platform I would say. For example for a big LITTLE platform, running
the big core on max not only increases the amount of power it uses but also the
amount of heat it generates, which is really scary.

Performance governor is fine for testing but it is not used for battery operated
devices in production code.

>> We will surely overheat most of the SoCs

>> and burn them out.

>

> IMO, the problem is on OPP table.  Every entry in the table should be

> safe for running for any time.


Not necessarily. The highest OPPs might be suggested to run only
for small period of time only. And there is nothing wrong in it.

Over that if some of the OPPs fail to get registered from the OPP
table in DT, it surely hints at some sort of bugs in the code or DT.

Why should we try to go ahead if only a part of the DT table gets
registered? I don't find a compelling reason for that. It will never
happen normally anyway, as we wouldn't have any errors.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index f8512ca2bf41..c8fe815774ff 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -433,7 +433,7 @@  static int _of_add_opp_table_v1(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
-	int nr;
+	int nr, ret;
 
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop)
@@ -456,9 +456,13 @@  static int _of_add_opp_table_v1(struct device *dev)
 		unsigned long freq = be32_to_cpup(val++) * 1000;
 		unsigned long volt = be32_to_cpup(val++);
 
-		if (_opp_add_v1(dev, freq, volt, false))
-			dev_warn(dev, "%s: Failed to add OPP %ld\n",
-				 __func__, freq);
+		ret = _opp_add_v1(dev, freq, volt, false);
+		if (ret) {
+			dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
+				__func__, freq, ret);
+			dev_pm_opp_of_remove_table(dev);
+			return ret;
+		}
 		nr -= 2;
 	}