[PATCH 0/3] OPP: Simplify set_required_opp handling

[PATCH 0/3] OPP: Simplify set_required_opp handling

[Viresh Kumar]

Hello,

The required-opps configuration is closely tied to genpd and performance
states at the moment and it is not very obvious that required-opps can
live without genpds. Though we don't support configuring required-opps
for non-genpd cases currently.

This patchset aims at cleaning up this a bit, just like what's done for clk and
regulators. This also makes it possible for platforms to provide their own
version of set_required_opps() helper, which can be used to configure the
devfreq device propertly.

Jun,

I haven't found time to test this through yet, though there isn't much anyway I
guess. Can you see if these can solve your problem properly ?

Viresh Kumar (3):
  OPP: Handle all genpd cases together in _set_required_opps()
  OPP: Move required opps configuration to specialized callback
  OPP: Allow platforms to add a set_required_opps() callback

 drivers/opp/core.c     | 113 ++++++++++++++++++++++++++++-------------
 drivers/opp/of.c       |   3 ++
 drivers/opp/opp.h      |   4 ++
 include/linux/pm_opp.h |   5 ++
 4 files changed, 91 insertions(+), 34 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH 1/3] OPP: Handle all genpd cases together in _set_required_opps()

[Viresh Kumar]

There is no real need of keeping separate code for single genpd case, it
can be made to work with a simple change.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e87567dbe99f..6d7016ce8c53 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -959,7 +959,8 @@ static int _set_required_opps(struct device *dev,
 			      struct dev_pm_opp *opp, bool up)
 {
 	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
-	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
+	struct device **genpd_virt_devs =
+		opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
 	int i, ret = 0;
 
 	if (!required_opp_tables)
@@ -979,12 +980,6 @@ static int _set_required_opps(struct device *dev,
 		return -ENOENT;
 	}
 
-	/* Single genpd case */
-	if (!genpd_virt_devs)
-		return _set_required_opp(dev, dev, opp, 0);
-
-	/* Multiple genpd case */
-
 	/*
 	 * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
 	 * after it is freed from another thread.
-- 
2.31.1.272.g89b43f80a514

[PATCH 2/3] OPP: Move required opps configuration to specialized callback

[Viresh Kumar]

The required-opps configuration is closely tied to genpd and performance
states at the moment and it is not very obvious that required-opps can
live without genpds. Though we don't support configuring required-opps
for non-genpd cases currently.

This commit aims at separating these parts, where configuring genpds
would be a special case of configuring the required-opps.

Add a specialized callback, set_required_opps(), to the opp table and
set it to different callbacks accordingly.

This shouldn't result in any functional changes for now.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6d7016ce8c53..954c94865cf5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -935,8 +935,8 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 	return 0;
 }
 
-static int _set_required_opp(struct device *dev, struct device *pd_dev,
-			     struct dev_pm_opp *opp, int i)
+static int _set_performance_state(struct device *dev, struct device *pd_dev,
+				  struct dev_pm_opp *opp, int i)
 {
 	unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
 	int ret;
@@ -953,33 +953,20 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev,
 	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)
+static int _opp_set_required_opps_generic(struct device *dev,
+	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
+{
+	dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n");
+	return -ENOENT;
+}
+
+static int _opp_set_required_opps_genpd(struct device *dev,
+	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
 {
-	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
 	struct device **genpd_virt_devs =
 		opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
 	int i, ret = 0;
 
-	if (!required_opp_tables)
-		return 0;
-
-	/* required-opps not fully initialized yet */
-	if (lazy_linking_pending(opp_table))
-		return -EBUSY;
-
-	/*
-	 * We only support genpd's OPPs in the "required-opps" for now, as we
-	 * don't know much about other use cases. Error out if the required OPP
-	 * doesn't belong to a genpd.
-	 */
-	if (unlikely(!required_opp_tables[0]->is_genpd)) {
-		dev_err(dev, "required-opps don't belong to a genpd\n");
-		return -ENOENT;
-	}
-
 	/*
 	 * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
 	 * after it is freed from another thread.
@@ -987,15 +974,15 @@ static int _set_required_opps(struct device *dev,
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
 	/* Scaling up? Set required OPPs in normal order, else reverse */
-	if (up) {
+	if (!scaling_down) {
 		for (i = 0; i < opp_table->required_opp_count; i++) {
-			ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i);
+			ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
 			if (ret)
 				break;
 		}
 	} else {
 		for (i = opp_table->required_opp_count - 1; i >= 0; i--) {
-			ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i);
+			ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
 			if (ret)
 				break;
 		}
@@ -1006,6 +993,34 @@ static int _set_required_opps(struct device *dev,
 	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)
+{
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(opp_table))
+		return -EBUSY;
+
+	if (opp_table->set_required_opps)
+		return opp_table->set_required_opps(dev, opp_table, opp, up);
+
+	return 0;
+}
+
+/* Update set_required_opps handler */
+void _update_set_required_opps(struct opp_table *opp_table)
+{
+	/* Already set */
+	if (opp_table->set_required_opps)
+		return;
+
+	/* All required OPPs will belong to genpd or none */
+	if (opp_table->required_opp_tables[0]->is_genpd)
+		opp_table->set_required_opps = _opp_set_required_opps_genpd;
+	else
+		opp_table->set_required_opps = _opp_set_required_opps_generic;
+}
+
 static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e55c6095adf0..93da3c797afc 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -196,6 +196,8 @@ 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);
+	else
+		_update_set_required_opps(opp_table);
 
 	goto put_np;
 
@@ -411,6 +413,7 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
 
 		/* All required opp-tables found, remove from lazy list */
 		if (!lazy) {
+			_update_set_required_opps(opp_table);
 			list_del_init(&opp_table->lazy);
 
 			list_for_each_entry(opp, &opp_table->opp_list, node)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 3a6e077df386..2a057c42ddf4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -184,6 +184,7 @@ enum opp_table_access {
  * @enabled: Set to true if the device's resources are enabled/configured.
  * @genpd_performance_state: Device's power domain support performance state.
  * @is_genpd: Marks if the OPP table belongs to a genpd.
+ * @set_required_opps: Helper responsible to set required OPPs.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -234,6 +235,8 @@ struct opp_table {
 	bool enabled;
 	bool genpd_performance_state;
 	bool is_genpd;
+	int (*set_required_opps)(struct device *dev,
+		struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
@@ -257,6 +260,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp
 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 _required_opps_available(struct dev_pm_opp *opp, int count);
+void _update_set_required_opps(struct opp_table *opp_table);
 
 static inline bool lazy_linking_pending(struct opp_table *opp_table)
 {
-- 
2.31.1.272.g89b43f80a514

[PATCH 3/3] OPP: Allow platforms to add a set_required_opps() callback

[Viresh Kumar]

We currently configure required-opps only for genpds, as we aren't
really sure what's required of the other use cases. This patch allows
platforms to add their own set_required_opps() helper.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
Jun,

I think this can replace the cpufreq notification from the devfreq driver
(get_target_freq_with_cpufreq()), which is used currently only by
mtk-cci-devfreq.c. Instead of the notification, the aggregation of the requests
for the devfreq device can be done within this callback.

What do you say ?
---
 drivers/opp/core.c     | 35 +++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h      |  4 ++--
 include/linux/pm_opp.h |  5 +++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 954c94865cf5..22985ad7af79 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2421,8 +2421,35 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
 }
 
+/**
+ * _opp_set_required_opps_helper() - Register custom set required opps helper.
+ * @dev: Device for which the helper is getting registered.
+ * @set_required_opps: Custom set required opps helper.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ */
+static void _opp_set_required_opps_helper(struct opp_table *opp_table,
+					  set_required_opps_t set_required_opps)
+{
+	opp_table->set_required_opps = set_required_opps;
+}
+
+/**
+ * _opp_put_required_opps_helper() - Releases resources blocked for
+ *					 required opps helper.
+ * @opp_table: OPP table returned from _opp_set_required_opps_helper().
+ *
+ * Release resources blocked for platform specific required opps helper.
+ */
+static void _opp_put_required_opps_helper(struct opp_table *opp_table)
+{
+	opp_table->set_required_opps = NULL;
+}
+
 static void _opp_clear_config(struct opp_config_data *data)
 {
+	if (data->flags & OPP_CONFIG_REQUIRED_OPPS)
+		_opp_put_required_opps_helper(data->opp_table);
 	if (data->flags & OPP_CONFIG_GENPD)
 		_opp_detach_genpd(data->opp_table);
 	if (data->flags & OPP_CONFIG_REGULATOR)
@@ -2546,6 +2573,14 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 		data->flags |= OPP_CONFIG_GENPD;
 	}
 
+	/* Required opps helper */
+	if (config->set_required_opps) {
+		_opp_set_required_opps_helper(opp_table,
+					      config->set_required_opps);
+
+		data->flags |= OPP_CONFIG_REQUIRED_OPPS;
+	}
+
 	ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX),
 		       GFP_KERNEL);
 	if (ret)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 2a057c42ddf4..54cfeb894f5d 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -35,6 +35,7 @@ extern struct list_head opp_tables, lazy_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_OPPS	BIT(6)
 
 /**
  * struct opp_config_data - data for set config operations
@@ -235,8 +236,7 @@ struct opp_table {
 	bool enabled;
 	bool genpd_performance_state;
 	bool is_genpd;
-	int (*set_required_opps)(struct device *dev,
-		struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
+	set_required_opps_t set_required_opps;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dc1fb5890792..5a8c0cc50c96 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -64,6 +64,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 int (*set_required_opps_t)(struct device *dev, struct opp_table *opp_table,
+			struct dev_pm_opp *opp, bool scaling_down);
+
 /**
  * struct dev_pm_opp_config - Device OPP configuration values
  * @clk_names: Clk names, NULL terminated array.
@@ -76,6 +79,7 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
  * @genpd_names: Null terminated array of pointers containing names of genpd to
  *		 attach.
  * @virt_devs: Pointer to return the array of virtual devices.
+ * @set_required_opps: Custom set required opps helper.
  *
  * This structure contains platform specific OPP configurations for the device.
  */
@@ -90,6 +94,7 @@ struct dev_pm_opp_config {
 	const char * const *regulator_names;
 	const char * const *genpd_names;
 	struct device ***virt_devs;
+	set_required_opps_t set_required_opps;
 };
 
 #if defined(CONFIG_PM_OPP)
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Jun Nie]

Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月22日周三 19:06写道：
>
> Hello,
>
> The required-opps configuration is closely tied to genpd and performance
> states at the moment and it is not very obvious that required-opps can
> live without genpds. Though we don't support configuring required-opps
> for non-genpd cases currently.
>
> This patchset aims at cleaning up this a bit, just like what's done for clk and
> regulators. This also makes it possible for platforms to provide their own
> version of set_required_opps() helper, which can be used to configure the
> devfreq device propertly.
>
> Jun,
>
> I haven't found time to test this through yet, though there isn't much anyway I
> guess. Can you see if these can solve your problem properly ?

Hi Viresh,

It looks promising. The function get_target_freq_with_cpufreq() can be wrapped
to act as set_required_opps() callback. But my case is a bit
complicated. CPU opp
depends on both genpd opp and devfreq opp. So the genpd_virt_devs array need
to be modified or add another array for devfreq case. While genpd_virt_devs is
bounded with genpd directly and coupled with "power-domains" list in
device tree.
Current required-opp nodes are designed to be aligned with the list. I
am considering
what's the best way for back compatibility.

Hi Chanwoo,

Do you have any comments on this proposal? This proposal arise because opp
lib reports error when cpufreq driver try to set required opp for
non-genpd case.
Another possible fix is to ignore non-genpd opp in opp lib. But a
unified and recursive
opp management looks nicer, just like clock tree management.

>
> Viresh Kumar (3):
>   OPP: Handle all genpd cases together in _set_required_opps()
>   OPP: Move required opps configuration to specialized callback
>   OPP: Allow platforms to add a set_required_opps() callback
>
>  drivers/opp/core.c     | 113 ++++++++++++++++++++++++++++-------------
>  drivers/opp/of.c       |   3 ++
>  drivers/opp/opp.h      |   4 ++
>  include/linux/pm_opp.h |   5 ++
>  4 files changed, 91 insertions(+), 34 deletions(-)
>
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Viresh Kumar]

On 23-02-23, 17:56, Jun Nie wrote:
> It looks promising. The function get_target_freq_with_cpufreq() can be wrapped
> to act as set_required_opps() callback.

> But my case is a bit complicated. CPU opp depends on both genpd opp and
> devfreq opp.

I was wondering if we will have such a case soon enough or not :)
 
> So the genpd_virt_devs array need
> to be modified or add another array for devfreq case. While genpd_virt_devs is
> bounded with genpd directly and coupled with "power-domains" list in
> device tree.
> Current required-opp nodes are designed to be aligned with the list. I
> am considering
> what's the best way for back compatibility.

Please look at the top commit here:

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

Will this be enough for your use case ? I will post everything again once we are
settled on a solution.

-- 
viresh

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Jun Nie]

On Fri, Feb 24, 2023 at 07:47:13AM +0530, Viresh Kumar wrote:
> On 23-02-23, 17:56, Jun Nie wrote:
> > It looks promising. The function get_target_freq_with_cpufreq() can be wrapped
> > to act as set_required_opps() callback.
> 
> > But my case is a bit complicated. CPU opp depends on both genpd opp and
> > devfreq opp.
> 
> I was wondering if we will have such a case soon enough or not :)
>  
> > So the genpd_virt_devs array need
> > to be modified or add another array for devfreq case. While genpd_virt_devs is
> > bounded with genpd directly and coupled with "power-domains" list in
> > device tree.
> > Current required-opp nodes are designed to be aligned with the list. I
> > am considering
> > what's the best way for back compatibility.
> 
> Please look at the top commit here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/propagate
> 
> Will this be enough for your use case ? I will post everything again once we are
> settled on a solution.

For the opp lib, this is right direction. We still need to find a method to
pass devfreq device node to opp lib, just genpd_virt_devs for power domain.
But I am not clear below is the right way yet and this also involves wider
changes. Or the opp's owner, devfreq device can be referred via opp lib?
If so, we do not need to add devfreq-devs to cpu node at all.

		cpu1: cpu@101 {
			compatible = "arm,cortex-a53";
			device_type = "cpu";
			power-domains = <&cpr>;
			power-domain-names = "cpr";
			devfreq-devs = <&cci>;
			devfreq-names = "cci";
			operating-points-v2 = <&cluster1_opp_table>;
		};

		opp-200000000 {
			opp-hz = /bits/ 64 <200000000>;
			required-opps = <&cpr_opp3>, <&cci_opp1>;
		};

> 
> -- 
> viresh

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Viresh Kumar]

On 24-02-23, 16:17, Jun Nie wrote:
> For the opp lib, this is right direction. We still need to find a method to
> pass devfreq device node to opp lib, just genpd_virt_devs for power domain.

I am not really sure I understood it all. What is "device node" ? DT node or
struct device ? What exactly do you need ?

> But I am not clear below is the right way yet and this also involves wider
> changes. Or the opp's owner, devfreq device can be referred via opp lib?
> If so, we do not need to add devfreq-devs to cpu node at all.
> 
> 		cpu1: cpu@101 {
> 			compatible = "arm,cortex-a53";
> 			device_type = "cpu";
> 			power-domains = <&cpr>;
> 			power-domain-names = "cpr";
> 			devfreq-devs = <&cci>;
> 			devfreq-names = "cci";

Why do you need these ?

> 			operating-points-v2 = <&cluster1_opp_table>;
> 		};
> 
> 		opp-200000000 {
> 			opp-hz = /bits/ 64 <200000000>;
> 			required-opps = <&cpr_opp3>, <&cci_opp1>;

This looks fine though.

> 		};

-- 
viresh

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Jun Nie]

Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 12:23写道：
>
> On 24-02-23, 16:17, Jun Nie wrote:
> > For the opp lib, this is right direction. We still need to find a method to
> > pass devfreq device node to opp lib, just genpd_virt_devs for power domain.
>
> I am not really sure I understood it all. What is "device node" ? DT node or
> struct device ? What exactly do you need ?

Sorry for not expressing it accurately. I should say devfreq devices
pointers, just
devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs
dts nodes below.

>
> > But I am not clear below is the right way yet and this also involves wider
> > changes. Or the opp's owner, devfreq device can be referred via opp lib?
> > If so, we do not need to add devfreq-devs to cpu node at all.
> >
> >               cpu1: cpu@101 {
> >                       compatible = "arm,cortex-a53";
> >                       device_type = "cpu";
> >                       power-domains = <&cpr>;
> >                       power-domain-names = "cpr";
> >                       devfreq-devs = <&cci>;
> >                       devfreq-names = "cci";
>
> Why do you need these ?
>
> >                       operating-points-v2 = <&cluster1_opp_table>;
> >               };
> >
> >               opp-200000000 {
> >                       opp-hz = /bits/ 64 <200000000>;
> >                       required-opps = <&cpr_opp3>, <&cci_opp1>;
>
> This looks fine though.
>
> >               };
>
> --
> viresh

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Viresh Kumar]

On 27-02-23, 17:21, Jun Nie wrote:
> Sorry for not expressing it accurately. I should say devfreq devices
> pointers, just
> devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs
> dts nodes below.

Won't something like dev_pm_opp_set_clkname() would be enough here too ? We
already do this kind of work for clks and regulators.

Having power domain specific information within CPU nodes isn't a requirement of
the OPP core, but the general requirement of genpd core instead.

-- 
viresh

Re: [PATCH 1/3] OPP: Handle all genpd cases together in _set_required_opps()

[Ulf Hansson]

On Wed, 22 Feb 2023 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> There is no real need of keeping separate code for single genpd case, it
> can be made to work with a simple change.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/opp/core.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e87567dbe99f..6d7016ce8c53 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -959,7 +959,8 @@ static int _set_required_opps(struct device *dev,
>                               struct dev_pm_opp *opp, bool up)
>  {
>         struct opp_table **required_opp_tables = opp_table->required_opp_tables;
> -       struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
> +       struct device **genpd_virt_devs =
> +               opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
>         int i, ret = 0;
>
>         if (!required_opp_tables)
> @@ -979,12 +980,6 @@ static int _set_required_opps(struct device *dev,
>                 return -ENOENT;
>         }
>
> -       /* Single genpd case */
> -       if (!genpd_virt_devs)
> -               return _set_required_opp(dev, dev, opp, 0);
> -
> -       /* Multiple genpd case */
> -
>         /*
>          * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
>          * after it is freed from another thread.
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH 2/3] OPP: Move required opps configuration to specialized callback

[Ulf Hansson]

On Wed, 22 Feb 2023 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The required-opps configuration is closely tied to genpd and performance
> states at the moment and it is not very obvious that required-opps can
> live without genpds. Though we don't support configuring required-opps
> for non-genpd cases currently.
>
> This commit aims at separating these parts, where configuring genpds
> would be a special case of configuring the required-opps.
>
> Add a specialized callback, set_required_opps(), to the opp table and
> set it to different callbacks accordingly.
>
> This shouldn't result in any functional changes for now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This looks reasonable to me, but I guess it also depends on whether
you will land patch3 or not?

Nevertheless, feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/opp/core.c | 69 ++++++++++++++++++++++++++++------------------
>  drivers/opp/of.c   |  3 ++
>  drivers/opp/opp.h  |  4 +++
>  3 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6d7016ce8c53..954c94865cf5 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -935,8 +935,8 @@ static int _set_opp_bw(const struct opp_table *opp_table,
>         return 0;
>  }
>
> -static int _set_required_opp(struct device *dev, struct device *pd_dev,
> -                            struct dev_pm_opp *opp, int i)
> +static int _set_performance_state(struct device *dev, struct device *pd_dev,
> +                                 struct dev_pm_opp *opp, int i)
>  {
>         unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
>         int ret;
> @@ -953,33 +953,20 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev,
>         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)
> +static int _opp_set_required_opps_generic(struct device *dev,
> +       struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
> +{
> +       dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n");
> +       return -ENOENT;
> +}
> +
> +static int _opp_set_required_opps_genpd(struct device *dev,
> +       struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
>  {
> -       struct opp_table **required_opp_tables = opp_table->required_opp_tables;
>         struct device **genpd_virt_devs =
>                 opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
>         int i, ret = 0;
>
> -       if (!required_opp_tables)
> -               return 0;
> -
> -       /* required-opps not fully initialized yet */
> -       if (lazy_linking_pending(opp_table))
> -               return -EBUSY;
> -
> -       /*
> -        * We only support genpd's OPPs in the "required-opps" for now, as we
> -        * don't know much about other use cases. Error out if the required OPP
> -        * doesn't belong to a genpd.
> -        */
> -       if (unlikely(!required_opp_tables[0]->is_genpd)) {
> -               dev_err(dev, "required-opps don't belong to a genpd\n");
> -               return -ENOENT;
> -       }
> -
>         /*
>          * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
>          * after it is freed from another thread.
> @@ -987,15 +974,15 @@ static int _set_required_opps(struct device *dev,
>         mutex_lock(&opp_table->genpd_virt_dev_lock);
>
>         /* Scaling up? Set required OPPs in normal order, else reverse */
> -       if (up) {
> +       if (!scaling_down) {
>                 for (i = 0; i < opp_table->required_opp_count; i++) {
> -                       ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i);
> +                       ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
>                         if (ret)
>                                 break;
>                 }
>         } else {
>                 for (i = opp_table->required_opp_count - 1; i >= 0; i--) {
> -                       ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i);
> +                       ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
>                         if (ret)
>                                 break;
>                 }
> @@ -1006,6 +993,34 @@ static int _set_required_opps(struct device *dev,
>         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)
> +{
> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(opp_table))
> +               return -EBUSY;
> +
> +       if (opp_table->set_required_opps)
> +               return opp_table->set_required_opps(dev, opp_table, opp, up);
> +
> +       return 0;
> +}
> +
> +/* Update set_required_opps handler */
> +void _update_set_required_opps(struct opp_table *opp_table)
> +{
> +       /* Already set */
> +       if (opp_table->set_required_opps)
> +               return;
> +
> +       /* All required OPPs will belong to genpd or none */
> +       if (opp_table->required_opp_tables[0]->is_genpd)
> +               opp_table->set_required_opps = _opp_set_required_opps_genpd;
> +       else
> +               opp_table->set_required_opps = _opp_set_required_opps_generic;
> +}
> +
>  static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  {
>         struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index e55c6095adf0..93da3c797afc 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -196,6 +196,8 @@ 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);
> +       else
> +               _update_set_required_opps(opp_table);
>
>         goto put_np;
>
> @@ -411,6 +413,7 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
>
>                 /* All required opp-tables found, remove from lazy list */
>                 if (!lazy) {
> +                       _update_set_required_opps(opp_table);
>                         list_del_init(&opp_table->lazy);
>
>                         list_for_each_entry(opp, &opp_table->opp_list, node)
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 3a6e077df386..2a057c42ddf4 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -184,6 +184,7 @@ enum opp_table_access {
>   * @enabled: Set to true if the device's resources are enabled/configured.
>   * @genpd_performance_state: Device's power domain support performance state.
>   * @is_genpd: Marks if the OPP table belongs to a genpd.
> + * @set_required_opps: Helper responsible to set required OPPs.
>   * @dentry:    debugfs dentry pointer of the real device directory (not links).
>   * @dentry_name: Name of the real dentry.
>   *
> @@ -234,6 +235,8 @@ struct opp_table {
>         bool enabled;
>         bool genpd_performance_state;
>         bool is_genpd;
> +       int (*set_required_opps)(struct device *dev,
> +               struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
>
>  #ifdef CONFIG_DEBUG_FS
>         struct dentry *dentry;
> @@ -257,6 +260,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp
>  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 _required_opps_available(struct dev_pm_opp *opp, int count);
> +void _update_set_required_opps(struct opp_table *opp_table);
>
>  static inline bool lazy_linking_pending(struct opp_table *opp_table)
>  {
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Jun Nie]

Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 17:29写道：
>
> On 27-02-23, 17:21, Jun Nie wrote:
> > Sorry for not expressing it accurately. I should say devfreq devices
> > pointers, just
> > devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs
> > dts nodes below.
>
> Won't something like dev_pm_opp_set_clkname() would be enough here too ? We
> already do this kind of work for clks and regulators.

Thanks! It is a possible solution. I will try to spare time on this as
higher priority tasks
are on my list.

>
> Having power domain specific information within CPU nodes isn't a requirement of
> the OPP core, but the general requirement of genpd core instead.
>
> --
> viresh

Re: [PATCH 2/3] OPP: Move required opps configuration to specialized callback

[Viresh Kumar]

On 01-03-23, 14:06, Ulf Hansson wrote:
> On Wed, 22 Feb 2023 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > The required-opps configuration is closely tied to genpd and performance
> > states at the moment and it is not very obvious that required-opps can
> > live without genpds. Though we don't support configuring required-opps
> > for non-genpd cases currently.
> >
> > This commit aims at separating these parts, where configuring genpds
> > would be a special case of configuring the required-opps.
> >
> > Add a specialized callback, set_required_opps(), to the opp table and
> > set it to different callbacks accordingly.
> >
> > This shouldn't result in any functional changes for now.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> This looks reasonable to me, but I guess it also depends on whether
> you will land patch3 or not?

Not necessarily, its better to have a clear error path to avoid
unnecessary confusion.

I have applied first two patches now. Thanks.

-- 
viresh

Re: [PATCH 0/3] OPP: Simplify set_required_opp handling

[Viresh Kumar]

On 06-03-23, 18:48, Jun Nie wrote:
> Viresh Kumar <viresh.kumar@linaro.org> 于2023年2月27日周一 17:29写道：
> >
> > On 27-02-23, 17:21, Jun Nie wrote:
> > > Sorry for not expressing it accurately. I should say devfreq devices
> > > pointers, just
> > > devfreq_virt_devs vs genpd_virt_devs. Then you know why I add devfreq-devs
> > > dts nodes below.
> >
> > Won't something like dev_pm_opp_set_clkname() would be enough here too ? We
> > already do this kind of work for clks and regulators.
> 
> Thanks! It is a possible solution. I will try to spare time on this as
> higher priority tasks
> are on my list.

I have applied first two patches. I would like to apply the third one
with some user code. I will wait for your code to merge that.

-- 
viresh