linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Add required-opps support to devfreq passive gov
@ 2019-06-25 21:33 Saravana Kannan
  2019-06-25 21:33 ` [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Saravana Kannan @ 2019-06-25 21:33 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

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

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 (4):
  OPP: Allow required-opps even if the device doesn't have power-domains
  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 | 20 ++++++++---
 drivers/opp/core.c                 | 56 +++++++++++++++++++++++++++++-
 drivers/opp/of.c                   | 14 --------
 include/linux/devfreq.h            |  1 +
 include/linux/pm_opp.h             | 11 ++++++
 6 files changed, 88 insertions(+), 20 deletions(-)

-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains
  2019-06-25 21:33 [PATCH v2 0/4] Add required-opps support to devfreq passive gov Saravana Kannan
@ 2019-06-25 21:33 ` Saravana Kannan
  2019-07-16 17:17   ` Sibi Sankar
  2019-06-25 21:33 ` [PATCH v2 2/4] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Saravana Kannan @ 2019-06-25 21:33 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

A Device-A can have a (minimum) performance requirement on another
Device-B to be able to function correctly. This performance requirement
on Device-B can also change based on the current performance level of
Device-A.

The existing required-opps feature fits well to describe this need. So,
instead of limiting required-opps to point to only PM-domain devices,
allow it to point to any device.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c |  2 +-
 drivers/opp/of.c   | 14 --------------
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..74c7bdc6f463 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
 		return 0;
 
 	/* Single genpd case */
-	if (!genpd_virt_devs) {
+	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
 		pstate = opp->required_opps[0]->pstate;
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
 		if (ret) {
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index c10c782d15aa..7c8336e94aff 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	 */
 	count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
 					      "#power-domain-cells");
-	if (!count_pd)
-		goto put_np;
-
 	if (count_pd > 1) {
 		genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
 					GFP_KERNEL);
@@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 
 		if (IS_ERR(required_opp_tables[i]))
 			goto free_required_tables;
-
-		/*
-		 * We only support genpd's OPPs in the "required-opps" for now,
-		 * as we don't know how much about other cases. Error out if the
-		 * required OPP doesn't belong to a genpd.
-		 */
-		if (!required_opp_tables[i]->is_genpd) {
-			dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
-				required_np);
-			goto free_required_tables;
-		}
 	}
 
 	goto put_np;
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] OPP: Add function to look up required OPP's for a given OPP
  2019-06-25 21:33 [PATCH v2 0/4] Add required-opps support to devfreq passive gov Saravana Kannan
  2019-06-25 21:33 ` [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
@ 2019-06-25 21:33 ` Saravana Kannan
  2019-06-25 21:33 ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq Saravana Kannan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Saravana Kannan @ 2019-06-25 21:33 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

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>
---
 drivers/opp/core.c     | 54 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h | 11 +++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 74c7bdc6f463..4f7870bffbf8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1830,6 +1830,60 @@ void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
 		dev_err(virt_dev, "Failed to find required device entry\n");
 }
 
+/**
+ * dev_pm_opp_xlate_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.
+ * @pstate: OPP 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 NULL on errors.
+ */
+struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
+					struct opp_table *dst_table,
+					struct dev_pm_opp *src_opp)
+{
+	struct dev_pm_opp *opp, *dest_opp = NULL;
+	int i;
+
+	if (!src_table || !dst_table || !src_opp)
+		return NULL;
+
+	for (i = 0; i < src_table->required_opp_count; i++) {
+		if (src_table->required_opp_tables[i]->np == dst_table->np)
+			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 NULL;
+	}
+
+	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 b150fe97ce5a..bc5c68bdfc8d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -134,6 +134,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index);
 void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
+struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
+					struct opp_table *dst_table,
+					struct dev_pm_opp *src_opp);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -307,6 +310,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table
 	return -ENOTSUPP;
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_xlate_opp(
+						struct opp_table *src_table,
+						struct opp_table *dst_table,
+						struct dev_pm_opp *src_opp)
+{
+	return NULL;
+}
+
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	return -ENOTSUPP;
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
  2019-06-25 21:33 [PATCH v2 0/4] Add required-opps support to devfreq passive gov Saravana Kannan
  2019-06-25 21:33 ` [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
  2019-06-25 21:33 ` [PATCH v2 2/4] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
@ 2019-06-25 21:33 ` Saravana Kannan
  2019-06-26  1:57   ` Chanwoo Choi
  2019-07-16 17:36   ` Sibi Sankar
  2019-06-25 21:33 ` [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 13+ messages in thread
From: Saravana Kannan @ 2019-06-25 21:33 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

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>
---
 drivers/devfreq/devfreq.c | 6 ++++++
 include/linux/devfreq.h   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6b6991f0e873..ac62b78dc035 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -597,6 +597,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);
 }
@@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->max_freq = devfreq->scaling_max_freq;
 
 	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, "devfreq%d",
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index fbffa74bfc1b..0d877c9513d7 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -156,6 +156,7 @@ struct devfreq {
 	struct devfreq_dev_profile *profile;
 	const struct devfreq_governor *governor;
 	char governor_name[DEVFREQ_NAME_LEN];
+	struct opp_table *opp_table;
 	struct notifier_block nb;
 	struct delayed_work work;
 
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor
  2019-06-25 21:33 [PATCH v2 0/4] Add required-opps support to devfreq passive gov Saravana Kannan
                   ` (2 preceding siblings ...)
  2019-06-25 21:33 ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq Saravana Kannan
@ 2019-06-25 21:33 ` Saravana Kannan
  2019-06-26  1:59   ` Chanwoo Choi
       [not found] ` <CGME20190625213355epcas5p3805e632818ee999a91e48ef9a610a084@epcms1p4>
       [not found] ` <CGME20190625213358epcas4p2391bcf70b54646880cbb60eae8b73acf@epcms1p1>
  5 siblings, 1 reply; 13+ messages in thread
From: Saravana Kannan @ 2019-06-25 21:33 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

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>
---
 drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 3bc29acbd54e..f6de03de7a64 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -22,7 +22,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 = NULL, *p_opp = NULL;
 	int i, count, ret = 0;
 
 	/*
@@ -59,13 +59,20 @@ 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);
+	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
+	if (IS_ERR(p_opp)) {
+		ret = PTR_ERR(p_opp);
 		goto out;
 	}
 
-	dev_pm_opp_put(opp);
+	if (devfreq->opp_table && parent_devfreq->opp_table)
+		opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
+					   devfreq->opp_table, p_opp);
+	if (opp) {
+		*freq = dev_pm_opp_get_freq(opp);
+		dev_pm_opp_put(opp);
+		goto out;
+	}
 
 	/*
 	 * Get the OPP table's index of decided freqeuncy by governor
@@ -92,6 +99,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	*freq = child_freq;
 
 out:
+	if (!IS_ERR_OR_NULL(opp))
+		dev_pm_opp_put(p_opp);
+
 	return ret;
 }
 
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
       [not found] ` <CGME20190625213355epcas5p3805e632818ee999a91e48ef9a610a084@epcms1p4>
@ 2019-06-26  1:30   ` MyungJoo Ham
  0 siblings, 0 replies; 13+ messages in thread
From: MyungJoo Ham @ 2019-06-26  1:30 UTC (permalink / raw)
  To: Kyungmin Park, Chanwoo Choi, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

>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>
>---
> drivers/devfreq/devfreq.c | 6 ++++++
> include/linux/devfreq.h   | 1 +
> 2 files changed, 7 insertions(+)

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>


Cheers,
MyungJoo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor
       [not found] ` <CGME20190625213358epcas4p2391bcf70b54646880cbb60eae8b73acf@epcms1p1>
@ 2019-06-26  1:31   ` MyungJoo Ham
  0 siblings, 0 replies; 13+ messages in thread
From: MyungJoo Ham @ 2019-06-26  1:31 UTC (permalink / raw)
  To: Kyungmin Park, Chanwoo Choi, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, Rafael J. Wysocki
  Cc: Saravana Kannan, kernel-team, linux-pm, linux-kernel

>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>
>---
> drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

Cheers,
MyungJoo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
  2019-06-25 21:33 ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq Saravana Kannan
@ 2019-06-26  1:57   ` Chanwoo Choi
  2019-07-16 17:36   ` Sibi Sankar
  1 sibling, 0 replies; 13+ messages in thread
From: Chanwoo Choi @ 2019-06-26  1:57 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel, cpgs (cpgs@samsung.com)

Hi Saravana,

On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> 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>
> ---
>  drivers/devfreq/devfreq.c | 6 ++++++
>  include/linux/devfreq.h   | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,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);
>  }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->max_freq = devfreq->scaling_max_freq;
>  
>  	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, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
>  	struct devfreq_dev_profile *profile;
>  	const struct devfreq_governor *governor;
>  	char governor_name[DEVFREQ_NAME_LEN];
> +	struct opp_table *opp_table;
>  	struct notifier_block nb;
>  	struct delayed_work work;
>  
> 

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor
  2019-06-25 21:33 ` [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
@ 2019-06-26  1:59   ` Chanwoo Choi
  0 siblings, 0 replies; 13+ messages in thread
From: Chanwoo Choi @ 2019-06-26  1:59 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel, cpgs (cpgs@samsung.com)

Hi Saravana,

On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> 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>
> ---
>  drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e..f6de03de7a64 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -22,7 +22,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 = NULL, *p_opp = NULL;
>  	int i, count, ret = 0;
>  
>  	/*
> @@ -59,13 +59,20 @@ 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);
> +	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> +	if (IS_ERR(p_opp)) {
> +		ret = PTR_ERR(p_opp);
>  		goto out;
>  	}
>  
> -	dev_pm_opp_put(opp);
> +	if (devfreq->opp_table && parent_devfreq->opp_table)
> +		opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
> +					   devfreq->opp_table, p_opp);
> +	if (opp) {
> +		*freq = dev_pm_opp_get_freq(opp);
> +		dev_pm_opp_put(opp);
> +		goto out;
> +	}
>  
>  	/*
>  	 * Get the OPP table's index of decided freqeuncy by governor
> @@ -92,6 +99,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  	*freq = child_freq;
>  
>  out:
> +	if (!IS_ERR_OR_NULL(opp))
> +		dev_pm_opp_put(p_opp);
> +
>  	return ret;
>  }
>  
> 

I agree this approach. It is necessary.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains
  2019-06-25 21:33 ` [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
@ 2019-07-16 17:17   ` Sibi Sankar
  2019-07-16 18:54     ` Saravana Kannan
  0 siblings, 1 reply; 13+ messages in thread
From: Sibi Sankar @ 2019-07-16 17:17 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel

Hey Saravana,
Thanks for taking time to post out this series.

On 6/26/19 3:03 AM, Saravana Kannan wrote:
> A Device-A can have a (minimum) performance requirement on another
> Device-B to be able to function correctly. This performance requirement
> on Device-B can also change based on the current performance level of
> Device-A.
> 
> The existing required-opps feature fits well to describe this need. So,
> instead of limiting required-opps to point to only PM-domain devices,
> allow it to point to any device.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>   drivers/opp/core.c |  2 +-
>   drivers/opp/of.c   | 14 --------------
>   2 files changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..74c7bdc6f463 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
>   		return 0;
>   
>   	/* Single genpd case */
> -	if (!genpd_virt_devs) {
> +	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
https://patchwork.kernel.org/patch/10940671/
This was already removed as a part of ^^ and is in linux-next.

>   		pstate = opp->required_opps[0]->pstate;
>   		ret = dev_pm_genpd_set_performance_state(dev, pstate);
>   		if (ret) {
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..7c8336e94aff 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>   	 */
>   	count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
>   					      "#power-domain-cells");
> -	if (!count_pd)
> -		goto put_np;
> -
>   	if (count_pd > 1) {
>   		genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
>   					GFP_KERNEL);
> @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>   
>   		if (IS_ERR(required_opp_tables[i]))
>   			goto free_required_tables;
> -
> -		/*
> -		 * We only support genpd's OPPs in the "required-opps" for now,
> -		 * as we don't know how much about other cases. Error out if the
> -		 * required OPP doesn't belong to a genpd.
> -		 */
> -		if (!required_opp_tables[i]->is_genpd) {
> -			dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> -				required_np);
> -			goto free_required_tables;
> -		}

I expect the series to not work as is in its current state since I
see a circular dependency here. The required-opp tables of the parent
devfreq won't be populated until we add the opp-table of the child
devfreq node while the child devfreq using passive governor would
return -EPROBE_DEFER until the parent devfreq probes.

The same applies to this patch -> https://patchwork.kernel.org/patch
/11046147/ I posted out based on your series. So we would probably have
to address the dependency here.

>   	}
>   
>   	goto put_np;
> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
  2019-06-25 21:33 ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq Saravana Kannan
  2019-06-26  1:57   ` Chanwoo Choi
@ 2019-07-16 17:36   ` Sibi Sankar
  2019-07-16 19:12     ` Saravana Kannan
  1 sibling, 1 reply; 13+ messages in thread
From: Sibi Sankar @ 2019-07-16 17:36 UTC (permalink / raw)
  To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
  Cc: kernel-team, linux-pm, linux-kernel, adharmap

Hey Saravana,

On 6/26/19 3:03 AM, Saravana Kannan wrote:
> 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>
> ---
>   drivers/devfreq/devfreq.c | 6 ++++++
>   include/linux/devfreq.h   | 1 +
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,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);
>   }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>   	devfreq->max_freq = devfreq->scaling_max_freq;
>   
>   	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, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
>   	struct devfreq_dev_profile *profile;
>   	const struct devfreq_governor *governor;
>   	char governor_name[DEVFREQ_NAME_LEN];
> +	struct opp_table *opp_table;

please add it to the function docs as well

>   	struct notifier_block nb;
>   	struct delayed_work work;
>   
> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains
  2019-07-16 17:17   ` Sibi Sankar
@ 2019-07-16 18:54     ` Saravana Kannan
  0 siblings, 0 replies; 13+ messages in thread
From: Saravana Kannan @ 2019-07-16 18:54 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Tue, Jul 16, 2019 at 10:18 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
> Thanks for taking time to post out this series.
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > A Device-A can have a (minimum) performance requirement on another
> > Device-B to be able to function correctly. This performance requirement
> > on Device-B can also change based on the current performance level of
> > Device-A.
> >
> > The existing required-opps feature fits well to describe this need. So,
> > instead of limiting required-opps to point to only PM-domain devices,
> > allow it to point to any device.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >   drivers/opp/core.c |  2 +-
> >   drivers/opp/of.c   | 14 --------------
> >   2 files changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0e7703fe733f..74c7bdc6f463 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> >               return 0;
> >
> >       /* Single genpd case */
> > -     if (!genpd_virt_devs) {
> > +     if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
> https://patchwork.kernel.org/patch/10940671/
> This was already removed as a part of ^^ and is in linux-next.

Thanks for the heads up.

>
> >               pstate = opp->required_opps[0]->pstate;
> >               ret = dev_pm_genpd_set_performance_state(dev, pstate);
> >               if (ret) {
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index c10c782d15aa..7c8336e94aff 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >        */
> >       count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> >                                             "#power-domain-cells");
> > -     if (!count_pd)
> > -             goto put_np;
> > -
> >       if (count_pd > 1) {
> >               genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> >                                       GFP_KERNEL);
> > @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >
> >               if (IS_ERR(required_opp_tables[i]))
> >                       goto free_required_tables;
> > -
> > -             /*
> > -              * We only support genpd's OPPs in the "required-opps" for now,
> > -              * as we don't know how much about other cases. Error out if the
> > -              * required OPP doesn't belong to a genpd.
> > -              */
> > -             if (!required_opp_tables[i]->is_genpd) {
> > -                     dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> > -                             required_np);
> > -                     goto free_required_tables;
> > -             }
>
> I expect the series to not work as is in its current state since I
> see a circular dependency here. The required-opp tables of the parent
> devfreq won't be populated until we add the opp-table of the child
> devfreq node while the child devfreq using passive governor would
> return -EPROBE_DEFER until the parent devfreq probes.

I looked into this when I wrote the patches. I thought I handled it
correctly. Let me take another look.

-Saravana

> The same applies to this patch -> https://patchwork.kernel.org/patch
> /11046147/ I posted out based on your series. So we would probably have
> to address the dependency here.
>
> >       }
> >
> >       goto put_np;
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
  2019-07-16 17:36   ` Sibi Sankar
@ 2019-07-16 19:12     ` Saravana Kannan
  0 siblings, 0 replies; 13+ messages in thread
From: Saravana Kannan @ 2019-07-16 19:12 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML, adharmap

On Tue, Jul 16, 2019 at 10:36 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > 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>
> > ---
> >   drivers/devfreq/devfreq.c | 6 ++++++
> >   include/linux/devfreq.h   | 1 +
> >   2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 6b6991f0e873..ac62b78dc035 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -597,6 +597,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);
> >   }
> > @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >       devfreq->max_freq = devfreq->scaling_max_freq;
> >
> >       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, "devfreq%d",
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index fbffa74bfc1b..0d877c9513d7 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -156,6 +156,7 @@ struct devfreq {
> >       struct devfreq_dev_profile *profile;
> >       const struct devfreq_governor *governor;
> >       char governor_name[DEVFREQ_NAME_LEN];
> > +     struct opp_table *opp_table;
>
> please add it to the function docs as well

Will do.

-Saravana

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-07-16 19:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-25 21:33 [PATCH v2 0/4] Add required-opps support to devfreq passive gov Saravana Kannan
2019-06-25 21:33 ` [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
2019-07-16 17:17   ` Sibi Sankar
2019-07-16 18:54     ` Saravana Kannan
2019-06-25 21:33 ` [PATCH v2 2/4] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
2019-06-25 21:33 ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq Saravana Kannan
2019-06-26  1:57   ` Chanwoo Choi
2019-07-16 17:36   ` Sibi Sankar
2019-07-16 19:12     ` Saravana Kannan
2019-06-25 21:33 ` [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
2019-06-26  1:59   ` Chanwoo Choi
     [not found] ` <CGME20190625213355epcas5p3805e632818ee999a91e48ef9a610a084@epcms1p4>
2019-06-26  1:30   ` [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq MyungJoo Ham
     [not found] ` <CGME20190625213358epcas4p2391bcf70b54646880cbb60eae8b73acf@epcms1p1>
2019-06-26  1:31   ` [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor MyungJoo Ham

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).