| Message ID | 1f876a6d176784f9ab901aea7ff521d53522682d.1481015522.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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
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
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; }
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