[PATCH 0/7] OPP/pmdomain: Assign required_devs for required OPPs through genpd

[PATCH 0/7] OPP/pmdomain: Assign required_devs for required OPPs through genpd

[Ulf Hansson]

Through dev_pm_opp_set_config() the _opp_attach_genpd() allows consumer
drivers to hook up a device to its PM domains. This works for both a single
and multiple PM domains. Their corresponding virtual devices that are
created by genpd during attach, are later being assigned as the
required_devs for the corresponding required OPPs.

In principle this works fine, but there are some problems. Especially as
the index for a "required-opps" may not necessarily need to match the index
for the "power-domain" in DT, in which case things gets screwed up.

This series intends to improve the situation, but it's also the first step to
enable consumers of _opp_attach_genpd() to start migrating to use the new
dev_pm_domain_attach_list() instead. Down the road, we should then be able to
drop _opp_attach_genpd() completely.

To test this, I have used a QEMU setup, with local PM test-drivers to try to
test all various combinations of single/multi power/performance PM domains for
a device. Hopefully I have covered it all, but I would certainly appreciate if
someone could help to run a slew of tests on some HWs.

Note: Patch1 is actually a fix that has been posted [1] separately too. I
decided to include here for completeness.

Kind regards
Ulf Hansson

[1]
https://lore.kernel.org/linux-kernel/20240618155013.323322-1-ulf.hansson@linaro.org/

Ulf Hansson (7):
  OPP: Fix support for required OPPs for multiple PM domains
  OPP: Drop a redundant in-parameter to _set_opp_level()
  OPP: Rework _set_required_devs() to manage a single device per call
  OPP: Introduce an OF helper function to inform if required-opps is
    used
  pmdomain: core: Manage the default required OPP from a separate
    function
  OPP/pmdomain: Set the required_dev for a required OPP during genpd
    attach
  pmdomain: core: Drop the redundant dev_to_genpd_dev()

 drivers/opp/core.c        | 185 ++++++++++++++++++--------------------
 drivers/opp/of.c          |  32 +++++++
 drivers/opp/opp.h         |   4 +-
 drivers/pmdomain/core.c   | 103 ++++++++++++++++-----
 include/linux/pm_domain.h |   6 --
 include/linux/pm_opp.h    |  16 +++-
 6 files changed, 215 insertions(+), 131 deletions(-)

-- 
2.34.1

[PATCH 1/7] OPP: Fix support for required OPPs for multiple PM domains

[Ulf Hansson]

In _set_opp() we are normally bailing out when trying to set an OPP that is
the current one. This make perfect sense, but becomes a problem when
_set_required_opps() calls it recursively.

More precisely, when a required OPP is being shared by multiple PM domains,
we end up skipping to request the corresponding performance-state for all
of the PM domains, but the first one. Let's fix the problem, by calling
_set_opp_level() from _set_required_opps() instead.

Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
Cc: stable@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c | 47 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index cb4611fe1b5b..45eca65f27f9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1061,6 +1061,28 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 	return 0;
 }
 
+static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
+			  struct dev_pm_opp *opp)
+{
+	unsigned int level = 0;
+	int ret = 0;
+
+	if (opp) {
+		if (opp->level == OPP_LEVEL_UNSET)
+			return 0;
+
+		level = opp->level;
+	}
+
+	/* Request a new performance state through the device's PM domain. */
+	ret = dev_pm_domain_set_performance_state(dev, level);
+	if (ret)
+		dev_err(dev, "Failed to set performance state %u (%d)\n", level,
+			ret);
+
+	return ret;
+}
+
 /* This is only called for PM domain for now */
 static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 			      struct dev_pm_opp *opp, bool up)
@@ -1091,7 +1113,8 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 		if (devs[index]) {
 			required_opp = opp ? opp->required_opps[index] : NULL;
 
-			ret = dev_pm_opp_set_opp(devs[index], required_opp);
+			ret = _set_opp_level(devs[index], opp_table,
+					     required_opp);
 			if (ret)
 				return ret;
 		}
@@ -1102,28 +1125,6 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 	return 0;
 }
 
-static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
-			  struct dev_pm_opp *opp)
-{
-	unsigned int level = 0;
-	int ret = 0;
-
-	if (opp) {
-		if (opp->level == OPP_LEVEL_UNSET)
-			return 0;
-
-		level = opp->level;
-	}
-
-	/* Request a new performance state through the device's PM domain. */
-	ret = dev_pm_domain_set_performance_state(dev, level);
-	if (ret)
-		dev_err(dev, "Failed to set performance state %u (%d)\n", level,
-			ret);
-
-	return ret;
-}
-
 static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
-- 
2.34.1

[PATCH 2/7] OPP: Drop a redundant in-parameter to _set_opp_level()

[Ulf Hansson]

The in-parameter "opp_table" isn't needed by _set_opp_level(). Let's
therefore drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 45eca65f27f9..02ba963d11ff 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1061,8 +1061,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 	return 0;
 }
 
-static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
-			  struct dev_pm_opp *opp)
+static int _set_opp_level(struct device *dev, struct dev_pm_opp *opp)
 {
 	unsigned int level = 0;
 	int ret = 0;
@@ -1113,8 +1112,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 		if (devs[index]) {
 			required_opp = opp ? opp->required_opps[index] : NULL;
 
-			ret = _set_opp_level(devs[index], opp_table,
-					     required_opp);
+			ret = _set_opp_level(devs[index], required_opp);
 			if (ret)
 				return ret;
 		}
@@ -1172,7 +1170,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 	if (opp_table->regulators)
 		regulator_disable(opp_table->regulators[0]);
 
-	ret = _set_opp_level(dev, opp_table, NULL);
+	ret = _set_opp_level(dev, NULL);
 	if (ret)
 		goto out;
 
@@ -1221,7 +1219,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 			return ret;
 		}
 
-		ret = _set_opp_level(dev, opp_table, opp);
+		ret = _set_opp_level(dev, opp);
 		if (ret)
 			return ret;
 
@@ -1268,7 +1266,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 			return ret;
 		}
 
-		ret = _set_opp_level(dev, opp_table, opp);
+		ret = _set_opp_level(dev, opp);
 		if (ret)
 			return ret;
 
-- 
2.34.1

Re: [PATCH 2/7] OPP: Drop a redundant in-parameter to _set_opp_level()

[Viresh Kumar]

On 19-06-24, 16:08, Ulf Hansson wrote:
> The in-parameter "opp_table" isn't needed by _set_opp_level(). Let's
> therefore drop it.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/opp/core.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

Applied. Thanks.

-- 
viresh

[PATCH 3/7] OPP: Rework _set_required_devs() to manage a single device per call

[Ulf Hansson]

At this point there are no consumer drivers that makes use of
_set_required_devs(), hence it should be straightforward to rework the code
to enable it to better integrate with the genpd attach procedure.

During genpd attach, one device is being attached to its PM domain.
Therefore, let's also update the _set_required_devs() to work with this
behaviour and instead trust callers to fill out one required_dev per call.

Moving forward and as shown from a subsequent change, genpd becomes the
first user of the reworked _set_required_dev().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c     | 89 +++++++++++++++++++++++++++++-------------
 drivers/opp/opp.h      |  4 +-
 include/linux/pm_opp.h | 10 +++--
 3 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 02ba963d11ff..bc1ed1d3d60d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2483,9 +2483,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 
 }
 
-static int _opp_set_required_devs(struct opp_table *opp_table,
-				  struct device *dev,
-				  struct device **required_devs)
+static int _opp_set_required_dev(struct opp_table *opp_table,
+				 struct device *dev,
+				 struct device *required_dev,
+				 struct opp_table *required_opp_table)
 {
 	int i;
 
@@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table,
 		return -EINVAL;
 	}
 
-	/* Another device that shares the OPP table has set the required devs ? */
-	if (opp_table->required_devs[0])
-		return 0;
+	/* Genpd core takes care of propagation to parent genpd */
+	if (opp_table->is_genpd) {
+		dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
+		return -EOPNOTSUPP;
+	}
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
-		/* Genpd core takes care of propagation to parent genpd */
-		if (required_devs[i] && opp_table->is_genpd &&
-		    opp_table->required_opp_tables[i]->is_genpd) {
-			dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
-			return -EOPNOTSUPP;
-		}
+		struct opp_table *table = opp_table->required_opp_tables[i];
+
+		/*
+		 * The OPP table should be available at this point. If not, it's
+		 * not the one we are looking for.
+		 */
+		if (IS_ERR(table))
+			continue;
+
+		/* Move to the next available index. */
+		if (opp_table->required_devs[i])
+			continue;
 
-		opp_table->required_devs[i] = required_devs[i];
+		/*
+		 * We need to compare the nodes for the OPP tables, rather than
+		 * the OPP tables themselves, as we may have separate instances.
+		 */
+		if (required_opp_table->np == table->np) {
+
+			/* Cross check the OPP tables and fix it if needed. */
+			if (required_opp_table != table) {
+				dev_pm_opp_put_opp_table(table);
+				_get_opp_table_kref(required_opp_table);
+				opp_table->required_opp_tables[i] = required_opp_table;
+			}
+
+			opp_table->required_devs[i] = required_dev;
+
+			/*
+			 * Add the required_dev as a user of the OPP table, so
+			 * we can call dev_pm_opp_set_opp() on it directly.
+			 */
+			if (!_add_opp_dev(required_dev, required_opp_table)) {
+				dev_err(dev, "Failed to add the device to the required OPP table\n");
+				return -ENOMEM;
+			}
+
+			return i;
+		}
 	}
 
-	return 0;
+	dev_err(dev, "Missing OPP table, unable to set the required dev\n");
+	return -ENODEV;
 }
 
-static void _opp_put_required_devs(struct opp_table *opp_table)
+static void _opp_put_required_dev(struct opp_table *opp_table,
+				  unsigned int index)
 {
-	int i;
-
-	for (i = 0; i < opp_table->required_opp_count; i++)
-		opp_table->required_devs[i] = NULL;
+	opp_table->required_devs[index] = NULL;
 }
 
 static void _opp_clear_config(struct opp_config_data *data)
 {
-	if (data->flags & OPP_CONFIG_REQUIRED_DEVS)
-		_opp_put_required_devs(data->opp_table);
+	if (data->flags & OPP_CONFIG_REQUIRED_DEV)
+		_opp_put_required_dev(data->opp_table, data->index);
 	else if (data->flags & OPP_CONFIG_GENPD)
 		_opp_detach_genpd(data->opp_table);
 
@@ -2640,7 +2673,7 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 
 	/* Attach genpds */
 	if (config->genpd_names) {
-		if (config->required_devs)
+		if (config->required_dev)
 			goto err;
 
 		ret = _opp_attach_genpd(opp_table, dev, config->genpd_names,
@@ -2649,13 +2682,15 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 			goto err;
 
 		data->flags |= OPP_CONFIG_GENPD;
-	} else if (config->required_devs) {
-		ret = _opp_set_required_devs(opp_table, dev,
-					     config->required_devs);
-		if (ret)
+	} else if (config->required_dev && config->required_opp_table) {
+		ret = _opp_set_required_dev(opp_table, dev,
+					    config->required_dev,
+					    config->required_opp_table);
+		if (ret < 0)
 			goto err;
 
-		data->flags |= OPP_CONFIG_REQUIRED_DEVS;
+		data->index = ret;
+		data->flags |= OPP_CONFIG_REQUIRED_DEV;
 	}
 
 	ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX),
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index cff1fabd1ae3..5b5a4bd89c9e 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -35,12 +35,13 @@ extern struct list_head opp_tables;
 #define OPP_CONFIG_PROP_NAME		BIT(3)
 #define OPP_CONFIG_SUPPORTED_HW		BIT(4)
 #define OPP_CONFIG_GENPD		BIT(5)
-#define OPP_CONFIG_REQUIRED_DEVS	BIT(6)
+#define OPP_CONFIG_REQUIRED_DEV		BIT(6)
 
 /**
  * struct opp_config_data - data for set config operations
  * @opp_table: OPP table
  * @flags: OPP config flags
+ * @index: The position in the array of required_devs
  *
  * This structure stores the OPP config information for each OPP table
  * configuration by the callers.
@@ -48,6 +49,7 @@ extern struct list_head opp_tables;
 struct opp_config_data {
 	struct opp_table *opp_table;
 	unsigned int flags;
+	unsigned int index;
 };
 
 /**
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dd7c8441af42..2b6599f6037d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -63,10 +63,11 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
  * @supported_hw_count: Number of elements in the array.
  * @regulator_names: Array of pointers to the names of the regulator, NULL terminated.
  * @genpd_names: Null terminated array of pointers containing names of genpd to
- *		attach. Mutually exclusive with required_devs.
+ *		attach. Mutually exclusive with required_dev.
  * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually
- *		exclusive with required_devs.
- * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs.
+ *		exclusive with required_dev.
+ * @required_dev: Required OPP device. Mutually exclusive with genpd_names/virt_devs.
+ * @required_opp_table: The corresponding required OPP table for @required_dev.
  *
  * This structure contains platform specific OPP configurations for the device.
  */
@@ -81,7 +82,8 @@ struct dev_pm_opp_config {
 	const char * const *regulator_names;
 	const char * const *genpd_names;
 	struct device ***virt_devs;
-	struct device **required_devs;
+	struct device *required_dev;
+	struct opp_table *required_opp_table;
 };
 
 #define OPP_LEVEL_UNSET			U32_MAX
-- 
2.34.1

Re: [PATCH 3/7] OPP: Rework _set_required_devs() to manage a single device per call

[Viresh Kumar]

On 19-06-24, 16:08, Ulf Hansson wrote:
> @@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table,
>  		return -EINVAL;
>  	}
>  
> -	/* Another device that shares the OPP table has set the required devs ? */
> -	if (opp_table->required_devs[0])
> -		return 0;
> +	/* Genpd core takes care of propagation to parent genpd */
> +	if (opp_table->is_genpd) {

A genpd can have non-genpd devices in the required OPPs and so this
isn't sufficient. What we were ignoring earlier was genpd having
another genpd as required opp.

> +		dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
> +		return -EOPNOTSUPP;
> +	}
>  
>  	for (i = 0; i < opp_table->required_opp_count; i++) {
> -		/* Genpd core takes care of propagation to parent genpd */
> -		if (required_devs[i] && opp_table->is_genpd &&
> -		    opp_table->required_opp_tables[i]->is_genpd) {
> -			dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
> -			return -EOPNOTSUPP;
> -		}
> +		struct opp_table *table = opp_table->required_opp_tables[i];
> +
> +		/*
> +		 * The OPP table should be available at this point. If not, it's
> +		 * not the one we are looking for.
> +		 */
> +		if (IS_ERR(table))
> +			continue;
> +
> +		/* Move to the next available index. */
> +		if (opp_table->required_devs[i])
> +			continue;
>  
> -		opp_table->required_devs[i] = required_devs[i];
> +		/*
> +		 * We need to compare the nodes for the OPP tables, rather than
> +		 * the OPP tables themselves, as we may have separate instances.
> +		 */
> +		if (required_opp_table->np == table->np) {
> +

We don't keep such empty lines in OPP core generally at this place.

> +			/* Cross check the OPP tables and fix it if needed. */

Copy the bigger comment from_opp_attach_genpd() here too. It helps
understanding why required_opp_tables entry is getting replaced.

> +			if (required_opp_table != table) {
> +				dev_pm_opp_put_opp_table(table);
> +				_get_opp_table_kref(required_opp_table);
> +				opp_table->required_opp_tables[i] = required_opp_table;
> +			}
> +
> +			opp_table->required_devs[i] = required_dev;
> +
> +			/*
> +			 * Add the required_dev as a user of the OPP table, so
> +			 * we can call dev_pm_opp_set_opp() on it directly.
> +			 */
> +			if (!_add_opp_dev(required_dev, required_opp_table)) {
> +				dev_err(dev, "Failed to add the device to the required OPP table\n");
> +				return -ENOMEM;
> +			}
> +
> +			return i;
> +		}
>  	}

-- 
viresh

Re: [PATCH 3/7] OPP: Rework _set_required_devs() to manage a single device per call

[Ulf Hansson]

On Wed, 26 Jun 2024 at 08:33, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-06-24, 16:08, Ulf Hansson wrote:
> > @@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table,
> >               return -EINVAL;
> >       }
> >
> > -     /* Another device that shares the OPP table has set the required devs ? */
> > -     if (opp_table->required_devs[0])
> > -             return 0;
> > +     /* Genpd core takes care of propagation to parent genpd */
> > +     if (opp_table->is_genpd) {
>
> A genpd can have non-genpd devices in the required OPPs and so this
> isn't sufficient. What we were ignoring earlier was genpd having
> another genpd as required opp.

Unless I am mistaken, I don't think that is a scenario we should care
about here.

_opp_set_required_dev() is being called for a device that is about to
be attached to its corresponding genpd.

Yes, in some cases, we attach a genpd provider's device to its
genpd-parent, but that is not to control the required-opps.

>
> > +             dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
> > +             return -EOPNOTSUPP;
> > +     }
> >
> >       for (i = 0; i < opp_table->required_opp_count; i++) {
> > -             /* Genpd core takes care of propagation to parent genpd */
> > -             if (required_devs[i] && opp_table->is_genpd &&
> > -                 opp_table->required_opp_tables[i]->is_genpd) {
> > -                     dev_err(dev, "%s: Operation not supported for genpds\n", __func__);
> > -                     return -EOPNOTSUPP;
> > -             }
> > +             struct opp_table *table = opp_table->required_opp_tables[i];
> > +
> > +             /*
> > +              * The OPP table should be available at this point. If not, it's
> > +              * not the one we are looking for.
> > +              */
> > +             if (IS_ERR(table))
> > +                     continue;
> > +
> > +             /* Move to the next available index. */
> > +             if (opp_table->required_devs[i])
> > +                     continue;
> >
> > -             opp_table->required_devs[i] = required_devs[i];
> > +             /*
> > +              * We need to compare the nodes for the OPP tables, rather than
> > +              * the OPP tables themselves, as we may have separate instances.
> > +              */
> > +             if (required_opp_table->np == table->np) {
> > +
>
> We don't keep such empty lines in OPP core generally at this place.

Yep, let me drop it!

>
> > +                     /* Cross check the OPP tables and fix it if needed. */
>
> Copy the bigger comment from_opp_attach_genpd() here too. It helps
> understanding why required_opp_tables entry is getting replaced.

Right, makes sense!

>
> > +                     if (required_opp_table != table) {
> > +                             dev_pm_opp_put_opp_table(table);
> > +                             _get_opp_table_kref(required_opp_table);
> > +                             opp_table->required_opp_tables[i] = required_opp_table;
> > +                     }
> > +
> > +                     opp_table->required_devs[i] = required_dev;
> > +
> > +                     /*
> > +                      * Add the required_dev as a user of the OPP table, so
> > +                      * we can call dev_pm_opp_set_opp() on it directly.
> > +                      */
> > +                     if (!_add_opp_dev(required_dev, required_opp_table)) {
> > +                             dev_err(dev, "Failed to add the device to the required OPP table\n");
> > +                             return -ENOMEM;
> > +                     }
> > +
> > +                     return i;
> > +             }
> >       }
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH 3/7] OPP: Rework _set_required_devs() to manage a single device per call

[Viresh Kumar]

On 11-07-24, 12:19, Ulf Hansson wrote:
> Unless I am mistaken, I don't think that is a scenario we should care
> about here.
> 
> _opp_set_required_dev() is being called for a device that is about to
> be attached to its corresponding genpd.
> 
> Yes, in some cases, we attach a genpd provider's device to its
> genpd-parent, but that is not to control the required-opps.

I understand and I am okay with what you are suggesting, we can fix it later on
if required anyway.

But just to give my reasoning behind that is that we want to avoid a very
specific case here and allow everything else. The special case being genpd
propagation, so I would normally not do a blanket ban but just that case.

But as I said, its okay :)

-- 
viresh

[PATCH 4/7] OPP: Introduce an OF helper function to inform if required-opps is used

[Ulf Hansson]

As being shown from a subsequent change to genpd, it's useful to understand
if a device's OF node has an OPP-table described and whether it contains
OPP nodes that makes use of the required-opps DT property.

For this reason, let's introduce an OPP OF helper function called
dev_pm_opp_of_has_required_opp().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/of.c       | 32 ++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 282eb5966fd0..55c8cfef97d4 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1443,6 +1443,38 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
 }
 EXPORT_SYMBOL_GPL(of_get_required_opp_performance_state);
 
+/**
+ * dev_pm_opp_of_has_required_opp - Find out if a required-opps exists.
+ * @dev: The device to investigate.
+ *
+ * Returns true if the device's node has a "operating-points-v2" property and if
+ * the corresponding node for the opp-table describes opp nodes that uses the
+ * "required-opps" property.
+ *
+ * Return: True if a required-opps is present, else false.
+ */
+bool dev_pm_opp_of_has_required_opp(struct device *dev)
+{
+	struct device_node *opp_np, *np;
+	int count;
+
+	opp_np = _opp_of_get_opp_desc_node(dev->of_node, 0);
+	if (!opp_np)
+		return false;
+
+	np = of_get_next_available_child(opp_np, NULL);
+	of_node_put(opp_np);
+	if (!np) {
+		dev_warn(dev, "Empty OPP table\n");
+		return false;
+	}
+
+	count = of_count_phandle_with_args(np, "required-opps", NULL);
+	of_node_put(np);
+
+	return count > 0;
+}
+
 /**
  * dev_pm_opp_get_of_node() - Gets the DT node corresponding to an opp
  * @opp:	opp for which DT node has to be returned for
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2b6599f6037d..5fade5c4de40 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -476,6 +476,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpuma
 struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
 struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
 int of_get_required_opp_performance_state(struct device_node *np, int index);
+bool dev_pm_opp_of_has_required_opp(struct device *dev);
 int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table);
 int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus);
 int dev_pm_opp_calc_power(struct device *dev, unsigned long *uW,
@@ -554,6 +555,11 @@ static inline int of_get_required_opp_performance_state(struct device_node *np,
 	return -EOPNOTSUPP;
 }
 
+static inline bool dev_pm_opp_of_has_required_opp(struct device *dev)
+{
+	return false;
+}
+
 static inline int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table)
 {
 	return -EOPNOTSUPP;
-- 
2.34.1

Re: [PATCH 4/7] OPP: Introduce an OF helper function to inform if required-opps is used

[Viresh Kumar]

On 19-06-24, 16:08, Ulf Hansson wrote:
> As being shown from a subsequent change to genpd, it's useful to understand
> if a device's OF node has an OPP-table described and whether it contains
> OPP nodes that makes use of the required-opps DT property.
> 
> For this reason, let's introduce an OPP OF helper function called
> dev_pm_opp_of_has_required_opp().
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/opp/of.c       | 32 ++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 38 insertions(+)

Applied. Thanks.

-- 
viresh

[PATCH 5/7] pmdomain: core: Manage the default required OPP from a separate function

[Ulf Hansson]

To improve the readability of the code in __genpd_dev_pm_attach(), let's
move out the required OPP handling into a separate function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 83d978743659..74ebb8a423be 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2774,12 +2774,34 @@ static void genpd_dev_pm_sync(struct device *dev)
 	genpd_queue_power_off_work(pd);
 }
 
+static int genpd_set_required_opp(struct device *dev, unsigned int index)
+{
+	int ret, pstate;
+
+	/* Set the default performance state */
+	pstate = of_get_required_opp_performance_state(dev->of_node, index);
+	if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
+		ret = pstate;
+		goto err;
+	} else if (pstate > 0) {
+		ret = dev_pm_genpd_set_performance_state(dev, pstate);
+		if (ret)
+			goto err;
+		dev_gpd_data(dev)->default_pstate = pstate;
+	}
+
+	return 0;
+err:
+	dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
+		dev_to_genpd(dev)->name, ret);
+	return ret;
+}
+
 static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 				 unsigned int index, bool power_on)
 {
 	struct of_phandle_args pd_args;
 	struct generic_pm_domain *pd;
-	int pstate;
 	int ret;
 
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
@@ -2808,17 +2830,9 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 	dev->pm_domain->detach = genpd_dev_pm_detach;
 	dev->pm_domain->sync = genpd_dev_pm_sync;
 
-	/* Set the default performance state */
-	pstate = of_get_required_opp_performance_state(dev->of_node, index);
-	if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
-		ret = pstate;
+	ret = genpd_set_required_opp(dev, index);
+	if (ret)
 		goto err;
-	} else if (pstate > 0) {
-		ret = dev_pm_genpd_set_performance_state(dev, pstate);
-		if (ret)
-			goto err;
-		dev_gpd_data(dev)->default_pstate = pstate;
-	}
 
 	if (power_on) {
 		genpd_lock(pd);
@@ -2840,8 +2854,6 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 	return 1;
 
 err:
-	dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
-		pd->name, ret);
 	genpd_remove_device(pd, dev);
 	return ret;
 }
-- 
2.34.1

[PATCH 6/7] OPP/pmdomain: Set the required_dev for a required OPP during genpd attach

[Ulf Hansson]

Through dev_pm_opp_set_config() the _opp_attach_genpd() allows consumer
drivers to hook up a device to its PM domains. This works for both a single
and multiple PM domains. Their corresponding virtual devices that are
created by genpd during attach, are later being assigned as the
required_devs for the corresponding required OPPs.

In principle this works fine, but there are some problems. Especially as
the index for a "required-opps" may not necessarily need to match the index
for the "power-domain" in DT, in which case things gets screwed up.

To improve the situation, let's instead assign the required_devs during
device attach in genpd, by using _opp_set_required_dev(). At this point the
genpd and the genpd's OPP table are known for the device in question, which
then can be used to find the correct index for the required-dev.

As a part of this change, genpd also starts to assign the required_devs
even for the single PM domain case, as a way to align the behaviour.
Furthermore, to maintain the existing behaviour for consumers of
_opp_attach_genpd(), let's adapt it to the new genpd behaviour.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c      | 45 +--------------------------------
 drivers/pmdomain/core.c | 55 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bc1ed1d3d60d..7e567b479c3d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2369,7 +2369,6 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 			continue;
 
 		dev_pm_domain_detach(opp_table->required_devs[index], false);
-		opp_table->required_devs[index] = NULL;
 	}
 }
 
@@ -2393,8 +2392,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 			const char * const *names, struct device ***virt_devs)
 {
-	struct device *virt_dev, *gdev;
-	struct opp_table *genpd_table;
+	struct device *virt_dev;
 	int index = 0, ret = -EINVAL;
 	const char * const *name = names;
 
@@ -2427,47 +2425,6 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 			goto err;
 		}
 
-		/*
-		 * The required_opp_tables parsing is not perfect, as the OPP
-		 * core does the parsing solely based on the DT node pointers.
-		 * The core sets the required_opp_tables entry to the first OPP
-		 * table in the "opp_tables" list, that matches with the node
-		 * pointer.
-		 *
-		 * If the target DT OPP table is used by multiple devices and
-		 * they all create separate instances of 'struct opp_table' from
-		 * it, then it is possible that the required_opp_tables entry
-		 * may be set to the incorrect sibling device.
-		 *
-		 * Cross check it again and fix if required.
-		 */
-		gdev = dev_to_genpd_dev(virt_dev);
-		if (IS_ERR(gdev))
-			return PTR_ERR(gdev);
-
-		genpd_table = _find_opp_table(gdev);
-		if (!IS_ERR(genpd_table)) {
-			if (genpd_table != opp_table->required_opp_tables[index]) {
-				dev_pm_opp_put_opp_table(opp_table->required_opp_tables[index]);
-				opp_table->required_opp_tables[index] = genpd_table;
-			} else {
-				dev_pm_opp_put_opp_table(genpd_table);
-			}
-		}
-
-		/*
-		 * Add the virtual genpd device as a user of the OPP table, so
-		 * we can call dev_pm_opp_set_opp() on it directly.
-		 *
-		 * This will be automatically removed when the OPP table is
-		 * removed, don't need to handle that here.
-		 */
-		if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) {
-			ret = -ENOMEM;
-			goto err;
-		}
-
-		opp_table->required_devs[index] = virt_dev;
 		index++;
 		name++;
 	}
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 74ebb8a423be..a38d08862a61 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2774,6 +2774,57 @@ static void genpd_dev_pm_sync(struct device *dev)
 	genpd_queue_power_off_work(pd);
 }
 
+static struct opp_table *genpd_find_opp_table(struct generic_pm_domain *genpd,
+					      unsigned int depth)
+{
+	struct opp_table *opp_table;
+	struct gpd_link *link;
+
+	if (genpd->opp_table)
+		return genpd->opp_table;
+
+	list_for_each_entry(link, &genpd->child_links, child_node) {
+		struct generic_pm_domain *parent = link->parent;
+
+		genpd_lock_nested(parent, depth + 1);
+		opp_table = genpd_find_opp_table(parent, depth + 1);
+		genpd_unlock(parent);
+
+		if (opp_table)
+			return opp_table;
+	}
+
+	return NULL;
+}
+
+static int genpd_set_required_opp_dev(struct device *dev,
+				      struct device *base_dev)
+{
+	struct generic_pm_domain *genpd = dev_to_genpd(dev);
+	struct opp_table *opp_table;
+	int ret = 0;
+
+	if (!dev_pm_opp_of_has_required_opp(base_dev))
+		return 0;
+
+	genpd_lock(genpd);
+	opp_table = genpd_find_opp_table(genpd, 0);
+	genpd_unlock(genpd);
+
+	if (opp_table) {
+		struct dev_pm_opp_config config = {
+			.required_dev = dev,
+			.required_opp_table = opp_table,
+		};
+
+		ret = devm_pm_opp_set_config(base_dev, &config);
+		if (ret < 0)
+			dev_err(dev, "failed to set opp config %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int genpd_set_required_opp(struct device *dev, unsigned int index)
 {
 	int ret, pstate;
@@ -2830,6 +2881,10 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 	dev->pm_domain->detach = genpd_dev_pm_detach;
 	dev->pm_domain->sync = genpd_dev_pm_sync;
 
+	ret = genpd_set_required_opp_dev(dev, base_dev);
+	if (ret)
+		goto err;
+
 	ret = genpd_set_required_opp(dev, index);
 	if (ret)
 		goto err;
-- 
2.34.1

Re: [PATCH 6/7] OPP/pmdomain: Set the required_dev for a required OPP during genpd attach

[Viresh Kumar]

On 19-06-24, 16:08, Ulf Hansson wrote:
> @@ -2393,8 +2392,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>  static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>  			const char * const *names, struct device ***virt_devs)

I was expecting that we can get rid of this routine completely and OPP
core won't be required to handle this anymore.

-- 
viresh

Re: [PATCH 6/7] OPP/pmdomain: Set the required_dev for a required OPP during genpd attach

[Ulf Hansson]

On Wed, 26 Jun 2024 at 08:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-06-24, 16:08, Ulf Hansson wrote:
> > @@ -2393,8 +2392,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> >  static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
> >                       const char * const *names, struct device ***virt_devs)
>
> I was expecting that we can get rid of this routine completely and OPP
> core won't be required to handle this anymore.

Yes, it's the next step. To enable that, I will first need to move the
current users of _opp_attach_genpd() to use the new
dev_pm_domain_attach_list() instead.

It's pretty straight forward, but I decided to make that as a step on
top of the $subject series.

Kind regards
Uffe

[PATCH 7/7] pmdomain: core: Drop the redundant dev_to_genpd_dev()

[Ulf Hansson]

There's no longer any users of dev_to_genpd_dev(), hence let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c   | 10 ----------
 include/linux/pm_domain.h |  6 ------
 2 files changed, 16 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index a38d08862a61..4abedbb65354 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -184,16 +184,6 @@ static struct generic_pm_domain *dev_to_genpd(struct device *dev)
 	return pd_to_genpd(dev->pm_domain);
 }
 
-struct device *dev_to_genpd_dev(struct device *dev)
-{
-	struct generic_pm_domain *genpd = dev_to_genpd(dev);
-
-	if (IS_ERR(genpd))
-		return ERR_CAST(genpd);
-
-	return &genpd->dev;
-}
-
 static int genpd_stop_dev(const struct generic_pm_domain *genpd,
 			  struct device *dev)
 {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index f24546a3d3db..772d3280d35f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -260,7 +260,6 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off);
 int pm_genpd_remove(struct generic_pm_domain *genpd);
-struct device *dev_to_genpd_dev(struct device *dev);
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
 int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
 int dev_pm_genpd_remove_notifier(struct device *dev);
@@ -308,11 +307,6 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
 	return -EOPNOTSUPP;
 }
 
-static inline struct device *dev_to_genpd_dev(struct device *dev)
-{
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
 static inline int dev_pm_genpd_set_performance_state(struct device *dev,
 						     unsigned int state)
 {
-- 
2.34.1

Re: [PATCH 0/7] OPP/pmdomain: Assign required_devs for required OPPs through genpd

[Konrad Dybcio]

On 19.06.2024 4:08 PM, Ulf Hansson wrote:
> Through dev_pm_opp_set_config() the _opp_attach_genpd() allows consumer
> drivers to hook up a device to its PM domains. This works for both a single
> and multiple PM domains. Their corresponding virtual devices that are
> created by genpd during attach, are later being assigned as the
> required_devs for the corresponding required OPPs.
> 
> In principle this works fine, but there are some problems. Especially as
> the index for a "required-opps" may not necessarily need to match the index
> for the "power-domain" in DT, in which case things gets screwed up.

So, is this series essentially tackling a problem like this:

pdp_A: power-domain-provider@aaaaaa {
	[...]

	opp-table {
		pdp_A_opp0: opp-0 {
			opp-level = <0>;
		};
	};
};

pdp_B: power-domain-provider@bbbbbbb {
	[...]

	opp-table {
		pdp_B_opp0: opp-0 {
			opp-level = <0>;
		};
	};
};

nice-device@ccccccc {
	[...]

	power-domains = <&pdp_A>,
			<&pdp_B>;
	// order doesn't match /\
	required-opps = <&pdp_B_opp0>,
			<&pdp_A_opp0>;
};


?

Konrad

Re: [PATCH 0/7] OPP/pmdomain: Assign required_devs for required OPPs through genpd

[Ulf Hansson]

On Sat, 22 Jun 2024 at 14:18, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 19.06.2024 4:08 PM, Ulf Hansson wrote:
> > Through dev_pm_opp_set_config() the _opp_attach_genpd() allows consumer
> > drivers to hook up a device to its PM domains. This works for both a single
> > and multiple PM domains. Their corresponding virtual devices that are
> > created by genpd during attach, are later being assigned as the
> > required_devs for the corresponding required OPPs.
> >
> > In principle this works fine, but there are some problems. Especially as
> > the index for a "required-opps" may not necessarily need to match the index
> > for the "power-domain" in DT, in which case things gets screwed up.
>
> So, is this series essentially tackling a problem like this:
>
> pdp_A: power-domain-provider@aaaaaa {
>         [...]
>
>         opp-table {
>                 pdp_A_opp0: opp-0 {
>                         opp-level = <0>;
>                 };
>         };
> };
>
> pdp_B: power-domain-provider@bbbbbbb {
>         [...]
>
>         opp-table {
>                 pdp_B_opp0: opp-0 {
>                         opp-level = <0>;
>                 };
>         };
> };
>
> nice-device@ccccccc {
>         [...]
>
>         power-domains = <&pdp_A>,
>                         <&pdp_B>;
>         // order doesn't match /\
>         required-opps = <&pdp_B_opp0>,
>                         <&pdp_A_opp0>;
> };
>
>
> ?
>

Correct, this should be taken care off by $subject series.

Kind regards
Uffe