[PATCH v3 0/5] Add required-opps support to devfreq passive gov

[PATCH v3 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

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

v2 -> v3:
- Rebased onto linux-next.
- Added documentation comment for new fields.
- Added support for lazy required-opps linking.
- Updated Ack/Reviewed-bys.
v1 -> v2:
- Cached OPP table reference in devfreq to avoid looking up every time.
- Renamed variable in passive governor to be more intuitive.
- Updated cover letter with examples.


Saravana Kannan (5):
  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
  OPP: Improve require-opps linking
  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                 | 84 ++++++++++++++++++++++---
 drivers/opp/of.c                   | 98 +++++++++++++-----------------
 drivers/opp/opp.h                  |  5 ++
 include/linux/devfreq.h            |  2 +
 include/linux/pm_opp.h             | 11 ++++
 7 files changed, 156 insertions(+), 70 deletions(-)

-- 
2.22.0.510.g264f2c817a-goog

[PATCH v3 1/5] OPP: Allow required-opps even if the device doesn't have power-domains

[Saravana Kannan]

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   | 11 -----------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c094d5d20fd7..438fcd134d93 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -707,7 +707,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 = likely(opp) ? opp->required_opps[0]->pstate : 0;
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
 		if (ret) {
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index b313aca9894f..ff88eaf66b56 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -197,17 +197,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.510.g264f2c817a-goog

[PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

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 438fcd134d93..72c055a3f6b7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
 
+/**
+ * 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 af5021f27cb7..36f52b9cf24a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -131,6 +131,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
 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);
@@ -304,6 +307,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.510.g264f2c817a-goog

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Viresh Kumar]

On 17-07-19, 15:23, Saravana Kannan wrote:
> 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 438fcd134d93..72c055a3f6b7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
>  
> +/**
> + * 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.

You should use @ before parameters in the comments as well ? Just like
you did that below.

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

Please name it dev_pm_opp_xlate_required_opp().

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

Why can't we just compare the table pointers instead ? Yeah, I know
that's how I wrote that in the other xlate function, but I am confused
now :)

> +			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 af5021f27cb7..36f52b9cf24a 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -131,6 +131,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
>  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
>  void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
>  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);
> @@ -304,6 +307,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;
> +}
> +

Keep the order of declaring routines same, so this goes before the
other xlate routine.

>  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  {
>  	return -ENOTSUPP;
> -- 
> 2.22.0.510.g264f2c817a-goog

-- 
viresh

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 17-07-19, 15:23, Saravana Kannan wrote:
> > 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 438fcd134d93..72c055a3f6b7 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
> >
> > +/**
> > + * 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.
>
> You should use @ before parameters in the comments as well ? Just like
> you did that below.

And I should probably be deleting the @pstate phantom parameter :)

> > + *
> > + * 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,
>
> Please name it dev_pm_opp_xlate_required_opp().

Ok

>
> > +                                     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)
>
> Why can't we just compare the table pointers instead ? Yeah, I know
> that's how I wrote that in the other xlate function, but I am confused
> now :)

I almost said "not sure. Let me just compare pointers".
I think (not sure) it has to do with the same OPP table being used to
create multiple OPP table copies if the "shared OPP table" flag isn't
set?
Can you confirm if this makes sense? If so, I can add a comment patch
that adds comments to the existing code and then copies it into this
function in this patch.

> > +                     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 af5021f27cb7..36f52b9cf24a 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -131,6 +131,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> >  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
> >  void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> >  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);
> > @@ -304,6 +307,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;
> > +}
> > +
>
> Keep the order of declaring routines same, so this goes before the
> other xlate routine.

Will do.

-Saravana

> >  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  {
> >       return -ENOTSUPP;
> > --
> > 2.22.0.510.g264f2c817a-goog
>
> --
> viresh

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Viresh Kumar]

On 23-07-19, 17:23, Saravana Kannan wrote:
> On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > 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 438fcd134d93..72c055a3f6b7 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
> > >
> > > +/**
> > > + * 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.
> >
> > You should use @ before parameters in the comments as well ? Just like
> > you did that below.
> 
> And I should probably be deleting the @pstate phantom parameter :)
> 
> > > + *
> > > + * 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,
> >
> > Please name it dev_pm_opp_xlate_required_opp().
> 
> Ok
> 
> >
> > > +                                     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)
> >
> > Why can't we just compare the table pointers instead ? Yeah, I know
> > that's how I wrote that in the other xlate function, but I am confused
> > now :)
> 
> I almost said "not sure. Let me just compare pointers".
> I think (not sure) it has to do with the same OPP table being used to
> create multiple OPP table copies if the "shared OPP table" flag isn't
> set?
> Can you confirm if this makes sense? If so, I can add a comment patch
> that adds comments to the existing code and then copies it into this
> function in this patch.

Right, that was the reason but we also need to fix ...

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

... this as well. We must be comparing node pointers here as well.

> > > +                     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 af5021f27cb7..36f52b9cf24a 100644
> > > --- a/include/linux/pm_opp.h
> > > +++ b/include/linux/pm_opp.h
> > > @@ -131,6 +131,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> > >  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
> > >  void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> > >  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);
> > > @@ -304,6 +307,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;
> > > +}
> > > +
> >
> > Keep the order of declaring routines same, so this goes before the
> > other xlate routine.
> 
> Will do.
> 
> -Saravana
> 
> > >  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> > >  {
> > >       return -ENOTSUPP;
> > > --
> > > 2.22.0.510.g264f2c817a-goog
> >
> > --
> > viresh

-- 
viresh

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 17:23, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > > 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 438fcd134d93..72c055a3f6b7 100644
> > > > --- a/drivers/opp/core.c
> > > > +++ b/drivers/opp/core.c
> > > > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
> > > >
> > > > +/**
> > > > + * 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.
> > >
> > > You should use @ before parameters in the comments as well ? Just like
> > > you did that below.
> >
> > And I should probably be deleting the @pstate phantom parameter :)
> >
> > > > + *
> > > > + * 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,
> > >
> > > Please name it dev_pm_opp_xlate_required_opp().
> >
> > Ok
> >
> > >
> > > > +                                     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)
> > >
> > > Why can't we just compare the table pointers instead ? Yeah, I know
> > > that's how I wrote that in the other xlate function, but I am confused
> > > now :)
> >
> > I almost said "not sure. Let me just compare pointers".
> > I think (not sure) it has to do with the same OPP table being used to
> > create multiple OPP table copies if the "shared OPP table" flag isn't
> > set?
> > Can you confirm if this makes sense? If so, I can add a comment patch
> > that adds comments to the existing code and then copies it into this
> > function in this patch.
>
> Right, that was the reason but we also need to fix ...

I know I gave that explanation but I'm still a bit confused by the
existing logic. If the same DT OPP table is used to create multiple in
memory OPP tables, how do you device which in memory OPP table is the
right one to point to?

>
> > > > +                     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) {
>
> ... this as well. We must be comparing node pointers here as well.

Not really, if an in memory OPP entry is not part of an in memory OPP
table list, I don't think it should be considered part of the OPP
table just because the node pointer is the same. I think that's
explicitly wrong and the above code is correct as is.

-Saravana

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Viresh Kumar]

On 24-07-19, 20:46, Saravana Kannan wrote:
> On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 23-07-19, 17:23, Saravana Kannan wrote:

> > > I almost said "not sure. Let me just compare pointers".
> > > I think (not sure) it has to do with the same OPP table being used to
> > > create multiple OPP table copies if the "shared OPP table" flag isn't
> > > set?
> > > Can you confirm if this makes sense? If so, I can add a comment patch
> > > that adds comments to the existing code and then copies it into this
> > > function in this patch.
> >
> > Right, that was the reason but we also need to fix ...
> 
> I know I gave that explanation but I'm still a bit confused by the
> existing logic. If the same DT OPP table is used to create multiple in
> memory OPP tables, how do you device which in memory OPP table is the
> right one to point to?

This is a bit broken actually, we don't see any problems right now but
may eventually have to fix it someday.

We pick the first in-memory OPP table that was created using the DT
OPP table. This is done because the DT doesn't provide any explicit
linking to the required-opp device right now.

Right now the required-opps is only used for power domains and so it
is working fine. It may work fine for your case as well. But once we
have a case we want to use required-opps in a single OPP table for
both power-domains and master/slave thing you are proposing, we may
see more problems.

> > > > > +                     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) {
> >
> > ... this as well. We must be comparing node pointers here as well.
> 
> Not really, if an in memory OPP entry is not part of an in memory OPP
> table list, I don't think it should be considered part of the OPP
> table just because the node pointer is the same. I think that's
> explicitly wrong and the above code is correct as is.

I understand what you are saying, but because we match the very first
OPP table that was there in the list we need to match the DT node here
as well.

Or somehow we make sure to have the correct in-memory OPP table being
pointed by the required-opp-table array. Then we don't need the node
pointer anywhere here.

-- 
viresh

Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

On Wed, Jul 24, 2019 at 10:38 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-07-19, 20:46, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 23-07-19, 17:23, Saravana Kannan wrote:
>
> > > > I almost said "not sure. Let me just compare pointers".
> > > > I think (not sure) it has to do with the same OPP table being used to
> > > > create multiple OPP table copies if the "shared OPP table" flag isn't
> > > > set?
> > > > Can you confirm if this makes sense? If so, I can add a comment patch
> > > > that adds comments to the existing code and then copies it into this
> > > > function in this patch.
> > >
> > > Right, that was the reason but we also need to fix ...
> >
> > I know I gave that explanation but I'm still a bit confused by the
> > existing logic. If the same DT OPP table is used to create multiple in
> > memory OPP tables, how do you device which in memory OPP table is the
> > right one to point to?
>
> This is a bit broken actually, we don't see any problems right now but
> may eventually have to fix it someday.
>
> We pick the first in-memory OPP table that was created using the DT
> OPP table. This is done because the DT doesn't provide any explicit
> linking to the required-opp device right now.
>
> Right now the required-opps is only used for power domains and so it
> is working fine. It may work fine for your case as well. But once we
> have a case we want to use required-opps in a single OPP table for
> both power-domains and master/slave thing you are proposing, we may
> see more problems.
>
> > > > > > +                     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) {
> > >
> > > ... this as well. We must be comparing node pointers here as well.
> >
> > Not really, if an in memory OPP entry is not part of an in memory OPP
> > table list, I don't think it should be considered part of the OPP
> > table just because the node pointer is the same. I think that's
> > explicitly wrong and the above code is correct as is.
>
> I understand what you are saying, but because we match the very first
> OPP table that was there in the list we need to match the DT node here
> as well.
>
> Or somehow we make sure to have the correct in-memory OPP table being
> pointed by the required-opp-table array. Then we don't need the node
> pointer anywhere here.

Ah, right. I'll fix this.

-Saravana

[PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

Currently, the linking of required-opps fails silently if the
destination OPP table hasn't been added before the source OPP table is
added. This puts an unnecessary requirement that the destination table
be added before the source table is added.

In reality, the destination table is needed only when we try to
translate from source OPP to destination OPP. So, instead of
completely failing, retry linking the tables when the translation is
attempted.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c | 32 +++++++++++-----
 drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
 drivers/opp/opp.h  |  5 +++
 3 files changed, 71 insertions(+), 57 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 72c055a3f6b7..cafe6ec05d6c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -706,8 +706,11 @@ static int _set_required_opps(struct device *dev,
 	if (!required_opp_tables)
 		return 0;
 
+	_of_lazy_link_required_tables(opp_table);
+
 	/* Single genpd case */
-	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
+	if (!genpd_virt_devs && required_opp_tables[0]
+			     && required_opp_tables[0]->is_genpd) {
 		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
 		if (ret) {
@@ -726,11 +729,16 @@ static int _set_required_opps(struct device *dev,
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
-		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
-
 		if (!genpd_virt_devs[i])
 			continue;
 
+		if (!opp->required_opps[i]) {
+			ret = -ENODEV;
+			break;
+		}
+
+		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+
 		ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], pstate);
 		if (ret) {
 			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
@@ -1907,8 +1915,11 @@ struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
 	if (!src_table || !dst_table || !src_opp)
 		return NULL;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
-		if (src_table->required_opp_tables[i]->np == dst_table->np)
+		if (src_table->required_opp_tables[i]
+		    && src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
 	}
 
@@ -1971,6 +1982,8 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	if (!src_table->required_opp_count)
 		return pstate;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
 		if (src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
@@ -1986,15 +1999,16 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 
 	list_for_each_entry(opp, &src_table->opp_list, node) {
 		if (opp->pstate == pstate) {
-			dest_pstate = opp->required_opps[i]->pstate;
-			goto unlock;
+			if (opp->required_opps[i])
+				dest_pstate = opp->required_opps[i]->pstate;
+			break;
 		}
 	}
 
-	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
-	       dst_table);
+	if (dest_pstate < 0)
+		pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
+		       src_table, dst_table);
 
-unlock:
 	mutex_unlock(&src_table->lock);
 
 	return dest_pstate;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ff88eaf66b56..4e527245fd59 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -145,7 +145,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (IS_ERR_OR_NULL(required_opp_tables[i]))
-			break;
+			continue;
 
 		dev_pm_opp_put_opp_table(required_opp_tables[i]);
 	}
@@ -165,8 +165,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 					     struct device_node *opp_np)
 {
 	struct opp_table **required_opp_tables;
-	struct device_node *required_np, *np;
-	int count, i;
+	struct device_node *np;
+	int count;
 
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
@@ -176,35 +176,57 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	}
 
 	count = of_count_phandle_with_args(np, "required-opps", NULL);
+	of_node_put(np);
 	if (!count)
-		goto put_np;
+		return;
 
 	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
 				      GFP_KERNEL);
 	if (!required_opp_tables)
-		goto put_np;
+		return;
 
 	opp_table->required_opp_tables = required_opp_tables;
 	opp_table->required_opp_count = count;
+}
 
-	for (i = 0; i < count; i++) {
-		required_np = of_parse_required_opp(np, i);
-		if (!required_np)
-			goto free_required_tables;
+void _of_lazy_link_required_tables(struct opp_table *src)
+{
+	struct dev_pm_opp *src_opp, *tmp_opp;
+	struct opp_table *req_table;
+	struct device_node *req_np;
+	int i;
 
-		required_opp_tables[i] = _find_table_of_opp_np(required_np);
-		of_node_put(required_np);
+	mutex_lock(&src->lock);
 
-		if (IS_ERR(required_opp_tables[i]))
-			goto free_required_tables;
-	}
+	if (list_empty(&src->opp_list))
+		goto out;
 
-	goto put_np;
+	src_opp = list_first_entry(&src->opp_list, struct dev_pm_opp, node);
 
-free_required_tables:
-	_opp_table_free_required_tables(opp_table);
-put_np:
-	of_node_put(np);
+	for (i = 0; i < src->required_opp_count; i++) {
+		if (src->required_opp_tables[i])
+			continue;
+
+		req_np = of_parse_required_opp(src_opp->np, i);
+		if (!req_np)
+			continue;
+
+		req_table = _find_table_of_opp_np(req_np);
+		of_node_put(req_np);
+		if (!req_table)
+			continue;
+
+		src->required_opp_tables[i] = req_table;
+		list_for_each_entry(tmp_opp, &src->opp_list, node) {
+			req_np = of_parse_required_opp(tmp_opp->np, i);
+			tmp_opp->required_opps[i] = _find_opp_of_np(req_table,
+								    req_np);
+			of_node_put(req_np);
+		}
+	}
+
+out:
+	mutex_unlock(&src->lock);
 }
 
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
@@ -267,7 +289,7 @@ void _of_opp_free_required_opps(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (!required_opps[i])
-			break;
+			continue;
 
 		/* Put the reference back */
 		dev_pm_opp_put(required_opps[i]);
@@ -282,9 +304,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 				       struct dev_pm_opp *opp)
 {
 	struct dev_pm_opp **required_opps;
-	struct opp_table *required_table;
-	struct device_node *np;
-	int i, ret, count = opp_table->required_opp_count;
+	int count = opp_table->required_opp_count;
 
 	if (!count)
 		return 0;
@@ -295,32 +315,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 
 	opp->required_opps = required_opps;
 
-	for (i = 0; i < count; i++) {
-		required_table = opp_table->required_opp_tables[i];
-
-		np = of_parse_required_opp(opp->np, i);
-		if (unlikely(!np)) {
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-
-		required_opps[i] = _find_opp_of_np(required_table, np);
-		of_node_put(np);
-
-		if (!required_opps[i]) {
-			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
-			       __func__, opp->np, i);
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-	}
-
 	return 0;
-
-free_required_opps:
-	_of_opp_free_required_opps(opp_table, opp);
-
-	return ret;
 }
 
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..5b074eb7da07 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -225,12 +225,17 @@ void _of_clear_opp_table(struct opp_table *opp_table);
 struct opp_table *_managed_opp(struct device *dev, int index);
 void _of_opp_free_required_opps(struct opp_table *opp_table,
 				struct dev_pm_opp *opp);
+void _of_lazy_link_required_tables(struct opp_table *src);
 #else
 static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
 static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
 static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
 static inline void _of_opp_free_required_opps(struct opp_table *opp_table,
 					      struct dev_pm_opp *opp) {}
+void bool _of_lazy_link_required_tables(struct opp_table *src)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.22.0.510.g264f2c817a-goog

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

$subject doesn't have correct property name.

On 17-07-19, 15:23, Saravana Kannan wrote:
> Currently, the linking of required-opps fails silently if the
> destination OPP table hasn't been added before the source OPP table is
> added. This puts an unnecessary requirement that the destination table
> be added before the source table is added.
> 
> In reality, the destination table is needed only when we try to
> translate from source OPP to destination OPP. So, instead of
> completely failing, retry linking the tables when the translation is
> attempted.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c | 32 +++++++++++-----
>  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
>  drivers/opp/opp.h  |  5 +++
>  3 files changed, 71 insertions(+), 57 deletions(-)

Here is the general feedback and requirements I have:

- We shouldn't do it from _set_required_opps() but way earlier, it
  shouldn't affect the fast path (where we change the frequency).

- Programming required-opps for half of the properties isn't correct,
  i.e. in case only few of the required-opps are parsed until now. So
  setting of rate shouldn't even start unless the OPP table is fully
  initialized with all required-opps in it.

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

Resending again due to accidental HTML (minor rewording/typo fixes too).

On Tue, Jul 23, 2019 at 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> $subject doesn't have correct property name.
>
> On 17-07-19, 15:23, Saravana Kannan wrote:
> > Currently, the linking of required-opps fails silently if the
> > destination OPP table hasn't been added before the source OPP table is
> > added. This puts an unnecessary requirement that the destination table
> > be added before the source table is added.
> >
> > In reality, the destination table is needed only when we try to
> > translate from source OPP to destination OPP. So, instead of
> > completely failing, retry linking the tables when the translation is
> > attempted.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c | 32 +++++++++++-----
> >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> >  drivers/opp/opp.h  |  5 +++
> >  3 files changed, 71 insertions(+), 57 deletions(-)
>
> Here is the general feedback and requirements I have:
>
> - We shouldn't do it from _set_required_opps() but way earlier, it
>   shouldn't affect the fast path (where we change the frequency).

I don't think there's any other intermediate OPP API call where we can
try to lazily link the tables. Also if the tables are already fully
linked, I don't think this is really going to slow down the fast path
in anyway (because it's just a few NULL checks).

If you have other ideas, I'm willing to look at it. But as it is right
now seems the best to me.

> - Programming required-opps for half of the properties isn't correct,
>   i.e. in case only few of the required-opps are parsed until now. So
>   setting of rate shouldn't even start unless the OPP table is fully
>   initialized with all required-opps in it.

Ok, I can fix this.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 23-07-19, 07:47, Saravana Kannan wrote:
> On Tue, Jul 23, 2019, 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > $subject doesn't have correct property name.
> >
> > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > Currently, the linking of required-opps fails silently if the
> > > destination OPP table hasn't been added before the source OPP table is
> > > added. This puts an unnecessary requirement that the destination table
> > > be added before the source table is added.
> > >
> > > In reality, the destination table is needed only when we try to
> > > translate from source OPP to destination OPP. So, instead of
> > > completely failing, retry linking the tables when the translation is
> > > attempted.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/opp/core.c | 32 +++++++++++-----
> > >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> > >  drivers/opp/opp.h  |  5 +++
> > >  3 files changed, 71 insertions(+), 57 deletions(-)
> >
> > Here is the general feedback and requirements I have:
> >
> > - We shouldn't do it from _set_required_opps() but way earlier, it
> >   shouldn't affect the fast path (where we change the frequency).
> >
> 
> I don't think there's any other intermediate OPP call where we can try to
> link the tables. Also if there tables are already fully linked, this is
> just checking for not NULL for a few elements in the array.

We should be doing this whenever a new OPP table is created, and see
if someone else was waiting for this OPP table to come alive. Also we
must make sure that we do this linking only if the new OPP table has
its own required-opps links fixed, otherwise delay further.

> I don't think
> this is really going to allow down the fast path in anyway.

> If you have other ideas, I'm willing to look at it. But as it is right now
> seems the best to me.
> 
Even then I don't want to add these checks to those places. For the
opp-set-rate routine, add another flag to the OPP table which
indicates if we are ready to do dvfs or not and mark it true only
after the required-opps are all set.

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 07:47, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019, 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > $subject doesn't have correct property name.
> > >
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > > Currently, the linking of required-opps fails silently if the
> > > > destination OPP table hasn't been added before the source OPP table is
> > > > added. This puts an unnecessary requirement that the destination table
> > > > be added before the source table is added.
> > > >
> > > > In reality, the destination table is needed only when we try to
> > > > translate from source OPP to destination OPP. So, instead of
> > > > completely failing, retry linking the tables when the translation is
> > > > attempted.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c | 32 +++++++++++-----
> > > >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> > > >  drivers/opp/opp.h  |  5 +++
> > > >  3 files changed, 71 insertions(+), 57 deletions(-)
> > >
> > > Here is the general feedback and requirements I have:
> > >
> > > - We shouldn't do it from _set_required_opps() but way earlier, it
> > >   shouldn't affect the fast path (where we change the frequency).
> > >
> >
> > I don't think there's any other intermediate OPP call where we can try to
> > link the tables. Also if there tables are already fully linked, this is
> > just checking for not NULL for a few elements in the array.
>
> We should be doing this whenever a new OPP table is created, and see
> if someone else was waiting for this OPP table to come alive.

Searching the global OPP table list seems a ton more wasteful than
doing the lazy linking. I'd rather not do this.

> Also we
> must make sure that we do this linking only if the new OPP table has
> its own required-opps links fixed, otherwise delay further.

This can be done. Although even without doing that, this patch is
making things better by not failing silently like it does today? Can I
do this later as a separate patch set series?

> > I don't think
> > this is really going to allow down the fast path in anyway.
>
> > If you have other ideas, I'm willing to look at it. But as it is right now
> > seems the best to me.
> >
> Even then I don't want to add these checks to those places. For the
> opp-set-rate routine, add another flag to the OPP table which
> indicates if we are ready to do dvfs or not and mark it true only
> after the required-opps are all set.

Honestly, this seems like extra memory and micro optimization without
any data to back it. Show me data that checking all these table
pointers is noticeably slower than what I'm doing. What's the max
"required tables count" you've seen in upstream so far?

I'd even argue that doing it the way I do might actually reduce the
cache misses/warm the cache because those pointers are going to be
searched/used right after anyway.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 24-07-19, 21:09, Saravana Kannan wrote:
> On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > We should be doing this whenever a new OPP table is created, and see
> > if someone else was waiting for this OPP table to come alive.
> 
> Searching the global OPP table list seems a ton more wasteful than
> doing the lazy linking. I'd rather not do this.

We can see how best to optimize that, but it will be done only once
while a new OPP table is created and putting stress there is the right
thing to do IMO. And doing anything like that in a place like
opp-set-rate is the worst one. It will be a bad choice by design if
you ask me and so I am very much against that.

> > Also we
> > must make sure that we do this linking only if the new OPP table has
> > its own required-opps links fixed, otherwise delay further.
> 
> This can be done. Although even without doing that, this patch is
> making things better by not failing silently like it does today? Can I
> do this later as a separate patch set series?

I would like this to get fixed now in a proper way, there is no hurry
for a quick fix currently. No band-aids please.

> > Even then I don't want to add these checks to those places. For the
> > opp-set-rate routine, add another flag to the OPP table which
> > indicates if we are ready to do dvfs or not and mark it true only
> > after the required-opps are all set.
> 
> Honestly, this seems like extra memory and micro optimization without
> any data to back it.

Again, opp-set-rate isn't supposed to do something like this. It
shouldn't handle initializations of things, that is broken design.

> Show me data that checking all these table
> pointers is noticeably slower than what I'm doing. What's the max
> "required tables count" you've seen in upstream so far?

Running anything extra (specially some initialization stuff) in
opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
is my responsibility to not allow such things to happen.

> I'd even argue that doing it the way I do might actually reduce the
> cache misses/warm the cache because those pointers are going to be
> searched/used right after anyway.

So you want to make the cache hot with data, by running some code at a
place where it is not required to be run really, and the fact that
most of the data cached may not get used anyway ? And that is an
improvement somehow ?

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Wed, Jul 24, 2019 at 10:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-07-19, 21:09, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > We should be doing this whenever a new OPP table is created, and see
> > > if someone else was waiting for this OPP table to come alive.
> >
> > Searching the global OPP table list seems a ton more wasteful than
> > doing the lazy linking. I'd rather not do this.
>
> We can see how best to optimize that, but it will be done only once
> while a new OPP table is created and putting stress there is the right
> thing to do IMO. And doing anything like that in a place like
> opp-set-rate is the worst one. It will be a bad choice by design if
> you ask me and so I am very much against that.
>
> > > Also we
> > > must make sure that we do this linking only if the new OPP table has
> > > its own required-opps links fixed, otherwise delay further.
> >
> > This can be done. Although even without doing that, this patch is
> > making things better by not failing silently like it does today? Can I
> > do this later as a separate patch set series?
>
> I would like this to get fixed now in a proper way, there is no hurry
> for a quick fix currently. No band-aids please.
>
> > > Even then I don't want to add these checks to those places. For the
> > > opp-set-rate routine, add another flag to the OPP table which
> > > indicates if we are ready to do dvfs or not and mark it true only
> > > after the required-opps are all set.
> >
> > Honestly, this seems like extra memory and micro optimization without
> > any data to back it.
>
> Again, opp-set-rate isn't supposed to do something like this. It
> shouldn't handle initializations of things, that is broken design.
>
> > Show me data that checking all these table
> > pointers is noticeably slower than what I'm doing. What's the max
> > "required tables count" you've seen in upstream so far?
>
> Running anything extra (specially some initialization stuff) in
> opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
> is my responsibility to not allow such things to happen.

Doing operations lazily right before they are needed isn't something
new in the kernel. It's done all over the place (VFP save/restore?).
It's not worth arguing though -- so I'll agree to disagree but follow
the Maintainer's preference.

> > I'd even argue that doing it the way I do might actually reduce the
> > cache misses/warm the cache because those pointers are going to be
> > searched/used right after anyway.
>
> So you want to make the cache hot with data, by running some code at a
> place where it is not required to be run really, and the fact that
> most of the data cached may not get used anyway ? And that is an
> improvement somehow ?

My point is that both of us are hypothesizing and for some
micro-optimization like this, data is needed.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Thu, Jul 25, 2019 at 6:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 24, 2019 at 10:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 24-07-19, 21:09, Saravana Kannan wrote:
> > > On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > We should be doing this whenever a new OPP table is created, and see
> > > > if someone else was waiting for this OPP table to come alive.
> > >
> > > Searching the global OPP table list seems a ton more wasteful than
> > > doing the lazy linking. I'd rather not do this.
> >
> > We can see how best to optimize that, but it will be done only once
> > while a new OPP table is created and putting stress there is the right
> > thing to do IMO. And doing anything like that in a place like
> > opp-set-rate is the worst one. It will be a bad choice by design if
> > you ask me and so I am very much against that.
> >
> > > > Also we
> > > > must make sure that we do this linking only if the new OPP table has
> > > > its own required-opps links fixed, otherwise delay further.
> > >
> > > This can be done. Although even without doing that, this patch is
> > > making things better by not failing silently like it does today? Can I
> > > do this later as a separate patch set series?
> >
> > I would like this to get fixed now in a proper way, there is no hurry
> > for a quick fix currently. No band-aids please.
> >
> > > > Even then I don't want to add these checks to those places. For the
> > > > opp-set-rate routine, add another flag to the OPP table which
> > > > indicates if we are ready to do dvfs or not and mark it true only
> > > > after the required-opps are all set.
> > >
> > > Honestly, this seems like extra memory and micro optimization without
> > > any data to back it.
> >
> > Again, opp-set-rate isn't supposed to do something like this. It
> > shouldn't handle initializations of things, that is broken design.
> >
> > > Show me data that checking all these table
> > > pointers is noticeably slower than what I'm doing. What's the max
> > > "required tables count" you've seen in upstream so far?
> >
> > Running anything extra (specially some initialization stuff) in
> > opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
> > is my responsibility to not allow such things to happen.
>
> Doing operations lazily right before they are needed isn't something
> new in the kernel. It's done all over the place (VFP save/restore?).
> It's not worth arguing though -- so I'll agree to disagree but follow
> the Maintainer's preference.
>

I was taking a closer look at the OPP framework code to try and do
what you ask above, but it's kind of a mess. The whole "the same OPP
table can be used by multiple devices without the opp-shared flag set"
is effectively breaking "required-opps" at a minimum and probably a
lot more cases. I don't think I can rewrite my patch the way you want
it without fixing the existing bugs.

Let's take this example DT (leaving out the irrelevant part):

OPP table 1:
    required-opps = <OPP table 2 entry>;

OPP table 2:
    <opp-shared property not set>

Device A:
    operating-points-v2 = <&OPP table 1>

Device B:
    operating-points-v2 = <&OPP table 2>

Device C:
    operating-points-v2 = <&OPP table 2>

Let's say device B and C add their OPP tables. They both get their own
"in-memory" copy of the OPP table. They can then enabled/disable
different OPP entries (rows) and not affect each other's OPP table.
Which is how it's expected to work.

Now if device A adds its OPP table 1, the "in-memory"
required_opp_tables pointer of OPP table 1 can end up pointing to
either Device A's copy of the OPP table or Device B's copy of the OPP
table depending on which happens to be added first. This effectively
random linking of OPP tables is mutually exclusive to the point of
required-opps.

Also, at a DT definition level, OPP table 1 pointing to OPP table 2
when OPP table 2 is used by more than one device doesn't make any
sense. Which device/genpd is OPP table 1 saying it "requires to
operate at a certain level"?

So I propose that we should consider the OPP table DT configuration
invalid if one OPP table points to another OPP tables that's NOT
shared but is ALSO pointed to by multiple devices. Basically the
example above would be considered an invalid DT configuration. Does
that sound okay to you? If I make changes to enforce that, will that
be acceptable?

If this sounds okay to you, then in the example above, assume Device C
isn't present. Then when OPP table 1 is added by device A, if OPP
table 2 hasn't been added already, I can just go ahead and allocate
OPP table 2. And then when device B tries to add OPP table 2, I can
just tie device B to OPP table 2 and fill up any of the missing
pieces.

This sounds better than trying to loop through existing OPP tables and
seeing if any other table is waiting for the newly added table and
marking the waiting tables as "linked". Especially because it gets a
lot more complicated and inefficient when you consider a chain of OPP
tables and many-to-many linking of OPP tables.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 26-07-19, 14:23, Saravana Kannan wrote:
> I was taking a closer look at the OPP framework code to try and do
> what you ask above, but it's kind of a mess. The whole "the same OPP
> table can be used by multiple devices without the opp-shared flag set"
> is effectively breaking "required-opps" at a minimum and probably a
> lot more cases. I don't think I can rewrite my patch the way you want
> it without fixing the existing bugs.
> 
> Let's take this example DT (leaving out the irrelevant part):
> 
> OPP table 1:
>     required-opps = <OPP table 2 entry>;
> 
> OPP table 2:
>     <opp-shared property not set>
> 
> Device A:
>     operating-points-v2 = <&OPP table 1>
> 
> Device B:
>     operating-points-v2 = <&OPP table 2>
> 
> Device C:
>     operating-points-v2 = <&OPP table 2>
> 
> Let's say device B and C add their OPP tables. They both get their own
> "in-memory" copy of the OPP table. They can then enabled/disable
> different OPP entries (rows) and not affect each other's OPP table.
> Which is how it's expected to work.
> 
> Now if device A adds its OPP table 1, the "in-memory"
> required_opp_tables pointer of OPP table 1 can end up pointing to
> either Device A's copy of the OPP table or Device B's copy of the OPP
> table depending on which happens to be added first. This effectively
> random linking of OPP tables is mutually exclusive to the point of
> required-opps.

> Also, at a DT definition level, OPP table 1 pointing to OPP table 2
> when OPP table 2 is used by more than one device doesn't make any
> sense. Which device/genpd is OPP table 1 saying it "requires to
> operate at a certain level"?

I will say that this data is present at least for the present set of
users, i.e. power domains. Just that it isn't present so directly
within the OPP table. But if you look at the device's node, it will
point you to some power domain, etc.

I didn't do anything about it earlier because it required more work
and I thought "required-opps" property is there to get us some data
and it does get that data to us right now. Just that we don't know the
device for which this data is there in the OPP core.

If we do want to get this linking, how should we get it ?

- Using another property in device's DT node, like
  "required-opp-devices" ? I didn't like it earlier because that would
  be like duplicating data we already had for the power domains.

- Using some in-kernel function, using which other drivers can give us
  this data ? Maybe yes. Probably that's the best way of doing it ?

> So I propose that we should consider the OPP table DT configuration
> invalid if one OPP table points to another OPP tables that's NOT
> shared but is ALSO pointed to by multiple devices. Basically the
> example above would be considered an invalid DT configuration. Does
> that sound okay to you? If I make changes to enforce that, will that
> be acceptable?

I don't think that would be the right thing to do as the idea of
sharing the DT OPP tables to avoid duplicate tables in DT was the
correct thing to do and is getting used very much right now as well.

Perhaps we should fix the problem instead.

> If this sounds okay to you, then in the example above, assume Device C
> isn't present. Then when OPP table 1 is added by device A, if OPP
> table 2 hasn't been added already, I can just go ahead and allocate
> OPP table 2. And then when device B tries to add OPP table 2, I can
> just tie device B to OPP table 2 and fill up any of the missing
> pieces.

I can assure you that it would be a big mess. Specially the reference
counting using which we free OPP tables and OPPs right now will come
in your way.

> This sounds better than trying to loop through existing OPP tables and
> seeing if any other table is waiting for the newly added table and
> marking the waiting tables as "linked". Especially because it gets a
> lot more complicated and inefficient when you consider a chain of OPP
> tables and many-to-many linking of OPP tables.

Firstly, this is all going to be initialization code and will not run
after boot normally. And we will probably not have very complex
linking cases as well I believe. Maybe 2-3 levels at the best. And
this will keep code much simpler compared to the above idea.

So here is the thing. I think you should separate out the lazy-linking
thing from this patchset as this is a completely different problem you
are trying to solve and is unnecessarily blocking other patches.

And if you don't want to get into solving the lazy linking thing
yourself, because that is consuming your time unnecessarily, then I
can see if I can put some of my cycles on it.

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Hsin-Yi Wang]

On Wed, Jul 17, 2019 at 10:23 PM Saravana Kannan <saravanak@google.com> wrote:

> -free_required_tables:
> -       _opp_table_free_required_tables(opp_table);
> -put_np:
> -       of_node_put(np);
> +       for (i = 0; i < src->required_opp_count; i++) {
> +               if (src->required_opp_tables[i])
> +                       continue;
> +
> +               req_np = of_parse_required_opp(src_opp->np, i);
> +               if (!req_np)
> +                       continue;
> +
> +               req_table = _find_table_of_opp_np(req_np);
Not yet tested in v4, but in v3:
In _find_table_of_opp_np(), there's a lockdep check:
lockdep_assert_held(&opp_table_lock);
which would lead to lockdep warnings.

Call trace:
_find_table_of_opp_np
_of_lazy_link_required_tables
dev_pm_opp_xlate_opp

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Tue, Jul 30, 2019 at 4:03 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, Jul 17, 2019 at 10:23 PM Saravana Kannan <saravanak@google.com> wrote:
>
> > -free_required_tables:
> > -       _opp_table_free_required_tables(opp_table);
> > -put_np:
> > -       of_node_put(np);
> > +       for (i = 0; i < src->required_opp_count; i++) {
> > +               if (src->required_opp_tables[i])
> > +                       continue;
> > +
> > +               req_np = of_parse_required_opp(src_opp->np, i);
> > +               if (!req_np)
> > +                       continue;
> > +
> > +               req_table = _find_table_of_opp_np(req_np);
> Not yet tested in v4, but in v3:
> In _find_table_of_opp_np(), there's a lockdep check:
> lockdep_assert_held(&opp_table_lock);
> which would lead to lockdep warnings.
>
> Call trace:
> _find_table_of_opp_np
> _of_lazy_link_required_tables
> dev_pm_opp_xlate_opp

Thanks for testing. I'll keep this in mind based on where the rest of
the discussion goes.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 17-07-19, 15:23, Saravana Kannan wrote:
> Currently, the linking of required-opps fails silently if the
> destination OPP table hasn't been added before the source OPP table is
> added. This puts an unnecessary requirement that the destination table
> be added before the source table is added.

Here is my version of lazy-linking. Please give it a try. I have tested it by
faking hardware, it would be better if it gets tested properly in some platform.

--
viresh

-------------------------8<-------------------------
From 8df083ca64d82ff57f778689271cc1be75aa99c4 Mon Sep 17 00:00:00 2001
Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 25 Nov 2019 13:57:58 +0530
Subject: [PATCH] opp: Allow lazy-linking of required-opps

The OPP core currently requires the required opp tables to be available
before the dependent OPP table is added, as it needs to create links
from the dependent OPP table to the required ones. This may not be
convenient to all the platforms though, as this requires strict ordering
of probing of drivers.

This patch allows lazy-linking of the required-opps. The OPP tables for
which the required-opp-tables aren't available at the time of their
initialization, are added to a special list of OPP tables:
pending_opp_tables. Later on, whenever a new OPP table is registered
with the OPP core, we check if it is required by an OPP table in the
pending list; if yes, then we complete the linking then and there.

An OPP table is marked unusable until the time all its required-opp
tables are available. And if lazy-linking fails for an OPP table, the
OPP core disables all of its OPPs to make sure no one can use them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  13 ++++++
 drivers/opp/of.c   | 113 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/opp/opp.h  |   4 +-
 3 files changed, 124 insertions(+), 6 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index ba43e6a3dc0a..cafd468443a6 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -27,6 +27,10 @@
  * various states of availability.
  */
 LIST_HEAD(opp_tables);
+
+/* The root of the list of opp-tables that aren't fully initialized yet */
+LIST_HEAD(pending_opp_tables);
+
 /* Lock to allow exclusive modification to the device and opp lists */
 DEFINE_MUTEX(opp_table_lock);
 
@@ -754,6 +758,10 @@ static int _set_required_opps(struct device *dev,
 	if (!required_opp_tables)
 		return 0;
 
+	/* required-opps not fully initialized yet */
+	if (!list_empty(&opp_table->pending))
+		return -EBUSY;
+
 	/* Single genpd case */
 	if (!genpd_virt_devs) {
 		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
@@ -964,6 +972,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	mutex_init(&opp_table->lock);
 	mutex_init(&opp_table->genpd_virt_dev_lock);
 	INIT_LIST_HEAD(&opp_table->dev_list);
+	INIT_LIST_HEAD(&opp_table->pending);
 
 	/* Mark regulator count uninitialized */
 	opp_table->regulator_count = -1;
@@ -1946,6 +1955,10 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	if (!pstate)
 		return 0;
 
+	/* required-opps not fully initialized yet */
+	if (!list_empty(&src_table->pending))
+		return -EBUSY;
+
 	/*
 	 * Normally the src_table will have the "required_opps" property set to
 	 * point to one of the OPPs in the dst_table, but in some cases the
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 9cd8f0adacae..a17bb47c39a5 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -143,7 +143,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (IS_ERR_OR_NULL(required_opp_tables[i]))
-			break;
+			continue;
 
 		dev_pm_opp_put_opp_table(required_opp_tables[i]);
 	}
@@ -152,6 +152,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	opp_table->required_opp_count = 0;
 	opp_table->required_opp_tables = NULL;
+	list_del(&opp_table->pending);
 }
 
 /*
@@ -164,6 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 {
 	struct opp_table **required_opp_tables;
 	struct device_node *required_np, *np;
+	bool pending = false;
 	int count, i;
 
 	/* Traversing the first OPP node is all we need */
@@ -193,8 +195,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		required_opp_tables[i] = _find_table_of_opp_np(required_np);
 		of_node_put(required_np);
 
-		if (IS_ERR(required_opp_tables[i]))
-			goto free_required_tables;
+		if (IS_ERR(required_opp_tables[i])) {
+			pending = true;
+			continue;
+		}
 
 		/*
 		 * We only support genpd's OPPs in the "required-opps" for now,
@@ -208,6 +212,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		}
 	}
 
+	/* Let's do the linking later on */
+	if (pending)
+		list_add(&opp_table->pending, &pending_opp_tables);
+
 	goto put_np;
 
 free_required_tables:
@@ -276,7 +284,7 @@ void _of_opp_free_required_opps(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (!required_opps[i])
-			break;
+			continue;
 
 		/* Put the reference back */
 		dev_pm_opp_put(required_opps[i]);
@@ -307,6 +315,10 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	for (i = 0; i < count; i++) {
 		required_table = opp_table->required_opp_tables[i];
 
+		/* Required table not added yet, we will link later */
+		if (IS_ERR_OR_NULL(required_table))
+			continue;
+
 		np = of_parse_required_opp(opp->np, i);
 		if (unlikely(!np)) {
 			ret = -ENODEV;
@@ -332,6 +344,97 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	return ret;
 }
 
+static int lazy_link_required_opps(struct opp_table *opp_table,
+				   struct opp_table *required_opp_table,
+				   int index)
+{
+	struct device_node *required_np;
+	struct dev_pm_opp *opp;
+
+	list_for_each_entry(opp, &opp_table->opp_list, node) {
+		required_np = of_parse_required_opp(opp->np, index);
+		if (unlikely(!required_np))
+			return -ENODEV;
+
+		opp->required_opps[index] = _find_opp_of_np(required_opp_table, required_np);
+		of_node_put(required_np);
+
+		if (!opp->required_opps[index]) {
+			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
+					__func__, opp->np, index);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
+static void lazy_link_required_opp_table(struct opp_table *required_opp_table)
+{
+	struct opp_table *opp_table, *temp, **required_opp_tables;
+	struct device_node *required_np, *opp_np, *required_table_np;
+	int i, ret;
+
+	/*
+	 * We only support genpd's OPPs in the "required-opps" for now,
+	 * as we don't know much about other cases.
+	 */
+	if (!required_opp_table->is_genpd)
+		return;
+
+	mutex_lock(&opp_table_lock);
+
+	list_for_each_entry_safe(opp_table, temp, &pending_opp_tables, pending) {
+		bool pending = false;
+
+		/* opp_np can't be invalid here */
+		opp_np = of_get_next_available_child(opp_table->np, NULL);
+
+		for (i = 0; i < opp_table->required_opp_count; i++) {
+			required_opp_tables = opp_table->required_opp_tables;
+
+			if (!IS_ERR_OR_NULL(required_opp_tables[i]))
+				continue;
+
+			/* required_np can't be invalid here */
+			required_np = of_parse_required_opp(opp_np, i);
+			required_table_np = of_get_parent(required_np);
+
+			of_node_put(required_table_np);
+			of_node_put(required_np);
+
+			if (required_table_np != required_opp_table->np) {
+				pending = true;
+				continue;
+			}
+
+			required_opp_tables[i] = required_opp_table;
+			_get_opp_table_kref(required_opp_table);
+
+			/* Link OPPs now */
+			ret = lazy_link_required_opps(opp_table, required_opp_table, i);
+			if (ret) {
+				struct dev_pm_opp *opp;
+
+				/* Mark OPPs unusable on error */
+				list_for_each_entry(opp, &opp_table->opp_list, node)
+					opp->available = false;
+				break;
+			}
+		}
+
+		of_node_put(opp_np);
+
+		/* All required opp-tables found, remove from pending list */
+		if (!pending) {
+			list_del(&opp_table->pending);
+			INIT_LIST_HEAD(&opp_table->pending);
+		}
+	}
+
+	mutex_unlock(&opp_table_lock);
+}
+
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			      struct device_node *np)
 {
@@ -702,6 +805,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	if (pstate_count)
 		opp_table->genpd_performance_state = true;
 
+	lazy_link_required_opp_table(opp_table);
+
 	return 0;
 
 remove_static_opp:
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index d14e27102730..1acbea35d58e 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -25,7 +25,7 @@ struct regulator;
 /* Lock to allow exclusive modification to the device and opp lists */
 extern struct mutex opp_table_lock;
 
-extern struct list_head opp_tables;
+extern struct list_head opp_tables, pending_opp_tables;
 
 /*
  * Internal data structure organization with the OPP layer library is as
@@ -160,7 +160,7 @@ enum opp_table_access {
  * meant for book keeping and private to OPP library.
  */
 struct opp_table {
-	struct list_head node;
+	struct list_head node, pending;
 
 	struct blocking_notifier_head head;
 	struct list_head dev_list;
-- 
2.21.0.rc0.269.g1a574e7a288b

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 25-11-19, 16:58, Viresh Kumar wrote:
> Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Mon, 25 Nov 2019 13:57:58 +0530
> Subject: [PATCH] opp: Allow lazy-linking of required-opps

Forgot to mention that this is based of pm/linux-next + following series

https://lore.kernel.org/lkml/befccaf76d647f30e03c115ed7a096ebd5384ecd.1574074666.git.viresh.kumar@linaro.org/

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Mon, Nov 25, 2019 at 3:29 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-11-19, 16:58, Viresh Kumar wrote:
> > Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
> > From: Viresh Kumar <viresh.kumar@linaro.org>
> > Date: Mon, 25 Nov 2019 13:57:58 +0530
> > Subject: [PATCH] opp: Allow lazy-linking of required-opps
>
> Forgot to mention that this is based of pm/linux-next + following series
>
> https://lore.kernel.org/lkml/befccaf76d647f30e03c115ed7a096ebd5384ecd.1574074666.git.viresh.kumar@linaro.org/

Thanks Viresh. Is there a git I can pull a branch that has your lazy
linking patch series and whatever dependencies it has?

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 05-12-19, 11:11, Saravana Kannan wrote:
> On Mon, Nov 25, 2019 at 3:29 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 25-11-19, 16:58, Viresh Kumar wrote:
> > > Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
> > > From: Viresh Kumar <viresh.kumar@linaro.org>
> > > Date: Mon, 25 Nov 2019 13:57:58 +0530
> > > Subject: [PATCH] opp: Allow lazy-linking of required-opps
> >
> > Forgot to mention that this is based of pm/linux-next + following series
> >
> > https://lore.kernel.org/lkml/befccaf76d647f30e03c115ed7a096ebd5384ecd.1574074666.git.viresh.kumar@linaro.org/
> 
> Thanks Viresh. Is there a git I can pull a branch that has your lazy
> linking patch series and whatever dependencies it has?

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/lazy-linking

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 25-11-19, 16:58, Viresh Kumar wrote:
> >From 8df083ca64d82ff57f778689271cc1be75aa99c4 Mon Sep 17 00:00:00 2001
> Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Mon, 25 Nov 2019 13:57:58 +0530
> Subject: [PATCH] opp: Allow lazy-linking of required-opps
> 
> The OPP core currently requires the required opp tables to be available
> before the dependent OPP table is added, as it needs to create links
> from the dependent OPP table to the required ones. This may not be
> convenient to all the platforms though, as this requires strict ordering
> of probing of drivers.
> 
> This patch allows lazy-linking of the required-opps. The OPP tables for
> which the required-opp-tables aren't available at the time of their
> initialization, are added to a special list of OPP tables:
> pending_opp_tables. Later on, whenever a new OPP table is registered
> with the OPP core, we check if it is required by an OPP table in the
> pending list; if yes, then we complete the linking then and there.
> 
> An OPP table is marked unusable until the time all its required-opp
> tables are available. And if lazy-linking fails for an OPP table, the
> OPP core disables all of its OPPs to make sure no one can use them.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c |  13 ++++++
>  drivers/opp/of.c   | 113 +++++++++++++++++++++++++++++++++++++++++++--
>  drivers/opp/opp.h  |   4 +-
>  3 files changed, 124 insertions(+), 6 deletions(-)

I was hoping to queue this up for next release, any update on getting
this tested ?


-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Sibi Sankar]

Hey Viresh,


On 1/27/20 11:41 AM, Viresh Kumar wrote:
> On 25-11-19, 16:58, Viresh Kumar wrote:
>> >From 8df083ca64d82ff57f778689271cc1be75aa99c4 Mon Sep 17 00:00:00 2001
>> Message-Id: <8df083ca64d82ff57f778689271cc1be75aa99c4.1574681211.git.viresh.kumar@linaro.org>
>> From: Viresh Kumar <viresh.kumar@linaro.org>
>> Date: Mon, 25 Nov 2019 13:57:58 +0530
>> Subject: [PATCH] opp: Allow lazy-linking of required-opps
>>
>> The OPP core currently requires the required opp tables to be available
>> before the dependent OPP table is added, as it needs to create links
>> from the dependent OPP table to the required ones. This may not be
>> convenient to all the platforms though, as this requires strict ordering
>> of probing of drivers.
>>
>> This patch allows lazy-linking of the required-opps. The OPP tables for
>> which the required-opp-tables aren't available at the time of their
>> initialization, are added to a special list of OPP tables:
>> pending_opp_tables. Later on, whenever a new OPP table is registered
>> with the OPP core, we check if it is required by an OPP table in the
>> pending list; if yes, then we complete the linking then and there.
>>
>> An OPP table is marked unusable until the time all its required-opp
>> tables are available. And if lazy-linking fails for an OPP table, the
>> OPP core disables all of its OPPs to make sure no one can use them.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   drivers/opp/core.c |  13 ++++++
>>   drivers/opp/of.c   | 113 +++++++++++++++++++++++++++++++++++++++++++--
>>   drivers/opp/opp.h  |   4 +-
>>   3 files changed, 124 insertions(+), 6 deletions(-)
> 
> I was hoping to queue this up for next release, any update on getting
> this tested ?

I don't have a gen-pd use case to test against but with the is_genpd
check removed it works as expected when I used it against this
series: https://patchwork.kernel.org/cover/11353185/

In the lazy_link_required_opps fn shouldn't we skip the dynamic
opps in the the opp list?

With ^^ addressed:
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Tested-by: Sibi Sankar <sibis@codeaurora.org>

> 
> 

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

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 29-01-20, 19:04, Sibi Sankar wrote:
> I don't have a gen-pd use case to test against but with the is_genpd
> check removed it works as expected when I used it against this
> series: https://patchwork.kernel.org/cover/11353185/
> 
> In the lazy_link_required_opps fn shouldn't we skip the dynamic
> opps in the the opp list?

Tables with dynamic OPPs should not be there in pending_opp_tables
list and so that function shall never get called for them.

> With ^^ addressed:
> Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
> Tested-by: Sibi Sankar <sibis@codeaurora.org>

Thanks Sibi.

@Saravana: Can you please give your feedback as well? I don't want to
push something that may end up breaking something else :)

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Hsin-Yi Wang]

On Thu, Jan 30, 2020 at 12:21 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 29-01-20, 19:04, Sibi Sankar wrote:
> > I don't have a gen-pd use case to test against but with the is_genpd
> > check removed it works as expected when I used it against this
> > series: https://patchwork.kernel.org/cover/11353185/
> >
> > In the lazy_link_required_opps fn shouldn't we skip the dynamic
> > opps in the the opp list?
>
> Tables with dynamic OPPs should not be there in pending_opp_tables
> list and so that function shall never get called for them.
>
> > With ^^ addressed:
> > Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
> > Tested-by: Sibi Sankar <sibis@codeaurora.org>
>
> Thanks Sibi.
>
> @Saravana: Can you please give your feedback as well? I don't want to
> push something that may end up breaking something else :)
>

Hi Viresh and Saravana,

Do you still have plans to push this? I've tested on mt8183 cci with:

1. [v4,0/5] Add required-opps support to devfreq passive gov
(https://patchwork.kernel.org/project/linux-pm/cover/20190724014222.110767-1-saravanak@google.com/):
patch 2, 4, 5

2. opp: Allow lazy-linking of required-opps
(https://patchwork.kernel.org/project/linux-pm/patch/20190717222340.137578-4-saravanak@google.com/#23020727),
with minor diff to let non genpd use required-opp as well:
@@ -474,13 +474,6 @@ static void lazy_link_required_opp_table(struct
opp_table *required_opp_table)
        struct device_node *required_np, *opp_np, *required_table_np;
        int i, ret;

-       /*
-        * We only support genpd's OPPs in the "required-opps" for now,
-        * as we don't know much about other cases.
-        */
-       if (!required_opp_table->is_genpd)
-               return;
-
        mutex_lock(&opp_table_lock);

        list_for_each_entry_safe(opp_table, temp, &pending_opp_tables,
pending) {

3. PM / devfreq: Add cpu based scaling support to passive_governor and
mt8183 cci, cpufreq series
(https://patchwork.kernel.org/project/linux-mediatek/cover/1594348284-14199-1-git-send-email-andrew-sh.cheng@mediatek.com/)

Thanks

> --
> viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 18-01-21, 15:21, Hsin-Yi Wang wrote:
> Do you still have plans to push this? I've tested on mt8183 cci with:

I was never able to get Saravana to test this, if you are interested
in this stuff then I can rebase this and resend and we can see if it
works.

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Hsin-Yi Wang]

On Mon, Jan 18, 2021 at 3:34 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 15:21, Hsin-Yi Wang wrote:
> > Do you still have plans to push this? I've tested on mt8183 cci with:
>
> I was never able to get Saravana to test this, if you are interested
> in this stuff then I can rebase this and resend and we can see if it
> works.
>

Thanks. I can test this with the mt8183-cci series.

> --
> viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan]

On Sun, Jan 17, 2021 at 11:40 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Mon, Jan 18, 2021 at 3:34 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 18-01-21, 15:21, Hsin-Yi Wang wrote:
> > > Do you still have plans to push this? I've tested on mt8183 cci with:
> >
> > I was never able to get Saravana to test this, if you are interested
> > in this stuff then I can rebase this and resend and we can see if it
> > works.
> >

Yeah, got caught up with some other work. Sorry Viresh.

>
> Thanks. I can test this with the mt8183-cci series.

Thanks Hsin-Yi for offering to test this.

-Saravana

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 18-01-21, 15:39, Hsin-Yi Wang wrote:
> Thanks. I can test this with the mt8183-cci series.

Can you please give this a try ?

Apply over: 

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

-------------------------8<-------------------------
Subject: [PATCH] opp: Allow lazy-linking of required-opps

The OPP core currently requires the required opp tables to be available
before the dependent OPP table is added, as it needs to create links
from the dependent OPP table to the required ones. This may not be
convenient for all the platforms though, as this requires strict
ordering for probing the drivers.

This patch allows lazy-linking of the required-opps. The OPP tables for
which the required-opp-tables aren't available at the time of their
initialization, are added to a special list of OPP tables:
lazy_opp_tables. Later on, whenever a new OPP table is registered with
the OPP core, we check if it is required by an OPP table in the pending
list; if yes, then we complete the linking then and there.

An OPP table is marked unusable until the time all its required-opp
tables are available. And if lazy-linking fails for an OPP table, the
OPP core disables all of its OPPs to make sure no one can use them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  45 +++++++++++++----
 drivers/opp/of.c   | 122 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/opp/opp.h  |  10 +++-
 3 files changed, 161 insertions(+), 16 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7e4a51be5bb0..d886840628a0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -27,6 +27,10 @@
  * various states of availability.
  */
 LIST_HEAD(opp_tables);
+
+/* OPP tables with uninitialized required OPPs */
+LIST_HEAD(lazy_opp_tables);
+
 /* Lock to allow exclusive modification to the device and opp lists */
 DEFINE_MUTEX(opp_table_lock);
 /* Flag indicating that opp_tables list is being updated at the moment */
@@ -163,6 +167,10 @@ unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
 		return 0;
 	}
 
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(opp->opp_table))
+		return 0;
+
 	return opp->required_opps[index]->pstate;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
@@ -885,6 +893,10 @@ static int _set_required_opps(struct device *dev,
 	if (!required_opp_tables)
 		return 0;
 
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(opp_table))
+		return -EBUSY;
+
 	/* Single genpd case */
 	if (!genpd_virt_devs)
 		return _set_required_opp(dev, dev, opp, 0);
@@ -1182,6 +1194,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index,
 	mutex_init(&opp_table->lock);
 	mutex_init(&opp_table->genpd_virt_dev_lock);
 	INIT_LIST_HEAD(&opp_table->dev_list);
+	INIT_LIST_HEAD(&opp_table->lazy);
 
 	/* Mark regulator count uninitialized */
 	opp_table->regulator_count = -1;
@@ -1623,6 +1636,21 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 	return 0;
 }
 
+void _opp_required_opps_available(struct dev_pm_opp *opp, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (opp->required_opps[i]->available)
+			continue;
+
+		opp->available = false;
+		pr_warn("%s: OPP not supported by required OPP %pOF (%lu)\n",
+			 __func__, opp->required_opps[i]->np, opp->rate);
+		return;
+	}
+}
+
 /*
  * Returns:
  * 0: On success. And appropriate error message for duplicate OPPs.
@@ -1637,7 +1665,6 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table, bool rate_not_available)
 {
 	struct list_head *head;
-	unsigned int i;
 	int ret;
 
 	mutex_lock(&opp_table->lock);
@@ -1663,15 +1690,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 			 __func__, new_opp->rate);
 	}
 
-	for (i = 0; i < opp_table->required_opp_count; i++) {
-		if (new_opp->required_opps[i]->available)
-			continue;
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(opp_table))
+		return 0;
 
-		new_opp->available = false;
-		dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n",
-			 __func__, new_opp->required_opps[i]->np, new_opp->rate);
-		break;
-	}
+	_opp_required_opps_available(new_opp, opp_table->required_opp_count);
 
 	return 0;
 }
@@ -2377,6 +2400,10 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	if (!src_table || !src_table->required_opp_count)
 		return pstate;
 
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(src_table))
+		return -EBUSY;
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
 		if (src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 20ccdaab9384..31ac55714b57 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -144,7 +144,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (IS_ERR_OR_NULL(required_opp_tables[i]))
-			break;
+			continue;
 
 		dev_pm_opp_put_opp_table(required_opp_tables[i]);
 	}
@@ -153,6 +153,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	opp_table->required_opp_count = 0;
 	opp_table->required_opp_tables = NULL;
+	list_del(&opp_table->lazy);
 }
 
 /*
@@ -165,6 +166,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 {
 	struct opp_table **required_opp_tables;
 	struct device_node *required_np, *np;
+	bool lazy = false;
 	int count, i;
 
 	/* Traversing the first OPP node is all we need */
@@ -195,8 +197,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		required_opp_tables[i] = _find_table_of_opp_np(required_np);
 		of_node_put(required_np);
 
-		if (IS_ERR(required_opp_tables[i]))
-			goto free_required_tables;
+		if (IS_ERR(required_opp_tables[i])) {
+			lazy = true;
+			continue;
+		}
 
 		/*
 		 * We only support genpd's OPPs in the "required-opps" for now,
@@ -210,6 +214,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		}
 	}
 
+	/* Let's do the linking later on */
+	if (lazy)
+		list_add(&opp_table->lazy, &lazy_opp_tables);
+
 	goto put_np;
 
 free_required_tables:
@@ -278,14 +286,14 @@ void _of_opp_free_required_opps(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (!required_opps[i])
-			break;
+			continue;
 
 		/* Put the reference back */
 		dev_pm_opp_put(required_opps[i]);
 	}
 
-	kfree(required_opps);
 	opp->required_opps = NULL;
+	kfree(required_opps);
 }
 
 /* Populate all required OPPs which are part of "required-opps" list */
@@ -309,6 +317,10 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	for (i = 0; i < count; i++) {
 		required_table = opp_table->required_opp_tables[i];
 
+		/* Required table not added yet, we will link later */
+		if (IS_ERR_OR_NULL(required_table))
+			continue;
+
 		np = of_parse_required_opp(opp->np, i);
 		if (unlikely(!np)) {
 			ret = -ENODEV;
@@ -334,6 +346,104 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	return ret;
 }
 
+/* Link required OPPs for an individual OPP */
+static int lazy_link_required_opps(struct opp_table *opp_table,
+				   struct opp_table *new_table, int index)
+{
+	struct device_node *required_np;
+	struct dev_pm_opp *opp;
+
+	list_for_each_entry(opp, &opp_table->opp_list, node) {
+		required_np = of_parse_required_opp(opp->np, index);
+		if (unlikely(!required_np))
+			return -ENODEV;
+
+		opp->required_opps[index] = _find_opp_of_np(new_table, required_np);
+		of_node_put(required_np);
+
+		if (!opp->required_opps[index]) {
+			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
+			       __func__, opp->np, index);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
+/* Link required OPPs for all OPPs of the newly added OPP table */
+static void lazy_link_required_opp_table(struct opp_table *new_table)
+{
+	struct opp_table *opp_table, *temp, **required_opp_tables;
+	struct device_node *required_np, *opp_np, *required_table_np;
+	struct dev_pm_opp *opp;
+	int i, ret;
+
+	/*
+	 * We only support genpd's OPPs in the "required-opps" for now,
+	 * as we don't know much about other cases.
+	 */
+	if (!new_table->is_genpd)
+		return;
+
+	mutex_lock(&opp_table_lock);
+
+	list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
+		bool lazy = false;
+
+		/* opp_np can't be invalid here */
+		opp_np = of_get_next_available_child(opp_table->np, NULL);
+
+		for (i = 0; i < opp_table->required_opp_count; i++) {
+			required_opp_tables = opp_table->required_opp_tables;
+
+			/* Required opp-table is already parsed */
+			if (!IS_ERR(required_opp_tables[i]))
+				continue;
+
+			/* required_np can't be invalid here */
+			required_np = of_parse_required_opp(opp_np, i);
+			required_table_np = of_get_parent(required_np);
+
+			of_node_put(required_table_np);
+			of_node_put(required_np);
+
+			/*
+			 * Newly added table isn't the required opp-table for
+			 * opp_table.
+			 */
+			if (required_table_np != new_table->np) {
+				lazy = true;
+				continue;
+			}
+
+			required_opp_tables[i] = new_table;
+			_get_opp_table_kref(new_table);
+
+			/* Link OPPs now */
+			ret = lazy_link_required_opps(opp_table, new_table, i);
+			if (ret) {
+				/* The OPPs will be marked unusable */
+				lazy = false;
+				break;
+			}
+		}
+
+		of_node_put(opp_np);
+
+		/* All required opp-tables found, remove from lazy list */
+		if (!lazy) {
+			list_del(&opp_table->lazy);
+			INIT_LIST_HEAD(&opp_table->lazy);
+
+			list_for_each_entry(opp, &opp_table->opp_list, node)
+				_opp_required_opps_available(opp, opp_table->required_opp_count);
+		}
+	}
+
+	mutex_unlock(&opp_table_lock);
+}
+
 static int _bandwidth_supported(struct device *dev, struct opp_table *opp_table)
 {
 	struct device_node *np, *opp_np;
@@ -889,6 +999,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 		}
 	}
 
+	lazy_link_required_opp_table(opp_table);
+
 	return 0;
 
 remove_static_opp:
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 734df1f764ec..a5a10b685bf7 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -26,7 +26,7 @@ struct regulator;
 /* Lock to allow exclusive modification to the device and opp lists */
 extern struct mutex opp_table_lock;
 
-extern struct list_head opp_tables;
+extern struct list_head opp_tables, lazy_opp_tables;
 
 /*
  * Internal data structure organization with the OPP layer library is as
@@ -168,7 +168,7 @@ enum opp_table_access {
  * meant for book keeping and private to OPP library.
  */
 struct opp_table {
-	struct list_head node;
+	struct list_head node, lazy;
 
 	struct blocking_notifier_head head;
 	struct list_head dev_list;
@@ -230,6 +230,12 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp
 struct opp_table *_add_opp_table(struct device *dev);
 struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
 void _put_opp_list_kref(struct opp_table *opp_table);
+void _opp_required_opps_available(struct dev_pm_opp *opp, int count);
+
+static inline bool lazy_linking_pending(struct opp_table *opp_table)
+{
+	return unlikely(!list_empty(&opp_table->lazy));
+}
 
 #ifdef CONFIG_OF
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Hsin-Yi Wang]

On Wed, Jan 27, 2021 at 7:54 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 15:39, Hsin-Yi Wang wrote:
> > Thanks. I can test this with the mt8183-cci series.
>
> Can you please give this a try ?
>
> Apply over:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next
>

Hi Viresh,

I tested this patch with devfreq passive governor[1] and mt8183
cci[2]. It's also working as expected.

[1] https://patchwork.kernel.org/project/linux-pm/cover/20190724014222.110767-1-saravanak@google.com/
(patch 2,4,5)
[2] https://patchwork.kernel.org/project/linux-mediatek/cover/1594348284-14199-1-git-send-email-andrew-sh.cheng@mediatek.com/

In my testing case, required_opp_table is not genpd case (mt8183 cci
is not genpd), so I remove the following constraint. Does that make
sense to you?

@@ -377,13 +377,6 @@ static void lazy_link_required_opp_table(struct
opp_table *new_table)
        struct dev_pm_opp *opp;
        int i, ret;

-       /*
-        * We only support genpd's OPPs in the "required-opps" for now,
-        * as we don't know much about other cases.
-        */
-       if (!new_table->is_genpd)
-               return;
-
        mutex_lock(&opp_table_lock);

Thanks.

Hsin-Yi
> -------------------------8<-------------------------
> Subject: [PATCH] opp: Allow lazy-linking of required-opps
>
> The OPP core currently requires the required opp tables to be available
> before the dependent OPP table is added, as it needs to create links
> from the dependent OPP table to the required ones. This may not be
> convenient for all the platforms though, as this requires strict
> ordering for probing the drivers.
>
> This patch allows lazy-linking of the required-opps. The OPP tables for
> which the required-opp-tables aren't available at the time of their
> initialization, are added to a special list of OPP tables:
> lazy_opp_tables. Later on, whenever a new OPP table is registered with
> the OPP core, we check if it is required by an OPP table in the pending
> list; if yes, then we complete the linking then and there.
>
> An OPP table is marked unusable until the time all its required-opp
> tables are available. And if lazy-linking fails for an OPP table, the
> OPP core disables all of its OPPs to make sure no one can use them.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c |  45 +++++++++++++----
>  drivers/opp/of.c   | 122 +++++++++++++++++++++++++++++++++++++++++++--
>  drivers/opp/opp.h  |  10 +++-
>  3 files changed, 161 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7e4a51be5bb0..d886840628a0 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -27,6 +27,10 @@
>   * various states of availability.
>   */
>  LIST_HEAD(opp_tables);
> +
> +/* OPP tables with uninitialized required OPPs */
> +LIST_HEAD(lazy_opp_tables);
> +
>  /* Lock to allow exclusive modification to the device and opp lists */
>  DEFINE_MUTEX(opp_table_lock);
>  /* Flag indicating that opp_tables list is being updated at the moment */
> @@ -163,6 +167,10 @@ unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
>                 return 0;
>         }
>
> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(opp->opp_table))
> +               return 0;
> +
>         return opp->required_opps[index]->pstate;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
> @@ -885,6 +893,10 @@ static int _set_required_opps(struct device *dev,
>         if (!required_opp_tables)
>                 return 0;
>
> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(opp_table))
> +               return -EBUSY;
> +
>         /* Single genpd case */
>         if (!genpd_virt_devs)
>                 return _set_required_opp(dev, dev, opp, 0);
> @@ -1182,6 +1194,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index,
>         mutex_init(&opp_table->lock);
>         mutex_init(&opp_table->genpd_virt_dev_lock);
>         INIT_LIST_HEAD(&opp_table->dev_list);
> +       INIT_LIST_HEAD(&opp_table->lazy);
>
>         /* Mark regulator count uninitialized */
>         opp_table->regulator_count = -1;
> @@ -1623,6 +1636,21 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
>         return 0;
>  }
>
> +void _opp_required_opps_available(struct dev_pm_opp *opp, int count)
> +{
> +       int i;
> +
> +       for (i = 0; i < count; i++) {
> +               if (opp->required_opps[i]->available)
> +                       continue;
> +
> +               opp->available = false;
> +               pr_warn("%s: OPP not supported by required OPP %pOF (%lu)\n",
> +                        __func__, opp->required_opps[i]->np, opp->rate);
> +               return;
> +       }
> +}
> +
>  /*
>   * Returns:
>   * 0: On success. And appropriate error message for duplicate OPPs.
> @@ -1637,7 +1665,6 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>              struct opp_table *opp_table, bool rate_not_available)
>  {
>         struct list_head *head;
> -       unsigned int i;
>         int ret;
>
>         mutex_lock(&opp_table->lock);
> @@ -1663,15 +1690,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>                          __func__, new_opp->rate);
>         }
>
> -       for (i = 0; i < opp_table->required_opp_count; i++) {
> -               if (new_opp->required_opps[i]->available)
> -                       continue;
> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(opp_table))
> +               return 0;
>
> -               new_opp->available = false;
> -               dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n",
> -                        __func__, new_opp->required_opps[i]->np, new_opp->rate);
> -               break;
> -       }
> +       _opp_required_opps_available(new_opp, opp_table->required_opp_count);
>
>         return 0;
>  }
> @@ -2377,6 +2400,10 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         if (!src_table || !src_table->required_opp_count)
>                 return pstate;
>
> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(src_table))
> +               return -EBUSY;
> +
>         for (i = 0; i < src_table->required_opp_count; i++) {
>                 if (src_table->required_opp_tables[i]->np == dst_table->np)
>                         break;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 20ccdaab9384..31ac55714b57 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -144,7 +144,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>
>         for (i = 0; i < opp_table->required_opp_count; i++) {
>                 if (IS_ERR_OR_NULL(required_opp_tables[i]))
> -                       break;
> +                       continue;
>
>                 dev_pm_opp_put_opp_table(required_opp_tables[i]);
>         }
> @@ -153,6 +153,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>
>         opp_table->required_opp_count = 0;
>         opp_table->required_opp_tables = NULL;
> +       list_del(&opp_table->lazy);
>  }
>
>  /*
> @@ -165,6 +166,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  {
>         struct opp_table **required_opp_tables;
>         struct device_node *required_np, *np;
> +       bool lazy = false;
>         int count, i;
>
>         /* Traversing the first OPP node is all we need */
> @@ -195,8 +197,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>                 required_opp_tables[i] = _find_table_of_opp_np(required_np);
>                 of_node_put(required_np);
>
> -               if (IS_ERR(required_opp_tables[i]))
> -                       goto free_required_tables;
> +               if (IS_ERR(required_opp_tables[i])) {
> +                       lazy = true;
> +                       continue;
> +               }
>
>                 /*
>                  * We only support genpd's OPPs in the "required-opps" for now,
> @@ -210,6 +214,10 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>                 }
>         }
>
> +       /* Let's do the linking later on */
> +       if (lazy)
> +               list_add(&opp_table->lazy, &lazy_opp_tables);
> +
>         goto put_np;
>
>  free_required_tables:
> @@ -278,14 +286,14 @@ void _of_opp_free_required_opps(struct opp_table *opp_table,
>
>         for (i = 0; i < opp_table->required_opp_count; i++) {
>                 if (!required_opps[i])
> -                       break;
> +                       continue;
>
>                 /* Put the reference back */
>                 dev_pm_opp_put(required_opps[i]);
>         }
>
> -       kfree(required_opps);
>         opp->required_opps = NULL;
> +       kfree(required_opps);
>  }
>
>  /* Populate all required OPPs which are part of "required-opps" list */
> @@ -309,6 +317,10 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
>         for (i = 0; i < count; i++) {
>                 required_table = opp_table->required_opp_tables[i];
>
> +               /* Required table not added yet, we will link later */
> +               if (IS_ERR_OR_NULL(required_table))
> +                       continue;
> +
>                 np = of_parse_required_opp(opp->np, i);
>                 if (unlikely(!np)) {
>                         ret = -ENODEV;
> @@ -334,6 +346,104 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
>         return ret;
>  }
>
> +/* Link required OPPs for an individual OPP */
> +static int lazy_link_required_opps(struct opp_table *opp_table,
> +                                  struct opp_table *new_table, int index)
> +{
> +       struct device_node *required_np;
> +       struct dev_pm_opp *opp;
> +
> +       list_for_each_entry(opp, &opp_table->opp_list, node) {
> +               required_np = of_parse_required_opp(opp->np, index);
> +               if (unlikely(!required_np))
> +                       return -ENODEV;
> +
> +               opp->required_opps[index] = _find_opp_of_np(new_table, required_np);
> +               of_node_put(required_np);
> +
> +               if (!opp->required_opps[index]) {
> +                       pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> +                              __func__, opp->np, index);
> +                       return -ENODEV;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +/* Link required OPPs for all OPPs of the newly added OPP table */
> +static void lazy_link_required_opp_table(struct opp_table *new_table)
> +{
> +       struct opp_table *opp_table, *temp, **required_opp_tables;
> +       struct device_node *required_np, *opp_np, *required_table_np;
> +       struct dev_pm_opp *opp;
> +       int i, ret;
> +
> +       /*
> +        * We only support genpd's OPPs in the "required-opps" for now,
> +        * as we don't know much about other cases.
> +        */
> +       if (!new_table->is_genpd)
> +               return;
> +
> +       mutex_lock(&opp_table_lock);
> +
> +       list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> +               bool lazy = false;
> +
> +               /* opp_np can't be invalid here */
> +               opp_np = of_get_next_available_child(opp_table->np, NULL);
> +
> +               for (i = 0; i < opp_table->required_opp_count; i++) {
> +                       required_opp_tables = opp_table->required_opp_tables;
> +
> +                       /* Required opp-table is already parsed */
> +                       if (!IS_ERR(required_opp_tables[i]))
> +                               continue;
> +
> +                       /* required_np can't be invalid here */
> +                       required_np = of_parse_required_opp(opp_np, i);
> +                       required_table_np = of_get_parent(required_np);
> +
> +                       of_node_put(required_table_np);
> +                       of_node_put(required_np);
> +
> +                       /*
> +                        * Newly added table isn't the required opp-table for
> +                        * opp_table.
> +                        */
> +                       if (required_table_np != new_table->np) {
> +                               lazy = true;
> +                               continue;
> +                       }
> +
> +                       required_opp_tables[i] = new_table;
> +                       _get_opp_table_kref(new_table);
> +
> +                       /* Link OPPs now */
> +                       ret = lazy_link_required_opps(opp_table, new_table, i);
> +                       if (ret) {
> +                               /* The OPPs will be marked unusable */
> +                               lazy = false;
> +                               break;
> +                       }
> +               }
> +
> +               of_node_put(opp_np);
> +
> +               /* All required opp-tables found, remove from lazy list */
> +               if (!lazy) {
> +                       list_del(&opp_table->lazy);
> +                       INIT_LIST_HEAD(&opp_table->lazy);
> +
> +                       list_for_each_entry(opp, &opp_table->opp_list, node)
> +                               _opp_required_opps_available(opp, opp_table->required_opp_count);
> +               }
> +       }
> +
> +       mutex_unlock(&opp_table_lock);
> +}
> +
>  static int _bandwidth_supported(struct device *dev, struct opp_table *opp_table)
>  {
>         struct device_node *np, *opp_np;
> @@ -889,6 +999,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
>                 }
>         }
>
> +       lazy_link_required_opp_table(opp_table);
> +
>         return 0;
>
>  remove_static_opp:
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 734df1f764ec..a5a10b685bf7 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -26,7 +26,7 @@ struct regulator;
>  /* Lock to allow exclusive modification to the device and opp lists */
>  extern struct mutex opp_table_lock;
>
> -extern struct list_head opp_tables;
> +extern struct list_head opp_tables, lazy_opp_tables;
>
>  /*
>   * Internal data structure organization with the OPP layer library is as
> @@ -168,7 +168,7 @@ enum opp_table_access {
>   * meant for book keeping and private to OPP library.
>   */
>  struct opp_table {
> -       struct list_head node;
> +       struct list_head node, lazy;
>
>         struct blocking_notifier_head head;
>         struct list_head dev_list;
> @@ -230,6 +230,12 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp
>  struct opp_table *_add_opp_table(struct device *dev);
>  struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
>  void _put_opp_list_kref(struct opp_table *opp_table);
> +void _opp_required_opps_available(struct dev_pm_opp *opp, int count);
> +
> +static inline bool lazy_linking_pending(struct opp_table *opp_table)
> +{
> +       return unlikely(!list_empty(&opp_table->lazy));
> +}
>
>  #ifdef CONFIG_OF
>  void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
>
> --
> viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar]

On 27-01-21, 22:40, Hsin-Yi Wang wrote:
> Hi Viresh,
> 
> I tested this patch with devfreq passive governor[1] and mt8183
> cci[2]. It's also working as expected.

I hope I can add your Tested-by for the patch then, right ?

> [1] https://patchwork.kernel.org/project/linux-pm/cover/20190724014222.110767-1-saravanak@google.com/
> (patch 2,4,5)
> [2] https://patchwork.kernel.org/project/linux-mediatek/cover/1594348284-14199-1-git-send-email-andrew-sh.cheng@mediatek.com/
> 
> In my testing case, required_opp_table is not genpd case (mt8183 cci
> is not genpd), so I remove the following constraint. Does that make
> sense to you?
> 
> @@ -377,13 +377,6 @@ static void lazy_link_required_opp_table(struct
> opp_table *new_table)
>         struct dev_pm_opp *opp;
>         int i, ret;
> 
> -       /*
> -        * We only support genpd's OPPs in the "required-opps" for now,
> -        * as we don't know much about other cases.
> -        */
> -       if (!new_table->is_genpd)
> -               return;
> -
>         mutex_lock(&opp_table_lock);

We will perhaps need more changes than that, but those should be done
separately when you try to add a user for the same.

-- 
viresh

Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Hsin-Yi Wang]

On Thu, Jan 28, 2021 at 12:13 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 27-01-21, 22:40, Hsin-Yi Wang wrote:
> > Hi Viresh,
> >
> > I tested this patch with devfreq passive governor[1] and mt8183
> > cci[2]. It's also working as expected.
>
> I hope I can add your Tested-by for the patch then, right ?
>
Yes, thanks!

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>

> > [1] https://patchwork.kernel.org/project/linux-pm/cover/20190724014222.110767-1-saravanak@google.com/
> > (patch 2,4,5)
> > [2] https://patchwork.kernel.org/project/linux-mediatek/cover/1594348284-14199-1-git-send-email-andrew-sh.cheng@mediatek.com/
> >
> > In my testing case, required_opp_table is not genpd case (mt8183 cci
> > is not genpd), so I remove the following constraint. Does that make
> > sense to you?
> >
> > @@ -377,13 +377,6 @@ static void lazy_link_required_opp_table(struct
> > opp_table *new_table)
> >         struct dev_pm_opp *opp;
> >         int i, ret;
> >
> > -       /*
> > -        * We only support genpd's OPPs in the "required-opps" for now,
> > -        * as we don't know much about other cases.
> > -        */
> > -       if (!new_table->is_genpd)
> > -               return;
> > -
> >         mutex_lock(&opp_table_lock);
>
> We will perhaps need more changes than that, but those should be done
> separately when you try to add a user for the same.
>

Ack.

> --
> viresh

[PATCH v3 4/5] PM / devfreq: Cache OPP table reference in devfreq

[Saravana Kannan]

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

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 784c08e4f931..7984b01d585d 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -594,6 +594,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);
 }
@@ -674,6 +676,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 2bae9ed3c783..1c05129f76c0 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -116,6 +116,7 @@ struct devfreq_dev_profile {
  * @profile:	device-specific devfreq profile
  * @governor:	method how to choose frequency based on the usage.
  * @governor_name:	devfreq governor name for use with this devfreq
+ * @opp_table:	Reference to OPP table of dev.parent, if one exists.
  * @nb:		notifier block used to notify devfreq object that it should
  *		reevaluate operable frequencies. Devfreq users may use
  *		devfreq.nb to the corresponding register notifier call chain.
@@ -153,6 +154,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.510.g264f2c817a-goog

[PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Saravana Kannan]

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

Signed-off-by: Saravana Kannan <saravanak@google.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 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 58308948b863..24ce94c80f06 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
 	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp = NULL, *p_opp = NULL;
 	int i, count, ret = 0;
 
 	/*
@@ -56,13 +56,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
@@ -89,6 +96,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.510.g264f2c817a-goog

Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Viresh Kumar]

On 17-07-19, 15:23, 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>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.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 58308948b863..24ce94c80f06 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  			= (struct devfreq_passive_data *)devfreq->data;
>  	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
>  	unsigned long child_freq = ULONG_MAX;
> -	struct dev_pm_opp *opp;
> +	struct dev_pm_opp *opp = NULL, *p_opp = NULL;

This won't be required if ...

>  	int i, count, ret = 0;
>  
>  	/*
> @@ -56,13 +56,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);

you put p_opp right here.

Also shouldn't you try to get p_opp under the above if block only? As
that is the only user of it ?

> +	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
> @@ -89,6 +96,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.510.g264f2c817a-goog

-- 
viresh

Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Saravana Kannan]

On Tue, Jul 23, 2019 at 3:04 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 17-07-19, 15:23, 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>
> > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > Acked-by: Chanwoo Choi <cw00.choi@samsung.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 58308948b863..24ce94c80f06 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> >                       = (struct devfreq_passive_data *)devfreq->data;
> >       struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> >       unsigned long child_freq = ULONG_MAX;
> > -     struct dev_pm_opp *opp;
> > +     struct dev_pm_opp *opp = NULL, *p_opp = NULL;
>
> This won't be required if ...
>
> >       int i, count, ret = 0;
> >
> >       /*
> > @@ -56,13 +56,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);
>
> you put p_opp right here.
>
> Also shouldn't you try to get p_opp under the above if block only? As
> that is the only user of it ?

No, p_opp (used to be called opp) was used even before my changes. If
there's no required-opps mapping this falls back to assuming the slave
device OPP to pick should be the same index as the master device's
opp.

So I believe this patch is correct as-is.

-Saravana

>
> > +     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
> > @@ -89,6 +96,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.510.g264f2c817a-goog
>
> --
> viresh

Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Viresh Kumar]

On 23-07-19, 17:26, Saravana Kannan wrote:
> On Tue, Jul 23, 2019 at 3:04 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 17-07-19, 15:23, Saravana Kannan wrote:
> > >       /*
> > > @@ -56,13 +56,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);
> >
> > you put p_opp right here.

What about this comment ?

> >
> > Also shouldn't you try to get p_opp under the above if block only? As
> > that is the only user of it ?
> 
> No, p_opp (used to be called opp) was used even before my changes. If
> there's no required-opps mapping this falls back to assuming the slave
> device OPP to pick should be the same index as the master device's
> opp.
> 
> So I believe this patch is correct as-is.

Right.

-- 
viresh

Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Saravana Kannan]

On Wed, Jul 24, 2019 at 8:01 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 17:26, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019 at 3:04 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > >       /*
> > > > @@ -56,13 +56,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);
> > >
> > > you put p_opp right here.
>
> What about this comment ?

Sorry, mixed this up with the other comment. Good point. Fixed. This
unintentionally fixed a copy-pasta bug I had in the "out" path.

-Saravana

>
> > >
> > > Also shouldn't you try to get p_opp under the above if block only? As
> > > that is the only user of it ?
> >
> > No, p_opp (used to be called opp) was used even before my changes. If
> > there's no required-opps mapping this falls back to assuming the slave
> > device OPP to pick should be the same index as the master device's
> > opp.
> >
> > So I believe this patch is correct as-is.
>
> Right.
>
> --
> viresh