The recent attempt to make genpd first lookup an OPP table for a device
that has been attached to it and then let the OPP core validate whether the
OPP table is correct, fails for some configurations.

More precisely, if a device has multiple power-domains, which all has an
OPP table associated, doesn't necessarily mean that the device's OPP table
needs multiple phandles specified in the required-opps. Instead it's
perfectly possible to use a single phandle in the required-opps, which is
typically where the current code fails to assign the correct required-dev.

To fix this problem, let's instead start by letting the OPP core find the
device node for the required OPP table and then let genpd search for a
corresponding OPP table, allowing us the find the correct required-dev to
assign for it.

Reported-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Fixes: f0d2dcc9b087 ("OPP/pmdomain: Set the required_dev for a required OPP during genpd attach")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c      | 15 +++++++-----
 drivers/pmdomain/core.c | 52 +++++++++++++++++++++++------------------
 include/linux/pm_opp.h  |  7 ++++--
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 66cac7a1d9db..538612886446 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2363,7 +2363,7 @@ static void _opp_put_config_regulators_helper(struct opp_table *opp_table)
 static int _opp_set_required_dev(struct opp_table *opp_table,
 				 struct device *dev,
 				 struct device *required_dev,
-				 struct opp_table *required_opp_table)
+				 config_required_dev_t config_required_dev)
 {
 	int i;
 
@@ -2380,6 +2380,7 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		struct opp_table *table = opp_table->required_opp_tables[i];
+		struct opp_table *required_opp_table;
 
 		/*
 		 * The OPP table should be available at this point. If not, it's
@@ -2396,7 +2397,9 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
 		 * 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) {
+		required_opp_table = config_required_dev(required_dev,
+							 table->np);
+		if (required_opp_table) {
 			/*
 			 * The required_opp_tables parsing is not perfect, as
 			 * the OPP core does the parsing solely based on the DT
@@ -2422,8 +2425,8 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
 		}
 	}
 
-	dev_err(dev, "Missing OPP table, unable to set the required dev\n");
-	return -ENODEV;
+	/* No matching OPP table found for the required_dev. */
+	return -ERANGE;
 }
 
 static void _opp_put_required_dev(struct opp_table *opp_table,
@@ -2547,10 +2550,10 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 		data->flags |= OPP_CONFIG_REGULATOR;
 	}
 
-	if (config->required_dev && config->required_opp_table) {
+	if (config->required_dev && config->config_required_dev) {
 		ret = _opp_set_required_dev(opp_table, dev,
 					    config->required_dev,
-					    config->required_opp_table);
+					    config->config_required_dev);
 		if (ret < 0)
 			goto err;
 
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index edef1a520110..0ff0b513b2a1 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2874,20 +2874,21 @@ 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)
+static struct opp_table *_genpd_find_opp_table(struct generic_pm_domain *genpd,
+					       struct device_node *np,
+					       unsigned int depth)
 {
-	struct opp_table *opp_table;
+	struct opp_table *opp_table = genpd->opp_table;
 	struct gpd_link *link;
 
-	if (genpd->opp_table)
-		return genpd->opp_table;
+	if (opp_table && (dev_pm_opp_table_to_of_node(opp_table) == np))
+		return 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);
+		opp_table = _genpd_find_opp_table(parent, np, depth + 1);
 		genpd_unlock(parent);
 
 		if (opp_table)
@@ -2897,12 +2898,27 @@ static struct opp_table *genpd_find_opp_table(struct generic_pm_domain *genpd,
 	return NULL;
 }
 
-static int genpd_set_required_opp_dev(struct device *dev,
-				      struct device *base_dev)
+static struct opp_table *genpd_find_opp_table(struct device *dev,
+					      struct device_node *np)
 {
 	struct generic_pm_domain *genpd = dev_to_genpd(dev);
 	struct opp_table *opp_table;
-	int ret = 0;
+
+	genpd_lock(genpd);
+	opp_table = _genpd_find_opp_table(genpd, np, 0);
+	genpd_unlock(genpd);
+
+	return opp_table;
+}
+
+static int genpd_set_required_opp_dev(struct device *dev,
+				      struct device *base_dev)
+{
+	struct dev_pm_opp_config config = {
+		.required_dev = dev,
+		.config_required_dev = genpd_find_opp_table,
+	};
+	int ret;
 
 	/* Limit support to non-providers for now. */
 	if (of_property_present(base_dev->of_node, "#power-domain-cells"))
@@ -2911,20 +2927,10 @@ static int genpd_set_required_opp_dev(struct device *dev,
 	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);
-	}
+	ret = devm_pm_opp_set_config(base_dev, &config);
+	ret = ret == -ERANGE ? 0 : ret;
+	if (ret < 0)
+		dev_err(dev, "failed to set opp config %d\n", ret);
 
 	return ret;
 }
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 7894e631cded..0ada4a5057c8 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -53,6 +53,9 @@ typedef int (*config_regulators_t)(struct device *dev,
 typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
 			struct dev_pm_opp *opp, void *data, bool scaling_down);
 
+typedef struct opp_table *(*config_required_dev_t)(struct device *dev,
+			struct device_node *opp_table_np);
+
 /**
  * struct dev_pm_opp_config - Device OPP configuration values
  * @clk_names: Clk names, NULL terminated array.
@@ -63,7 +66,7 @@ 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.
  * @required_dev: Required OPP device.
- * @required_opp_table: The corresponding required OPP table for @required_dev.
+ * @config_required_dev: Custom helper to find the required OPP table for $required_dev.
  *
  * This structure contains platform specific OPP configurations for the device.
  */
@@ -77,7 +80,7 @@ struct dev_pm_opp_config {
 	unsigned int supported_hw_count;
 	const char * const *regulator_names;
 	struct device *required_dev;
-	struct opp_table *required_opp_table;
+	config_required_dev_t config_required_dev;
 };
 
 #define OPP_LEVEL_UNSET			U32_MAX
-- 
2.34.1

On 03-09-24, 00:48, Ulf Hansson wrote:
> To fix this problem, let's instead start by letting the OPP core find the
> device node for the required OPP table and then let genpd search for a
> corresponding OPP table, allowing us the find the correct required-dev to
> assign for it.

Why was doing this necessary ?

-- 
viresh

On Tue, 3 Sept 2024 at 09:16, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-09-24, 00:48, Ulf Hansson wrote:
> > To fix this problem, let's instead start by letting the OPP core find the
> > device node for the required OPP table and then let genpd search for a
> > corresponding OPP table, allowing us the find the correct required-dev to
> > assign for it.
>
> Why was doing this necessary ?

Let me try to elaborate a bit more.

In the current code, genpd_find_opp_table() tries to find an OPP table
for the genpd that the device is getting attached to. Then genpd
passes that OPP table via devm_pm_opp_set_config(), to let the OPP
core to hook up a required-dev for it. This was a naive approach, as
that OPP table may not be the one that actually corresponds to a
required-opps for the required-dev. Consider the below in DT.

        opp_table_devA: opp-table-devA {
                compatible = "operating-points-v2";

                opp-devA-50 {
                        opp-hz = /bits/ 64 <2500>;
                        required-opps = <&opp_pd_50>; //corresponds to
pd_perf1's OPP table
                };
               ....

        devA {
                compatible = "foo,bar";
                power-domains = <&pd_perf0>, <&pd_perf1>; //both
pd_perf0 and pd_perf1 has OPP tables.
                power-domain-names = "perf0", "perf1";
                operating-points-v2 = <&opp_table_devA>;
        };

To make sure we assign the correct required-dev for cases like the
above, we need to let the OPP core to iterate through the available
required-opps and see if some of them are corresponding to the OPP
table for the genpd the required-dev belongs too.

To manage this in a non-genpd specific way, I added another callback
in struct dev_pm_opp_config. In this way, it should work for any
future possible required-devs types too, I think.

Kind regards
Uffe

On 03-09-24, 11:54, Ulf Hansson wrote:
> Let me try to elaborate a bit more.
> 
> In the current code, genpd_find_opp_table() tries to find an OPP table
> for the genpd that the device is getting attached to. Then genpd
> passes that OPP table via devm_pm_opp_set_config(), to let the OPP
> core to hook up a required-dev for it. This was a naive approach, as
> that OPP table may not be the one that actually corresponds to a
> required-opps for the required-dev. Consider the below in DT.
> 
>         opp_table_devA: opp-table-devA {
>                 compatible = "operating-points-v2";
> 
>                 opp-devA-50 {
>                         opp-hz = /bits/ 64 <2500>;
>                         required-opps = <&opp_pd_50>; //corresponds to
> pd_perf1's OPP table
>                 };
>                ....
> 
>         devA {
>                 compatible = "foo,bar";
>                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> pd_perf0 and pd_perf1 has OPP tables.
>                 power-domain-names = "perf0", "perf1";
>                 operating-points-v2 = <&opp_table_devA>;
>         };

I think another way forward would be to send an index along with
required-dev information (now that you do it one by one). That index
would be the index of the genpd in the genpd-list for the device. That
would make it work, isn't it ?

I would like to avoid (another) callback from the OPP core, we already
have few of them and I don't like them a lot. Moreover, genpd should
be able to get the right required opp, with an index. Unless I am
mistaken and this still doesn't solve it :)

> To make sure we assign the correct required-dev for cases like the
> above, we need to let the OPP core to iterate through the available
> required-opps and see if some of them are corresponding to the OPP
> table for the genpd the required-dev belongs too.
> 
> To manage this in a non-genpd specific way, I added another callback
> in struct dev_pm_opp_config. In this way, it should work for any
> future possible required-devs types too, I think.

-- 
viresh

On Tue, 3 Sept 2024 at 12:53, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-09-24, 11:54, Ulf Hansson wrote:
> > Let me try to elaborate a bit more.
> >
> > In the current code, genpd_find_opp_table() tries to find an OPP table
> > for the genpd that the device is getting attached to. Then genpd
> > passes that OPP table via devm_pm_opp_set_config(), to let the OPP
> > core to hook up a required-dev for it. This was a naive approach, as
> > that OPP table may not be the one that actually corresponds to a
> > required-opps for the required-dev. Consider the below in DT.
> >
> >         opp_table_devA: opp-table-devA {
> >                 compatible = "operating-points-v2";
> >
> >                 opp-devA-50 {
> >                         opp-hz = /bits/ 64 <2500>;
> >                         required-opps = <&opp_pd_50>; //corresponds to
> > pd_perf1's OPP table
> >                 };
> >                ....
> >
> >         devA {
> >                 compatible = "foo,bar";
> >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > pd_perf0 and pd_perf1 has OPP tables.
> >                 power-domain-names = "perf0", "perf1";
> >                 operating-points-v2 = <&opp_table_devA>;
> >         };
>
> I think another way forward would be to send an index along with
> required-dev information (now that you do it one by one). That index
> would be the index of the genpd in the genpd-list for the device. That
> would make it work, isn't it ?

I am not sure how that index will be much helpful, but maybe I am not
fully understanding what you propose.

Please note that the index of the power-domain doesn't need to match
the index of the required-opps.

It's only the phandle of the required-opps, at some index, that can
point out to which power-domain (and thus what required-dev) it
belongs to.

>
> I would like to avoid (another) callback from the OPP core, we already
> have few of them and I don't like them a lot. Moreover, genpd should
> be able to get the right required opp, with an index. Unless I am
> mistaken and this still doesn't solve it :)

Sorry, but I couldn't figure out a better option.

>
> > To make sure we assign the correct required-dev for cases like the
> > above, we need to let the OPP core to iterate through the available
> > required-opps and see if some of them are corresponding to the OPP
> > table for the genpd the required-dev belongs too.
> >
> > To manage this in a non-genpd specific way, I added another callback
> > in struct dev_pm_opp_config. In this way, it should work for any
> > future possible required-devs types too, I think.
>
> --
> viresh

Kind regards
Uffe

On 03-09-24, 13:43, Ulf Hansson wrote:
> On Tue, 3 Sept 2024 at 12:53, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 03-09-24, 11:54, Ulf Hansson wrote:
> > > In the current code, genpd_find_opp_table() tries to find an OPP table
> > > for the genpd that the device is getting attached to. Then genpd
> > > passes that OPP table via devm_pm_opp_set_config(), to let the OPP
> > > core to hook up a required-dev for it. This was a naive approach, as
> > > that OPP table may not be the one that actually corresponds to a
> > > required-opps for the required-dev. Consider the below in DT.
> > >
> > >         opp_table_devA: opp-table-devA {
> > >                 compatible = "operating-points-v2";
> > >
> > >                 opp-devA-50 {
> > >                         opp-hz = /bits/ 64 <2500>;
> > >                         required-opps = <&opp_pd_50>; //corresponds to
> > > pd_perf1's OPP table
> > >                 };
> > >                ....
> > >
> > >         devA {
> > >                 compatible = "foo,bar";
> > >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > > pd_perf0 and pd_perf1 has OPP tables.
> > >                 power-domain-names = "perf0", "perf1";
> > >                 operating-points-v2 = <&opp_table_devA>;
> > >         };
> >
> > I think another way forward would be to send an index along with
> > required-dev information (now that you do it one by one). That index
> > would be the index of the genpd in the genpd-list for the device. That
> > would make it work, isn't it ?
> 
> I am not sure how that index will be much helpful, but maybe I am not
> fully understanding what you propose.
> 
> Please note that the index of the power-domain doesn't need to match
> the index of the required-opps.

Yeah, I missed that, it doesn't happen via DT but by platform code. I
do see problems where situation would be a bit ambiguous. Your example
with a minor change to your code:

        opp_table_devA: opp-table-devA {
                compatible = "operating-points-v2";

                opp-devA-50 {
                        opp-hz = /bits/ 64 <2500>;
                        required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
                };
               ....

        devA {
                compatible = "foo,bar";
                power-domains = <&pd_perf0>, <&pd_perf1>; //both
pd_perf0 and pd_perf1 has OPP tables.
                power-domain-names = "perf0", "perf1";
                operating-points-v2 = <&opp_table_devA>;
        };

Here, I don't think there is a way for us to know which genpd does
opp_pd_50 belongs to and to which one opp_pd_51 does.

We solve this by sending clock_names and regulator_names in OPP
config structure. That gives the ordering in which required_opps are
present. The same needs to be done for genpd, and then genpd core
would be able to attach the right genpd with right required opp.

-- 
viresh

On Wed, 4 Sept 2024 at 08:40, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-09-24, 13:43, Ulf Hansson wrote:
> > On Tue, 3 Sept 2024 at 12:53, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 03-09-24, 11:54, Ulf Hansson wrote:
> > > > In the current code, genpd_find_opp_table() tries to find an OPP table
> > > > for the genpd that the device is getting attached to. Then genpd
> > > > passes that OPP table via devm_pm_opp_set_config(), to let the OPP
> > > > core to hook up a required-dev for it. This was a naive approach, as
> > > > that OPP table may not be the one that actually corresponds to a
> > > > required-opps for the required-dev. Consider the below in DT.
> > > >
> > > >         opp_table_devA: opp-table-devA {
> > > >                 compatible = "operating-points-v2";
> > > >
> > > >                 opp-devA-50 {
> > > >                         opp-hz = /bits/ 64 <2500>;
> > > >                         required-opps = <&opp_pd_50>; //corresponds to
> > > > pd_perf1's OPP table
> > > >                 };
> > > >                ....
> > > >
> > > >         devA {
> > > >                 compatible = "foo,bar";
> > > >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > > > pd_perf0 and pd_perf1 has OPP tables.
> > > >                 power-domain-names = "perf0", "perf1";
> > > >                 operating-points-v2 = <&opp_table_devA>;
> > > >         };
> > >
> > > I think another way forward would be to send an index along with
> > > required-dev information (now that you do it one by one). That index
> > > would be the index of the genpd in the genpd-list for the device. That
> > > would make it work, isn't it ?
> >
> > I am not sure how that index will be much helpful, but maybe I am not
> > fully understanding what you propose.
> >
> > Please note that the index of the power-domain doesn't need to match
> > the index of the required-opps.
>
> Yeah, I missed that, it doesn't happen via DT but by platform code. I
> do see problems where situation would be a bit ambiguous. Your example
> with a minor change to your code:
>
>         opp_table_devA: opp-table-devA {
>                 compatible = "operating-points-v2";
>
>                 opp-devA-50 {
>                         opp-hz = /bits/ 64 <2500>;
>                         required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
>                 };
>                ....
>
>         devA {
>                 compatible = "foo,bar";
>                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> pd_perf0 and pd_perf1 has OPP tables.
>                 power-domain-names = "perf0", "perf1";
>                 operating-points-v2 = <&opp_table_devA>;
>         };
>
> Here, I don't think there is a way for us to know which genpd does
> opp_pd_50 belongs to and to which one opp_pd_51 does.
>
> We solve this by sending clock_names and regulator_names in OPP
> config structure. That gives the ordering in which required_opps are
> present. The same needs to be done for genpd, and then genpd core
> would be able to attach the right genpd with right required opp.

No, we don't need this for gend as $subject patch is addressing this
problem too. Let me elaborate.

The OPP core holds the information about the devA's required-opps and
to what OPP table each required-opps belongs to
(opp_table->required_opp_tables[n]).

The genpd core holds the information about the allocated virtual
devices that it creates when it attached devA to its power-domains.
The virtual device(s) gets a genpd attached to it and that genpd also
has an OPP table associated with it (genpd->opp_table).

By asking the OPP core to walk through the array of allocated
required-opps for devA and to match it against a *one* of the virtual
devices' genpd->opp_table, we can figure out at what index we should
assign the virtual device to in the opp_table->required_devs[index].

Kind regards
Uffe

On 04-09-24, 14:57, Ulf Hansson wrote:
> > Yeah, I missed that, it doesn't happen via DT but by platform code. I
> > do see problems where situation would be a bit ambiguous. Your example
> > with a minor change to your code:
> >
> >         opp_table_devA: opp-table-devA {
> >                 compatible = "operating-points-v2";
> >
> >                 opp-devA-50 {
> >                         opp-hz = /bits/ 64 <2500>;
> >                         required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
> >                 };
> >                ....
> >
> >         devA {
> >                 compatible = "foo,bar";
> >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > pd_perf0 and pd_perf1 has OPP tables.
> >                 power-domain-names = "perf0", "perf1";
> >                 operating-points-v2 = <&opp_table_devA>;
> >         };
> >
> > Here, I don't think there is a way for us to know which genpd does
> > opp_pd_50 belongs to and to which one opp_pd_51 does.
> >
> > We solve this by sending clock_names and regulator_names in OPP
> > config structure. That gives the ordering in which required_opps are
> > present. The same needs to be done for genpd, and then genpd core
> > would be able to attach the right genpd with right required opp.
> 
> No, we don't need this for gend as $subject patch is addressing this
> problem too. Let me elaborate.
> 
> The OPP core holds the information about the devA's required-opps and
> to what OPP table each required-opps belongs to
> (opp_table->required_opp_tables[n]).
> 
> The genpd core holds the information about the allocated virtual
> devices that it creates when it attached devA to its power-domains.
> The virtual device(s) gets a genpd attached to it and that genpd also
> has an OPP table associated with it (genpd->opp_table).
> 
> By asking the OPP core to walk through the array of allocated
> required-opps for devA and to match it against a *one* of the virtual
> devices' genpd->opp_table, we can figure out at what index we should
> assign the virtual device to in the opp_table->required_devs[index].

How do we differentiate between two cases where the required-opps can
be defined as either of these:

required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)

OR

required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1

I thought this can't be fixed without some platform code telling how
the DT is really configured, i.e. order of the power domains in the
required-opps.

-- 
viresh

On Fri, 6 Sept 2024 at 08:14, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 04-09-24, 14:57, Ulf Hansson wrote:
> > > Yeah, I missed that, it doesn't happen via DT but by platform code. I
> > > do see problems where situation would be a bit ambiguous. Your example
> > > with a minor change to your code:
> > >
> > >         opp_table_devA: opp-table-devA {
> > >                 compatible = "operating-points-v2";
> > >
> > >                 opp-devA-50 {
> > >                         opp-hz = /bits/ 64 <2500>;
> > >                         required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
> > >                 };
> > >                ....
> > >
> > >         devA {
> > >                 compatible = "foo,bar";
> > >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > > pd_perf0 and pd_perf1 has OPP tables.
> > >                 power-domain-names = "perf0", "perf1";
> > >                 operating-points-v2 = <&opp_table_devA>;
> > >         };
> > >
> > > Here, I don't think there is a way for us to know which genpd does
> > > opp_pd_50 belongs to and to which one opp_pd_51 does.
> > >
> > > We solve this by sending clock_names and regulator_names in OPP
> > > config structure. That gives the ordering in which required_opps are
> > > present. The same needs to be done for genpd, and then genpd core
> > > would be able to attach the right genpd with right required opp.
> >
> > No, we don't need this for gend as $subject patch is addressing this
> > problem too. Let me elaborate.
> >
> > The OPP core holds the information about the devA's required-opps and
> > to what OPP table each required-opps belongs to
> > (opp_table->required_opp_tables[n]).
> >
> > The genpd core holds the information about the allocated virtual
> > devices that it creates when it attached devA to its power-domains.
> > The virtual device(s) gets a genpd attached to it and that genpd also
> > has an OPP table associated with it (genpd->opp_table).
> >
> > By asking the OPP core to walk through the array of allocated
> > required-opps for devA and to match it against a *one* of the virtual
> > devices' genpd->opp_table, we can figure out at what index we should
> > assign the virtual device to in the opp_table->required_devs[index].
>
> How do we differentiate between two cases where the required-opps can
> be defined as either of these:
>
> required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
>
> OR
>
> required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1
>
> I thought this can't be fixed without some platform code telling how
> the DT is really configured, i.e. order of the power domains in the
> required-opps.

I don't think we need platform code for this.

When registering a genpd provider, an OPP table gets assigned to it.
When hooking up a device to one of its genpd providers, that virtual
device then also gets a handle to its genpd's OPP table.

Each of the phandles in the required-opps points to another OPP table,
which OPP table should be associated with a specific genpd.

In other words, the information is there, we should not need anything
additional in DT.

Kind regards
Uffe

FYI, I am on holidays now :)

On Fri, 6 Sept 2024 at 14:19, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > How do we differentiate between two cases where the required-opps can
> > be defined as either of these:
> >
> > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
> >
> > OR
> >
> > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1
> >
> > I thought this can't be fixed without some platform code telling how
> > the DT is really configured, i.e. order of the power domains in the
> > required-opps.
>
> I don't think we need platform code for this.
>
> When registering a genpd provider, an OPP table gets assigned to it.

So we will create a real OPP table in code, which will point to the common
OPP table in DT. Fine.

> When hooking up a device to one of its genpd providers, that virtual
> device then also gets a handle to its genpd's OPP table.

Right.

If there are two genpds required for a device from the same genpd provider, the
picture isn't very clear at this point. i.e. which required OPP
belongs to which genpd,
as both have same table in DT.

> Each of the phandles in the required-opps points to another OPP table,
> which OPP table should be associated with a specific genpd.

Yes, but a simple order reversal in DT (which I sent in my last
email), will not be picked
by code at all. i.e. DT doesn't give the order in which required OPPs
are present.

> In other words, the information is there, we should not need anything
> additional in DT.

On Wed, 11 Sept 2024 at 08:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> FYI, I am on holidays now :)

Oh, nice! Enjoy!

>
> On Fri, 6 Sept 2024 at 14:19, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > How do we differentiate between two cases where the required-opps can
> > > be defined as either of these:
> > >
> > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
> > >
> > > OR
> > >
> > > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1
> > >
> > > I thought this can't be fixed without some platform code telling how
> > > the DT is really configured, i.e. order of the power domains in the
> > > required-opps.
> >
> > I don't think we need platform code for this.
> >
> > When registering a genpd provider, an OPP table gets assigned to it.
>
> So we will create a real OPP table in code, which will point to the common
> OPP table in DT. Fine.
>
> > When hooking up a device to one of its genpd providers, that virtual
> > device then also gets a handle to its genpd's OPP table.
>
> Right.
>
> If there are two genpds required for a device from the same genpd provider, the
> picture isn't very clear at this point. i.e. which required OPP
> belongs to which genpd,
> as both have same table in DT.

I agree that it's not very clear.

But to me, this seems like an orthogonal problem that really should
not be managed by platform specific code in consumer drivers.
Moreover, unless I am mistaken, I believe this isn't really a problem
for the currently supported use cases we have for required-opps. Or is
it?

That said, we already have two methods that helps us to deal with this issue:

1)
For a genpd OF provider that provides multiple genpds, the genpd/OPP
core tries to assign an OPP table for each genpd, based on the
power-domain index. In other words, if corresponding OPP-tables are
specified in the operating-points-v2 list, those would get assigned
accordingly.

2)
The genpd OF provider can control on a per genpd basis, whether there
should be an OPP table assigned to it. This is managed by assigning
the ->set_performance_state() callback for the genpd or leaving it
unassigned. Typically this works well, when there is one OPP-table
specified in the operating-points-v2 list for the provider - and only
one of the genpds that should use it.

If it turns out that we need something more flexible, I think we need
to look at extending the OPP/power-domain DT bindings. We would
probably need a "by-names" DT property, allowing us to specify the
mapping between the OPP-tables and the power-domains.

>
> > Each of the phandles in the required-opps points to another OPP table,
> > which OPP table should be associated with a specific genpd.
>
> Yes, but a simple order reversal in DT (which I sent in my last
> email), will not be picked
> by code at all. i.e. DT doesn't give the order in which required OPPs
> are present.

Assuming genpd OF providers are following 1) or 2), I don't think this
should be an issue.

>
> > In other words, the information is there, we should not need anything
> > additional in DT.

Kind regards
Uffe

On Wed, 11 Sept 2024 at 16:15, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 11 Sept 2024 at 08:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > FYI, I am on holidays now :)
>
> Oh, nice! Enjoy!
>
> >
> > On Fri, 6 Sept 2024 at 14:19, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > How do we differentiate between two cases where the required-opps can
> > > > be defined as either of these:
> > > >
> > > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
> > > >
> > > > OR
> > > >
> > > > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1
> > > >
> > > > I thought this can't be fixed without some platform code telling how
> > > > the DT is really configured, i.e. order of the power domains in the
> > > > required-opps.
> > >
> > > I don't think we need platform code for this.
> > >
> > > When registering a genpd provider, an OPP table gets assigned to it.
> >
> > So we will create a real OPP table in code, which will point to the common
> > OPP table in DT. Fine.
> >
> > > When hooking up a device to one of its genpd providers, that virtual
> > > device then also gets a handle to its genpd's OPP table.
> >
> > Right.
> >
> > If there are two genpds required for a device from the same genpd provider, the
> > picture isn't very clear at this point. i.e. which required OPP
> > belongs to which genpd,
> > as both have same table in DT.
>
> I agree that it's not very clear.
>
> But to me, this seems like an orthogonal problem that really should
> not be managed by platform specific code in consumer drivers.
> Moreover, unless I am mistaken, I believe this isn't really a problem
> for the currently supported use cases we have for required-opps. Or is
> it?

Answering my own question. After some further investigation, I am
afraid that your concern was correct.

One sm8250, venus is using three power-domains,"venus", "vcodec0",
"mx", but there is only one phandle in the required-opp.
"venus" and "vcodec0" correspond to the "videocc" power-domain, which
has a parent-domain that is the "rpmhpd".
"mx" corresponds to the "rpmhpd".
The rpmhpd power-domain has one shared OPP table used for all the
power-domains it provides. :-(

Because we also need to look for a matching OPP table for the
required-opp by walking the power-domain parents (needed on Tegra), we
simply can't tell what power-domain the required-opp belongs to.

>
> That said, we already have two methods that helps us to deal with this issue:
>
> 1)
> For a genpd OF provider that provides multiple genpds, the genpd/OPP
> core tries to assign an OPP table for each genpd, based on the
> power-domain index. In other words, if corresponding OPP-tables are
> specified in the operating-points-v2 list, those would get assigned
> accordingly.
>
> 2)
> The genpd OF provider can control on a per genpd basis, whether there
> should be an OPP table assigned to it. This is managed by assigning
> the ->set_performance_state() callback for the genpd or leaving it
> unassigned. Typically this works well, when there is one OPP-table
> specified in the operating-points-v2 list for the provider - and only
> one of the genpds that should use it.
>
> If it turns out that we need something more flexible, I think we need
> to look at extending the OPP/power-domain DT bindings. We would
> probably need a "by-names" DT property, allowing us to specify the
> mapping between the OPP-tables and the power-domains.
>
> >
> > > Each of the phandles in the required-opps points to another OPP table,
> > > which OPP table should be associated with a specific genpd.
> >
> > Yes, but a simple order reversal in DT (which I sent in my last
> > email), will not be picked
> > by code at all. i.e. DT doesn't give the order in which required OPPs
> > are present.
>
> Assuming genpd OF providers are following 1) or 2), I don't think this
> should be an issue.

It looks like I was wrong.

This whole problem boils down to that we are allowing the OPP table to
be shared for a genpd OF provider for multiple power-domains. We could
consider adding some new DT property to point out at what power-domain
the required-opps belongs to, but it doesn't really matter as we need
to keep supporting the current DTS.

Oh well, back to the drawing table to re-work this again. It looks
like we need to make it possible for consumer drivers to provide
additional information to dev_pm_domain_attach_list(), allowing it to
understand how the required-devs should be assigned.

Unless you have some better ideas?

[...]

Kind regards
Uffe