[PATCH v2 00/11] arm_scmi/cpufreq: Add generic performance scaling support

[PATCH v2 00/11] arm_scmi/cpufreq: Add generic performance scaling support

[Ulf Hansson]

Changes in v2:
	- Split up the series, to get the basic support in place as the first
	step. The part that remains is the integration with the OPP library, to
	allow consumer drivers to change performance level using the OPP lib.
	- Re-based on top v6.5-rc1.
	- Other changes will be described for each patch.

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.5-rc1.

Note that, I am runing this 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 your feedback!

Kind regards
Ulf Hansson


Ulf Hansson (11):
  firmware: arm_scmi: Extend perf protocol ops to get number of domains
  firmware: arm_scmi: Extend perf protocol ops to get information of a
    domain
  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
  cpufreq: scmi: Add support to parse domain-id using
    #power-domain-cells
  firmware: arm_scmi: Add the SCMI performance domain
  cpufreq: scmi: Drop redundant ifdef in scmi_cpufreq_probe()

 .../bindings/firmware/arm,scmi.yaml           |  11 +-
 drivers/base/power/domain.c                   |  11 +-
 drivers/cpufreq/scmi-cpufreq.c                |  48 ++++--
 drivers/firmware/arm_scmi/Kconfig             |  12 ++
 drivers/firmware/arm_scmi/Makefile            |   1 +
 drivers/firmware/arm_scmi/perf.c              |  60 ++++---
 drivers/firmware/arm_scmi/scmi_perf_domain.c  | 155 ++++++++++++++++++
 include/linux/pm_domain.h                     |   5 +
 include/linux/scmi_protocol.h                 |  18 +-
 9 files changed, 262 insertions(+), 59 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/scmi_perf_domain.c

-- 
2.34.1

[PATCH v2 01/11] 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>
---

Changes in v2:
	- None.

---
 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 e6fe4f73ffe6..71b39cbbdace 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 v2 02/11] firmware: arm_scmi: Extend perf protocol ops to get information of a domain

[Ulf Hansson]

Similar to other protocol ops, it's useful for an scmi module driver to get
some generic information of a performance domain. Therefore, let's add a
new callback to provide this information. The information is currently
limited to the name of the performance domain and whether the set-level
operation is supported, although this can easily be extended if we find the
need for it.

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

Changes in v2:
	- New patch (replacing two earlier patches).

---
 drivers/firmware/arm_scmi/perf.c | 21 ++++++++++++++++-----
 include/linux/scmi_protocol.h    |  8 ++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index cf7f0de4d6db..f3ea96dd845c 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -124,7 +124,6 @@ struct scmi_msg_resp_perf_describe_levels {
 
 struct perf_dom_info {
 	bool set_limits;
-	bool set_perf;
 	bool perf_limit_notify;
 	bool perf_level_notify;
 	bool perf_fastchannels;
@@ -132,7 +131,7 @@ struct perf_dom_info {
 	u32 sustained_freq_khz;
 	u32 sustained_perf_level;
 	u32 mult_factor;
-	char name[SCMI_MAX_STR_SIZE];
+	struct scmi_perf_domain_info info;
 	struct scmi_opp opp[MAX_OPPS];
 	struct scmi_fc_info *fc_info;
 };
@@ -209,7 +208,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 		flags = le32_to_cpu(attr->flags);
 
 		dom_info->set_limits = SUPPORTS_SET_LIMITS(flags);
-		dom_info->set_perf = SUPPORTS_SET_PERF_LVL(flags);
+		dom_info->info.set_perf = SUPPORTS_SET_PERF_LVL(flags);
 		dom_info->perf_limit_notify = SUPPORTS_PERF_LIMIT_NOTIFY(flags);
 		dom_info->perf_level_notify = SUPPORTS_PERF_LEVEL_NOTIFY(flags);
 		dom_info->perf_fastchannels = SUPPORTS_PERF_FASTCHANNELS(flags);
@@ -225,7 +224,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 			dom_info->mult_factor =
 					(dom_info->sustained_freq_khz * 1000) /
 					dom_info->sustained_perf_level;
-		strscpy(dom_info->name, attr->name, SCMI_SHORT_NAME_MAX_SIZE);
+		strscpy(dom_info->info.name, attr->name,
+			SCMI_SHORT_NAME_MAX_SIZE);
 	}
 
 	ph->xops->xfer_put(ph, t);
@@ -237,7 +237,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    dom_info->info.name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
@@ -340,6 +341,15 @@ static int scmi_perf_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
+static const struct scmi_perf_domain_info *
+scmi_perf_domain_info_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->info;
+}
+
 static int scmi_perf_mb_limits_set(const struct scmi_protocol_handle *ph,
 				   u32 domain, u32 max_perf, u32 min_perf)
 {
@@ -695,6 +705,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,
+	.domain_info_get = scmi_perf_domain_info_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 71b39cbbdace..ed032fe83c28 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -97,11 +97,17 @@ struct scmi_clk_proto_ops {
 			      u32 clk_id);
 };
 
+struct scmi_perf_domain_info {
+	char name[SCMI_MAX_STR_SIZE];
+	bool set_perf;
+};
+
 /**
  * struct scmi_perf_proto_ops - represents the various operations provided
  *	by SCMI Performance Protocol
  *
  * @num_domains_get: gets the number of supported performance domains
+ * @domain_info_get: get the information 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 +128,8 @@ struct scmi_clk_proto_ops {
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
+	const struct scmi_perf_domain_info __must_check *(*domain_info_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 v2 03/11] 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>
---

Changes in v2:
	- None.

---
 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 v2 04/11] 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 that
an additional OF parsing is needed each time the perf ops gets called, to
find the corresponding domain-id.

To avoid the above, 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>
---

Changes in v2:
	- None.

---
 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 f3ea96dd845c..6046c0eb5959 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -585,18 +585,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++) {
@@ -618,14 +614,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 */
@@ -685,12 +677,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 ed032fe83c28..71072fe8ccc9 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -140,9 +140,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,
@@ -150,7 +150,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 v2 05/11] 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>
---

Changes in v2:
	- None.

---
 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 6046c0eb5959..535174830946 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -572,18 +572,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)
 {
@@ -702,7 +690,6 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.limits_get = scmi_perf_limits_get,
 	.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 71072fe8ccc9..7098a0be79b9 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -112,7 +112,6 @@ struct scmi_perf_domain_info {
  * @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
@@ -138,7 +137,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 v2 06/11] 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>
---

Changes in v2:
	- None.

---
 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 v2 07/11] 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>
---

Changes in v2:
	- None.

---
 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 5cb2023581d4..c74edf80417f 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 v2 08/11] 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 domaintoo. The common way to describe this, is to use
the "power-domain" DT 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>
---

Changes in v2:
	- Updated the DT binding to require "oneOf" #power-domain-cells or
	#clock-cells.

---
 .../devicetree/bindings/firmware/arm,scmi.yaml        | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index b138f3d23df8..563a87dfb31a 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -149,8 +149,15 @@ properties:
       '#clock-cells':
         const: 1
 
-    required:
-      - '#clock-cells'
+      '#power-domain-cells':
+        const: 1
+
+    oneOf:
+      - required:
+          - '#clock-cells'
+
+      - required:
+          - '#power-domain-cells'
 
   protocol@14:
     $ref: '#/$defs/protocol-node'
-- 
2.34.1

[PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Ulf Hansson]

The performance domain-id can be described in DT using the power-domains
property or the clock property. The latter is already supported, so let's
add support for the power-domains too.

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

Changes in v2:
	- New patch.

---
 drivers/cpufreq/scmi-cpufreq.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 78f53e388094..b42f43d9bd89 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -72,13 +72,19 @@ static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 
 static int scmi_cpu_domain_id(struct device *cpu_dev)
 {
-	struct of_phandle_args clkspec;
+	struct of_phandle_args domain_id;
 
 	if (of_parse_phandle_with_args(cpu_dev->of_node, "clocks",
-				       "#clock-cells", 0, &clkspec))
-		return -EINVAL;
+				       "#clock-cells", 0, &domain_id)) {
 
-	return clkspec.args[0];
+		if (of_parse_phandle_with_args(cpu_dev->of_node,
+					       "power-domains",
+					       "#power-domain-cells", 0,
+					       &domain_id))
+			return -EINVAL;
+	}
+
+	return domain_id.args[0];
 }
 
 static int
-- 
2.34.1

[PATCH v2 10/11] 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 isn't part of this change, but
needs to be implemented from subsequent changes.

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

Changes in v2:
	- Converted to use the new ->domain_info_get() callback.

---
 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..4ed2401995be
--- /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;
+	const struct scmi_perf_domain_info *info;
+	u32 domain_id;
+};
+
+#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->info->set_perf)
+		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->info = perf_ops->domain_info_get(ph, i);
+
+		scmi_pd->domain_id = i;
+		scmi_pd->perf_ops = perf_ops;
+		scmi_pd->ph = ph;
+		scmi_pd->genpd.name = scmi_pd->info->name;
+		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 v2 11/11] cpufreq: scmi: Drop redundant ifdef in scmi_cpufreq_probe()

[Ulf Hansson]

We have stubs for devm_of_clk_add_hw_provider(), so there should be no need
to protect this with the '#ifdef CONFIG_COMMON_CLK'. Let's drop it to clean
up the code a bit.

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

Changes in v2:
	- New patch.

---

 drivers/cpufreq/scmi-cpufreq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index b42f43d9bd89..ab967e520355 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -326,11 +326,9 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
 	if (IS_ERR(perf_ops))
 		return PTR_ERR(perf_ops);
 
-#ifdef CONFIG_COMMON_CLK
 	/* dummy clock provider as needed by OPP if clocks property is used */
 	if (of_property_present(dev->of_node, "#clock-cells"))
 		devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, NULL);
-#endif
 
 	ret = cpufreq_register_driver(&scmi_cpufreq_driver);
 	if (ret) {
-- 
2.34.1

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Cristian Marussi]

On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
> 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 isn't part of this change, but
> needs to be implemented from subsequent changes.
> 

Hi Ulf,

a couple of remarks down below.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Converted to use the new ->domain_info_get() callback.
> 
> ---
>  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 

[snip]

> +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->info = perf_ops->domain_info_get(ph, i);
 
So here you are grabbing all the performance domains exposed by the
platform via PERF protocol and then a few lines down below you are
registering them with pm_genpd_init(), but the list of domains obtained
from the platform will contain NOT only devices but also CPUs possibly,
already managed by the SCMI CPUFreq driver.

In fact the SCMI CPUFreq driver, on his side, takes care to pick only
domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
parsing) but here you are registering all domains with GenPD upfront.

Is it not possible that, once registered, GenPD can decide, at some point
in the future, to try act on some of these domains associated with a CPU ?
(like Clock framework does at the end of boot trying to disable unused
 clocks...not familiar with internals of GenPD, though)

> +		scmi_pd->domain_id = i;
> +		scmi_pd->perf_ops = perf_ops;
> +		scmi_pd->ph = ph;
> +		scmi_pd->genpd.name = scmi_pd->info->name;
> +		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);

In SCMI world PERF levels should have nothing to do with the Power
state of a domain: you have the POWER protocol for that, so you should
not assume that perf level 0 means OFF, but you can use the POWER protocol
operation .state_get() to lookup the power state. (and you can grab both
perf and power ops from the same driver)

The tricky part would be to match the PERF domain at hand with the
related POWER domain to query the state for, I suppose.

Indeed, recently, while looking at SCMI v3.2 PERF evolutions, I was
tempted to just start rejecting any level_set() or set_freq() request
for ZERO since they really can be abused to power off a domain. (if the
platform complies...)

Apologize if I missed something about how GenPD behaviour...

Thanks,
Cristian

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> the "power-domain" DT bindings, so let's use that.
> 

One thing I forgot to ask earlier is how we can manage different domain IDs
for perf and power domains which is the case with current SCMI platforms as
the spec never mandated or can ever mandate the perf and power domains IDs
to match. They need not be same anyways.

--
Regards,
Sudeep

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Sudeep Holla]

On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> The performance domain-id can be described in DT using the power-domains
> property or the clock property. The latter is already supported, so let's
> add support for the power-domains too.
>

How is this supposed to work for the CPUs ? The CPU power domains are
generally PSCI on most of the platforms and the one using OSI explicitly
need to specify the details while ones using PC will not need to. Also they
can never be performance domains too. So I am not sure if I am following this
correctly.

--
Regards,
Sudeep

Re: [PATCH v2 11/11] cpufreq: scmi: Drop redundant ifdef in scmi_cpufreq_probe()

[Sudeep Holla]

On Thu, Jul 13, 2023 at 04:17:38PM +0200, Ulf Hansson wrote:
> We have stubs for devm_of_clk_add_hw_provider(), so there should be no need
> to protect this with the '#ifdef CONFIG_COMMON_CLK'. Let's drop it to clean
> up the code a bit.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- New patch.
> 
> ---
> 
>  drivers/cpufreq/scmi-cpufreq.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index b42f43d9bd89..ab967e520355 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -326,11 +326,9 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
>  	if (IS_ERR(perf_ops))
>  		return PTR_ERR(perf_ops);
>  
> -#ifdef CONFIG_COMMON_CLK

I am not sure if it is no longer possible but at the time of addition of this
it was possible to build with CONFIG_COMMON_CLK=n and any error was reported[1]
I didn't want to add Kconfig dependency as this driver doesn't use any other
clock apis and this was added to meet some OPP(?) requirement IIRC. We can
drop the call to devm_of_clk_add_hw_provider if that is not the case. I need
to check it again as I can't recall all the details right now.

--
Regards,
Sudeep

[1] https://www.uwsg.indiana.edu/hypermail/linux/kernel/2012.0/04953.html

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Sudeep Holla]

On Wed, Jul 19, 2023 at 03:51:45PM +0100, Cristian Marussi wrote:
> On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:

[...]

> > +	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->info = perf_ops->domain_info_get(ph, i);
>
> So here you are grabbing all the performance domains exposed by the
> platform via PERF protocol and then a few lines down below you are
> registering them with pm_genpd_init(), but the list of domains obtained
> from the platform will contain NOT only devices but also CPUs possibly,
> already managed by the SCMI CPUFreq driver.
>

Agreed, I pointed out briefly in the previous patch I think. I am not sure
how will that work if the performance and power domains are not 1-1 mapping
or if they are CPUs then this might confusing ? Not sure but looks like
we might be creating a spaghetti here :(.

> In fact the SCMI CPUFreq driver, on his side, takes care to pick only
> domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
> parsing) but here you are registering all domains with GenPD upfront.
>

+1

> Is it not possible that, once registered, GenPD can decide, at some point
> in the future, to try act on some of these domains associated with a CPU ?

IIRC, all unused genpd are turned off right. It may not impact here but still
super confusing as we will be creating power domains for the set of domains
actually advertised as power domains by the firmware. This will add another
set.

> (like Clock framework does at the end of boot trying to disable unused
>  clocks...not familiar with internals of GenPD, though)
>

Ah, I am reading too much serialised. Just agreed and wrote the same above.

> > +		scmi_pd->domain_id = i;
> > +		scmi_pd->perf_ops = perf_ops;
> > +		scmi_pd->ph = ph;
> > +		scmi_pd->genpd.name = scmi_pd->info->name;
> > +		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);
>
> In SCMI world PERF levels should have nothing to do with the Power
> state of a domain: you have the POWER protocol for that, so you should
> not assume that perf level 0 means OFF, but you can use the POWER protocol
> operation .state_get() to lookup the power state. (and you can grab both
> perf and power ops from the same driver)
>
> The tricky part would be to match the PERF domain at hand with the
> related POWER domain to query the state for, I suppose.
>

I wanted to ask the same. E.g. on juno, GPU has perf domain 2 and power domain
9. It would be good if we can how it would work there ? What is expected
from the gpu driver in terms of managing perf and power ? Does it need
to specify 2 power domains now and specify which is perf and which power in
its bindings ?

> Indeed, recently, while looking at SCMI v3.2 PERF evolutions, I was
> tempted to just start rejecting any level_set() or set_freq() request
> for ZERO since they really can be abused to power off a domain. (if the
> platform complies...)
>

Good point I need to dig the spec before I can comment on this.

--
Regards,
Sudeep

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > the "power-domain" DT bindings, so let's use that.
> >
>
> One thing I forgot to ask earlier is how we can manage different domain IDs
> for perf and power domains which is the case with current SCMI platforms as
> the spec never mandated or can ever mandate the perf and power domains IDs
> to match. They need not be same anyways.

Based upon what you describe above, I have modelled the perf-domain
and the power-domain as two separate power-domain providers.

A consumer device being hooked up to both domains, would specify the
domain IDs in the second power-domain-cell, along the lines of the
below. Then we would use power-domain-names to specify what each
power-domain represents.

power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
power-domain-names = "power", "perf";

I hope this makes it clearer!?

Kind regards
Uffe

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Ulf Hansson]

On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > The performance domain-id can be described in DT using the power-domains
> > property or the clock property. The latter is already supported, so let's
> > add support for the power-domains too.
> >
>
> How is this supposed to work for the CPUs ? The CPU power domains are
> generally PSCI on most of the platforms and the one using OSI explicitly
> need to specify the details while ones using PC will not need to. Also they
> can never be performance domains too. So I am not sure if I am following this
> correctly.

Your concerns are certainly correct, I completely forgot about this.
We need to specify what power-domain index belongs to what, by using
power-domain-names in DT. So a CPU node, that has both psci for power
and scmi for performance would then typically look like this:

power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
power-domain-names = "psci", "scmi";

I will take care of this in the next version - and thanks a lot for
pointing this out!

Kind regards
Uffe

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Fri, Jul 21, 2023 at 01:42:43PM +0200, Ulf Hansson wrote:
> On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > > the "power-domain" DT bindings, so let's use that.
> > >
> >
> > One thing I forgot to ask earlier is how we can manage different domain IDs
> > for perf and power domains which is the case with current SCMI platforms as
> > the spec never mandated or can ever mandate the perf and power domains IDs
> > to match. They need not be same anyways.
> 
> Based upon what you describe above, I have modelled the perf-domain
> and the power-domain as two separate power-domain providers.
> 
> A consumer device being hooked up to both domains, would specify the
> domain IDs in the second power-domain-cell, along the lines of the
> below. Then we would use power-domain-names to specify what each
> power-domain represents.
> 
> power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
> power-domain-names = "power", "perf";
>
> I hope this makes it clearer!?

Yes it make is clear definitely, but it does change the definition of the
generic binding of the "power-domains" property now. I am interesting in
the feedback from the binding maintainers with respect to that. Or is it
already present ? IIUC, the ones supported already are generally both
power and performance providers. May be it doesn't matter much, just
wanted to explicit ask and confirm those details.

--
Regards,
Sudeep

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Sudeep Holla]

On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > The performance domain-id can be described in DT using the power-domains
> > > property or the clock property. The latter is already supported, so let's
> > > add support for the power-domains too.
> > >
> >
> > How is this supposed to work for the CPUs ? The CPU power domains are
> > generally PSCI on most of the platforms and the one using OSI explicitly
> > need to specify the details while ones using PC will not need to. Also they
> > can never be performance domains too. So I am not sure if I am following this
> > correctly.
> 
> Your concerns are certainly correct, I completely forgot about this.
> We need to specify what power-domain index belongs to what, by using
> power-domain-names in DT. So a CPU node, that has both psci for power
> and scmi for performance would then typically look like this:
> 
> power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> power-domain-names = "psci", "scmi";
> 
> I will take care of this in the next version - and thanks a lot for
> pointing this out!


Yes something like this will work. Just curious will this impact the idle
paths ? By that I mean will the presence of additional domains add more
work or will they be skipped as early as possible with just one additional
check ?

-- 
Regards,
Sudeep

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Rob Herring]

On Fri, Jul 21, 2023 at 12:55:35PM +0100, Sudeep Holla wrote:
> On Fri, Jul 21, 2023 at 01:42:43PM +0200, Ulf Hansson wrote:
> > On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > > > the "power-domain" DT bindings, so let's use that.
> > > >
> > >
> > > One thing I forgot to ask earlier is how we can manage different domain IDs
> > > for perf and power domains which is the case with current SCMI platforms as
> > > the spec never mandated or can ever mandate the perf and power domains IDs
> > > to match. They need not be same anyways.
> > 
> > Based upon what you describe above, I have modelled the perf-domain
> > and the power-domain as two separate power-domain providers.
> > 
> > A consumer device being hooked up to both domains, would specify the
> > domain IDs in the second power-domain-cell, along the lines of the
> > below. Then we would use power-domain-names to specify what each
> > power-domain represents.
> > 
> > power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
> > power-domain-names = "power", "perf";
> >
> > I hope this makes it clearer!?
> 
> Yes it make is clear definitely, but it does change the definition of the
> generic binding of the "power-domains" property now. I am interesting in
> the feedback from the binding maintainers with respect to that. Or is it
> already present ? IIUC, the ones supported already are generally both
> power and performance providers. May be it doesn't matter much, just
> wanted to explicit ask and confirm those details.

I commented on v1.

Looks like abuse of "power-domains" to me, but nothing new really. 
Please define when to use a power domain vs. a perf domain and don't 
leave it up to the whims of the platform. Maybe perf domains was a 
mistake and they should be deprecated?

Rob

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Rob Herring]

On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > The performance domain-id can be described in DT using the power-domains
> > > property or the clock property. The latter is already supported, so let's
> > > add support for the power-domains too.
> > >
> >
> > How is this supposed to work for the CPUs ? The CPU power domains are
> > generally PSCI on most of the platforms and the one using OSI explicitly
> > need to specify the details while ones using PC will not need to. Also they
> > can never be performance domains too. So I am not sure if I am following this
> > correctly.
> 
> Your concerns are certainly correct, I completely forgot about this.
> We need to specify what power-domain index belongs to what, by using
> power-domain-names in DT. So a CPU node, that has both psci for power
> and scmi for performance would then typically look like this:
> 
> power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> power-domain-names = "psci", "scmi";

That is completely backwards. Entries are named based on the consumer 
side. The function of each clock or interrupt for example. Here your 
entries are based on the provider which should be opaque to the 
consumer.

Rob

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Ulf Hansson]

Hi Cristian,

On Wed, 19 Jul 2023 at 16:51, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
> > 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 isn't part of this change, but
> > needs to be implemented from subsequent changes.
> >
>
> Hi Ulf,
>
> a couple of remarks down below.
>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v2:
> >       - Converted to use the new ->domain_info_get() callback.
> >
> > ---
> >  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
>
> [snip]
>
> > +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->info = perf_ops->domain_info_get(ph, i);
>
> So here you are grabbing all the performance domains exposed by the
> platform via PERF protocol and then a few lines down below you are
> registering them with pm_genpd_init(), but the list of domains obtained
> from the platform will contain NOT only devices but also CPUs possibly,
> already managed by the SCMI CPUFreq driver.

Correct.

>
> In fact the SCMI CPUFreq driver, on his side, takes care to pick only
> domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
> parsing) but here you are registering all domains with GenPD upfront.

Right, genpds are acting as providers, which need to be registered
upfront to allow consumer devices to be attached when they get probed.

This isn't specific to this case, but how the genpd infrastructure is
working per design.

>
> Is it not possible that, once registered, GenPD can decide, at some point
> in the future, to try act on some of these domains associated with a CPU ?
> (like Clock framework does at the end of boot trying to disable unused
>  clocks...not familiar with internals of GenPD, though)

The "magic" that exists in genpd is to save/restore the performance
state at genpd_runtime_suspend|resume().

That means the consumer device needs to be attached and runtime PM
enabled, otherwise genpd will just leave the performance level
unchanged. In other words, the control is entirely at the consumer
driver (scmi cpufreq driver).

>
> > +             scmi_pd->domain_id = i;
> > +             scmi_pd->perf_ops = perf_ops;
> > +             scmi_pd->ph = ph;
> > +             scmi_pd->genpd.name = scmi_pd->info->name;
> > +             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);
>
> In SCMI world PERF levels should have nothing to do with the Power
> state of a domain: you have the POWER protocol for that, so you should
> not assume that perf level 0 means OFF, but you can use the POWER protocol
> operation .state_get() to lookup the power state. (and you can grab both
> perf and power ops from the same driver)

Well, I think this may be SCMI FW implementation specific, but
honestly I don't know exactly what the spec says about this. In any
case, I don't think it's a good idea to mix this with the POWER
domain, as that's something that is entirely different. We have no
clue of those relationships (domain IDs).

My main idea behind this, is just to give the genpd internals a
reasonably defined value for its power state.

>
> The tricky part would be to match the PERF domain at hand with the
> related POWER domain to query the state for, I suppose.
>
> Indeed, recently, while looking at SCMI v3.2 PERF evolutions, I was
> tempted to just start rejecting any level_set() or set_freq() request
> for ZERO since they really can be abused to power off a domain. (if the
> platform complies...)

I didn't know that this was against the spec, but in a way what you
say, seems reasonable.

>
> Apologize if I missed something about how GenPD behaviour...

Np, thanks a lot for reviewing! Much appreciated!

>
> Thanks,
> Cristian

Kind regards
Uffe

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Fri, Jul 21, 2023 at 08:33:04AM -0600, Rob Herring wrote:
> On Fri, Jul 21, 2023 at 12:55:35PM +0100, Sudeep Holla wrote:
> > On Fri, Jul 21, 2023 at 01:42:43PM +0200, Ulf Hansson wrote:
> > > On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > > > > the "power-domain" DT bindings, so let's use that.
> > > > >
> > > >
> > > > One thing I forgot to ask earlier is how we can manage different domain IDs
> > > > for perf and power domains which is the case with current SCMI platforms as
> > > > the spec never mandated or can ever mandate the perf and power domains IDs
> > > > to match. They need not be same anyways.
> > > 
> > > Based upon what you describe above, I have modelled the perf-domain
> > > and the power-domain as two separate power-domain providers.
> > > 
> > > A consumer device being hooked up to both domains, would specify the
> > > domain IDs in the second power-domain-cell, along the lines of the
> > > below. Then we would use power-domain-names to specify what each
> > > power-domain represents.
> > > 
> > > power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
> > > power-domain-names = "power", "perf";
> > >
> > > I hope this makes it clearer!?
> > 
> > Yes it make is clear definitely, but it does change the definition of the
> > generic binding of the "power-domains" property now. I am interesting in
> > the feedback from the binding maintainers with respect to that. Or is it
> > already present ? IIUC, the ones supported already are generally both
> > power and performance providers. May be it doesn't matter much, just
> > wanted to explicit ask and confirm those details.
> 
> I commented on v1.
> 
> Looks like abuse of "power-domains" to me, but nothing new really. 
> Please define when to use a power domain vs. a perf domain and don't 
> leave it up to the whims of the platform. Maybe perf domains was a 
> mistake and they should be deprecated?
> 

Just a thought here, instead of deprecating it I was thinking if possible
to keep the power-domains and performance-domains separate and just extend
the genpd to handle the latter. There by we are not mixing up and creating
confusions that need more specific definitions in the binding(which is not
a big deal) but platforms getting it wrong inspite of that is a big problem.
Keep it separate makes it more aligned to the hardware and doesn't dilute
the definitions and probably avoids any possible mistakes due to that.

Sorry Ulf I am just not yet convinced to mix them up yet 😉 and wish you
don't convince me to. Let me know why the above suggestion won't work.

-- 
Regards,
Sudeep

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Fri, 21 Jul 2023 at 20:38, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Jul 21, 2023 at 08:33:04AM -0600, Rob Herring wrote:
> > On Fri, Jul 21, 2023 at 12:55:35PM +0100, Sudeep Holla wrote:
> > > On Fri, Jul 21, 2023 at 01:42:43PM +0200, Ulf Hansson wrote:
> > > > On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > > > > > the "power-domain" DT bindings, so let's use that.
> > > > > >
> > > > >
> > > > > One thing I forgot to ask earlier is how we can manage different domain IDs
> > > > > for perf and power domains which is the case with current SCMI platforms as
> > > > > the spec never mandated or can ever mandate the perf and power domains IDs
> > > > > to match. They need not be same anyways.
> > > >
> > > > Based upon what you describe above, I have modelled the perf-domain
> > > > and the power-domain as two separate power-domain providers.
> > > >
> > > > A consumer device being hooked up to both domains, would specify the
> > > > domain IDs in the second power-domain-cell, along the lines of the
> > > > below. Then we would use power-domain-names to specify what each
> > > > power-domain represents.
> > > >
> > > > power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
> > > > power-domain-names = "power", "perf";
> > > >
> > > > I hope this makes it clearer!?
> > >
> > > Yes it make is clear definitely, but it does change the definition of the
> > > generic binding of the "power-domains" property now. I am interesting in
> > > the feedback from the binding maintainers with respect to that. Or is it
> > > already present ? IIUC, the ones supported already are generally both
> > > power and performance providers. May be it doesn't matter much, just
> > > wanted to explicit ask and confirm those details.
> >
> > I commented on v1.
> >
> > Looks like abuse of "power-domains" to me, but nothing new really.
> > Please define when to use a power domain vs. a perf domain and don't
> > leave it up to the whims of the platform. Maybe perf domains was a
> > mistake and they should be deprecated?
> >
>
> Just a thought here, instead of deprecating it I was thinking if possible
> to keep the power-domains and performance-domains separate and just extend
> the genpd to handle the latter. There by we are not mixing up and creating
> confusions that need more specific definitions in the binding(which is not
> a big deal) but platforms getting it wrong inspite of that is a big problem.
> Keep it separate makes it more aligned to the hardware and doesn't dilute
> the definitions and probably avoids any possible mistakes due to that.
>
> Sorry Ulf I am just not yet convinced to mix them up yet 😉 and wish you
> don't convince me to. Let me know why the above suggestion won't work.

The main point I think we need to consider too, is that on some
platforms, the power-domain and the performance-domain are managed
together by the FW. It is not really two separate things and hence it
would not quite be correct to describe it as two different types of
providers in DT.

If we should follow your suggestion above, to use the
performance-domain bindings, then I think we need an additional new
binding to cover the above mentioned case too. This would lead us into
having one binding for the power-domain, another for the
performance-domain and a third for the power+performance-domain.

In my opinion this sounds quite like a mess. I would rather keep using
the power-domain bindings for all these cases. Of course, it's a bit
of a stretch too, but I think it should be less confusing in the end,
assuming we extend/clarify the description of the power-domain
bindings, of course.

Did that convince you? :-)

Kind regards
Uffe

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Ulf Hansson]

On Fri, 21 Jul 2023 at 16:37, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > The performance domain-id can be described in DT using the power-domains
> > > > property or the clock property. The latter is already supported, so let's
> > > > add support for the power-domains too.
> > > >
> > >
> > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > need to specify the details while ones using PC will not need to. Also they
> > > can never be performance domains too. So I am not sure if I am following this
> > > correctly.
> >
> > Your concerns are certainly correct, I completely forgot about this.
> > We need to specify what power-domain index belongs to what, by using
> > power-domain-names in DT. So a CPU node, that has both psci for power
> > and scmi for performance would then typically look like this:
> >
> > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > power-domain-names = "psci", "scmi";
>
> That is completely backwards. Entries are named based on the consumer
> side. The function of each clock or interrupt for example. Here your
> entries are based on the provider which should be opaque to the
> consumer.

Okay, so you would rather prefer something along the lines of the below?

power-domain-names = "power", "perf";

The "psci" name is already part of the current cpus DT binding
(Documentation/devicetree/bindings/arm/cpus.yaml), so then it looks
like that deserves an update too. Right?

Kind regards
Uffe

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Ulf Hansson]

On Fri, 21 Jul 2023 at 13:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > The performance domain-id can be described in DT using the power-domains
> > > > property or the clock property. The latter is already supported, so let's
> > > > add support for the power-domains too.
> > > >
> > >
> > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > need to specify the details while ones using PC will not need to. Also they
> > > can never be performance domains too. So I am not sure if I am following this
> > > correctly.
> >
> > Your concerns are certainly correct, I completely forgot about this.
> > We need to specify what power-domain index belongs to what, by using
> > power-domain-names in DT. So a CPU node, that has both psci for power
> > and scmi for performance would then typically look like this:
> >
> > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > power-domain-names = "psci", "scmi";
> >
> > I will take care of this in the next version - and thanks a lot for
> > pointing this out!
>
>
> Yes something like this will work. Just curious will this impact the idle
> paths ? By that I mean will the presence of additional domains add more
> work or will they be skipped as early as possible with just one additional
> check ?

Unless I misunderstand your concern, I don't think there is any impact
on the idle path whatsoever. This should be entirely orthogonal.

The scmi-cpufreq driver should only have to care about the
scmi-performance domain, while the cpuidle-psci driver cares only
about psci.

Did that make sense?

Kind regards
Uffe

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Ulf Hansson]

On Wed, 19 Jul 2023 at 17:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jul 19, 2023 at 03:51:45PM +0100, Cristian Marussi wrote:
> > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
>
> [...]
>
> > > +   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->info = perf_ops->domain_info_get(ph, i);
> >
> > So here you are grabbing all the performance domains exposed by the
> > platform via PERF protocol and then a few lines down below you are
> > registering them with pm_genpd_init(), but the list of domains obtained
> > from the platform will contain NOT only devices but also CPUs possibly,
> > already managed by the SCMI CPUFreq driver.
> >
>
> Agreed, I pointed out briefly in the previous patch I think. I am not sure
> how will that work if the performance and power domains are not 1-1 mapping
> or if they are CPUs then this might confusing ? Not sure but looks like
> we might be creating a spaghetti here :(.

I assume the discussions around the DT bindings are making it more
clear on how this should work. The scmi performance-domain and the
scmi power-domain are two separate providers.

>
> > In fact the SCMI CPUFreq driver, on his side, takes care to pick only
> > domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
> > parsing) but here you are registering all domains with GenPD upfront.
> >
>
> +1
>
> > Is it not possible that, once registered, GenPD can decide, at some point
> > in the future, to try act on some of these domains associated with a CPU ?
>
> IIRC, all unused genpd are turned off right. It may not impact here but still
> super confusing as we will be creating power domains for the set of domains
> actually advertised as power domains by the firmware. This will add another
> set.
>
> > (like Clock framework does at the end of boot trying to disable unused
> >  clocks...not familiar with internals of GenPD, though)
> >
>
> Ah, I am reading too much serialised. Just agreed and wrote the same above.
>
> > > +           scmi_pd->domain_id = i;
> > > +           scmi_pd->perf_ops = perf_ops;
> > > +           scmi_pd->ph = ph;
> > > +           scmi_pd->genpd.name = scmi_pd->info->name;
> > > +           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);
> >
> > In SCMI world PERF levels should have nothing to do with the Power
> > state of a domain: you have the POWER protocol for that, so you should
> > not assume that perf level 0 means OFF, but you can use the POWER protocol
> > operation .state_get() to lookup the power state. (and you can grab both
> > perf and power ops from the same driver)
> >
> > The tricky part would be to match the PERF domain at hand with the
> > related POWER domain to query the state for, I suppose.
> >
>
> I wanted to ask the same. E.g. on juno, GPU has perf domain 2 and power domain
> 9. It would be good if we can how it would work there ? What is expected
> from the gpu driver in terms of managing perf and power ? Does it need
> to specify 2 power domains now and specify which is perf and which power in
> its bindings ?

Yes, correct.

Note that, we already have plenty of consumer devices/drivers that are
managing multiple PM domains. They use
dev_pm_domain_attach_by_id|name() to attach their devices to their
corresponding domain(s). In addition, they often use device_link_add()
to simplify runtime PM management.

That said, we should expect to see some boilerplate code in consumer
drivers that deals with this attaching/detaching of multiple PM
domains. That's a separate problem we may want to address later on. In
fact, it's already been discussed earlier at LKML (I can't find the
link right now).

[...]

Kind regards
Uffe

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Cristian Marussi]

On Fri, Jul 21, 2023 at 05:19:51PM +0200, Ulf Hansson wrote:
> Hi Cristian,
> 

Hi,

> On Wed, 19 Jul 2023 at 16:51, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
> > > 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 isn't part of this change, but
> > > needs to be implemented from subsequent changes.
> > >
> >
> > Hi Ulf,
> >
> > a couple of remarks down below.
> >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > >       - Converted to use the new ->domain_info_get() callback.
> > >
> > > ---
> > >  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
> >
> > [snip]
> >
> > > +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->info = perf_ops->domain_info_get(ph, i);
> >
> > So here you are grabbing all the performance domains exposed by the
> > platform via PERF protocol and then a few lines down below you are
> > registering them with pm_genpd_init(), but the list of domains obtained
> > from the platform will contain NOT only devices but also CPUs possibly,
> > already managed by the SCMI CPUFreq driver.
> 
> Correct.
> 
> >
> > In fact the SCMI CPUFreq driver, on his side, takes care to pick only
> > domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
> > parsing) but here you are registering all domains with GenPD upfront.
> 
> Right, genpds are acting as providers, which need to be registered
> upfront to allow consumer devices to be attached when they get probed.
> 
> This isn't specific to this case, but how the genpd infrastructure is
> working per design.
> 
> >
> > Is it not possible that, once registered, GenPD can decide, at some point
> > in the future, to try act on some of these domains associated with a CPU ?
> > (like Clock framework does at the end of boot trying to disable unused
> >  clocks...not familiar with internals of GenPD, though)
> 
> The "magic" that exists in genpd is to save/restore the performance
> state at genpd_runtime_suspend|resume().
> 
> That means the consumer device needs to be attached and runtime PM
> enabled, otherwise genpd will just leave the performance level
> unchanged. In other words, the control is entirely at the consumer
> driver (scmi cpufreq driver).
> 

Ok, so if the DT is well formed and a CPU-related perf domain is not
wrongly referred from a driver looking for a device perf-domain, the
genPD subsystem wont act on the CPUs domains.

> >
> > > +             scmi_pd->domain_id = i;
> > > +             scmi_pd->perf_ops = perf_ops;
> > > +             scmi_pd->ph = ph;
> > > +             scmi_pd->genpd.name = scmi_pd->info->name;
> > > +             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);
> >
> > In SCMI world PERF levels should have nothing to do with the Power
> > state of a domain: you have the POWER protocol for that, so you should
> > not assume that perf level 0 means OFF, but you can use the POWER protocol
> > operation .state_get() to lookup the power state. (and you can grab both
> > perf and power ops from the same driver)
> 
> Well, I think this may be SCMI FW implementation specific, but
> honestly I don't know exactly what the spec says about this. In any
> case, I don't think it's a good idea to mix this with the POWER
> domain, as that's something that is entirely different. We have no
> clue of those relationships (domain IDs).
> 
> My main idea behind this, is just to give the genpd internals a
> reasonably defined value for its power state.
> 

The thing is that in the SCMI world you cannot assume that perf_level 0
means powered off, the current SCP/SCMI platform fw, as an example, wont
advertise a 0-perf-level and wont act on such a request, because you are
supposed to use POWER protocol to get/set the power-state of a device.

So it could be fine, as long as genPD wont try to set the level to 0
expecting the domain to be as a consequence also powered off and as
long as it is fine for these genpd domains to be always initialized
as ON. (since perf_level could never be found as zero..)

Thanks,
Cristian

Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain

[Ulf Hansson]

[...]

> > >
> > > Is it not possible that, once registered, GenPD can decide, at some point
> > > in the future, to try act on some of these domains associated with a CPU ?
> > > (like Clock framework does at the end of boot trying to disable unused
> > >  clocks...not familiar with internals of GenPD, though)
> >
> > The "magic" that exists in genpd is to save/restore the performance
> > state at genpd_runtime_suspend|resume().
> >
> > That means the consumer device needs to be attached and runtime PM
> > enabled, otherwise genpd will just leave the performance level
> > unchanged. In other words, the control is entirely at the consumer
> > driver (scmi cpufreq driver).
> >
>
> Ok, so if the DT is well formed and a CPU-related perf domain is not
> wrongly referred from a driver looking for a device perf-domain, the
> genPD subsystem wont act on the CPUs domains.

Yes, correct!

>
> > >
> > > > +             scmi_pd->domain_id = i;
> > > > +             scmi_pd->perf_ops = perf_ops;
> > > > +             scmi_pd->ph = ph;
> > > > +             scmi_pd->genpd.name = scmi_pd->info->name;
> > > > +             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);
> > >
> > > In SCMI world PERF levels should have nothing to do with the Power
> > > state of a domain: you have the POWER protocol for that, so you should
> > > not assume that perf level 0 means OFF, but you can use the POWER protocol
> > > operation .state_get() to lookup the power state. (and you can grab both
> > > perf and power ops from the same driver)
> >
> > Well, I think this may be SCMI FW implementation specific, but
> > honestly I don't know exactly what the spec says about this. In any
> > case, I don't think it's a good idea to mix this with the POWER
> > domain, as that's something that is entirely different. We have no
> > clue of those relationships (domain IDs).
> >
> > My main idea behind this, is just to give the genpd internals a
> > reasonably defined value for its power state.
> >
>
> The thing is that in the SCMI world you cannot assume that perf_level 0
> means powered off, the current SCP/SCMI platform fw, as an example, wont
> advertise a 0-perf-level and wont act on such a request, because you are
> supposed to use POWER protocol to get/set the power-state of a device.

Right, thanks for clarifying this!

>
> So it could be fine, as long as genPD wont try to set the level to 0
> expecting the domain to be as a consequence also powered off and as
> long as it is fine for these genpd domains to be always initialized
> as ON. (since perf_level could never be found as zero..)

Okay, so to me, it sounds like that we should set GENPD_FLAG_ALWAYS_ON
for the genpd. This makes it clear that the domain can't be powered
off.

Moreover, we should prevent genpd internals from setting the
performance state to 0 for the scmi performance domain. Let me look
into how to best deal with that.

Kind regards
Uffe

Re: [PATCH v2 09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

[Ulf Hansson]

On Wed, 26 Jul 2023 at 13:20, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 21 Jul 2023 at 16:37, Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > > The performance domain-id can be described in DT using the power-domains
> > > > > property or the clock property. The latter is already supported, so let's
> > > > > add support for the power-domains too.
> > > > >
> > > >
> > > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > > need to specify the details while ones using PC will not need to. Also they
> > > > can never be performance domains too. So I am not sure if I am following this
> > > > correctly.
> > >
> > > Your concerns are certainly correct, I completely forgot about this.
> > > We need to specify what power-domain index belongs to what, by using
> > > power-domain-names in DT. So a CPU node, that has both psci for power
> > > and scmi for performance would then typically look like this:
> > >
> > > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > > power-domain-names = "psci", "scmi";
> >
> > That is completely backwards. Entries are named based on the consumer
> > side. The function of each clock or interrupt for example. Here your
> > entries are based on the provider which should be opaque to the
> > consumer.
>
> Okay, so you would rather prefer something along the lines of the below?
>
> power-domain-names = "power", "perf";
>
> The "psci" name is already part of the current cpus DT binding
> (Documentation/devicetree/bindings/arm/cpus.yaml), so then it looks
> like that deserves an update too. Right?

Rob, may I have your thoughts around this? Is this an acceptable way
forward? Please advise me on what power-domain-names I should use
then.

Or, if you are insisting on using the performance-domain bindings?

Kind regards
Uffe

Re: [PATCH v2 08/11] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Wed, 26 Jul 2023 at 13:12, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 21 Jul 2023 at 20:38, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Jul 21, 2023 at 08:33:04AM -0600, Rob Herring wrote:
> > > On Fri, Jul 21, 2023 at 12:55:35PM +0100, Sudeep Holla wrote:
> > > > On Fri, Jul 21, 2023 at 01:42:43PM +0200, Ulf Hansson wrote:
> > > > > On Wed, 19 Jul 2023 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 13, 2023 at 04:17:35PM +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 domaintoo. The common way to describe this, is to use
> > > > > > > the "power-domain" DT bindings, so let's use that.
> > > > > > >
> > > > > >
> > > > > > One thing I forgot to ask earlier is how we can manage different domain IDs
> > > > > > for perf and power domains which is the case with current SCMI platforms as
> > > > > > the spec never mandated or can ever mandate the perf and power domains IDs
> > > > > > to match. They need not be same anyways.
> > > > >
> > > > > Based upon what you describe above, I have modelled the perf-domain
> > > > > and the power-domain as two separate power-domain providers.
> > > > >
> > > > > A consumer device being hooked up to both domains, would specify the
> > > > > domain IDs in the second power-domain-cell, along the lines of the
> > > > > below. Then we would use power-domain-names to specify what each
> > > > > power-domain represents.
> > > > >
> > > > > power-domains = <&scmi_pd 2>, <&scmi_dvfs 4>;
> > > > > power-domain-names = "power", "perf";
> > > > >
> > > > > I hope this makes it clearer!?
> > > >
> > > > Yes it make is clear definitely, but it does change the definition of the
> > > > generic binding of the "power-domains" property now. I am interesting in
> > > > the feedback from the binding maintainers with respect to that. Or is it
> > > > already present ? IIUC, the ones supported already are generally both
> > > > power and performance providers. May be it doesn't matter much, just
> > > > wanted to explicit ask and confirm those details.
> > >
> > > I commented on v1.
> > >
> > > Looks like abuse of "power-domains" to me, but nothing new really.
> > > Please define when to use a power domain vs. a perf domain and don't
> > > leave it up to the whims of the platform. Maybe perf domains was a
> > > mistake and they should be deprecated?
> > >
> >
> > Just a thought here, instead of deprecating it I was thinking if possible
> > to keep the power-domains and performance-domains separate and just extend
> > the genpd to handle the latter. There by we are not mixing up and creating
> > confusions that need more specific definitions in the binding(which is not
> > a big deal) but platforms getting it wrong inspite of that is a big problem.
> > Keep it separate makes it more aligned to the hardware and doesn't dilute
> > the definitions and probably avoids any possible mistakes due to that.
> >
> > Sorry Ulf I am just not yet convinced to mix them up yet 😉 and wish you
> > don't convince me to. Let me know why the above suggestion won't work.
>
> The main point I think we need to consider too, is that on some
> platforms, the power-domain and the performance-domain are managed
> together by the FW. It is not really two separate things and hence it
> would not quite be correct to describe it as two different types of
> providers in DT.
>
> If we should follow your suggestion above, to use the
> performance-domain bindings, then I think we need an additional new
> binding to cover the above mentioned case too. This would lead us into
> having one binding for the power-domain, another for the
> performance-domain and a third for the power+performance-domain.
>
> In my opinion this sounds quite like a mess. I would rather keep using
> the power-domain bindings for all these cases. Of course, it's a bit
> of a stretch too, but I think it should be less confusing in the end,
> assuming we extend/clarify the description of the power-domain
> bindings, of course.
>
> Did that convince you? :-)

Sudeep, Rob,

Can we try to conclude on the way forward?

Is it acceptable to keep using the power-domain bindings (with some
clarifications) for performance domains or should we start moving to
the performance-domain bindings?

If moving to the performance-domain binding, should we start migrating
existing users of the power-domain binding too - or what is your take
on this?

Kind regards
Uffe