[PATCH 00/16] arm_scmi/opp/dvfs: Add generic performance scaling support

[PATCH 00/16] arm_scmi/opp/dvfs: Add generic performance scaling support

[Ulf Hansson]

The current SCMI performance scaling support is limited to cpufreq. This series
extends the support, so it can be used for all kind of devices and not only for
CPUs.

The changes are spread over a couple of different subsystems, although the
changes that affects the other subsystems than the arm_scmi directory are
mostly smaller. The series is based upon v6.4-rc5. That said, let's figure out
on how to best move forward with this. I am of course happy to help in any way.

Note that, so far this is only be tested on the Qemu virt platform with Optee
running an SCMI server. If you want some more details about my test setup, I am
certainly open to share that with you!

Looking forward to get your feedback!

Kind regards
Ulf Hansson


Ulf Hansson (16):
  firmware: arm_scmi: Extend perf protocol ops to get number of domains
  firmware: arm_scmi: Extend perf protocol ops to get the name of a
    domain
  firmware: arm_scmi: Extend perf protocol ops to inform of set level
    support
  cpufreq: scmi: Prepare to move OF parsing of domain-id to cpufreq
  firmware: arm_scmi: Align perf ops to use domain-id as in-parameter
  firmware: arm_scmi: Drop redundant ->device_domain_id() from perf ops
  cpufreq: scmi: Avoid one OF parsing in scmi_get_sharing_cpus()
  PM: domains: Allow genpd providers to manage OPP tables directly by
    its FW
  dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13
  firmware: arm_scmi: Add the SCMI performance domain
  OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility
  OPP: Extend dev_pm_opp_data with performance level
  OPP: Extend dev_pm_opp_data with OPP provider support
  firmware: arm_scmi: Simplify error path in scmi_dvfs_device_opps_add()
  firmware: arm_scmi: Extend perf support with OPP from genpd providers
  firmware: arm_scmi: Add generic OPP support to the SCMI performance
    domain

 .../bindings/firmware/arm,scmi.yaml           |   4 +-
 drivers/base/power/domain.c                   |  11 +-
 drivers/cpufreq/scmi-cpufreq.c                |  40 ++--
 drivers/firmware/arm_scmi/Kconfig             |  12 ++
 drivers/firmware/arm_scmi/Makefile            |   1 +
 drivers/firmware/arm_scmi/perf.c              |  81 ++++----
 drivers/firmware/arm_scmi/scmi_perf_domain.c  | 189 ++++++++++++++++++
 drivers/opp/core.c                            |  69 ++++++-
 drivers/opp/of.c                              |  11 +-
 drivers/opp/opp.h                             |   3 +-
 include/linux/pm_domain.h                     |   5 +
 include/linux/pm_opp.h                        |  27 +++
 include/linux/scmi_protocol.h                 |  14 +-
 13 files changed, 388 insertions(+), 79 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/scmi_perf_domain.c

-- 
2.34.1

[PATCH 01/16] firmware: arm_scmi: Extend perf protocol ops to get number of domains

[Ulf Hansson]

Similar to other protocol ops, it's useful for an scmi module driver to get
the number of supported performance domains, hence let's make this
available by adding a new perf protocol callback. Note that, a user is
being added from subsequent changes.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/perf.c | 8 ++++++++
 include/linux/scmi_protocol.h    | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ecf5c4de851b..cf7f0de4d6db 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -333,6 +333,13 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
 	return ret;
 }
 
+static int scmi_perf_num_domains_get(const struct scmi_protocol_handle *ph)
+{
+	struct scmi_perf_info *pi = ph->get_priv(ph);
+
+	return pi->num_domains;
+}
+
 static int scmi_perf_mb_limits_set(const struct scmi_protocol_handle *ph,
 				   u32 domain, u32 max_perf, u32 min_perf)
 {
@@ -687,6 +694,7 @@ scmi_power_scale_get(const struct scmi_protocol_handle *ph)
 }
 
 static const struct scmi_perf_proto_ops perf_proto_ops = {
+	.num_domains_get = scmi_perf_num_domains_get,
 	.limits_set = scmi_perf_limits_set,
 	.limits_get = scmi_perf_limits_get,
 	.level_set = scmi_perf_level_set,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0ce5746a4470..4e42274a68b7 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -101,6 +101,7 @@ struct scmi_clk_proto_ops {
  * struct scmi_perf_proto_ops - represents the various operations provided
  *	by SCMI Performance Protocol
  *
+ * @num_domains_get: gets the number of supported performance domains
  * @limits_set: sets limits on the performance level of a domain
  * @limits_get: gets limits on the performance level of a domain
  * @level_set: sets the performance level of a domain
@@ -120,6 +121,7 @@ struct scmi_clk_proto_ops {
  *	or in some other (abstract) scale
  */
 struct scmi_perf_proto_ops {
+	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
 	int (*limits_set)(const struct scmi_protocol_handle *ph, u32 domain,
 			  u32 max_perf, u32 min_perf);
 	int (*limits_get)(const struct scmi_protocol_handle *ph, u32 domain,
-- 
2.34.1

[PATCH 02/16] firmware: arm_scmi: Extend perf protocol ops to get the name of a domain

[Ulf Hansson]

Similar to other protocol ops, it's useful for an scmi module driver to get
the name of a performance domain, hence let's make this available by adding
a new perf protocol callback. Note that, a user is being added from
subsequent changes.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/perf.c | 10 ++++++++++
 include/linux/scmi_protocol.h    |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index cf7f0de4d6db..5a6ed42bfb55 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -340,6 +340,15 @@ static int scmi_perf_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
+static const char *
+scmi_perf_name_get(const struct scmi_protocol_handle *ph, u32 domain)
+{
+	struct scmi_perf_info *pi = ph->get_priv(ph);
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	return dom->name;
+}
+
 static int scmi_perf_mb_limits_set(const struct scmi_protocol_handle *ph,
 				   u32 domain, u32 max_perf, u32 min_perf)
 {
@@ -695,6 +704,7 @@ scmi_power_scale_get(const struct scmi_protocol_handle *ph)
 
 static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.num_domains_get = scmi_perf_num_domains_get,
+	.name_get = scmi_perf_name_get,
 	.limits_set = scmi_perf_limits_set,
 	.limits_get = scmi_perf_limits_get,
 	.level_set = scmi_perf_level_set,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 4e42274a68b7..07152a0baee3 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -102,6 +102,7 @@ struct scmi_clk_proto_ops {
  *	by SCMI Performance Protocol
  *
  * @num_domains_get: gets the number of supported performance domains
+ * @name_get: gets the name of a performance domain
  * @limits_set: sets limits on the performance level of a domain
  * @limits_get: gets limits on the performance level of a domain
  * @level_set: sets the performance level of a domain
@@ -122,6 +123,8 @@ struct scmi_clk_proto_ops {
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
+	const char *(*name_get)(const struct scmi_protocol_handle *ph,
+				u32 domain);
 	int (*limits_set)(const struct scmi_protocol_handle *ph, u32 domain,
 			  u32 max_perf, u32 min_perf);
 	int (*limits_get)(const struct scmi_protocol_handle *ph, u32 domain,
-- 
2.34.1

[PATCH 03/16] firmware: arm_scmi: Extend perf protocol ops to inform of set level support

[Ulf Hansson]

As a subsequent change show, it's useful for an scmi module driver to know
if a performance domain can support the set level operation, hence let's
make this available by adding a new perf protocol callback.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/perf.c | 10 ++++++++++
 include/linux/scmi_protocol.h    |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 5a6ed42bfb55..216bcd68d549 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -440,6 +440,15 @@ static int scmi_perf_limits_get(const struct scmi_protocol_handle *ph,
 	return scmi_perf_mb_limits_get(ph, domain, max_perf, min_perf);
 }
 
+static bool
+scmi_perf_can_level_set(const struct scmi_protocol_handle *ph, u32 domain)
+{
+	struct scmi_perf_info *pi = ph->get_priv(ph);
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	return dom->set_perf;
+}
+
 static int scmi_perf_mb_level_set(const struct scmi_protocol_handle *ph,
 				  u32 domain, u32 level, bool poll)
 {
@@ -707,6 +716,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.name_get = scmi_perf_name_get,
 	.limits_set = scmi_perf_limits_set,
 	.limits_get = scmi_perf_limits_get,
+	.can_level_set = scmi_perf_can_level_set,
 	.level_set = scmi_perf_level_set,
 	.level_get = scmi_perf_level_get,
 	.device_domain_id = scmi_dev_domain_id,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 07152a0baee3..99c3e985c40f 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -129,6 +129,7 @@ struct scmi_perf_proto_ops {
 			  u32 max_perf, u32 min_perf);
 	int (*limits_get)(const struct scmi_protocol_handle *ph, u32 domain,
 			  u32 *max_perf, u32 *min_perf);
+	bool (*can_level_set)(const struct scmi_protocol_handle *ph, u32 domain);
 	int (*level_set)(const struct scmi_protocol_handle *ph, u32 domain,
 			 u32 level, bool poll);
 	int (*level_get)(const struct scmi_protocol_handle *ph, u32 domain,
-- 
2.34.1

[PATCH 04/16] cpufreq: scmi: Prepare to move OF parsing of domain-id to cpufreq

[Ulf Hansson]

The OF parsing of the clock domain specifier seems to better belong in the
scmi cpufreq driver, rather than being implemented behind the generic
->device_domain_id() perf protocol ops.

To prepare to remove the ->device_domain_id() ops, let's implement the OF
parsing in the scmi cpufreq driver instead.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpufreq/scmi-cpufreq.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index f34e6382a4c5..7d05d48c0337 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -70,13 +70,24 @@ static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 	return 0;
 }
 
+static int scmi_cpu_domain_id(struct device *cpu_dev)
+{
+	struct of_phandle_args clkspec;
+
+	if (of_parse_phandle_with_args(cpu_dev->of_node, "clocks",
+				       "#clock-cells", 0, &clkspec))
+		return -EINVAL;
+
+	return clkspec.args[0];
+}
+
 static int
 scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 {
 	int cpu, domain, tdomain;
 	struct device *tcpu_dev;
 
-	domain = perf_ops->device_domain_id(cpu_dev);
+	domain = scmi_cpu_domain_id(cpu_dev);
 	if (domain < 0)
 		return domain;
 
@@ -88,7 +99,7 @@ scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 		if (!tcpu_dev)
 			continue;
 
-		tdomain = perf_ops->device_domain_id(tcpu_dev);
+		tdomain = scmi_cpu_domain_id(tcpu_dev);
 		if (tdomain == domain)
 			cpumask_set_cpu(cpu, cpumask);
 	}
@@ -104,7 +115,7 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
 	unsigned long Hz;
 	int ret, domain;
 
-	domain = perf_ops->device_domain_id(cpu_dev);
+	domain = scmi_cpu_domain_id(cpu_dev);
 	if (domain < 0)
 		return domain;
 
@@ -209,7 +220,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->cpu_dev = cpu_dev;
-	priv->domain_id = perf_ops->device_domain_id(cpu_dev);
+	priv->domain_id = scmi_cpu_domain_id(cpu_dev);
 
 	policy->driver_data = priv;
 	policy->freq_table = freq_table;
-- 
2.34.1

[PATCH 05/16] firmware: arm_scmi: Align perf ops to use domain-id as in-parameter

[Ulf Hansson]

Most scmi_perf_proto_ops are already using an "u32 domain" as an
in-parameter to indicate what performance domain we shall operate upon.
However, some of the ops are using a "struct device *dev", which means an
an additional OF parsing is needed each time the perf ops gets called, to
find the corresponding domain-id.

To avoid a potential unnecessary OF parsing, but also to make the code more
consistent, let's replace the in-parameter "struct device *dev" with an
"u32 domain". Note that, this requires us to make some corresponding
changes to the scmi cpufreq driver, so let's do that too.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpufreq/scmi-cpufreq.c   | 14 +++++++++-----
 drivers/firmware/arm_scmi/perf.c | 18 +++++-------------
 include/linux/scmi_protocol.h    |  6 +++---
 3 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 7d05d48c0337..125e8a8421fb 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -137,7 +137,7 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
 
 static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 {
-	int ret, nr_opp;
+	int ret, nr_opp, domain;
 	unsigned int latency;
 	struct device *cpu_dev;
 	struct scmi_data *priv;
@@ -149,6 +149,10 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 		return -ENODEV;
 	}
 
+	domain = scmi_cpu_domain_id(cpu_dev);
+	if (domain < 0)
+		return domain;
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -187,7 +191,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	  */
 	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
 	if (nr_opp <= 0) {
-		ret = perf_ops->device_opps_add(ph, cpu_dev);
+		ret = perf_ops->device_opps_add(ph, cpu_dev, domain);
 		if (ret) {
 			dev_warn(cpu_dev, "failed to add opps to the device\n");
 			goto out_free_cpumask;
@@ -220,7 +224,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->cpu_dev = cpu_dev;
-	priv->domain_id = scmi_cpu_domain_id(cpu_dev);
+	priv->domain_id = domain;
 
 	policy->driver_data = priv;
 	policy->freq_table = freq_table;
@@ -228,14 +232,14 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	/* SCMI allows DVFS request for any domain from any CPU */
 	policy->dvfs_possible_from_any_cpu = true;
 
-	latency = perf_ops->transition_latency_get(ph, cpu_dev);
+	latency = perf_ops->transition_latency_get(ph, domain);
 	if (!latency)
 		latency = CPUFREQ_ETERNAL;
 
 	policy->cpuinfo.transition_latency = latency;
 
 	policy->fast_switch_possible =
-		perf_ops->fast_switch_possible(ph, cpu_dev);
+		perf_ops->fast_switch_possible(ph, domain);
 
 	return 0;
 
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 216bcd68d549..563fa44b1a71 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -593,18 +593,14 @@ static int scmi_dev_domain_id(struct device *dev)
 }
 
 static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
-				     struct device *dev)
+				     struct device *dev, u32 domain)
 {
-	int idx, ret, domain;
+	int idx, ret;
 	unsigned long freq;
 	struct scmi_opp *opp;
 	struct perf_dom_info *dom;
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 
-	domain = scmi_dev_domain_id(dev);
-	if (domain < 0)
-		return domain;
-
 	dom = pi->dom_info + domain;
 
 	for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) {
@@ -626,14 +622,10 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_dvfs_transition_latency_get(const struct scmi_protocol_handle *ph,
-				 struct device *dev)
+				 u32 domain)
 {
 	struct perf_dom_info *dom;
 	struct scmi_perf_info *pi = ph->get_priv(ph);
-	int domain = scmi_dev_domain_id(dev);
-
-	if (domain < 0)
-		return domain;
 
 	dom = pi->dom_info + domain;
 	/* uS to nS */
@@ -693,12 +685,12 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph,
 }
 
 static bool scmi_fast_switch_possible(const struct scmi_protocol_handle *ph,
-				      struct device *dev)
+				      u32 domain)
 {
 	struct perf_dom_info *dom;
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 
-	dom = pi->dom_info + scmi_dev_domain_id(dev);
+	dom = pi->dom_info + domain;
 
 	return dom->fc_info && dom->fc_info[PERF_FC_LEVEL].set_addr;
 }
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 99c3e985c40f..34ecaeeb51bc 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -136,9 +136,9 @@ struct scmi_perf_proto_ops {
 			 u32 *level, bool poll);
 	int (*device_domain_id)(struct device *dev);
 	int (*transition_latency_get)(const struct scmi_protocol_handle *ph,
-				      struct device *dev);
+				      u32 domain);
 	int (*device_opps_add)(const struct scmi_protocol_handle *ph,
-			       struct device *dev);
+			       struct device *dev, u32 domain);
 	int (*freq_set)(const struct scmi_protocol_handle *ph, u32 domain,
 			unsigned long rate, bool poll);
 	int (*freq_get)(const struct scmi_protocol_handle *ph, u32 domain,
@@ -146,7 +146,7 @@ struct scmi_perf_proto_ops {
 	int (*est_power_get)(const struct scmi_protocol_handle *ph, u32 domain,
 			     unsigned long *rate, unsigned long *power);
 	bool (*fast_switch_possible)(const struct scmi_protocol_handle *ph,
-				     struct device *dev);
+				     u32 domain);
 	enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
 };
 
-- 
2.34.1

[PATCH 06/16] firmware: arm_scmi: Drop redundant ->device_domain_id() from perf ops

[Ulf Hansson]

There are no longer any users of the ->device_domain_id() ops in the
scmi_perf_proto_ops, therefore let's remove it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/perf.c | 13 -------------
 include/linux/scmi_protocol.h    |  2 --
 2 files changed, 15 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 563fa44b1a71..9c5340f590e4 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -580,18 +580,6 @@ static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
 	*p_fc = fc;
 }
 
-/* Device specific ops */
-static int scmi_dev_domain_id(struct device *dev)
-{
-	struct of_phandle_args clkspec;
-
-	if (of_parse_phandle_with_args(dev->of_node, "clocks", "#clock-cells",
-				       0, &clkspec))
-		return -EINVAL;
-
-	return clkspec.args[0];
-}
-
 static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
 				     struct device *dev, u32 domain)
 {
@@ -711,7 +699,6 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.can_level_set = scmi_perf_can_level_set,
 	.level_set = scmi_perf_level_set,
 	.level_get = scmi_perf_level_get,
-	.device_domain_id = scmi_dev_domain_id,
 	.transition_latency_get = scmi_dvfs_transition_latency_get,
 	.device_opps_add = scmi_dvfs_device_opps_add,
 	.freq_set = scmi_dvfs_freq_set,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 34ecaeeb51bc..21aea1b2a355 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -107,7 +107,6 @@ struct scmi_clk_proto_ops {
  * @limits_get: gets limits on the performance level of a domain
  * @level_set: sets the performance level of a domain
  * @level_get: gets the performance level of a domain
- * @device_domain_id: gets the scmi domain id for a given device
  * @transition_latency_get: gets the DVFS transition latency for a given device
  * @device_opps_add: adds all the OPPs for a given device
  * @freq_set: sets the frequency for a given device using sustained frequency
@@ -134,7 +133,6 @@ struct scmi_perf_proto_ops {
 			 u32 level, bool poll);
 	int (*level_get)(const struct scmi_protocol_handle *ph, u32 domain,
 			 u32 *level, bool poll);
-	int (*device_domain_id)(struct device *dev);
 	int (*transition_latency_get)(const struct scmi_protocol_handle *ph,
 				      u32 domain);
 	int (*device_opps_add)(const struct scmi_protocol_handle *ph,
-- 
2.34.1

[PATCH 07/16] cpufreq: scmi: Avoid one OF parsing in scmi_get_sharing_cpus()

[Ulf Hansson]

The domain-id for the cpu_dev has already been parsed at the point when
scmi_get_sharing_cpus() is getting called. Let's pass it as an in-parameter
to avoid the unnecessary OF parsing.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpufreq/scmi-cpufreq.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 125e8a8421fb..78f53e388094 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -82,15 +82,12 @@ static int scmi_cpu_domain_id(struct device *cpu_dev)
 }
 
 static int
-scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
+scmi_get_sharing_cpus(struct device *cpu_dev, int domain,
+		      struct cpumask *cpumask)
 {
-	int cpu, domain, tdomain;
+	int cpu, tdomain;
 	struct device *tcpu_dev;
 
-	domain = scmi_cpu_domain_id(cpu_dev);
-	if (domain < 0)
-		return domain;
-
 	for_each_possible_cpu(cpu) {
 		if (cpu == cpu_dev->id)
 			continue;
@@ -163,7 +160,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	/* Obtain CPUs that share SCMI performance controls */
-	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
+	ret = scmi_get_sharing_cpus(cpu_dev, domain, policy->cpus);
 	if (ret) {
 		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
 		goto out_free_cpumask;
-- 
2.34.1

[PATCH 08/16] PM: domains: Allow genpd providers to manage OPP tables directly by its FW

[Ulf Hansson]

In some cases the OPP tables aren't specified in device tree, but rather
encoded in the FW. To allow a genpd provider to specify them dynamically
instead, let's add a new genpd flag, GENPD_FLAG_OPP_TABLE_FW.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 11 ++++++-----
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 32084e38b73d..cb35c040216a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
 #define genpd_is_active_wakeup(genpd)	(genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
 #define genpd_is_cpu_domain(genpd)	(genpd->flags & GENPD_FLAG_CPU_DOMAIN)
 #define genpd_is_rpm_always_on(genpd)	(genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
+#define genpd_is_opp_table_fw(genpd)	(genpd->flags & GENPD_FLAG_OPP_TABLE_FW)
 
 static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
 		const struct generic_pm_domain *genpd)
@@ -2328,7 +2329,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 	genpd->dev.of_node = np;
 
 	/* Parse genpd OPP table */
-	if (genpd->set_performance_state) {
+	if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 		ret = dev_pm_opp_of_add_table(&genpd->dev);
 		if (ret)
 			return dev_err_probe(&genpd->dev, ret, "Failed to add OPP table\n");
@@ -2343,7 +2344,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
 	if (ret) {
-		if (genpd->set_performance_state) {
+		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
 		}
@@ -2387,7 +2388,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		genpd->dev.of_node = np;
 
 		/* Parse genpd OPP table */
-		if (genpd->set_performance_state) {
+		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
 			if (ret) {
 				dev_err_probe(&genpd->dev, ret,
@@ -2423,7 +2424,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		genpd->provider = NULL;
 		genpd->has_provider = false;
 
-		if (genpd->set_performance_state) {
+		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
 		}
@@ -2455,7 +2456,7 @@ void of_genpd_del_provider(struct device_node *np)
 				if (gpd->provider == &np->fwnode) {
 					gpd->has_provider = false;
 
-					if (!gpd->set_performance_state)
+					if (genpd_is_opp_table_fw(gpd) || !gpd->set_performance_state)
 						continue;
 
 					dev_pm_opp_put_opp_table(gpd->opp_table);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index f776fb93eaa0..05ad8cefdff1 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -61,6 +61,10 @@
  * GENPD_FLAG_MIN_RESIDENCY:	Enable the genpd governor to consider its
  *				components' next wakeup when determining the
  *				optimal idle state.
+ *
+ * GENPD_FLAG_OPP_TABLE_FW:	The genpd provider supports performance states,
+ *				but its corresponding OPP tables are not
+ *				described in DT, but are given directly by FW.
  */
 #define GENPD_FLAG_PM_CLK	 (1U << 0)
 #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
@@ -69,6 +73,7 @@
 #define GENPD_FLAG_CPU_DOMAIN	 (1U << 4)
 #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
 #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
+#define GENPD_FLAG_OPP_TABLE_FW	 (1U << 7)
 
 enum gpd_status {
 	GENPD_STATE_ON = 0,	/* PM domain is on */
-- 
2.34.1

[PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

The protocol@13 node is describing the performance scaling option for the
ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
performance scaling is in many cases not limited to switching a clock's
frequency.

Therefore, let's extend the binding so the interface can be modelled as a
generic "performance domain" too. The common way to describe this, is to
use the "power-domain" bindings, so let's use that.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..cff9d1e4cea1 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -145,8 +145,8 @@ properties:
       '#clock-cells':
         const: 1
 
-    required:
-      - '#clock-cells'
+      '#power-domain-cells':
+        const: 1
 
   protocol@14:
     $ref: '#/$defs/protocol-node'
-- 
2.34.1

[PATCH 10/16] firmware: arm_scmi: Add the SCMI performance domain

[Ulf Hansson]

To enable support for performance scaling (DVFS) for generic devices with
the SCMI performance protocol, let's add an SCMI performance domain. This
is being modelled as a genpd provider, with support for performance scaling
through genpd's ->set_performance_state() callback.

Note that, this adds the initial support that allows consumer drivers for
attached devices, to vote for a new performance state via calling the
dev_pm_genpd_set_performance_state(). However, this should be avoided as
it's in most cases preferred to use the OPP library to vote for a new OPP
instead. The support using the OPP library is implemented from subsequent
changes.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/Kconfig            |  12 ++
 drivers/firmware/arm_scmi/Makefile           |   1 +
 drivers/firmware/arm_scmi/scmi_perf_domain.c | 155 +++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/scmi_perf_domain.c

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index ea0f5083ac47..706d1264d038 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -181,6 +181,18 @@ config ARM_SCMI_POWER_DOMAIN
 	  will be called scmi_pm_domain. Note this may needed early in boot
 	  before rootfs may be available.
 
+config ARM_SCMI_PERF_DOMAIN
+	tristate "SCMI performance domain driver"
+	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
+	default y
+	select PM_GENERIC_DOMAINS if PM
+	help
+	  This enables support for the SCMI performance domains which can be
+	  enabled or disabled via the SCP firmware.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called scmi_perf_domain.
+
 config ARM_SCMI_POWER_CONTROL
 	tristate "SCMI system power control driver"
 	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index b31d78fa66cc..afee66a65dcb 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
 
 obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
+obj-$(CONFIG_ARM_SCMI_PERF_DOMAIN) += scmi_perf_domain.o
 obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
 
 ifeq ($(CONFIG_THUMB2_KERNEL)$(CONFIG_CC_IS_CLANG),yy)
diff --git a/drivers/firmware/arm_scmi/scmi_perf_domain.c b/drivers/firmware/arm_scmi/scmi_perf_domain.c
new file mode 100644
index 000000000000..9be90a7d94de
--- /dev/null
+++ b/drivers/firmware/arm_scmi/scmi_perf_domain.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SCMI performance domain support.
+ *
+ * Copyright (C) 2023 Linaro Ltd.
+ */
+
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pm_domain.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+struct scmi_perf_domain {
+	struct generic_pm_domain genpd;
+	const struct scmi_perf_proto_ops *perf_ops;
+	const struct scmi_protocol_handle *ph;
+	u32 domain_id;
+	bool can_level_set;
+};
+
+#define to_scmi_pd(pd) container_of(pd, struct scmi_perf_domain, genpd)
+
+static int
+scmi_pd_set_perf_state(struct generic_pm_domain *genpd, unsigned int state)
+{
+	struct scmi_perf_domain *pd = to_scmi_pd(genpd);
+	int ret;
+
+	if (!pd->can_level_set)
+		return 0;
+
+	ret = pd->perf_ops->level_set(pd->ph, pd->domain_id, state, true);
+	if (ret)
+		dev_warn(&genpd->dev, "Failed with %d when trying to set %d perf level",
+			 ret, state);
+
+	return ret;
+}
+
+static int scmi_perf_domain_probe(struct scmi_device *sdev)
+{
+	struct device *dev = &sdev->dev;
+	const struct scmi_handle *handle = sdev->handle;
+	const struct scmi_perf_proto_ops *perf_ops;
+	struct scmi_protocol_handle *ph;
+	struct scmi_perf_domain *scmi_pd;
+	struct genpd_onecell_data *scmi_pd_data;
+	struct generic_pm_domain **domains;
+	int num_domains, i, ret = 0;
+	u32 perf_level;
+
+	if (!handle)
+		return -ENODEV;
+
+	/* The OF node must specify us as a power-domain provider. */
+	if (!of_find_property(dev->of_node, "#power-domain-cells", NULL))
+		return 0;
+
+	perf_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PERF, &ph);
+	if (IS_ERR(perf_ops))
+		return PTR_ERR(perf_ops);
+
+	num_domains = perf_ops->num_domains_get(ph);
+	if (num_domains < 0) {
+		dev_warn(dev, "Failed with %d when getting num perf domains\n",
+			 num_domains);
+		return num_domains;
+	} else if (!num_domains) {
+		return 0;
+	}
+
+	scmi_pd = devm_kcalloc(dev, num_domains, sizeof(*scmi_pd), GFP_KERNEL);
+	if (!scmi_pd)
+		return -ENOMEM;
+
+	scmi_pd_data = devm_kzalloc(dev, sizeof(*scmi_pd_data), GFP_KERNEL);
+	if (!scmi_pd_data)
+		return -ENOMEM;
+
+	domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	for (i = 0; i < num_domains; i++, scmi_pd++) {
+		scmi_pd->domain_id = i;
+		scmi_pd->perf_ops = perf_ops;
+		scmi_pd->ph = ph;
+		scmi_pd->can_level_set = perf_ops->can_level_set(ph, i);
+
+		scmi_pd->genpd.name = perf_ops->name_get(ph, i);
+		scmi_pd->genpd.flags = GENPD_FLAG_OPP_TABLE_FW;
+		scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state;
+
+		ret = perf_ops->level_get(ph, i, &perf_level, false);
+		if (ret) {
+			dev_dbg(dev, "Failed to get perf level for %s",
+				 scmi_pd->genpd.name);
+			perf_level = 0;
+		}
+
+		/* Let the perf level indicate the power-state too. */
+		ret = pm_genpd_init(&scmi_pd->genpd, NULL, perf_level == 0);
+		if (ret)
+			goto err;
+
+		domains[i] = &scmi_pd->genpd;
+	}
+
+	scmi_pd_data->domains = domains;
+	scmi_pd_data->num_domains = num_domains;
+
+	ret = of_genpd_add_provider_onecell(dev->of_node, scmi_pd_data);
+	if (ret)
+		goto err;
+
+	dev_set_drvdata(dev, scmi_pd_data);
+	dev_info(dev, "Initialized %d performance domains", num_domains);
+	return 0;
+err:
+	for (i--; i >= 0; i--)
+		pm_genpd_remove(domains[i]);
+	return ret;
+}
+
+static void scmi_perf_domain_remove(struct scmi_device *sdev)
+{
+	struct device *dev = &sdev->dev;
+	struct genpd_onecell_data *scmi_pd_data = dev_get_drvdata(dev);
+	int i;
+
+	of_genpd_del_provider(dev->of_node);
+
+	for (i = 0; i < scmi_pd_data->num_domains; i++)
+		pm_genpd_remove(scmi_pd_data->domains[i]);
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_PERF, "perf" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_perf_domain_driver = {
+	.name		= "scmi-perf-domain",
+	.probe		= scmi_perf_domain_probe,
+	.remove		= scmi_perf_domain_remove,
+	.id_table	= scmi_id_table,
+};
+module_scmi_driver(scmi_perf_domain_driver);
+
+MODULE_AUTHOR("Ulf Hansson <ulf.hansson@linaro.org>");
+MODULE_DESCRIPTION("ARM SCMI perf domain driver");
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

[PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Ulf Hansson]

The dev_pm_opp_add() API is limited to add dynamic OPPs with a frequency
and/or voltage level. To enable more flexibility, let's add a new
dev_pm_opp_add_dynamic() API, that's takes a struct dev_pm_opp_data*
instead of list of in-parameters.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++----------
 drivers/opp/of.c       | 11 ++++++----
 drivers/opp/opp.h      |  2 +-
 include/linux/pm_opp.h | 18 ++++++++++++++++
 4 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 954c94865cf5..0e6ee2980f88 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
  * _opp_add_v1() - Allocate a OPP based on v1 bindings.
  * @opp_table:	OPP table
  * @dev:	device for which we do this operation
- * @freq:	Frequency in Hz for this OPP
- * @u_volt:	Voltage in uVolts for this OPP
+ * @opp:	The OPP to add
  * @dynamic:	Dynamically added OPPs.
  *
  * This function adds an opp definition to the opp table and returns status.
@@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
  * -ENOMEM	Memory allocation failure
  */
 int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
-		unsigned long freq, long u_volt, bool dynamic)
+		struct dev_pm_opp_data *opp, bool dynamic)
 {
 	struct dev_pm_opp *new_opp;
-	unsigned long tol;
+	unsigned long tol, u_volt = opp->u_volt;
 	int ret;
 
 	if (!assert_single_clk(opp_table))
@@ -1954,7 +1953,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 		return -ENOMEM;
 
 	/* populate the opp table */
-	new_opp->rates[0] = freq;
+	new_opp->rates[0] = opp->freq;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
 	new_opp->supplies[0].u_volt = u_volt;
 	new_opp->supplies[0].u_volt_min = u_volt - tol;
@@ -2738,10 +2737,9 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 }
 
 /**
- * dev_pm_opp_add()  - Add an OPP table from a table definitions
- * @dev:	device for which we do this operation
- * @freq:	Frequency in Hz for this OPP
- * @u_volt:	Voltage in uVolts for this OPP
+ * dev_pm_opp_add_dynamic()  - Add an OPP table from a table definitions
+ * @dev:	The device for which we do this operation
+ * @opp:	The OPP to be added
  *
  * This function adds an opp definition to the opp table and returns status.
  * The opp is made available by default and it can be controlled using
@@ -2754,7 +2752,7 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
  *		Duplicate OPPs (both freq and volt are same) and !opp->available
  * -ENOMEM	Memory allocation failure
  */
-int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+int dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp)
 {
 	struct opp_table *opp_table;
 	int ret;
@@ -2766,12 +2764,41 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	/* Fix regulator count for dynamic OPPs */
 	opp_table->regulator_count = 1;
 
-	ret = _opp_add_v1(opp_table, dev, freq, u_volt, true);
+	ret = _opp_add_v1(opp_table, dev, opp, true);
 	if (ret)
 		dev_pm_opp_put_opp_table(opp_table);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dev_pm_opp_add_dynamic);
+
+/**
+ * dev_pm_opp_add()  - Add an OPP table from a table definitions
+ * @dev:	device for which we do this operation
+ * @freq:	Frequency in Hz for this OPP
+ * @u_volt:	Voltage in uVolts for this OPP
+ *
+ * This function adds an opp definition to the opp table and returns status.
+ * The opp is made available by default and it can be controlled using
+ * dev_pm_opp_enable/disable functions.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ */
+int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+{
+	struct dev_pm_opp_data opp;
+
+	memset(&opp, 0, sizeof(opp));
+	opp.freq = freq;
+	opp.u_volt = u_volt;
+
+	return dev_pm_opp_add_dynamic(dev, &opp);
+}
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 8246e9b7afe7..b96f6304f497 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1079,13 +1079,16 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
 
 	val = prop->value;
 	while (nr) {
-		unsigned long freq = be32_to_cpup(val++) * 1000;
-		unsigned long volt = be32_to_cpup(val++);
+		struct dev_pm_opp_data opp;
 
-		ret = _opp_add_v1(opp_table, dev, freq, volt, false);
+		memset(&opp, 0, sizeof(opp));
+		opp.freq = be32_to_cpup(val++) * 1000;
+		opp.u_volt = be32_to_cpup(val++);
+
+		ret = _opp_add_v1(opp_table, dev, &opp, false);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
-				__func__, freq, ret);
+				__func__, opp.freq, ret);
 			goto remove_static_opp;
 		}
 		nr -= 2;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 2a057c42ddf4..b15770b2305e 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -255,7 +255,7 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
 void _opp_free(struct dev_pm_opp *opp);
 int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
-int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
+int _opp_add_v1(struct opp_table *opp_table, struct device *dev, struct dev_pm_opp_data *opp, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
 struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
 void _put_opp_list_kref(struct opp_table *opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dc1fb5890792..305cd87b394c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -92,6 +92,16 @@ struct dev_pm_opp_config {
 	struct device ***virt_devs;
 };
 
+/**
+ * struct dev_pm_opp_data - The data to use to initialize an OPP.
+ * @freq: The clock rate in Hz for the OPP.
+ * @u_volt: The voltage in uV for the OPP.
+ */
+struct dev_pm_opp_data {
+	unsigned long freq;
+	unsigned long u_volt;
+};
+
 #if defined(CONFIG_PM_OPP)
 
 struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
@@ -142,6 +152,8 @@ void dev_pm_opp_put(struct dev_pm_opp *opp);
 
 int dev_pm_opp_add(struct device *dev, unsigned long freq,
 		   unsigned long u_volt);
+int dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp);
+
 void dev_pm_opp_remove(struct device *dev, unsigned long freq);
 void dev_pm_opp_remove_all_dynamic(struct device *dev);
 
@@ -297,6 +309,12 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq,
 	return -EOPNOTSUPP;
 }
 
+static inline int
+dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 {
 }
-- 
2.34.1

[PATCH 12/16] OPP: Extend dev_pm_opp_data with performance level

[Ulf Hansson]

Let's extend the dev_pm_opp_data with a level variable, to allow users to
specify a corresponding performance level for a dynamically added OPP.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e6ee2980f88..79b4b44ced3e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1954,6 +1954,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 
 	/* populate the opp table */
 	new_opp->rates[0] = opp->freq;
+	new_opp->level = opp->level;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
 	new_opp->supplies[0].u_volt = u_volt;
 	new_opp->supplies[0].u_volt_min = u_volt - tol;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 305cd87b394c..2c6f67736579 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -94,10 +94,12 @@ struct dev_pm_opp_config {
 
 /**
  * struct dev_pm_opp_data - The data to use to initialize an OPP.
+ * @level: The performance level for the OPP.
  * @freq: The clock rate in Hz for the OPP.
  * @u_volt: The voltage in uV for the OPP.
  */
 struct dev_pm_opp_data {
+	unsigned int level;
 	unsigned long freq;
 	unsigned long u_volt;
 };
-- 
2.34.1

[PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

To allow a dynamically added OPP to be coupled with a specific OPP provider
type, let's add a new enum variable in the struct dev_pm_opp_data.
Moreover, let's add support for a DEV_PM_OPP_TYPE_GENPD type, corresponding
to genpd's performance states support.

More precisely, this allows a genpd provider to dynamically add OPPs when a
device gets attached to it, that later can be used by a consumer driver
when it needs to change the performance level for its corresponding device.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c     | 19 +++++++++++++++++++
 drivers/opp/opp.h      |  1 +
 include/linux/pm_opp.h |  7 +++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 79b4b44ced3e..81a3418e2eaf 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 			return ret;
 		}
 
+		if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
+			ret = dev_pm_genpd_set_performance_state(dev, opp->level);
+			if (ret) {
+				dev_err(dev, "Failed to set performance level: %d\n",
+					ret);
+				return ret;
+			}
+		}
+
 		ret = _set_opp_bw(opp_table, opp, dev);
 		if (ret) {
 			dev_err(dev, "Failed to set bw: %d\n", ret);
@@ -1155,6 +1164,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 			return ret;
 		}
 
+		if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
+			ret = dev_pm_genpd_set_performance_state(dev, opp->level);
+			if (ret) {
+				dev_err(dev, "Failed to set performance level: %d\n",
+					ret);
+				return ret;
+			}
+		}
+
 		ret = _set_required_opps(dev, opp_table, opp, false);
 		if (ret) {
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
@@ -1955,6 +1973,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 	/* populate the opp table */
 	new_opp->rates[0] = opp->freq;
 	new_opp->level = opp->level;
+	new_opp->provider = opp->provider;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
 	new_opp->supplies[0].u_volt = u_volt;
 	new_opp->supplies[0].u_volt_min = u_volt - tol;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index b15770b2305e..ee2b3bd89213 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -104,6 +104,7 @@ struct dev_pm_opp {
 	unsigned int pstate;
 	unsigned long *rates;
 	unsigned int level;
+	enum dev_pm_opp_provider_type provider;
 
 	struct dev_pm_opp_supply *supplies;
 	struct dev_pm_opp_icc_bw *bandwidth;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2c6f67736579..4c40199c7728 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -26,6 +26,11 @@ enum dev_pm_opp_event {
 	OPP_EVENT_ADJUST_VOLTAGE,
 };
 
+enum dev_pm_opp_provider_type {
+	DEV_PM_OPP_TYPE_NONE = 0,
+	DEV_PM_OPP_TYPE_GENPD,
+};
+
 /**
  * struct dev_pm_opp_supply - Power supply voltage/current values
  * @u_volt:	Target voltage in microvolts corresponding to this OPP
@@ -97,11 +102,13 @@ struct dev_pm_opp_config {
  * @level: The performance level for the OPP.
  * @freq: The clock rate in Hz for the OPP.
  * @u_volt: The voltage in uV for the OPP.
+ * @provider: The type of provider for the OPP.
  */
 struct dev_pm_opp_data {
 	unsigned int level;
 	unsigned long freq;
 	unsigned long u_volt;
+	enum dev_pm_opp_provider_type provider;
 };
 
 #if defined(CONFIG_PM_OPP)
-- 
2.34.1

[PATCH 14/16] firmware: arm_scmi: Simplify error path in scmi_dvfs_device_opps_add()

[Ulf Hansson]

Let's simplify the code in scmi_dvfs_device_opps_add() by using
dev_pm_opp_remove_all_dynamic() in its error path.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/perf.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 9c5340f590e4..03a496ccc603 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -585,23 +585,18 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
 {
 	int idx, ret;
 	unsigned long freq;
-	struct scmi_opp *opp;
 	struct perf_dom_info *dom;
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 
 	dom = pi->dom_info + domain;
 
-	for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) {
-		freq = opp->perf * dom->mult_factor;
+	for (idx = 0; idx < dom->opp_count; idx++) {
+		freq = dom->opp[idx].perf * dom->mult_factor;
 
 		ret = dev_pm_opp_add(dev, freq, 0);
 		if (ret) {
 			dev_warn(dev, "failed to add opp %luHz\n", freq);
-
-			while (idx-- > 0) {
-				freq = (--opp)->perf * dom->mult_factor;
-				dev_pm_opp_remove(dev, freq);
-			}
+			dev_pm_opp_remove_all_dynamic(dev);
 			return ret;
 		}
 	}
-- 
2.34.1

[PATCH 15/16] firmware: arm_scmi: Extend perf support with OPP from genpd providers

[Ulf Hansson]

To enable a genpd provider to add OPPs for its attached devices,  let's
convert into using the dev_pm_opp_add_dynamic() API, in favor of the
current dev_pm_opp_add() API. This allows us to specify the frequency, the
performance level and the OPP provider type for each OPP that it may be
adding.

Moreover, to let callers of the ->device_opps_add() ops, to specify the OPP
provider let's add a new in-parameter to it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpufreq/scmi-cpufreq.c   |  2 +-
 drivers/firmware/arm_scmi/perf.c | 15 ++++++++++-----
 include/linux/scmi_protocol.h    |  2 +-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 78f53e388094..a3f89a4ca899 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -188,7 +188,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	  */
 	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
 	if (nr_opp <= 0) {
-		ret = perf_ops->device_opps_add(ph, cpu_dev, domain);
+		ret = perf_ops->device_opps_add(ph, cpu_dev, domain, false);
 		if (ret) {
 			dev_warn(cpu_dev, "failed to add opps to the device\n");
 			goto out_free_cpumask;
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 03a496ccc603..b6cebe45fbc8 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -581,21 +581,26 @@ static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
-				     struct device *dev, u32 domain)
+				     struct device *dev, u32 domain, bool genpd)
 {
 	int idx, ret;
-	unsigned long freq;
+	struct dev_pm_opp_data opp_data;
 	struct perf_dom_info *dom;
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 
 	dom = pi->dom_info + domain;
 
 	for (idx = 0; idx < dom->opp_count; idx++) {
-		freq = dom->opp[idx].perf * dom->mult_factor;
+		memset(&opp_data, 0, sizeof(opp_data));
+		opp_data.level = dom->opp[idx].perf;
+		opp_data.freq = dom->opp[idx].perf * dom->mult_factor;
+		opp_data.provider = genpd ? DEV_PM_OPP_TYPE_GENPD :
+					    DEV_PM_OPP_TYPE_NONE;
 
-		ret = dev_pm_opp_add(dev, freq, 0);
+		ret = dev_pm_opp_add_dynamic(dev, &opp_data);
 		if (ret) {
-			dev_warn(dev, "failed to add opp %luHz\n", freq);
+			dev_warn(dev, "failed to add opp %luHz\n",
+				 opp_data.freq);
 			dev_pm_opp_remove_all_dynamic(dev);
 			return ret;
 		}
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 21aea1b2a355..ec421107f94c 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -136,7 +136,7 @@ struct scmi_perf_proto_ops {
 	int (*transition_latency_get)(const struct scmi_protocol_handle *ph,
 				      u32 domain);
 	int (*device_opps_add)(const struct scmi_protocol_handle *ph,
-			       struct device *dev, u32 domain);
+			       struct device *dev, u32 domain, bool genpd);
 	int (*freq_set)(const struct scmi_protocol_handle *ph, u32 domain,
 			unsigned long rate, bool poll);
 	int (*freq_get)(const struct scmi_protocol_handle *ph, u32 domain,
-- 
2.34.1

[PATCH 16/16] firmware: arm_scmi: Add generic OPP support to the SCMI performance domain

[Ulf Hansson]

To allow a consumer driver to use the generic OPP library to scale the
performance for its device, let's dynamically add the OPP table when the
device gets attached to the SCMI performance domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/arm_scmi/scmi_perf_domain.c | 34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/firmware/arm_scmi/scmi_perf_domain.c b/drivers/firmware/arm_scmi/scmi_perf_domain.c
index 9be90a7d94de..e9767ca1d34f 100644
--- a/drivers/firmware/arm_scmi/scmi_perf_domain.c
+++ b/drivers/firmware/arm_scmi/scmi_perf_domain.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
 #include <linux/scmi_protocol.h>
 #include <linux/slab.h>
 
@@ -39,6 +40,37 @@ scmi_pd_set_perf_state(struct generic_pm_domain *genpd, unsigned int state)
 	return ret;
 }
 
+static int
+scmi_pd_attach_dev(struct generic_pm_domain *genpd, struct device *dev)
+{
+	struct scmi_perf_domain *pd = to_scmi_pd(genpd);
+	int ret;
+
+	/*
+	 * Allow the device to be attached, but don't add the OPP table unless
+	 * the performance level can be changed.
+	 */
+	if (!pd->can_level_set)
+		return 0;
+
+	ret = pd->perf_ops->device_opps_add(pd->ph, dev, pd->domain_id, true);
+	if (ret)
+		dev_warn(dev, "failed to add OPPs for the device\n");
+
+	return ret;
+}
+
+static void
+scmi_pd_detach_dev(struct generic_pm_domain *genpd, struct device *dev)
+{
+	struct scmi_perf_domain *pd = to_scmi_pd(genpd);
+
+	if (!pd->can_level_set)
+		return;
+
+	dev_pm_opp_remove_all_dynamic(dev);
+}
+
 static int scmi_perf_domain_probe(struct scmi_device *sdev)
 {
 	struct device *dev = &sdev->dev;
@@ -92,6 +124,8 @@ static int scmi_perf_domain_probe(struct scmi_device *sdev)
 		scmi_pd->genpd.name = perf_ops->name_get(ph, i);
 		scmi_pd->genpd.flags = GENPD_FLAG_OPP_TABLE_FW;
 		scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state;
+		scmi_pd->genpd.attach_dev = scmi_pd_attach_dev;
+		scmi_pd->genpd.detach_dev = scmi_pd_detach_dev;
 
 		ret = perf_ops->level_get(ph, i, &perf_level, false);
 		if (ret) {
-- 
2.34.1

Re: [PATCH 00/16] arm_scmi/opp/dvfs: Add generic performance scaling support

[Cristian Marussi]

On Wed, Jun 07, 2023 at 02:46:12PM +0200, Ulf Hansson wrote:
> The current SCMI performance scaling support is limited to cpufreq. This series
> extends the support, so it can be used for all kind of devices and not only for
> CPUs.
> 
> The changes are spread over a couple of different subsystems, although the
> changes that affects the other subsystems than the arm_scmi directory are
> mostly smaller. The series is based upon v6.4-rc5. That said, let's figure out
> on how to best move forward with this. I am of course happy to help in any way.
> 
> Note that, so far this is only be tested on the Qemu virt platform with Optee
> running an SCMI server. If you want some more details about my test setup, I am
> certainly open to share that with you!
> 
> Looking forward to get your feedback!
> 

Hi Ulf,

thanks for this first of all.

I'll have a look at this properly in the next weeks, in the meantime
just a small minor remark after having had a quick look.

You expose a few new perf_ops to fit your needs and in fact PERF was
still not exposing those data for (apparent) lack of users needing
those. (and/or historical reason I think)

My concern is that this would lead to a growing number of ops as soon as
more data will be needed by future users; indeed other protocols do
expose more data but use a different approach: instead of custom ops
they let the user access a common static info structure like


+       int (*num_domains_get)(const struct scmi_protocol_handle *ph);
+       const struct scmi_perf_dom_info __must_check *(*info_get)
+               (const struct scmi_protocol_handle *ph, u32 domain);

and expose the related common info struct in scmi_protocol.h too.
Another reason to stick to this aproach would be consistency with other
protos (even though I think PERF is not the only lacking info_get)

Now, since really there was already a hidden user for this perf data
(that would be me :P ... in terms of an unpublished SCMI test-driver),
I happen to have a tested patch that just expose those 2 above ops and
exports scmi_perf_dom_info and related structures to scmi_protocol.h

If you (and Sudeep) agree with this approach of limiting the number of
exposed ops in favour of sharing upfront some static info data, I can
quickly cleanup and post this patch for you to pick it up in your next
iteration.

(really I'd have more conversion of this kind also for other remaining
 protos but these are unrelated to your series and I'd post it later)

Thanks,
Cristian

Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Viresh Kumar]

On 07-06-23, 14:46, Ulf Hansson wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 954c94865cf5..0e6ee2980f88 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>   * _opp_add_v1() - Allocate a OPP based on v1 bindings.
>   * @opp_table:	OPP table
>   * @dev:	device for which we do this operation
> - * @freq:	Frequency in Hz for this OPP
> - * @u_volt:	Voltage in uVolts for this OPP
> + * @opp:	The OPP to add
>   * @dynamic:	Dynamically added OPPs.
>   *
>   * This function adds an opp definition to the opp table and returns status.
> @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>   * -ENOMEM	Memory allocation failure
>   */
>  int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> -		unsigned long freq, long u_volt, bool dynamic)
> +		struct dev_pm_opp_data *opp, bool dynamic)

The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
different name here please for the data ?

> +/**
> + * dev_pm_opp_add()  - Add an OPP table from a table definitions
> + * @dev:	device for which we do this operation
> + * @freq:	Frequency in Hz for this OPP
> + * @u_volt:	Voltage in uVolts for this OPP
> + *
> + * This function adds an opp definition to the opp table and returns status.
> + * The opp is made available by default and it can be controlled using
> + * dev_pm_opp_enable/disable functions.
> + *
> + * Return:
> + * 0		On success OR
> + *		Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST	Freq are same and volt are different OR
> + *		Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM	Memory allocation failure
> + */
> +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)

Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
of documentation too.

> +{
> +	struct dev_pm_opp_data opp;
> +
> +	memset(&opp, 0, sizeof(opp));

What about
        struct dev_pm_opp_data data = {0};

I think it is guaranteed that all the fields will be 0 now, not the padding of
course, but we don't care about that here.

> +	opp.freq = freq;
> +	opp.u_volt = u_volt;

Or maybe just

        struct dev_pm_opp_data data = {
                .freq = freq,
                .u_volt = u_volt,
        };

Rest must be 0.

-- 
viresh

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Viresh Kumar]

On 07-06-23, 14:46, Ulf Hansson wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 79b4b44ced3e..81a3418e2eaf 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  			return ret;
>  		}
>  
> +		if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
> +			ret = dev_pm_genpd_set_performance_state(dev, opp->level);
> +			if (ret) {
> +				dev_err(dev, "Failed to set performance level: %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +

I don't like this :)

We already have these calls in place from within _set_required_opps(), and we
should try to get this done in a way that those calls themselves get the
performance state configured.

-- 
viresh

Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Ulf Hansson]

On Thu, 8 Jun 2023 at 07:29, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 954c94865cf5..0e6ee2980f88 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * _opp_add_v1() - Allocate a OPP based on v1 bindings.
> >   * @opp_table:       OPP table
> >   * @dev:     device for which we do this operation
> > - * @freq:    Frequency in Hz for this OPP
> > - * @u_volt:  Voltage in uVolts for this OPP
> > + * @opp:     The OPP to add
> >   * @dynamic: Dynamically added OPPs.
> >   *
> >   * This function adds an opp definition to the opp table and returns status.
> > @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * -ENOMEM   Memory allocation failure
> >   */
> >  int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> > -             unsigned long freq, long u_volt, bool dynamic)
> > +             struct dev_pm_opp_data *opp, bool dynamic)
>
> The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
> different name here please for the data ?

Certainly, what do you suggest?

>
> > +/**
> > + * dev_pm_opp_add()  - Add an OPP table from a table definitions
> > + * @dev:     device for which we do this operation
> > + * @freq:    Frequency in Hz for this OPP
> > + * @u_volt:  Voltage in uVolts for this OPP
> > + *
> > + * This function adds an opp definition to the opp table and returns status.
> > + * The opp is made available by default and it can be controlled using
> > + * dev_pm_opp_enable/disable functions.
> > + *
> > + * Return:
> > + * 0         On success OR
> > + *           Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST   Freq are same and volt are different OR
> > + *           Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM   Memory allocation failure
> > + */
> > +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>
> Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
> of documentation too.

Good idea!

>
> > +{
> > +     struct dev_pm_opp_data opp;
> > +
> > +     memset(&opp, 0, sizeof(opp));
>
> What about
>         struct dev_pm_opp_data data = {0};
>
> I think it is guaranteed that all the fields will be 0 now, not the padding of
> course, but we don't care about that here.
>
> > +     opp.freq = freq;
> > +     opp.u_volt = u_volt;
>
> Or maybe just
>
>         struct dev_pm_opp_data data = {
>                 .freq = freq,
>                 .u_volt = u_volt,
>         };
>
> Rest must be 0.

Good suggestions both, I will change to whatever is best suitable!

Thanks for reviewing!

Kind regards
Uffe

Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Viresh Kumar]

On 08-06-23, 10:59, Ulf Hansson wrote:
> Certainly, what do you suggest?

`data` :)

-- 
viresh

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

On Thu, 8 Jun 2023 at 07:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 79b4b44ced3e..81a3418e2eaf 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
> >                       return ret;
> >               }
> >
> > +             if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
> > +                     ret = dev_pm_genpd_set_performance_state(dev, opp->level);
> > +                     if (ret) {
> > +                             dev_err(dev, "Failed to set performance level: %d\n",
> > +                                     ret);
> > +                             return ret;
> > +                     }
> > +             }
> > +
>
> I don't like this :)
>
> We already have these calls in place from within _set_required_opps(), and we
> should try to get this done in a way that those calls themselves get the
> performance state configured.

I was looking at that, but wanted to keep things as simple as possible
in the $subject series.

The required opps are also different, as it's getting parsed from DT
both for the genpd provider and the consumer. The point is, there are
more code involved but just _set_required_opps().

For example, _set_performance_state() (which is the one that calls
dev_pm_genpd_set_performance_state()) is designed to be used for
required opps. Does it really make sense to rework
_set_performance_state() so it can be used for this case too, just to
avoid another call to dev_pm_genpd_set_performance_state() somewhere
in the code?

One improvement we can make though, is to add a helper function,
"_set_opp_level()", which we call from _set_opp(). This can then
replace the call to _set_required_opps() and the code above that I am
adding for DEV_PM_OPP_TYPE_GENPD. At least that should keep the code
_set_opp() a bit more readable.

What do you think?

Kind regards
Uffe

Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Ulf Hansson]

On Thu, 8 Jun 2023 at 11:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-06-23, 10:59, Ulf Hansson wrote:
> > Certainly, what do you suggest?
>
> `data` :)

I thought you meant renaming the struct. :-)

Yes, I move to data instead!

Uffe

Re: [PATCH 00/16] arm_scmi/opp/dvfs: Add generic performance scaling support

[Ulf Hansson]

On Wed, 7 Jun 2023 at 16:43, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Wed, Jun 07, 2023 at 02:46:12PM +0200, Ulf Hansson wrote:
> > The current SCMI performance scaling support is limited to cpufreq. This series
> > extends the support, so it can be used for all kind of devices and not only for
> > CPUs.
> >
> > The changes are spread over a couple of different subsystems, although the
> > changes that affects the other subsystems than the arm_scmi directory are
> > mostly smaller. The series is based upon v6.4-rc5. That said, let's figure out
> > on how to best move forward with this. I am of course happy to help in any way.
> >
> > Note that, so far this is only be tested on the Qemu virt platform with Optee
> > running an SCMI server. If you want some more details about my test setup, I am
> > certainly open to share that with you!
> >
> > Looking forward to get your feedback!
> >
>
> Hi Ulf,
>
> thanks for this first of all.
>
> I'll have a look at this properly in the next weeks, in the meantime
> just a small minor remark after having had a quick look.
>
> You expose a few new perf_ops to fit your needs and in fact PERF was
> still not exposing those data for (apparent) lack of users needing
> those. (and/or historical reason I think)
>
> My concern is that this would lead to a growing number of ops as soon as
> more data will be needed by future users; indeed other protocols do
> expose more data but use a different approach: instead of custom ops
> they let the user access a common static info structure like
>
>
> +       int (*num_domains_get)(const struct scmi_protocol_handle *ph);
> +       const struct scmi_perf_dom_info __must_check *(*info_get)
> +               (const struct scmi_protocol_handle *ph, u32 domain);
>
> and expose the related common info struct in scmi_protocol.h too.
> Another reason to stick to this aproach would be consistency with other
> protos (even though I think PERF is not the only lacking info_get)
>
> Now, since really there was already a hidden user for this perf data
> (that would be me :P ... in terms of an unpublished SCMI test-driver),
> I happen to have a tested patch that just expose those 2 above ops and
> exports scmi_perf_dom_info and related structures to scmi_protocol.h
>
> If you (and Sudeep) agree with this approach of limiting the number of
> exposed ops in favour of sharing upfront some static info data, I can
> quickly cleanup and post this patch for you to pick it up in your next
> iteration.

I think your suggestions make perfect sense to me too.

While I was adding the new ops in scmi_perf_proto_ops, I was merely
trying to get inspiration from the scmi_power_proto_ops, it seems like
those need an update too.

Although, there is no need for you to send a patch for "perf" at this
moment - as this piece of code is easy for me to put together myself.
I will simply replace a few of the patches in the series with a new
one, no problem at all.

>
> (really I'd have more conversion of this kind also for other remaining
>  protos but these are unrelated to your series and I'd post it later)

Yes, that can be handled separately, and I leave that for you to manage.

Kind regards
Uffe

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Viresh Kumar]

On 08-06-23, 11:37, Ulf Hansson wrote:
> The required opps are also different, as it's getting parsed from DT
> both for the genpd provider and the consumer. The point is, there are
> more code involved but just _set_required_opps().
> 
> For example, _set_performance_state() (which is the one that calls
> dev_pm_genpd_set_performance_state()) is designed to be used for
> required opps. Does it really make sense to rework
> _set_performance_state() so it can be used for this case too, just to
> avoid another call to dev_pm_genpd_set_performance_state() somewhere
> in the code?

What we need here, in you case, is really the required-opp thing, without the
DT parsing. The genpd will have an OPP table here, and devices (you are adding
OPP table dynamically for) shall have the genpd's OPPs as their required OPPs,
since for setting OPPs of the device, it is *required* to have OPP of the genpd
set too. Just like how it happens with DT. No special handling will be required
in dev_pm_opp_set_opp() path in this case and existing code will just work. You
just need to set the required-opp tables properly.

-- 
viresh

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

On Thu, 8 Jun 2023 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-06-23, 11:37, Ulf Hansson wrote:
> > The required opps are also different, as it's getting parsed from DT
> > both for the genpd provider and the consumer. The point is, there are
> > more code involved but just _set_required_opps().
> >
> > For example, _set_performance_state() (which is the one that calls
> > dev_pm_genpd_set_performance_state()) is designed to be used for
> > required opps. Does it really make sense to rework
> > _set_performance_state() so it can be used for this case too, just to
> > avoid another call to dev_pm_genpd_set_performance_state() somewhere
> > in the code?
>
> What we need here, in you case, is really the required-opp thing, without the
> DT parsing. The genpd will have an OPP table here, and devices (you are adding
> OPP table dynamically for) shall have the genpd's OPPs as their required OPPs,
> since for setting OPPs of the device, it is *required* to have OPP of the genpd
> set too. Just like how it happens with DT. No special handling will be required
> in dev_pm_opp_set_opp() path in this case and existing code will just work. You
> just need to set the required-opp tables properly.

Okay, if I understand your point you want to avoid creating OPPs for
each device, but rather coupling them with the genpd provider's OPP
table. Right?

Note that, there is no such thing as a "required opp" in the SCMI
performance protocol case. A device is compatible to use all of the
OPPs that its corresponding SCMI performance domain provides. Should
we rename the required opp things in the OPP core to better reflect
this too?

That said, we still need to be able to add OPPs dynamically when not
based on DT. The difference would be that we add the OPPs when
initializing the genpd provider instead of when attaching the devices.
In other words, we still need something along the lines of the new
dev_pm_opp_add_dynamic() API that $subject series is introducing, I
think.

Moreover, to tie the consumer device's OPP table to their genpd
provider's OPP table (call it required-opp or whatever), we need
another OPP helper function that we can call from the genpd provider's
->attach_dev() callback. Similarly, we need to be able to remove this
connection when genpd's ->detach_dev() callback is invoked. I will
think of something here.

Finally, I want to point out that there is work going on in parallel
with this, that is adding performance state support for the ACPI PM
domain. The ACPI PM domain, isn't a genpd provider but implements it's
own PM domain. The important point is, that it will have its own
variant of the dev_pm_genpd_set_performance_state() that we may need
to call from the OPP library.

Kind regards
Uffe

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Viresh Kumar]

On 08-06-23, 13:45, Ulf Hansson wrote:
> Okay, if I understand your point you want to avoid creating OPPs for
> each device, but rather coupling them with the genpd provider's OPP
> table. Right?

Not exactly :)

> Note that, there is no such thing as a "required opp" in the SCMI
> performance protocol case. A device is compatible to use all of the
> OPPs that its corresponding SCMI performance domain provides. Should
> we rename the required opp things in the OPP core to better reflect
> this too?

Not really :)

> That said, we still need to be able to add OPPs dynamically when not
> based on DT. The difference would be that we add the OPPs when
> initializing the genpd provider instead of when attaching the devices.
> In other words, we still need something along the lines of the new
> dev_pm_opp_add_dynamic() API that $subject series is introducing, I
> think.

That's fine.

> Moreover, to tie the consumer device's OPP table to their genpd
> provider's OPP table (call it required-opp or whatever), we need
> another OPP helper function that we can call from the genpd provider's
> ->attach_dev() callback. Similarly, we need to be able to remove this
> connection when genpd's ->detach_dev() callback is invoked. I will
> think of something here.

Something like set/link/config_required_opp()..

> Finally, I want to point out that there is work going on in parallel
> with this, that is adding performance state support for the ACPI PM
> domain. The ACPI PM domain, isn't a genpd provider but implements it's
> own PM domain. The important point is, that it will have its own
> variant of the dev_pm_genpd_set_performance_state() that we may need
> to call from the OPP library.

Okay.

Let me explain how structures are linked currently with help of below (badly
made) drawing. The 'level' fields are only set for Genpd's OPP entries and not
devices. The genpd OPP table normally has only this information, where it
presents all the possible level values, separate OPP for each of them.

Now the devices don't have `level` set in their OPP table, DT or in C
structures. All they have are links for required OPPs, which help us find the
required level or performance state for a particular genpd.

  +-----------------+                    +------------------+
  |  Device A       |                    | GENPD OPP Table  |
  +-----------------+  required-opps     +------------------+   required-opps
  |OPP1: freq: x1   +--------------------+ OPP1:            +--------------+
  +-----------------+                    |      level: 1    |              |
  |OPP2: freq: y1   +-----------+        +------------------+              |
  +-----------------+           |        | OPP2:            +---------+    |
  |OPP3: freq: z1   +--------+  +--------+      level: 2    |         |    |
  +-----------------+        |           +------------------+         |    |
                             |           | OPP3:            +--+      |    |
                             |           |      level: 3    |  |      |    |
                             |           +------------------+  |      |    |
  +-----------------+        |           | OPP4:            |  |      |    |
  |  Device B       |        +-----------+      level: 4    |  |      |    |
  +-----------------+                    +------------------+  |      |    |
  |OPP1: freq: x2   +------------------------------------------+      |    |
  +-----------------+                                                 |    |
  |OPP2: freq: y2   +-------------------------------------------------+    |
  +-----------------+                                                      |
  |OPP3: freq: z2   +------------------------------------------------------+
  +-----------------+


What I am asking you to do now is, create an OPP table for the Genpd first, with
OPPs for each possible level. Now the Genpd layer creates the OPP table for
Device A, where it won't fill the levels, but all other fields and then use a
new API to add required OPPs for the device's OPP, which will point into Genpd's
OPP entries. And with that the existing code will work fine without any other
modifications.

Does this help ?

--

viresh

Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

On Fri, 9 Jun 2023 at 07:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-06-23, 13:45, Ulf Hansson wrote:
> > Okay, if I understand your point you want to avoid creating OPPs for
> > each device, but rather coupling them with the genpd provider's OPP
> > table. Right?
>
> Not exactly :)
>
> > Note that, there is no such thing as a "required opp" in the SCMI
> > performance protocol case. A device is compatible to use all of the
> > OPPs that its corresponding SCMI performance domain provides. Should
> > we rename the required opp things in the OPP core to better reflect
> > this too?
>
> Not really :)

I think it may be confusing, but okay, let's leave it as is.

>
> > That said, we still need to be able to add OPPs dynamically when not
> > based on DT. The difference would be that we add the OPPs when
> > initializing the genpd provider instead of when attaching the devices.
> > In other words, we still need something along the lines of the new
> > dev_pm_opp_add_dynamic() API that $subject series is introducing, I
> > think.
>
> That's fine.
>
> > Moreover, to tie the consumer device's OPP table to their genpd
> > provider's OPP table (call it required-opp or whatever), we need
> > another OPP helper function that we can call from the genpd provider's
> > ->attach_dev() callback. Similarly, we need to be able to remove this
> > connection when genpd's ->detach_dev() callback is invoked. I will
> > think of something here.
>
> Something like set/link/config_required_opp()..
>
> > Finally, I want to point out that there is work going on in parallel
> > with this, that is adding performance state support for the ACPI PM
> > domain. The ACPI PM domain, isn't a genpd provider but implements it's
> > own PM domain. The important point is, that it will have its own
> > variant of the dev_pm_genpd_set_performance_state() that we may need
> > to call from the OPP library.
>
> Okay.
>
> Let me explain how structures are linked currently with help of below (badly
> made) drawing. The 'level' fields are only set for Genpd's OPP entries and not
> devices. The genpd OPP table normally has only this information, where it
> presents all the possible level values, separate OPP for each of them.
>
> Now the devices don't have `level` set in their OPP table, DT or in C
> structures. All they have are links for required OPPs, which help us find the
> required level or performance state for a particular genpd.
>
>   +-----------------+                    +------------------+
>   |  Device A       |                    | GENPD OPP Table  |
>   +-----------------+  required-opps     +------------------+   required-opps
>   |OPP1: freq: x1   +--------------------+ OPP1:            +--------------+
>   +-----------------+                    |      level: 1    |              |
>   |OPP2: freq: y1   +-----------+        +------------------+              |
>   +-----------------+           |        | OPP2:            +---------+    |
>   |OPP3: freq: z1   +--------+  +--------+      level: 2    |         |    |
>   +-----------------+        |           +------------------+         |    |
>                              |           | OPP3:            +--+      |    |
>                              |           |      level: 3    |  |      |    |
>                              |           +------------------+  |      |    |
>   +-----------------+        |           | OPP4:            |  |      |    |
>   |  Device B       |        +-----------+      level: 4    |  |      |    |
>   +-----------------+                    +------------------+  |      |    |
>   |OPP1: freq: x2   +------------------------------------------+      |    |
>   +-----------------+                                                 |    |
>   |OPP2: freq: y2   +-------------------------------------------------+    |
>   +-----------------+                                                      |
>   |OPP3: freq: z2   +------------------------------------------------------+
>   +-----------------+
>

Thanks for taking the time to explain things further! It's not
entirely easy to follow all the things in the OPP library.

However, I think you are mixing up the "level" field with the "pstate"
field in the struct dev_pm_opp. The pstate field is solely for genpd
and the required opps, while level is *generic* for all types of
devices. Right?

More precisely, _read_opp_key() parses for the "opp-level" property
from DT to set the ->level field. Additionally, the generic OPP
helpers functions, like dev_pm_opp_get_level(),
dev_pm_opp_find_level_exact(), dev_pm_opp_find_level_ceil() allows any
type of consumer driver to operate on the level for an OPP for its
device. Please have a look at the apple-soc-cpufreq, for example.

>
> What I am asking you to do now is, create an OPP table for the Genpd first, with
> OPPs for each possible level. Now the Genpd layer creates the OPP table for
> Device A, where it won't fill the levels, but all other fields and then use a
> new API to add required OPPs for the device's OPP, which will point into Genpd's
> OPP entries. And with that the existing code will work fine without any other
> modifications.
>
> Does this help ?

Well, I think what you propose may be doable, but I fail to understand
the benefit of implementing it like that.

As I said earlier, there is no such thing as a required OPP for the
SCMI performance domain and neither are the OPP tables described in
DT.

Moreover, as I understand it, we already have the generic "level" to
use, why not use it here?

Kind regards
Uffe

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Rob Herring]

On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> The protocol@13 node is describing the performance scaling option for the
> ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> performance scaling is in many cases not limited to switching a clock's
> frequency.
> 
> Therefore, let's extend the binding so the interface can be modelled as a
> generic "performance domain" too. The common way to describe this, is to
> use the "power-domain" bindings, so let's use that.

What's wrong with the performance-domain binding?

> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..cff9d1e4cea1 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -145,8 +145,8 @@ properties:
>        '#clock-cells':
>          const: 1
>  
> -    required:
> -      - '#clock-cells'
> +      '#power-domain-cells':
> +        const: 1
>  
>    protocol@14:
>      $ref: '#/$defs/protocol-node'
> -- 
> 2.34.1
> 

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> The protocol@13 node is describing the performance scaling option for the
> ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> performance scaling is in many cases not limited to switching a clock's
> frequency.
> 
> Therefore, let's extend the binding so the interface can be modelled as a
> generic "performance domain" too. The common way to describe this, is to
> use the "power-domain" bindings, so let's use that.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..cff9d1e4cea1 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -145,8 +145,8 @@ properties:
>        '#clock-cells':
>          const: 1
>  
> -    required:
> -      - '#clock-cells'

I am yet to look at the patches, just looked at this binding changes for now.

Won't this break compatibility with existing DTBs. IMO, this is strict
no no, you can't drop #clock-cells. I wanted to add performance-domains
here as alternative but decided to not as I knew you were working on this.

-- 
Regards,
Sudeep

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > The protocol@13 node is describing the performance scaling option for the
> > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > performance scaling is in many cases not limited to switching a clock's
> > frequency.
> >
> > Therefore, let's extend the binding so the interface can be modelled as a
> > generic "performance domain" too. The common way to describe this, is to
> > use the "power-domain" bindings, so let's use that.
>
> What's wrong with the performance-domain binding?

In my opinion I think the performance-domain binding is superfluous.
We already have plenty of power-domains that do performance scaling
too - and they stick with the power-domain binding, as it's
sufficient.

That said, I would rather follow the defacto standard that has been
used for many years in the kernel. Do you have a preference that we
should stick to?

Kind regards
Uffe

>
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 5824c43e9893..cff9d1e4cea1 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -145,8 +145,8 @@ properties:
> >        '#clock-cells':
> >          const: 1
> >
> > -    required:
> > -      - '#clock-cells'
> > +      '#power-domain-cells':
> > +        const: 1
> >
> >    protocol@14:
> >      $ref: '#/$defs/protocol-node'
> > --
> > 2.34.1
> >

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > The protocol@13 node is describing the performance scaling option for the
> > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > performance scaling is in many cases not limited to switching a clock's
> > frequency.
> >
> > Therefore, let's extend the binding so the interface can be modelled as a
> > generic "performance domain" too. The common way to describe this, is to
> > use the "power-domain" bindings, so let's use that.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 5824c43e9893..cff9d1e4cea1 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -145,8 +145,8 @@ properties:
> >        '#clock-cells':
> >          const: 1
> >
> > -    required:
> > -      - '#clock-cells'
>
> I am yet to look at the patches, just looked at this binding changes for now.
>
> Won't this break compatibility with existing DTBs. IMO, this is strict
> no no, you can't drop #clock-cells. I wanted to add performance-domains
> here as alternative but decided to not as I knew you were working on this.

Thanks for reviewing!

The point with the suggested change was to allow any kind of
combination of using #clock-cells and/or #power-domain-cells. Honestly
I didn't really know how to best express that in the binding, but
maybe someone can help me out here?

I think enforcing #clock-cells to be used is unnecessary. Making it
optional should not break existing DTBs, right?

Moreover, currently it seems to be only Juno that uses "protocol@13"
and the "#clock-cells" (at least by looking at the DTSes in-kernel).
So, I wonder if it's really such a big deal to update the DT bindings
for "protocol@13" at this point, but I may not have the complete
picture.

Kind regards
Uffe

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Thu, Jun 15, 2023 at 11:39:06AM +0200, Ulf Hansson wrote:
> On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > The protocol@13 node is describing the performance scaling option for the
> > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > performance scaling is in many cases not limited to switching a clock's
> > > frequency.
> > >
> > > Therefore, let's extend the binding so the interface can be modelled as a
> > > generic "performance domain" too. The common way to describe this, is to
> > > use the "power-domain" bindings, so let's use that.
> > >
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 5824c43e9893..cff9d1e4cea1 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -145,8 +145,8 @@ properties:
> > >        '#clock-cells':
> > >          const: 1
> > >
> > > -    required:
> > > -      - '#clock-cells'
> >
> > I am yet to look at the patches, just looked at this binding changes for now.
> >
> > Won't this break compatibility with existing DTBs. IMO, this is strict
> > no no, you can't drop #clock-cells. I wanted to add performance-domains
> > here as alternative but decided to not as I knew you were working on this.
> 
> Thanks for reviewing!
> 
> The point with the suggested change was to allow any kind of
> combination of using #clock-cells and/or #power-domain-cells. Honestly
> I didn't really know how to best express that in the binding, but
> maybe someone can help me out here?
>

Even I don't know exact details, but there are rules like if this
property is present, some other property can't be there or something
on the similar lines. I have vague idea/recollection from my previous
experiments which probably was not needed then and hence I can't just
point to any examples unless I go and search myself.

> I think enforcing #clock-cells to be used is unnecessary. Making it
> optional should not break existing DTBs, right?

Correct. That is what I meant, it is either #clock-cells or
#power-domain-cells

>
> Moreover, currently it seems to be only Juno that uses "protocol@13"
> and the "#clock-cells" (at least by looking at the DTSes in-kernel).

Yes only one that has upstream DTS changes, but for sure it is used on
couple of other platforms. So for we are still far away from deprecate it
but we can eventually once users of it are ready to use new binding.

> So, I wonder if it's really such a big deal to update the DT bindings
> for "protocol@13" at this point, but I may not have the complete
> picture.
>

Yes it does break compatibility. Yes I know Juno is not a production
platform, but associating DT with kernel change makes is hard to switch
to older stable kernel versions without DT change which I really hate as
I will be wondering which SCMI perf is not working with stable kernel few
months down the line. So yes, we are not dropping the support for old
bindings even if it just for Juno(though I am sure it is not the only one).
I have spent time on such silly things when we were in the process of
pushing these bindings initially upstream. I prefer not to repeat that.

--
Regards,
Sudeep

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 15:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jun 15, 2023 at 11:39:06AM +0200, Ulf Hansson wrote:
> > On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > > The protocol@13 node is describing the performance scaling option for the
> > > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > > performance scaling is in many cases not limited to switching a clock's
> > > > frequency.
> > > >
> > > > Therefore, let's extend the binding so the interface can be modelled as a
> > > > generic "performance domain" too. The common way to describe this, is to
> > > > use the "power-domain" bindings, so let's use that.
> > > >
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > index 5824c43e9893..cff9d1e4cea1 100644
> > > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > @@ -145,8 +145,8 @@ properties:
> > > >        '#clock-cells':
> > > >          const: 1
> > > >
> > > > -    required:
> > > > -      - '#clock-cells'
> > >
> > > I am yet to look at the patches, just looked at this binding changes for now.
> > >
> > > Won't this break compatibility with existing DTBs. IMO, this is strict
> > > no no, you can't drop #clock-cells. I wanted to add performance-domains
> > > here as alternative but decided to not as I knew you were working on this.
> >
> > Thanks for reviewing!
> >
> > The point with the suggested change was to allow any kind of
> > combination of using #clock-cells and/or #power-domain-cells. Honestly
> > I didn't really know how to best express that in the binding, but
> > maybe someone can help me out here?
> >
>
> Even I don't know exact details, but there are rules like if this
> property is present, some other property can't be there or something
> on the similar lines. I have vague idea/recollection from my previous
> experiments which probably was not needed then and hence I can't just
> point to any examples unless I go and search myself.

I will figure it out, np!

>
> > I think enforcing #clock-cells to be used is unnecessary. Making it
> > optional should not break existing DTBs, right?
>
> Correct. That is what I meant, it is either #clock-cells or
> #power-domain-cells

Should we allow both? Or maybe that is just confusing?

In either case, I am converting the scmi cpufreq driver to cope with
using #power-domain-cells too, as that is useful regardless I think.
However, that's a separate series on top of $subject series.

>
> >
> > Moreover, currently it seems to be only Juno that uses "protocol@13"
> > and the "#clock-cells" (at least by looking at the DTSes in-kernel).
>
> Yes only one that has upstream DTS changes, but for sure it is used on
> couple of other platforms. So for we are still far away from deprecate it
> but we can eventually once users of it are ready to use new binding.

Okay, let's discuss when to deprecate it, but let's do that later on.

>
> > So, I wonder if it's really such a big deal to update the DT bindings
> > for "protocol@13" at this point, but I may not have the complete
> > picture.
> >
>
> Yes it does break compatibility. Yes I know Juno is not a production
> platform, but associating DT with kernel change makes is hard to switch
> to older stable kernel versions without DT change which I really hate as
> I will be wondering which SCMI perf is not working with stable kernel few
> months down the line. So yes, we are not dropping the support for old
> bindings even if it just for Juno(though I am sure it is not the only one).
> I have spent time on such silly things when we were in the process of
> pushing these bindings initially upstream. I prefer not to repeat that.

Okay, thanks for sharing the information. Let's simply follow the
regular path of how we deal with deprecating DT bindings then.

Kind regards
Uffe

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Rob Herring]

On Thu, Jun 15, 2023 at 3:11 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > The protocol@13 node is describing the performance scaling option for the
> > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > performance scaling is in many cases not limited to switching a clock's
> > > frequency.
> > >
> > > Therefore, let's extend the binding so the interface can be modelled as a
> > > generic "performance domain" too. The common way to describe this, is to
> > > use the "power-domain" bindings, so let's use that.
> >
> > What's wrong with the performance-domain binding?
>
> In my opinion I think the performance-domain binding is superfluous.
> We already have plenty of power-domains that do performance scaling
> too - and they stick with the power-domain binding, as it's
> sufficient.

IMO, power-domains should be for power islands which can be power
gated. I know they are abused though. Of course, when things are
hidden in firmware, you don't really know. A power-domain could be
just a clock or a clock could be multiple physical clocks.

> That said, I would rather follow the defacto standard that has been
> used for many years in the kernel. Do you have a preference that we
> should stick to?

If power-domains are sufficient, then why do we have
performance-domains? We need clear rules for when to use what.

Rob

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Fri, 14 Jul 2023 at 19:15, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jun 15, 2023 at 3:11 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > > The protocol@13 node is describing the performance scaling option for the
> > > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > > performance scaling is in many cases not limited to switching a clock's
> > > > frequency.
> > > >
> > > > Therefore, let's extend the binding so the interface can be modelled as a
> > > > generic "performance domain" too. The common way to describe this, is to
> > > > use the "power-domain" bindings, so let's use that.
> > >
> > > What's wrong with the performance-domain binding?
> >
> > In my opinion I think the performance-domain binding is superfluous.
> > We already have plenty of power-domains that do performance scaling
> > too - and they stick with the power-domain binding, as it's
> > sufficient.
>
> IMO, power-domains should be for power islands which can be power
> gated. I know they are abused though. Of course, when things are
> hidden in firmware, you don't really know. A power-domain could be
> just a clock or a clock could be multiple physical clocks.

I would also like to point out that it's perfectly possible that a
power-domain can be a combination of a "power-island" and a
performance-domain. In fact we have those cases already (Qcom, Tegra).

>
> > That said, I would rather follow the defacto standard that has been
> > used for many years in the kernel. Do you have a preference that we
> > should stick to?
>
> If power-domains are sufficient, then why do we have
> performance-domains? We need clear rules for when to use what.

Well, I think we invented the performance-domains bindings, especially
with CPUs in mind. So far, that's the only use-case too (Mediatek,
Apple). Even if I think the power-domains bindings would have worked
fine for these cases too, maybe we should limit the
performance-domains binding to be used for CPUs?

Anyway, for the more generic use-case, I think the power-domains DT
binding is better to stick with (it's what we have used for many years
by now), as it provides us with the flexibility of hooking up an
opp-table to the power-domain, allowing us to make the domain
"performance-capable" too.

Kind regards
Uffe