[PATCH v6 0/3] Add required-opps support to devfreq passive gov

[PATCH v6 0/3] Add required-opps support to devfreq passive gov

[Hsin-Yi Wang]

The devfreq passive governor scales the frequency of a "child" device based
on the current frequency of a "parent" device (not parent/child in the
sense of device hierarchy). As of today, the passive governor requires one
of the following to work correctly:
1. The parent and child device have the same number of frequencies
2. The child device driver passes a mapping function to translate from
   parent frequency to child frequency.

When (1) is not true, (2) is the only option right now. But often times,
all that is required is a simple mapping from parent's frequency to child's
frequency.

Since OPPs already support pointing to other "required-opps", add support
for using that to map from parent device frequency to child device
frequency. That way, every child device driver doesn't have to implement a
separate mapping function anytime (1) isn't true.

Some common (but not comprehensive) reason for needing a devfreq passive
governor to adjust the frequency of one device based on another are:

1. These were the combination of frequencies that were validated/screened
   during the manufacturing process.
2. These are the sensible performance combinations between two devices
   interacting with each other. So that when one runs fast the other
   doesn't become the bottleneck.
3. Hardware bugs requiring some kind of frequency ratio between devices.

For example, the following mapping can't be captured in DT as it stands
today because the parent and child device have different number of OPPs.
But with this patch series, this mapping can be captured cleanly.

In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something
like this with the following changes:

        bus_g2d_400: bus0 {
                compatible = "samsung,exynos-bus";
                clocks = <&cmu_top CLK_ACLK_G2D_400>;
                clock-names = "bus";
                operating-points-v2 = <&bus_g2d_400_opp_table>;
                status = "disabled";
        };

        bus_noc2: bus9 {
                compatible = "samsung,exynos-bus";
                clocks = <&cmu_mif CLK_ACLK_BUS2_400>;
                clock-names = "bus";
                operating-points-v2 = <&bus_noc2_opp_table>;
                status = "disabled";
        };

        bus_g2d_400_opp_table: opp_table2 {
                compatible = "operating-points-v2";
                opp-shared;

                opp-400000000 {
                        opp-hz = /bits/ 64 <400000000>;
                        opp-microvolt = <1075000>;
                        required-opps = <&noc2_400>;
                };
                opp-267000000 {
                        opp-hz = /bits/ 64 <267000000>;
                        opp-microvolt = <1000000>;
                        required-opps = <&noc2_200>;
                };
                opp-200000000 {
                        opp-hz = /bits/ 64 <200000000>;
                        opp-microvolt = <975000>;
                        required-opps = <&noc2_200>;
                };
                opp-160000000 {
                        opp-hz = /bits/ 64 <160000000>;
                        opp-microvolt = <962500>;
                        required-opps = <&noc2_134>;
                };
                opp-134000000 {
                        opp-hz = /bits/ 64 <134000000>;
                        opp-microvolt = <950000>;
                        required-opps = <&noc2_134>;
                };
                opp-100000000 {
                        opp-hz = /bits/ 64 <100000000>;
                        opp-microvolt = <937500>;
                        required-opps = <&noc2_100>;
                };
        };

        bus_noc2_opp_table: opp_table6 {
                compatible = "operating-points-v2";

                noc2_400: opp-400000000 {
                        opp-hz = /bits/ 64 <400000000>;
                };
                noc2_200: opp-200000000 {
                        opp-hz = /bits/ 64 <200000000>;
                };
                noc2_134: opp-134000000 {
                        opp-hz = /bits/ 64 <134000000>;
                };
                noc2_100: opp-100000000 {
                        opp-hz = /bits/ 64 <100000000>;
                };
        };

-Saravana

v5 -> v6:
- fix review comments

v4 -> v5:
- drop patch "OPP: Improve required-opps linking" and rebase to
  https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next
- Compare pointers in dev_pm_opp_xlate_required_opp().

v3 -> v4:
- Fixed documentation comments
- Fixed order of functions in .h file
- Renamed the new xlate API
- Caused _set_required_opps() to fail if all required opps tables aren't
  linked.
v2 -> v3:
- Rebased onto linux-next.
- Added documentation comment for new fields.
- Added support for lazy required-opps linking.
- Updated Ack/Reviewed-bys.
v1 -> v2:
- Cached OPP table reference in devfreq to avoid looking up every time.
- Renamed variable in passive governor to be more intuitive.
- Updated cover letter with examples.

Saravana Kannan (3):
  OPP: Add function to look up required OPP's for a given OPP
  PM / devfreq: Cache OPP table reference in devfreq
  PM / devfreq: Add required OPPs support to passive governor

 drivers/devfreq/devfreq.c          |  6 +++
 drivers/devfreq/governor_passive.c | 25 +++++++++----
 drivers/opp/core.c                 | 59 ++++++++++++++++++++++++++++++
 include/linux/devfreq.h            |  2 +
 include/linux/pm_opp.h             |  7 ++++
 5 files changed, 92 insertions(+), 7 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH v6 1/3] OPP: Add function to look up required OPP's for a given OPP

[Hsin-Yi Wang]

From: Saravana Kannan <saravanak@google.com>

Add a function that allows looking up required OPPs given a source OPP
table, destination OPP table and the source OPP.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/opp/core.c     | 59 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  7 +++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index dc95d29e94c1b..fba67ae40aefc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2398,6 +2398,65 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names,
 }
 EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
 
+/**
+ * dev_pm_opp_xlate_required_opp() - Find required OPP for @src_table OPP.
+ * @src_table: OPP table which has @dst_table as one of its required OPP table.
+ * @dst_table: Required OPP table of the @src_table.
+ *
+ * This function returns the OPP (present in @dst_table) pointed out by the
+ * "required-opps" property of the OPP (present in @src_table).
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ *
+ * Return: destination table OPP on success, otherwise -EINVAL or -ENODEV based
+ * on errors.
+ */
+struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
+						 struct opp_table *dst_table,
+						 struct dev_pm_opp *src_opp)
+{
+	struct dev_pm_opp *opp, *dest_opp = ERR_PTR(-EINVAL);
+	int i;
+
+	if (!src_table || !dst_table || !src_opp ||
+	    !src_table->required_opp_tables)
+		return ERR_PTR(-EINVAL);
+
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(src_table))
+		return ERR_PTR(-EINVAL);
+
+	for (i = 0; i < src_table->required_opp_count; i++) {
+		if (src_table->required_opp_tables[i] == dst_table)
+			break;
+	}
+
+	if (unlikely(i == src_table->required_opp_count)) {
+		pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
+		       __func__, src_table, dst_table);
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_lock(&src_table->lock);
+
+	list_for_each_entry(opp, &src_table->opp_list, node) {
+		if (opp == src_opp) {
+			dest_opp = opp->required_opps[i];
+			dev_pm_opp_get(dest_opp);
+			goto unlock;
+		}
+	}
+
+	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
+	       dst_table);
+
+unlock:
+	mutex_unlock(&src_table->lock);
+
+	return dest_opp;
+}
+
 /**
  * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
  * @src_table: OPP table which has dst_table as one of its required OPP table.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index ab1d15ce559db..c0371efa4a0f2 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -156,6 +156,7 @@ struct opp_table *devm_pm_opp_register_set_opp_helper(struct device *dev, int (*
 struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
 struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
@@ -367,6 +368,12 @@ static inline struct opp_table *devm_pm_opp_attach_genpd(struct device *dev,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
+				struct opp_table *dst_table, struct dev_pm_opp *src_opp)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
 {
 	return -EOPNOTSUPP;
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v6 1/3] OPP: Add function to look up required OPP's for a given OPP

[Viresh Kumar]

On 04-02-21, 16:14, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> Add a function that allows looking up required OPPs given a source OPP
> table, destination OPP table and the source OPP.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/opp/core.c     | 59 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  7 +++++
>  2 files changed, 66 insertions(+)

I made some changes while applying:

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fba67ae40aef..c3f3d9249cc5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2402,21 +2402,21 @@ EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
  * dev_pm_opp_xlate_required_opp() - Find required OPP for @src_table OPP.
  * @src_table: OPP table which has @dst_table as one of its required OPP table.
  * @dst_table: Required OPP table of the @src_table.
+ * @src_opp: OPP from the @src_table.
  *
  * This function returns the OPP (present in @dst_table) pointed out by the
- * "required-opps" property of the OPP (present in @src_table).
+ * "required-opps" property of the @src_opp (present in @src_table).
  *
  * The callers are required to call dev_pm_opp_put() for the returned OPP after
  * use.
  *
- * Return: destination table OPP on success, otherwise -EINVAL or -ENODEV based
- * on errors.
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
  */
 struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
                                                 struct opp_table *dst_table,
                                                 struct dev_pm_opp *src_opp)
 {
-       struct dev_pm_opp *opp, *dest_opp = ERR_PTR(-EINVAL);
+       struct dev_pm_opp *opp, *dest_opp = ERR_PTR(-ENODEV);
        int i;
 
        if (!src_table || !dst_table || !src_opp ||
@@ -2425,37 +2425,33 @@ struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
 
        /* required-opps not fully initialized yet */
        if (lazy_linking_pending(src_table))
-               return ERR_PTR(-EINVAL);
+               return ERR_PTR(-EBUSY);
 
        for (i = 0; i < src_table->required_opp_count; i++) {
-               if (src_table->required_opp_tables[i] == dst_table)
+               if (src_table->required_opp_tables[i] == dst_table) {
+                       mutex_lock(&src_table->lock);
+
+                       list_for_each_entry(opp, &src_table->opp_list, node) {
+                               if (opp == src_opp) {
+                                       dest_opp = opp->required_opps[i];
+                                       dev_pm_opp_get(dest_opp);
+                                       break;
+                               }
+                       }
+
+                       mutex_unlock(&src_table->lock);
                        break;
-       }
-
-       if (unlikely(i == src_table->required_opp_count)) {
-               pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
-                      __func__, src_table, dst_table);
-               return ERR_PTR(-ENODEV);
-       }
-
-       mutex_lock(&src_table->lock);
-
-       list_for_each_entry(opp, &src_table->opp_list, node) {
-               if (opp == src_opp) {
-                       dest_opp = opp->required_opps[i];
-                       dev_pm_opp_get(dest_opp);
-                       goto unlock;
                }
        }
 
-       pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
-              dst_table);
-
-unlock:
-       mutex_unlock(&src_table->lock);
+       if (IS_ERR(dest_opp)) {
+               pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
+                      src_table, dst_table);
+       }
 
        return dest_opp;
 }
+EXPORT_SYMBOL_GPL(dev_pm_opp_xlate_required_opp);
 
 /**
  * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.


-- 
viresh

[PATCH v6 2/3] PM / devfreq: Cache OPP table reference in devfreq

[Hsin-Yi Wang]

From: Saravana Kannan <saravanak@google.com>

The OPP table can be used often in devfreq. Trying to get it each time can
be expensive, so cache it in the devfreq struct.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/devfreq/devfreq.c | 6 ++++++
 include/linux/devfreq.h   | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6aa10de792b33..a5899c9ae16fc 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -757,6 +757,8 @@ static void devfreq_dev_release(struct device *dev)
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	if (devfreq->opp_table)
+		dev_pm_opp_put_opp_table(devfreq->opp_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -844,6 +846,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	}
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
+	devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
+	if (IS_ERR(devfreq->opp_table))
+		devfreq->opp_table = NULL;
+
 	atomic_set(&devfreq->suspend_count, 0);
 
 	dev_set_name(&devfreq->dev, "%s", dev_name(dev));
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index b6d3bae1c74d8..26ea0850be9bb 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -137,6 +137,7 @@ struct devfreq_stats {
  *		using devfreq.
  * @profile:	device-specific devfreq profile
  * @governor:	method how to choose frequency based on the usage.
+ * @opp_table:	Reference to OPP table of dev.parent, if one exists.
  * @nb:		notifier block used to notify devfreq object that it should
  *		reevaluate operable frequencies. Devfreq users may use
  *		devfreq.nb to the corresponding register notifier call chain.
@@ -173,6 +174,7 @@ struct devfreq {
 	struct device dev;
 	struct devfreq_dev_profile *profile;
 	const struct devfreq_governor *governor;
+	struct opp_table *opp_table;
 	struct notifier_block nb;
 	struct delayed_work work;
 
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v6 2/3] PM / devfreq: Cache OPP table reference in devfreq

[Chanwoo Choi]

Hi Hsin-Yi,

On 2/4/21 5:14 PM, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> The OPP table can be used often in devfreq. Trying to get it each time can
> be expensive, so cache it in the devfreq struct.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 6 ++++++
>  include/linux/devfreq.h   | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6aa10de792b33..a5899c9ae16fc 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -757,6 +757,8 @@ static void devfreq_dev_release(struct device *dev)
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	if (devfreq->opp_table)
> +		dev_pm_opp_put_opp_table(devfreq->opp_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -844,6 +846,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	}
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> +	devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> +	if (IS_ERR(devfreq->opp_table))
> +		devfreq->opp_table = NULL;
> +
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	dev_set_name(&devfreq->dev, "%s", dev_name(dev));
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b6d3bae1c74d8..26ea0850be9bb 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -137,6 +137,7 @@ struct devfreq_stats {
>   *		using devfreq.
>   * @profile:	device-specific devfreq profile
>   * @governor:	method how to choose frequency based on the usage.
> + * @opp_table:	Reference to OPP table of dev.parent, if one exists.
>   * @nb:		notifier block used to notify devfreq object that it should
>   *		reevaluate operable frequencies. Devfreq users may use
>   *		devfreq.nb to the corresponding register notifier call chain.
> @@ -173,6 +174,7 @@ struct devfreq {
>  	struct device dev;
>  	struct devfreq_dev_profile *profile;
>  	const struct devfreq_governor *governor;
> +	struct opp_table *opp_table;
>  	struct notifier_block nb;
>  	struct delayed_work work;
>  
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 2/3] PM / devfreq: Cache OPP table reference in devfreq

[Viresh Kumar]

On 04-02-21, 17:46, Chanwoo Choi wrote:
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

I am not sure if keeping both Acked-by and Reviewed-by make sense. I
am keeping Acked-by as it was the latest of the two, lemme know your
opinion as well.

-- 
viresh

[PATCH v6 3/3] PM / devfreq: Add required OPPs support to passive governor

[Hsin-Yi Wang]

From: Saravana Kannan <saravanak@google.com>

Look at the required OPPs of the "parent" device to determine the OPP that
is required from the slave device managed by the passive governor. This
allows having mappings between a parent device and a slave device even when
they don't have the same number of OPPs.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/devfreq/governor_passive.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 63332e4a65ae8..8fd51cc9b991a 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
 	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp, *p_opp = ERR_PTR(-ENODEV);
 	int i, count, ret = 0;
 
 	/*
@@ -29,7 +29,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	 */
 	if (p_data->get_target_freq) {
 		ret = p_data->get_target_freq(devfreq, freq);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -56,13 +56,22 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	 * list of parent device. Because in this case, *freq is temporary
 	 * value which is decided by ondemand governor.
 	 */
-	opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
-	if (IS_ERR(opp)) {
-		ret = PTR_ERR(opp);
-		goto out;
+	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
+	if (IS_ERR(p_opp)) {
+		ret = PTR_ERR(p_opp);
+		return ret;
 	}
 
-	dev_pm_opp_put(opp);
+	if (devfreq->opp_table && parent_devfreq->opp_table) {
+		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
+						    devfreq->opp_table, p_opp);
+		if (!IS_ERR(opp)) {
+			*freq = dev_pm_opp_get_freq(opp);
+			dev_pm_opp_put(opp);
+		} else
+			ret = PTR_ERR(opp);
+		goto out;
+	}
 
 	/*
 	 * Get the OPP table's index of decided freqeuncy by governor
@@ -89,6 +98,8 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	*freq = child_freq;
 
 out:
+	dev_pm_opp_put(p_opp);
+
 	return ret;
 }
 
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v6 3/3] PM / devfreq: Add required OPPs support to passive governor

[Chanwoo Choi]

Hi Hsin-Yi,

I have reviewed this patch already.
This version looks good to me.
I add my acked-by again.

Acked-by: Chanwoo Choi <cw00.choi@samsugn.com>

Thanks,
Chanwoo Choi

On 2/4/21 5:14 PM, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> Look at the required OPPs of the "parent" device to determine the OPP that
> is required from the slave device managed by the passive governor. This
> allows having mappings between a parent device and a slave device even when
> they don't have the same number of OPPs.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/devfreq/governor_passive.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 63332e4a65ae8..8fd51cc9b991a 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  			= (struct devfreq_passive_data *)devfreq->data;
>  	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
>  	unsigned long child_freq = ULONG_MAX;
> -	struct dev_pm_opp *opp;
> +	struct dev_pm_opp *opp, *p_opp = ERR_PTR(-ENODEV);
>  	int i, count, ret = 0;
>  
>  	/*
> @@ -29,7 +29,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	 */
>  	if (p_data->get_target_freq) {
>  		ret = p_data->get_target_freq(devfreq, freq);
> -		goto out;
> +		return ret;
>  	}
>  
>  	/*
> @@ -56,13 +56,22 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	 * list of parent device. Because in this case, *freq is temporary
>  	 * value which is decided by ondemand governor.
>  	 */
> -	opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> -	if (IS_ERR(opp)) {
> -		ret = PTR_ERR(opp);
> -		goto out;
> +	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> +	if (IS_ERR(p_opp)) {
> +		ret = PTR_ERR(p_opp);
> +		return ret;
>  	}
>  
> -	dev_pm_opp_put(opp);
> +	if (devfreq->opp_table && parent_devfreq->opp_table) {
> +		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
> +						    devfreq->opp_table, p_opp);
> +		if (!IS_ERR(opp)) {
> +			*freq = dev_pm_opp_get_freq(opp);
> +			dev_pm_opp_put(opp);
> +		} else
> +			ret = PTR_ERR(opp);
> +		goto out;
> +	}
>  
>  	/*
>  	 * Get the OPP table's index of decided freqeuncy by governor
> @@ -89,6 +98,8 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	*freq = child_freq;
>  
>  out:
> +	dev_pm_opp_put(p_opp);
> +
>  	return ret;
>  }
>  
> 

Re: [PATCH v6 3/3] PM / devfreq: Add required OPPs support to passive governor

[Viresh Kumar]

On 04-02-21, 16:14, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> Look at the required OPPs of the "parent" device to determine the OPP that
> is required from the slave device managed by the passive governor. This
> allows having mappings between a parent device and a slave device even when
> they don't have the same number of OPPs.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/devfreq/governor_passive.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)

I have made changes to this patch as well, though I didn't wanted to
do initially :)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 8fd51cc9b991..b094132bd20b 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -19,18 +19,16 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
                        = (struct devfreq_passive_data *)devfreq->data;
        struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
        unsigned long child_freq = ULONG_MAX;
-       struct dev_pm_opp *opp, *p_opp = ERR_PTR(-ENODEV);
-       int i, count, ret = 0;
+       struct dev_pm_opp *opp, *p_opp;
+       int i, count;
 
        /*
         * If the devfreq device with passive governor has the specific method
         * to determine the next frequency, should use the get_target_freq()
         * of struct devfreq_passive_data.
         */
-       if (p_data->get_target_freq) {
-               ret = p_data->get_target_freq(devfreq, freq);
-               return ret;
-       }
+       if (p_data->get_target_freq)
+               return p_data->get_target_freq(devfreq, freq);
 
        /*
         * If the parent and passive devfreq device uses the OPP table,
@@ -56,35 +54,35 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
         * list of parent device. Because in this case, *freq is temporary
         * value which is decided by ondemand governor.
         */
-       p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
-       if (IS_ERR(p_opp)) {
-               ret = PTR_ERR(p_opp);
-               return ret;
-       }
-
        if (devfreq->opp_table && parent_devfreq->opp_table) {
+               p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent,
+                                               freq, 0);
+               if (IS_ERR(p_opp))
+                       return PTR_ERR(p_opp);
+
                opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
                                                    devfreq->opp_table, p_opp);
-               if (!IS_ERR(opp)) {
-                       *freq = dev_pm_opp_get_freq(opp);
-                       dev_pm_opp_put(opp);
-               } else
-                       ret = PTR_ERR(opp);
-               goto out;
+               dev_pm_opp_put(p_opp);
+
+               if (IS_ERR(opp))
+                       return PTR_ERR(opp);
+
+               *freq = dev_pm_opp_get_freq(opp);
+               dev_pm_opp_put(opp);
+
+               return 0;
        }
 
        /*
-        * Get the OPP table's index of decided freqeuncy by governor
+        * Get the OPP table's index of decided frequency by governor
         * of parent device.
         */
        for (i = 0; i < parent_devfreq->profile->max_state; i++)
                if (parent_devfreq->profile->freq_table[i] == *freq)
                        break;
 
-       if (i == parent_devfreq->profile->max_state) {
-               ret = -EINVAL;
-               goto out;
-       }
+       if (i == parent_devfreq->profile->max_state)
+               return -EINVAL;
 
        /* Get the suitable frequency by using index of parent device. */
        if (i < devfreq->profile->max_state) {
@@ -97,10 +95,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
        /* Return the suitable frequency for passive device. */
        *freq = child_freq;
 
-out:
-       dev_pm_opp_put(p_opp);
-
-       return ret;
+       return 0;
 }
 
 static int devfreq_passive_notifier_call(struct notifier_block *nb,


-- 
viresh

Re: [PATCH v6 0/3] Add required-opps support to devfreq passive gov

[Viresh Kumar]

On 04-02-21, 16:14, Hsin-Yi Wang wrote:
> Saravana Kannan (3):
>   OPP: Add function to look up required OPP's for a given OPP
>   PM / devfreq: Cache OPP table reference in devfreq
>   PM / devfreq: Add required OPPs support to passive governor

I have applied all the patches with significant modifications, please
check the OPP tree and see if everything looks fine.

-- 
viresh