linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] Add required-opps support to devfreq passive gov
@ 2019-06-22  0:34 Saravana Kannan
  2019-06-22  0:34 ` [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22  0:34 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.

-Saravana

Saravana Kannan (3):
  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: Add required OPPs support to passive governor

 drivers/devfreq/governor_passive.c | 25 +++++++++++--
 drivers/opp/core.c                 | 56 +++++++++++++++++++++++++++++-
 drivers/opp/of.c                   | 14 --------
 include/linux/pm_opp.h             | 11 ++++++
 4 files changed, 89 insertions(+), 17 deletions(-)

-- 
2.22.0.410.gd8fdbe21b5-goog


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

* [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains
  2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
@ 2019-06-22  0:34 ` Saravana Kannan
  2019-06-22  0:34 ` [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22  0:34 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] 21+ messages in thread

* [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
  2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
  2019-06-22  0:34 ` [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
@ 2019-06-22  0:34 ` Saravana Kannan
  2019-06-22 11:49   ` Chanwoo Choi
  2019-06-22  0:34 ` [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
  2019-06-24  9:43 ` [PATCH v1 0/3] Add required-opps support to devfreq passive gov Viresh Kumar
  3 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22  0:34 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] 21+ messages in thread

* [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor
  2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
  2019-06-22  0:34 ` [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
  2019-06-22  0:34 ` [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
@ 2019-06-22  0:34 ` Saravana Kannan
  2019-06-22 12:00   ` Chanwoo Choi
  2019-06-24  9:43 ` [PATCH v1 0/3] Add required-opps support to devfreq passive gov Viresh Kumar
  3 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22  0:34 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 | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 3bc29acbd54e..bd4a98bb15b1 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -21,8 +21,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	struct devfreq_passive_data *p_data
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
+	struct opp_table *opp_table = NULL, *c_opp_table = NULL;
 	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp = NULL, *c_opp = NULL;
 	int i, count, ret = 0;
 
 	/*
@@ -65,7 +66,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 		goto out;
 	}
 
-	dev_pm_opp_put(opp);
+	opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);
+	if (IS_ERR_OR_NULL(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto out;
+	}
+
+	c_opp_table = dev_pm_opp_get_opp_table(devfreq->dev.parent);
+	if (!IS_ERR_OR_NULL(c_opp_table))
+		c_opp = dev_pm_opp_xlate_opp(opp_table, c_opp_table, opp);
+	if (c_opp) {
+		*freq = dev_pm_opp_get_freq(c_opp);
+		dev_pm_opp_put(c_opp);
+		goto out;
+	}
 
 	/*
 	 * Get the OPP table's index of decided freqeuncy by governor
@@ -92,6 +106,13 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	*freq = child_freq;
 
 out:
+	if (!IS_ERR_OR_NULL(opp_table))
+		dev_pm_opp_put_opp_table(opp_table);
+	if (!IS_ERR_OR_NULL(c_opp_table))
+		dev_pm_opp_put_opp_table(c_opp_table);
+	if (!IS_ERR_OR_NULL(opp))
+		dev_pm_opp_put(opp);
+
 	return ret;
 }
 
-- 
2.22.0.410.gd8fdbe21b5-goog


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

* Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
  2019-06-22  0:34 ` [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
@ 2019-06-22 11:49   ` Chanwoo Choi
  2019-06-22 21:41     ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2019-06-22 11:49 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, kernel-team,
	Linux PM list, linux-kernel

Hi,

Absolutely, I like this approach. I think that it is necessary to make
the connection
between frequencies of devices. But, I have a question on below.

2019년 6월 22일 (토) 오전 9:35, 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>
> ---
>  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];

Correct me if I am wrong. This patch assume that 'i' index is same on between
[1] and [2]. But in order to guarantee this assumption, all OPP entries
in the same opp_table have to have the same number of 'required-opps' properties
and keep the sequence among 'required-opps' entries.

[1] src_table->required_opp_tables[i]->np
[2] opp->required_opps[I];

For example, three OPP entries in the 'parent_bus_opp'
have the different sequence of 'required-opps' and the different
number of 'required-opps'. Is it no problem?

parent_bus_opp: opp_table {
    compatible = "operating-points-v2";

    opp2 {
        opp-hz = /bits/ 64 <200000>;
        required-opps = <&child_bus_a_opp2>, <&child_bus_b_opp2>,
<&child_bus_c_opp2>;
    };

    opp1 {
        opp-hz = /bits/ 64 <200000>;
        // change the sequence between child_bus_b_opp2  and child_bus_c_opp2
        required-opps = <&child_bus_a_opp2>, <&child_bus_c_opp2>,
<&child_bus_b_opp2>
    };

    opp0 {
        opp-hz = /bits/ 64 <200000>;
        // missing 'child_bus_a_opp2'
        required-opps = <&child_bus_c_opp2>, <&child_bus_b_opp2>
    };

}



> +                       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
>


-- 
Best Regards,
Chanwoo Choi

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

* Re: [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor
  2019-06-22  0:34 ` [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
@ 2019-06-22 12:00   ` Chanwoo Choi
  2019-06-22 21:45     ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2019-06-22 12:00 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, kernel-team,
	Linux PM list, linux-kernel

Hi,

Absolutely, I agree this approach.
But, I add some comments on below. please check them.

2019년 6월 22일 (토) 오전 9:36, 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>
> ---
>  drivers/devfreq/governor_passive.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e..bd4a98bb15b1 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -21,8 +21,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>         struct devfreq_passive_data *p_data
>                         = (struct devfreq_passive_data *)devfreq->data;
>         struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> +       struct opp_table *opp_table = NULL, *c_opp_table = NULL;

In this function, the base device is passive devfreq device.
So, I think that better to define the 'parent_opp_table' instead of 'opp_table'
indicating the OPP table of parent devfreq device. And better to define
just 'opp_table' instead of 'c_opp_table' indicating the passive devfreq device.
- opp_table -> parent_opp_table
- c_opp_table -> opp_table

>         unsigned long child_freq = ULONG_MAX;
> -       struct dev_pm_opp *opp;
> +       struct dev_pm_opp *opp = NULL, *c_opp = NULL;

Ditto. I think that better to define the variables as following:
- opp -> parent_opp
- c_cpp -> opp

>         int i, count, ret = 0;
>
>         /*
> @@ -65,7 +66,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>                 goto out;
>         }
>
> -       dev_pm_opp_put(opp);
> +       opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);

devfreq_passive_get_target_freq() is called frequently for DVFS support.
I think that you have to add 'struct opp_table *opp_table' instance to
'struct devfreq'
and then get 'opp_table' instance in the devfreq_add_device().

devfreq_add_device() already get the OPP information by using
dev_pm_opp_get_suspend_opp_freq().
You can add following code nearby dev_pm_opp_get_suspend_opp_freq() in
devfreq_add_device().
- devfreq->opp_table = dev_pm_opp_get_opp_table(dev);


> +       if (IS_ERR_OR_NULL(opp_table)) {
> +               ret = PTR_ERR(opp_table);
> +               goto out;
> +       }
> +
> +       c_opp_table = dev_pm_opp_get_opp_table(devfreq->dev.parent);
> +       if (!IS_ERR_OR_NULL(c_opp_table))
> +               c_opp = dev_pm_opp_xlate_opp(opp_table, c_opp_table, opp);
> +       if (c_opp) {
> +               *freq = dev_pm_opp_get_freq(c_opp);
> +               dev_pm_opp_put(c_opp);
> +               goto out;
> +       }
>
>         /*
>          * Get the OPP table's index of decided freqeuncy by governor
> @@ -92,6 +106,13 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>         *freq = child_freq;
>
>  out:
> +       if (!IS_ERR_OR_NULL(opp_table))
> +               dev_pm_opp_put_opp_table(opp_table);
> +       if (!IS_ERR_OR_NULL(c_opp_table))
> +               dev_pm_opp_put_opp_table(c_opp_table);
> +       if (!IS_ERR_OR_NULL(opp))
> +               dev_pm_opp_put(opp);
> +
>         return ret;
>  }
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>


-- 
Best Regards,
Chanwoo Choi

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

* Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
  2019-06-22 11:49   ` Chanwoo Choi
@ 2019-06-22 21:41     ` Saravana Kannan
  2019-06-23  4:27       ` Chanwoo Choi
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22 21:41 UTC (permalink / raw)
  To: cwchoi00
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM list, linux-kernel

On Sat, Jun 22, 2019 at 4:50 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> Absolutely, I like this approach. I think that it is necessary to make
> the connection
> between frequencies of devices.

Happy to hear that.

> But, I have a question on below.
>
> 2019년 6월 22일 (토) 오전 9:35, 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>
> > ---
> >  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];
>
> Correct me if I am wrong. This patch assume that 'i' index is same on between
> [1] and [2]. But in order to guarantee this assumption, all OPP entries
> in the same opp_table have to have the same number of 'required-opps' properties
> and keep the sequence among 'required-opps' entries.
>
> [1] src_table->required_opp_tables[i]->np
> [2] opp->required_opps[I];
>
> For example, three OPP entries in the 'parent_bus_opp'
> have the different sequence of 'required-opps' and the different
> number of 'required-opps'. Is it no problem?
>
> parent_bus_opp: opp_table {
>     compatible = "operating-points-v2";
>
>     opp2 {
>         opp-hz = /bits/ 64 <200000>;
>         required-opps = <&child_bus_a_opp2>, <&child_bus_b_opp2>,
> <&child_bus_c_opp2>;
>     };
>
>     opp1 {
>         opp-hz = /bits/ 64 <200000>;
>         // change the sequence between child_bus_b_opp2  and child_bus_c_opp2
>         required-opps = <&child_bus_a_opp2>, <&child_bus_c_opp2>,
> <&child_bus_b_opp2>
>     };
>
>     opp0 {
>         opp-hz = /bits/ 64 <200000>;
>         // missing 'child_bus_a_opp2'
>         required-opps = <&child_bus_c_opp2>, <&child_bus_b_opp2>
>     };
>
> }
>

I get your question. If I'm not mistaken the OPP framework DT parsing
code makes the assumption that the required-opps list has the phandles
in the same order for each "row" in the OPP table. It actually only
looks at the first OPP entry to figure out the list of required OPP
tables.

Technically one can write code to deal with random order of the
required-opp list, but doesn't seem like that's worth it because
there's no need to have that order all mixed up in DT. And even if
someone wants to add support for that, I don't think improving the DT
parsing to handle random order would be part of this patch series.

-Saravana

> --
> Best Regards,
> Chanwoo Choi
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

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

* Re: [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor
  2019-06-22 12:00   ` Chanwoo Choi
@ 2019-06-22 21:45     ` Saravana Kannan
  0 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2019-06-22 21:45 UTC (permalink / raw)
  To: cwchoi00
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM list, linux-kernel

On Sat, Jun 22, 2019 at 5:01 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> Absolutely, I agree this approach.

Thanks!

> But, I add some comments on below. please check them.
>
> 2019년 6월 22일 (토) 오전 9:36, 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>
> > ---
> >  drivers/devfreq/governor_passive.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> > index 3bc29acbd54e..bd4a98bb15b1 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -21,8 +21,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> >         struct devfreq_passive_data *p_data
> >                         = (struct devfreq_passive_data *)devfreq->data;
> >         struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> > +       struct opp_table *opp_table = NULL, *c_opp_table = NULL;
>
> In this function, the base device is passive devfreq device.
> So, I think that better to define the 'parent_opp_table' instead of 'opp_table'
> indicating the OPP table of parent devfreq device. And better to define
> just 'opp_table' instead of 'c_opp_table' indicating the passive devfreq device.
> - opp_table -> parent_opp_table
> - c_opp_table -> opp_table

Sounds good. I did it that way at first, but I wanted to keep the diff
simple in my first patch series. So renamed it. I can do the rename
that you suggested. Makes sense to me.

> >         unsigned long child_freq = ULONG_MAX;
> > -       struct dev_pm_opp *opp;
> > +       struct dev_pm_opp *opp = NULL, *c_opp = NULL;
>
> Ditto. I think that better to define the variables as following:
> - opp -> parent_opp
> - c_cpp -> opp

Will do.

>
> >         int i, count, ret = 0;
> >
> >         /*
> > @@ -65,7 +66,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> >                 goto out;
> >         }
> >
> > -       dev_pm_opp_put(opp);
> > +       opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);
>
> devfreq_passive_get_target_freq() is called frequently for DVFS support.
> I think that you have to add 'struct opp_table *opp_table' instance to
> 'struct devfreq'
> and then get 'opp_table' instance in the devfreq_add_device().

Sounds good. I had wanted to do that anyway, but didn't think it was
part of this series. But I can add that change to this series.

> devfreq_add_device() already get the OPP information by using
> dev_pm_opp_get_suspend_opp_freq().
> You can add following code nearby dev_pm_opp_get_suspend_opp_freq() in
> devfreq_add_device().
> - devfreq->opp_table = dev_pm_opp_get_opp_table(dev);

Will do something like that.

I'll send out an updated patch series on Monday or Tuesday. Hopefully
Viresh would have replied by then to give his opinion on whether this
is okay by him.

Thanks,
Saravana

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

* Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
  2019-06-22 21:41     ` Saravana Kannan
@ 2019-06-23  4:27       ` Chanwoo Choi
  2019-06-23  6:07         ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2019-06-23  4:27 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM list, linux-kernel

Hi,

2019년 6월 23일 (일) 오전 6:42, Saravana Kannan <saravanak@google.com>님이 작성:
>
> On Sat, Jun 22, 2019 at 4:50 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
> >
> > Hi,
> >
> > Absolutely, I like this approach. I think that it is necessary to make
> > the connection
> > between frequencies of devices.
>
> Happy to hear that.
>
> > But, I have a question on below.
> >
> > 2019년 6월 22일 (토) 오전 9:35, 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>
> > > ---
> > >  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];
> >
> > Correct me if I am wrong. This patch assume that 'i' index is same on between
> > [1] and [2]. But in order to guarantee this assumption, all OPP entries
> > in the same opp_table have to have the same number of 'required-opps' properties
> > and keep the sequence among 'required-opps' entries.
> >
> > [1] src_table->required_opp_tables[i]->np
> > [2] opp->required_opps[I];
> >
> > For example, three OPP entries in the 'parent_bus_opp'
> > have the different sequence of 'required-opps' and the different
> > number of 'required-opps'. Is it no problem?
> >
> > parent_bus_opp: opp_table {
> >     compatible = "operating-points-v2";
> >
> >     opp2 {
> >         opp-hz = /bits/ 64 <200000>;
> >         required-opps = <&child_bus_a_opp2>, <&child_bus_b_opp2>,
> > <&child_bus_c_opp2>;
> >     };
> >
> >     opp1 {
> >         opp-hz = /bits/ 64 <200000>;
> >         // change the sequence between child_bus_b_opp2  and child_bus_c_opp2
> >         required-opps = <&child_bus_a_opp2>, <&child_bus_c_opp2>,
> > <&child_bus_b_opp2>
> >     };
> >
> >     opp0 {
> >         opp-hz = /bits/ 64 <200000>;
> >         // missing 'child_bus_a_opp2'
> >         required-opps = <&child_bus_c_opp2>, <&child_bus_b_opp2>
> >     };
> >
> > }
> >
>
> I get your question. If I'm not mistaken the OPP framework DT parsing
> code makes the assumption that the required-opps list has the phandles
> in the same order for each "row" in the OPP table. It actually only
> looks at the first OPP entry to figure out the list of required OPP
> tables.

Thanks for description. It is the limitation of 'required-opps' until now.

>
> Technically one can write code to deal with random order of the
> required-opp list, but doesn't seem like that's worth it because
> there's no need to have that order all mixed up in DT. And even if
> someone wants to add support for that, I don't think improving the DT
> parsing to handle random order would be part of this patch series.

I understand the existing ' required-opps' only consider the same sequence
of entries which are included in the same OPP table.

One more thing, 'required-opps' properties doesn't support
the other OPP enters of the different OPP table. Is it right of 'required-opps'?

Except for the random order, just each OPP might will requires
the different 'required-opps' of different OPP table. Even if it is
not related to random order, I think that this approach cannot
support them.

For example as following:
- opp2 used the OPP entries of 'child_bus_A' and 'child_bus_B' opp-table.
- opp1 used the OPP entries of 'child_bus_C' and 'child_bus_D' opp-table.

parent_bus_opp: opp_table {
    compatible = "operating-points-v2";

     opp2 {
         opp-hz = /bits/ 64 <200000>;
         required-opps = <&child_bus_A_opp2>, <&child_bus_B_opp2>;
    };

   opp1 {
         opp-hz = /bits/ 64 <200000>;
         required-opps = <&child_bus_C_opp0>, <&child_bus_D_opp0>;
    };
};

-- 
Best Regards,
Chanwoo Choi

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

* Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
  2019-06-23  4:27       ` Chanwoo Choi
@ 2019-06-23  6:07         ` Saravana Kannan
  0 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2019-06-23  6:07 UTC (permalink / raw)
  To: cwchoi00
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM list, linux-kernel

On Sat, Jun 22, 2019 at 9:28 PM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> 2019년 6월 23일 (일) 오전 6:42, Saravana Kannan <saravanak@google.com>님이 작성:
> >
> > On Sat, Jun 22, 2019 at 4:50 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > Absolutely, I like this approach. I think that it is necessary to make
> > > the connection
> > > between frequencies of devices.
> >
> > Happy to hear that.
> >
> > > But, I have a question on below.
> > >
> > > 2019년 6월 22일 (토) 오전 9:35, 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>
> > > > ---
> > > >  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];
> > >
> > > Correct me if I am wrong. This patch assume that 'i' index is same on between
> > > [1] and [2]. But in order to guarantee this assumption, all OPP entries
> > > in the same opp_table have to have the same number of 'required-opps' properties
> > > and keep the sequence among 'required-opps' entries.
> > >
> > > [1] src_table->required_opp_tables[i]->np
> > > [2] opp->required_opps[I];
> > >
> > > For example, three OPP entries in the 'parent_bus_opp'
> > > have the different sequence of 'required-opps' and the different
> > > number of 'required-opps'. Is it no problem?
> > >
> > > parent_bus_opp: opp_table {
> > >     compatible = "operating-points-v2";
> > >
> > >     opp2 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         required-opps = <&child_bus_a_opp2>, <&child_bus_b_opp2>,
> > > <&child_bus_c_opp2>;
> > >     };
> > >
> > >     opp1 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         // change the sequence between child_bus_b_opp2  and child_bus_c_opp2
> > >         required-opps = <&child_bus_a_opp2>, <&child_bus_c_opp2>,
> > > <&child_bus_b_opp2>
> > >     };
> > >
> > >     opp0 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         // missing 'child_bus_a_opp2'
> > >         required-opps = <&child_bus_c_opp2>, <&child_bus_b_opp2>
> > >     };
> > >
> > > }
> > >
> >
> > I get your question. If I'm not mistaken the OPP framework DT parsing
> > code makes the assumption that the required-opps list has the phandles
> > in the same order for each "row" in the OPP table. It actually only
> > looks at the first OPP entry to figure out the list of required OPP
> > tables.
>
> Thanks for description. It is the limitation of 'required-opps' until now.
>
> >
> > Technically one can write code to deal with random order of the
> > required-opp list, but doesn't seem like that's worth it because
> > there's no need to have that order all mixed up in DT. And even if
> > someone wants to add support for that, I don't think improving the DT
> > parsing to handle random order would be part of this patch series.
>
> I understand the existing ' required-opps' only consider the same sequence
> of entries which are included in the same OPP table.
>
> One more thing, 'required-opps' properties doesn't support
> the other OPP enters of the different OPP table. Is it right of 'required-opps'?

Not sure I fully understand the question.

> Except for the random order, just each OPP might will requires
> the different 'required-opps' of different OPP table. Even if it is
> not related to random order, I think that this approach cannot
> support them.
>
> For example as following:
> - opp2 used the OPP entries of 'child_bus_A' and 'child_bus_B' opp-table.
> - opp1 used the OPP entries of 'child_bus_C' and 'child_bus_D' opp-table.
>
> parent_bus_opp: opp_table {
>     compatible = "operating-points-v2";
>
>      opp2 {
>          opp-hz = /bits/ 64 <200000>;

I'm guessing this is a typo and let's assume you meant to sat 400000

>          required-opps = <&child_bus_A_opp2>, <&child_bus_B_opp2>;
>     };
>
>    opp1 {
>          opp-hz = /bits/ 64 <200000>;
>          required-opps = <&child_bus_C_opp0>, <&child_bus_D_opp0>;
>     };
> };

Is this a real use case? If it is, in reality parent_bus_opp_table
always has requirements on all 4 children bus, just that opp1 is okay
with the lowest frequency for some of the children?

So, in this example, you just need to always list all 4 child OPPs for
each parent OPP. And some of the children OPP values might not change
when going from one parent OPP to another.

-Saravana

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
                   ` (2 preceding siblings ...)
  2019-06-22  0:34 ` [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
@ 2019-06-24  9:43 ` Viresh Kumar
  2019-06-24 22:17   ` Saravana Kannan
  3 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2019-06-24  9:43 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, kernel-team,
	linux-pm, linux-kernel

On 21-06-19, 17:34, Saravana Kannan wrote:
> 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.

Can you please provide a real world example with DT code here so I
can understand it better ?

-- 
viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-24  9:43 ` [PATCH v1 0/3] Add required-opps support to devfreq passive gov Viresh Kumar
@ 2019-06-24 22:17   ` Saravana Kannan
  2019-06-25  4:10     ` Viresh Kumar
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-24 22:17 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Mon, Jun 24, 2019 at 2:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 21-06-19, 17:34, Saravana Kannan wrote:
> > 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.
>
> Can you please provide a real world example with DT code here so I
> can understand it better ?
>

Here's an example. This can't be done today, but can be done with this change.

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";

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

Thanks,
Saravana

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-24 22:17   ` Saravana Kannan
@ 2019-06-25  4:10     ` Viresh Kumar
  2019-06-25  5:00       ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2019-06-25  4:10 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On 24-06-19, 15:17, Saravana Kannan wrote:
> Here's an example. This can't be done today, but can be done with this change.
> 
> 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";
>         };

And what is the relation between these two busses ?

>         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";
> 
> -               opp-400000000 {
> +               noc2_400: opp-400000000 {
>                         opp-hz = /bits/ 64 <400000000>;
>                 };
> -               opp-200000000 {
> +               noc2_200: opp-200000000 {
>                         opp-hz = /bits/ 64 <200000000>;
>                 };
> -               opp-134000000 {
> +               noc2_134: opp-134000000 {
>                         opp-hz = /bits/ 64 <134000000>;
>                 };
> -               opp-100000000 {
> +               noc2_100: opp-100000000 {
>                         opp-hz = /bits/ 64 <100000000>;
>                 };
>         };
> 
> Thanks,
> Saravana

-- 
viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-25  4:10     ` Viresh Kumar
@ 2019-06-25  5:00       ` Saravana Kannan
  2019-06-25  5:22         ` Viresh Kumar
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-25  5:00 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Mon, Jun 24, 2019 at 9:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-06-19, 15:17, Saravana Kannan wrote:
> > Here's an example. This can't be done today, but can be done with this change.
> >
> > 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";
> >         };
>
> And what is the relation between these two busses ?

I can't speak for the Exynos hardware. Maybe Chanwoo knows.

But a couple of common reasons to do this between devices are:
1. These were the combination of frequencies that were
validated/screen 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.

All of the cases above are some real world scenarios I've come across.
CPU and L2/L3 on ARM systems are a good example of (2) but the passive
governor doesn't work with CPUs yet. But I plan to work on that later
as that's not related to this patch series.

-Saravana

> >         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";
> >
> > -               opp-400000000 {
> > +               noc2_400: opp-400000000 {
> >                         opp-hz = /bits/ 64 <400000000>;
> >                 };
> > -               opp-200000000 {
> > +               noc2_200: opp-200000000 {
> >                         opp-hz = /bits/ 64 <200000000>;
> >                 };
> > -               opp-134000000 {
> > +               noc2_134: opp-134000000 {
> >                         opp-hz = /bits/ 64 <134000000>;
> >                 };
> > -               opp-100000000 {
> > +               noc2_100: opp-100000000 {
> >                         opp-hz = /bits/ 64 <100000000>;
> >                 };
> >         };
> >
> > Thanks,
> > Saravana
>
> --
> viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-25  5:00       ` Saravana Kannan
@ 2019-06-25  5:22         ` Viresh Kumar
  2019-06-25  5:29           ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2019-06-25  5:22 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On 24-06-19, 22:00, Saravana Kannan wrote:
> All of the cases above are some real world scenarios I've come across.
> CPU and L2/L3 on ARM systems are a good example of (2) but the passive
> governor doesn't work with CPUs yet. But I plan to work on that later
> as that's not related to this patch series.

So in case of CPUs, the cache will be the parent device and CPU be the
children ? And CPUs nodes will contain the required-opps property ?

-- 
viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-25  5:22         ` Viresh Kumar
@ 2019-06-25  5:29           ` Saravana Kannan
  2019-06-26  6:32             ` Viresh Kumar
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-25  5:29 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Mon, Jun 24, 2019 at 10:22 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-06-19, 22:00, Saravana Kannan wrote:
> > All of the cases above are some real world scenarios I've come across.
> > CPU and L2/L3 on ARM systems are a good example of (2) but the passive
> > governor doesn't work with CPUs yet. But I plan to work on that later
> > as that's not related to this patch series.
>
> So in case of CPUs, the cache will be the parent device and CPU be the
> children ? And CPUs nodes will contain the required-opps property ?

No, the CPUs will be the "parent" and the cache will be the "child".
CPU is a special case when it comes to the actual software (not DT) as
we'll need the devfreq governor to look at all the CPUfreq policies to
decide the cache frequency (max of all their requirements).

I think "master" and "slave" would have been a better term as the
master device determines its frequency using whatever means and the
"slave" device just "follows" the master device.

-Saravana

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-25  5:29           ` Saravana Kannan
@ 2019-06-26  6:32             ` Viresh Kumar
  2019-06-26 18:10               ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2019-06-26  6:32 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On 24-06-19, 22:29, Saravana Kannan wrote:
> No, the CPUs will be the "parent" and the cache will be the "child".
> CPU is a special case when it comes to the actual software (not DT) as
> we'll need the devfreq governor to look at all the CPUfreq policies to
> decide the cache frequency (max of all their requirements).
> 
> I think "master" and "slave" would have been a better term as the
> master device determines its frequency using whatever means and the
> "slave" device just "follows" the master device.

Okay, so to confirm again this is what we will have:

- CPUs are called masters and Caches are slaves.

- The devfreq governor we are talking about takes care of changing
  frequency of caches (slaves) and chooses a target frequency for
  caches based on what the masters are running at.

- The CPUs OPP nodes will have required-opps property and will be
  pointing to the caches OPP nodes. The CPUs may already be using
  required-opps node for PM domain performance state thing.


Now the problem is "required-opp" means something really *required*
and it is not optional. Like a specific voltage level before we can
switch to a particular frequency. And this is how I have though of it
until now. And this shouldn't be handled asynchronously, i.e. required
OPPs must be set while configuring OPP of a device.

So, when a CPU changes frequency, we must change the performance state
of PM domain and change frequency/bw of the cache synchronously. And
in such a case "required-opps" can be a very good fit for this use
case.

But with what you are trying to do it is no longer required-opp but
good-to-have-opp. And that worries me.

-- 
viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-26  6:32             ` Viresh Kumar
@ 2019-06-26 18:10               ` Saravana Kannan
  2019-06-28  6:49                 ` Viresh Kumar
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-26 18:10 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Tue, Jun 25, 2019 at 11:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-06-19, 22:29, Saravana Kannan wrote:
> > No, the CPUs will be the "parent" and the cache will be the "child".
> > CPU is a special case when it comes to the actual software (not DT) as
> > we'll need the devfreq governor to look at all the CPUfreq policies to
> > decide the cache frequency (max of all their requirements).
> >
> > I think "master" and "slave" would have been a better term as the
> > master device determines its frequency using whatever means and the
> > "slave" device just "follows" the master device.
>
> Okay, so to confirm again this is what we will have:
>
> - CPUs are called masters and Caches are slaves.
>
> - The devfreq governor we are talking about takes care of changing
>   frequency of caches (slaves) and chooses a target frequency for
>   caches based on what the masters are running at.
>
> - The CPUs OPP nodes will have required-opps property and will be
>   pointing to the caches OPP nodes. The CPUs may already be using
>   required-opps node for PM domain performance state thing.
>
>
> Now the problem is "required-opp" means something really *required*
> and it is not optional.

But we could interpret it as "required" for different things.

> Like a specific voltage level before we can
> switch to a particular frequency.

Required for stability.

> And this is how I have though of it
> until now. And this shouldn't be handled asynchronously, i.e. required
> OPPs must be set while configuring OPP of a device.

The users of clocks are expected to set up the voltage correctly
before changing the frequency, the drivers are expected to power up
the device before trying to access its registers, etc. So I don't
think there is one correct way. Also OPP sets the pstate only if
dev_pm_opp_set_genpd_virt_dev() has been called to set them up. So
this is not a mandatory principle in the kernel that a framework
providing an API should have all the dependencies. So I don't think
we'll be violating some golden rule.

> So, when a CPU changes frequency, we must change the performance state
> of PM domain and change frequency/bw of the cache synchronously.

I mean, it's going to be changed when we get the CPUfreq transition
notifiers. From a correctness point of view, setting it inside the OPP
framework is not any better than doing it when we get the notifiers.

> And
> in such a case "required-opps" can be a very good fit for this use
> case.

Glad you agree :)

> But with what you are trying to do it is no longer required-opp but
> good-to-have-opp. And that worries me.

I see this as "required for good performance". So I don't see it as
redefining required-opps. If someone wants good performance/power
balance they follow the "required-opps". Technically even the PM
pstates are required for good power. Otherwise, the system could leave
the voltage at max and stuff would still work.

Also, the slave device might need to get input from multiple master
devices and aggregate the request before setting the slave device
frequency. So I don't think OPP  framework would be the right place to
deal with those things. For example, L3 might (will) have different
mappings for big vs little cores. So that needs to be aggregated and
set properly by the slave device driver. Also, GPU might have a
mapping for L3 too. In which case the L3 slave driver needs to take
input from even more masters before it decides its frequency. But most
importantly, we still need the ability to change governors for L3.
Again these are just examples with L3 and it can get more complicated
based on the situation.

Most importantly, instead of always going by mapping, one might decide
to scale the L3 based on some other governor (that looks at some HW
counter). Or just set it to performance governor for a use case for
which performance is more important. All of this comes for free with
devfreq and if we always set it from OPP framework we don't give this
required control to userspace.

I think going through devfreq is the right approach for this. And we
can always rewrite the software if we find problems in the future. But
as it stands today, this will help cases like exynos without the need
for a lot of changes. Hope I've convinced you.

-Saravana

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-26 18:10               ` Saravana Kannan
@ 2019-06-28  6:49                 ` Viresh Kumar
  2019-06-28 20:26                   ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2019-06-28  6:49 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On 26-06-19, 11:10, Saravana Kannan wrote:
> On Tue, Jun 25, 2019 at 11:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > So, when a CPU changes frequency, we must change the performance state
> > of PM domain and change frequency/bw of the cache synchronously.
> 
> I mean, it's going to be changed when we get the CPUfreq transition
> notifiers. From a correctness point of view, setting it inside the OPP
> framework is not any better than doing it when we get the notifiers.

That's what the problem is. All maintainers now a days ask people to
stay away from notifiers and we are making that base of another new
thing we are starting.

Over that, with many cpufreq drivers we have fast switching enabled
and notifiers disabled. How will they make these things work ? We
still want to scale L3 in those cases as well.

> I see this as "required for good performance". So I don't see it as
> redefining required-opps. If someone wants good performance/power
> balance they follow the "required-opps". Technically even the PM
> pstates are required for good power. Otherwise, the system could leave
> the voltage at max and stuff would still work.
> 
> Also, the slave device might need to get input from multiple master
> devices and aggregate the request before setting the slave device
> frequency. So I don't think OPP  framework would be the right place to
> deal with those things. For example, L3 might (will) have different
> mappings for big vs little cores. So that needs to be aggregated and
> set properly by the slave device driver. Also, GPU might have a
> mapping for L3 too. In which case the L3 slave driver needs to take
> input from even more masters before it decides its frequency. But most
> importantly, we still need the ability to change governors for L3.
> Again these are just examples with L3 and it can get more complicated
> based on the situation.
> 
> Most importantly, instead of always going by mapping, one might decide
> to scale the L3 based on some other governor (that looks at some HW
> counter). Or just set it to performance governor for a use case for
> which performance is more important. All of this comes for free with
> devfreq and if we always set it from OPP framework we don't give this
> required control to userspace.
> 
> I think going through devfreq is the right approach for this. And we
> can always rewrite the software if we find problems in the future. But
> as it stands today, this will help cases like exynos without the need
> for a lot of changes. Hope I've convinced you.

I understand the aggregation thing and fully support that the
aggregation can't happen in OPP core and must be done somewhere else.
But the input can go from OPP core while the frequency is changing,
isn't it ?

-- 
viresh

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-28  6:49                 ` Viresh Kumar
@ 2019-06-28 20:26                   ` Saravana Kannan
  2019-07-11 23:16                     ` Saravana Kannan
  0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2019-06-28 20:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

On Thu, Jun 27, 2019 at 11:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 26-06-19, 11:10, Saravana Kannan wrote:
> > On Tue, Jun 25, 2019 at 11:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> > > So, when a CPU changes frequency, we must change the performance state
> > > of PM domain and change frequency/bw of the cache synchronously.
> >
> > I mean, it's going to be changed when we get the CPUfreq transition
> > notifiers. From a correctness point of view, setting it inside the OPP
> > framework is not any better than doing it when we get the notifiers.
>
> That's what the problem is. All maintainers now a days ask people to
> stay away from notifiers and we are making that base of another new
> thing we are starting.

In that case we can just add direct calls in cpufreq.c to let devfreq
know about the frequency changes. But then again, CPU is just one
example for this use case. I'm just using that because people are more
familiar with that.

> Over that, with many cpufreq drivers we have fast switching enabled
> and notifiers disabled. How will they make these things work ? We
> still want to scale L3 in those cases as well.

Nothing is preventing them from using the xlate OPP API I added to
figure out all the CPU to L3 frequency mapping and then set the L3
frequency directly from the CPUfreq driver.

Also, whether we use OPP framework or devfreq to set the L3 frequency,
it's going to block fast switching because both these frameworks have
APIs that can sleep.

But really, most mobile use cases don't want to permanently tie L3
freq to CPU freq. Having it go through devfreq and being able to
switch governors is a very important need for mobile products.

Keep in mind that nothing in this series does any of the cpufreq stuff
yet. That'll need a few more changes. I was just using CPUfreq as an
example.

> > I see this as "required for good performance". So I don't see it as
> > redefining required-opps. If someone wants good performance/power
> > balance they follow the "required-opps". Technically even the PM
> > pstates are required for good power. Otherwise, the system could leave
> > the voltage at max and stuff would still work.
> >
> > Also, the slave device might need to get input from multiple master
> > devices and aggregate the request before setting the slave device
> > frequency. So I don't think OPP  framework would be the right place to
> > deal with those things. For example, L3 might (will) have different
> > mappings for big vs little cores. So that needs to be aggregated and
> > set properly by the slave device driver. Also, GPU might have a
> > mapping for L3 too. In which case the L3 slave driver needs to take
> > input from even more masters before it decides its frequency. But most
> > importantly, we still need the ability to change governors for L3.
> > Again these are just examples with L3 and it can get more complicated
> > based on the situation.
> >
> > Most importantly, instead of always going by mapping, one might decide
> > to scale the L3 based on some other governor (that looks at some HW
> > counter). Or just set it to performance governor for a use case for
> > which performance is more important. All of this comes for free with
> > devfreq and if we always set it from OPP framework we don't give this
> > required control to userspace.
> >
> > I think going through devfreq is the right approach for this. And we
> > can always rewrite the software if we find problems in the future. But
> > as it stands today, this will help cases like exynos without the need
> > for a lot of changes. Hope I've convinced you.
>
> I understand the aggregation thing and fully support that the
> aggregation can't happen in OPP core and must be done somewhere else.
> But the input can go from OPP core while the frequency is changing,
> isn't it ?

I'm not opposed to OPP sending input to devfreq to let it know that a
master device frequency change is happening. But I think this is kinda
orthogonal to this patch series.

Today the passive governor looks at the master device's devfreq
frequency changes to trigger the frequency change of the slave
devfreq. It neither supports tracking OPP frequency change nor CPUfreq
frequency change. If that's something we want to add, we can look into
that separately as passive governor (or a new governor) changes.

But then not all devices (CPUfreq or otherwise) use OPP to set the
frequencies. So it's beneficial to have all of these frameworks as
inputs for devfreq passive (like) governor. CPUfreq is actually a bit
more tricky because we'll also have to track hotplug, etc. So direct
calls from CPUfreq to devfreq (similar to cpufreq stats tracking)
would be good.

-Saravana

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

* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
  2019-06-28 20:26                   ` Saravana Kannan
@ 2019-07-11 23:16                     ` Saravana Kannan
  0 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2019-07-11 23:16 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Android Kernel Team, Linux PM, LKML

Bump. I saw your email in Sibi's patch series. His patch series is
just one use case/user of this feature. This patch series is useful in
general. Do plan to Ack this? Or thoughts on my earlier response?

Thanks,
Saravana

On Fri, Jun 28, 2019 at 1:26 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jun 27, 2019 at 11:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 26-06-19, 11:10, Saravana Kannan wrote:
> > > On Tue, Jun 25, 2019 at 11:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > > So, when a CPU changes frequency, we must change the performance state
> > > > of PM domain and change frequency/bw of the cache synchronously.
> > >
> > > I mean, it's going to be changed when we get the CPUfreq transition
> > > notifiers. From a correctness point of view, setting it inside the OPP
> > > framework is not any better than doing it when we get the notifiers.
> >
> > That's what the problem is. All maintainers now a days ask people to
> > stay away from notifiers and we are making that base of another new
> > thing we are starting.
>
> In that case we can just add direct calls in cpufreq.c to let devfreq
> know about the frequency changes. But then again, CPU is just one
> example for this use case. I'm just using that because people are more
> familiar with that.
>
> > Over that, with many cpufreq drivers we have fast switching enabled
> > and notifiers disabled. How will they make these things work ? We
> > still want to scale L3 in those cases as well.
>
> Nothing is preventing them from using the xlate OPP API I added to
> figure out all the CPU to L3 frequency mapping and then set the L3
> frequency directly from the CPUfreq driver.
>
> Also, whether we use OPP framework or devfreq to set the L3 frequency,
> it's going to block fast switching because both these frameworks have
> APIs that can sleep.
>
> But really, most mobile use cases don't want to permanently tie L3
> freq to CPU freq. Having it go through devfreq and being able to
> switch governors is a very important need for mobile products.
>
> Keep in mind that nothing in this series does any of the cpufreq stuff
> yet. That'll need a few more changes. I was just using CPUfreq as an
> example.
>
> > > I see this as "required for good performance". So I don't see it as
> > > redefining required-opps. If someone wants good performance/power
> > > balance they follow the "required-opps". Technically even the PM
> > > pstates are required for good power. Otherwise, the system could leave
> > > the voltage at max and stuff would still work.
> > >
> > > Also, the slave device might need to get input from multiple master
> > > devices and aggregate the request before setting the slave device
> > > frequency. So I don't think OPP  framework would be the right place to
> > > deal with those things. For example, L3 might (will) have different
> > > mappings for big vs little cores. So that needs to be aggregated and
> > > set properly by the slave device driver. Also, GPU might have a
> > > mapping for L3 too. In which case the L3 slave driver needs to take
> > > input from even more masters before it decides its frequency. But most
> > > importantly, we still need the ability to change governors for L3.
> > > Again these are just examples with L3 and it can get more complicated
> > > based on the situation.
> > >
> > > Most importantly, instead of always going by mapping, one might decide
> > > to scale the L3 based on some other governor (that looks at some HW
> > > counter). Or just set it to performance governor for a use case for
> > > which performance is more important. All of this comes for free with
> > > devfreq and if we always set it from OPP framework we don't give this
> > > required control to userspace.
> > >
> > > I think going through devfreq is the right approach for this. And we
> > > can always rewrite the software if we find problems in the future. But
> > > as it stands today, this will help cases like exynos without the need
> > > for a lot of changes. Hope I've convinced you.
> >
> > I understand the aggregation thing and fully support that the
> > aggregation can't happen in OPP core and must be done somewhere else.
> > But the input can go from OPP core while the frequency is changing,
> > isn't it ?
>
> I'm not opposed to OPP sending input to devfreq to let it know that a
> master device frequency change is happening. But I think this is kinda
> orthogonal to this patch series.
>
> Today the passive governor looks at the master device's devfreq
> frequency changes to trigger the frequency change of the slave
> devfreq. It neither supports tracking OPP frequency change nor CPUfreq
> frequency change. If that's something we want to add, we can look into
> that separately as passive governor (or a new governor) changes.
>
> But then not all devices (CPUfreq or otherwise) use OPP to set the
> frequencies. So it's beneficial to have all of these frameworks as
> inputs for devfreq passive (like) governor. CPUfreq is actually a bit
> more tricky because we'll also have to track hotplug, etc. So direct
> calls from CPUfreq to devfreq (similar to cpufreq stats tracking)
> would be good.
>
> -Saravana

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

end of thread, other threads:[~2019-07-11 23:17 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
2019-06-22 11:49   ` Chanwoo Choi
2019-06-22 21:41     ` Saravana Kannan
2019-06-23  4:27       ` Chanwoo Choi
2019-06-23  6:07         ` Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
2019-06-22 12:00   ` Chanwoo Choi
2019-06-22 21:45     ` Saravana Kannan
2019-06-24  9:43 ` [PATCH v1 0/3] Add required-opps support to devfreq passive gov Viresh Kumar
2019-06-24 22:17   ` Saravana Kannan
2019-06-25  4:10     ` Viresh Kumar
2019-06-25  5:00       ` Saravana Kannan
2019-06-25  5:22         ` Viresh Kumar
2019-06-25  5:29           ` Saravana Kannan
2019-06-26  6:32             ` Viresh Kumar
2019-06-26 18:10               ` Saravana Kannan
2019-06-28  6:49                 ` Viresh Kumar
2019-06-28 20:26                   ` Saravana Kannan
2019-07-11 23:16                     ` Saravana Kannan

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