On Tue, Jan 7, 2020 at 11:28 AM Sibi Sankar [off-list ref] wrote:

Hey Saravana,

Spent some time testing this series while
trying out dcvs on SDM845/SC7180. Apart from
the few minor issues it works quite well!

Thanks a lot for testing Sibi. Can you give a tested-by? Glad to hear
it works well.

On 2019-12-07 05:54, Saravana Kannan wrote:

quoted

Not all devices quantify their performance points in terms of
frequency.
Devices like interconnects quantify their performance points in terms
of
bandwidth. We need a way to represent these bandwidth levels in OPP.
So,
add support for parsing bandwidth OPPs from DT.

Signed-off-by: Saravana Kannan <redacted>
---
 drivers/opp/core.c | 15 +++++++++--
 drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
 drivers/opp/opp.h  |  5 ++++
 3 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index be7a7d332332..c79bbfac7289 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c

@@ -1282,11 +1282,21 @@ static bool

_opp_supported_by_regulators(struct dev_pm_opp *opp,
      return true;
 }

+int opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+{
+     if (opp1->rate != opp2->rate)
+             return opp1->rate < opp2->rate ? -1 : 1;
+     if (opp1->peak_bw != opp2->peak_bw)
+             return opp1->peak_bw < opp2->peak_bw ? -1 : 1;
+     return 0;
+}
+
 static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp
*new_opp,
                           struct opp_table *opp_table,
                           struct list_head **head)
 {
      struct dev_pm_opp *opp;
+     int opp_cmp;

      /*
       * Insert new OPP in order of increasing frequency and discard if

@@ -1297,12 +1307,13 @@ static int _opp_is_duplicate(struct device

*dev, struct dev_pm_opp *new_opp,
       * loop.
       */
      list_for_each_entry(opp, &opp_table->opp_list, node) {
-             if (new_opp->rate > opp->rate) {
+             opp_cmp = opp_compare_key(new_opp, opp);
+             if (opp_cmp > 0) {
                      *head = &opp->node;
                      continue;
              }

-             if (new_opp->rate < opp->rate)
+             if (opp_cmp < 0)
                      return 0;

              /* Duplicate OPPs */

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1cbb58240b80..b565da5a2b1f 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c

@@ -521,6 +521,44 @@ void dev_pm_opp_of_remove_table(struct device

*dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);

+static int _read_opp_key(struct dev_pm_opp *new_opp, struct
device_node *np,
+                      bool *rate_not_available)
+{
+     int ret;
+     u64 rate;
+     u32 bw;
+
+     ret = of_property_read_u64(np, "opp-hz", &rate);
+     if (!ret) {
+             /*
+              * Rate is defined as an unsigned long in clk API, and so
+              * casting explicitly to its type. Must be fixed once rate is 64
+              * bit guaranteed in clk API.
+              */
+             new_opp->rate = (unsigned long)rate;
+             goto out;
+     }
+
+     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
+     if (!ret) {
+             new_opp->peak_bw = bw;
+
+             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
+                     new_opp->avg_bw = bw;
+     }
+
+out:
+     *rate_not_available = !!ret;
+     /*
+      * If ret is 0 at this point, we have already found a key. If we
+      * haven't found a key yet, then ret already has an error value. In
+      * either case, we don't need to update ret.
+      */
+     of_property_read_u32(np, "opp-level", &new_opp->level);
+
+     return ret;
+}
+
 /**
  * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT
bindings)
  * @opp_table:       OPP table

@@ -558,26 +596,12 @@ static struct dev_pm_opp

*_opp_add_static_v2(struct opp_table *opp_table,
      if (!new_opp)
              return ERR_PTR(-ENOMEM);

-     ret = of_property_read_u64(np, "opp-hz", &rate);
-     if (ret < 0) {
-             /* "opp-hz" is optional for devices like power domains. */
-             if (!opp_table->is_genpd) {
-                     dev_err(dev, "%s: opp-hz not found\n", __func__);
-                     goto free_opp;
-             }
-
-             rate_not_available = true;
-     } else {
-             /*
-              * Rate is defined as an unsigned long in clk API, and so
-              * casting explicitly to its type. Must be fixed once rate is 64
-              * bit guaranteed in clk API.
-              */
-             new_opp->rate = (unsigned long)rate;
+     ret = _read_opp_key(new_opp, np, &rate_not_available);
+     if (ret) {

if (!opp_table->is_genpd) {

_read_opp_key returns an error for genpd
opps so please check if it is a genpd
opp_table before erroring out here.

Thanks. I'll fix it in the next version.

quoted

+             dev_err(dev, "%s: opp key field not found\n", __func__);
+             goto free_opp;
      }

-     of_property_read_u32(np, "opp-level", &new_opp->level);
-
      /* Check if the OPP supports hardware's hierarchy of versions or not
*/
      if (!_opp_is_supported(dev, opp_table, np)) {
              dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);

@@ -616,7 +640,8 @@ static struct dev_pm_opp

*_opp_add_static_v2(struct opp_table *opp_table,
      if (of_property_read_bool(np, "opp-suspend")) {
              if (opp_table->suspend_opp) {
                      /* Pick the OPP with higher rate as suspend OPP */
-                     if (new_opp->rate > opp_table->suspend_opp->rate) {
+                     if (opp_compare_key(new_opp,
+                                         opp_table->suspend_opp) > 1) {

shouldn't the condition be > 0?

Duh. Thanks. I'll fix it in the next version.

I'm guessing you tested with the fixes you pointed out?

-Saravana