[PATCH v3 0/9] SCMIv3.1 Powercap protocol and driver

[PATCH v3 0/9] SCMIv3.1 Powercap protocol and driver

[Cristian Marussi]

Hi all,

this short series introduces the last missing bit of SCMIv3.1, Powercap
protocol. Along the series, there is a small refactoring around the SCMI
FastChannels handling routines so as to reuse as much as possible the
pre-existent (and tested) FastChannel code from the Perf protocol.

New SCMI FC tracing support is added too along the way.

As a last step in the series an ARM SCMI based powercap driver is added,
which takes care to expose via the Powercap framework all the SCMI Powercap
zones that have been discovered asking the SCMI platform firmware.

Basic testing has been performed against an emulated SCMI platform
supporting SCMIv3.1 Powercap protocol using powercap-utils, with the
exclusion of the FCs bits whose generalization has been only tested for
regression on a JUNO platform sporting a regular SCP/SCMI v2.10 fw.

The series is based on sudeep/for-next/scmi [1] on top of:

commit 754f04cac362 ("firmware: arm_scmi: Relax CLOCK_DESCRIBE_RATES out-of-spec checks")

Thanks,
Cristian

v2 --> v3:
- added and used some SCMI Fastchannel tracing support
- reverted logic of Kconfig to configure usage of SCMI FC
- using strscpy with new SHORT_NAME_SZ in Powercap protocol
- added devm_protocol_acquire helper

v1 --> v2:
- fixed measurements thresholds updates to trigger notification
  enable update commands
- added a bit more comments
- usig bitfield.h macros
- fixed sparse complaint about missing static on global

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi
----



Cristian Marussi (9):
  dt-bindings: firmware: arm,scmi: Add powercap protocol
  firmware: arm_scmi: Add SCMIv3.1 Powercap protocol basic support
  firmware: arm_scmi: Generalize FastChannel support
  firmware: arm_scmi: Add SCMIv3.1 Powercap FastChannels support
  firmware: arm_scmi: Make use of FastChannels configurable
  include: trace: Add SCMI FastChannel tracing
  firmware: arm_scmi: Use FastChannel tracing
  firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks
  powercap: arm_scmi: Add SCMI Powercap based driver

 .../bindings/firmware/arm,scmi.yaml           |  10 +
 drivers/firmware/arm_scmi/Kconfig             |  13 +
 drivers/firmware/arm_scmi/Makefile            |   2 +-
 drivers/firmware/arm_scmi/bus.c               |  15 +-
 drivers/firmware/arm_scmi/driver.c            | 173 ++++
 drivers/firmware/arm_scmi/perf.c              | 225 ++---
 drivers/firmware/arm_scmi/powercap.c          | 866 ++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h         |  23 +
 drivers/powercap/Kconfig                      |  13 +
 drivers/powercap/Makefile                     |   1 +
 drivers/powercap/arm_scmi_powercap.c          | 537 +++++++++++
 include/linux/scmi_protocol.h                 | 129 +++
 include/trace/events/scmi.h                   |  25 +
 13 files changed, 1858 insertions(+), 174 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/powercap.c
 create mode 100644 drivers/powercap/arm_scmi_powercap.c

-- 
2.32.0

[PATCH v3 1/9] dt-bindings: firmware: arm,scmi: Add powercap protocol

[Cristian Marussi]

Add new SCMIv3.1 Powercap protocol bindings definitions and example.

Acked-by: Rob Herring <robh@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 948e2a38beed..1c0388da6721 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -183,6 +183,12 @@ properties:
             required:
               - reg
 
+  protocol@18:
+    type: object
+    properties:
+      reg:
+        const: 0x18
+
 additionalProperties: false
 
 patternProperties:
@@ -323,6 +329,10 @@ examples:
                     };
                 };
             };
+
+            scmi_powercap: protocol@18 {
+                reg = <0x18>;
+            };
         };
     };
 
-- 
2.32.0

[PATCH v3 2/9] firmware: arm_scmi: Add SCMIv3.1 Powercap protocol basic support

[Cristian Marussi]

Add support for SCMIv3.1 Powercap protocol, with the exception of Powercap
FastChannels, exposing all the new related Powercap protocol operations as
usual in include/linux/scmi_protocol.h.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- use strscpy

v1 --> v2:
- fixed measurements thresholds updates to trigger notification
  enable update commands
- added a bit more comments
- using bitfield macros
---
 drivers/firmware/arm_scmi/Makefile    |   2 +-
 drivers/firmware/arm_scmi/driver.c    |   2 +
 drivers/firmware/arm_scmi/powercap.c  | 753 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h |   1 +
 include/linux/scmi_protocol.h         | 125 +++++
 5 files changed, 882 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/powercap.c

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 8d4afadda38c..a02dc8ce5a7f 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -7,7 +7,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
 scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
 scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
 		    $(scmi-transport-y)
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 8b7ac6663d57..6ba1faaf5422 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2401,6 +2401,7 @@ static int __init scmi_driver_init(void)
 	scmi_sensors_register();
 	scmi_voltage_register();
 	scmi_system_register();
+	scmi_powercap_register();
 
 	return platform_driver_register(&scmi_driver);
 }
@@ -2417,6 +2418,7 @@ static void __exit scmi_driver_exit(void)
 	scmi_sensors_unregister();
 	scmi_voltage_unregister();
 	scmi_system_unregister();
+	scmi_powercap_unregister();
 
 	scmi_bus_exit();
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
new file mode 100644
index 000000000000..126855905e2d
--- /dev/null
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -0,0 +1,753 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Powercap Protocol
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+
+#define pr_fmt(fmt) "SCMI Notifications POWERCAP - " fmt
+
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+
+#include "protocols.h"
+#include "notify.h"
+
+enum scmi_powercap_protocol_cmd {
+	POWERCAP_DOMAIN_ATTRIBUTES = 0x3,
+	POWERCAP_CAP_GET = 0x4,
+	POWERCAP_CAP_SET = 0x5,
+	POWERCAP_PAI_GET = 0x6,
+	POWERCAP_PAI_SET = 0x7,
+	POWERCAP_DOMAIN_NAME_GET = 0x8,
+	POWERCAP_MEASUREMENTS_GET = 0x9,
+	POWERCAP_CAP_NOTIFY = 0xa,
+	POWERCAP_MEASUREMENTS_NOTIFY = 0xb,
+	POWERCAP_DESCRIBE_FASTCHANNEL = 0xc,
+};
+
+struct scmi_msg_resp_powercap_domain_attributes {
+	__le32 attributes;
+#define SUPPORTS_POWERCAP_CAP_CHANGE_NOTIFY(x)		((x) & BIT(31))
+#define SUPPORTS_POWERCAP_MEASUREMENTS_CHANGE_NOTIFY(x)	((x) & BIT(30))
+#define SUPPORTS_ASYNC_POWERCAP_CAP_SET(x)		((x) & BIT(29))
+#define SUPPORTS_EXTENDED_NAMES(x)			((x) & BIT(28))
+#define SUPPORTS_POWERCAP_CAP_CONFIGURATION(x)		((x) & BIT(27))
+#define SUPPORTS_POWERCAP_MONITORING(x)			((x) & BIT(26))
+#define SUPPORTS_POWERCAP_PAI_CONFIGURATION(x)		((x) & BIT(25))
+#define POWERCAP_POWER_UNIT(x)				\
+		(FIELD_GET(GENMASK(24, 23), (x)))
+#define	SUPPORTS_POWER_UNITS_MW(x)			\
+		(POWERCAP_POWER_UNIT(x) == 0x2)
+#define	SUPPORTS_POWER_UNITS_UW(x)			\
+		(POWERCAP_POWER_UNIT(x) == 0x1)
+	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
+	__le32 min_pai;
+	__le32 max_pai;
+	__le32 pai_step;
+	__le32 min_power_cap;
+	__le32 max_power_cap;
+	__le32 power_cap_step;
+	__le32 sustainable_power;
+	__le32 accuracy;
+	__le32 parent_id;
+};
+
+struct scmi_msg_powercap_set_cap_or_pai {
+	__le32 domain;
+	__le32 flags;
+#define CAP_SET_ASYNC		BIT(1)
+#define CAP_SET_IGNORE_DRESP	BIT(0)
+	__le32 value;
+};
+
+struct scmi_msg_resp_powercap_cap_set_complete {
+	__le32 domain;
+	__le32 power_cap;
+};
+
+struct scmi_msg_resp_powercap_meas_get {
+	__le32 power;
+	__le32 pai;
+};
+
+struct scmi_msg_powercap_notify_cap {
+	__le32 domain;
+	__le32 notify_enable;
+};
+
+struct scmi_msg_powercap_notify_thresh {
+	__le32 domain;
+	__le32 notify_enable;
+	__le32 power_thresh_low;
+	__le32 power_thresh_high;
+};
+
+struct scmi_powercap_cap_changed_notify_payld {
+	__le32 agent_id;
+	__le32 domain_id;
+	__le32 power_cap;
+	__le32 pai;
+};
+
+struct scmi_powercap_meas_changed_notify_payld {
+	__le32 agent_id;
+	__le32 domain_id;
+	__le32 power;
+};
+
+struct scmi_powercap_state {
+	bool meas_notif_enabled;
+	u64 thresholds;
+#define THRESH_LOW(p, id)				\
+	(lower_32_bits((p)->states[(id)].thresholds))
+#define THRESH_HIGH(p, id)				\
+	(upper_32_bits((p)->states[(id)].thresholds))
+};
+
+struct powercap_info {
+	u32 version;
+	int num_domains;
+	struct scmi_powercap_state *states;
+	struct scmi_powercap_info *powercaps;
+};
+
+static enum scmi_powercap_protocol_cmd evt_2_cmd[] = {
+	POWERCAP_CAP_NOTIFY,
+	POWERCAP_MEASUREMENTS_NOTIFY,
+};
+
+static int scmi_powercap_notify(const struct scmi_protocol_handle *ph,
+				u32 domain, int message_id, bool enable);
+
+static int
+scmi_powercap_attributes_get(const struct scmi_protocol_handle *ph,
+			     struct powercap_info *pi)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
+				      sizeof(u32), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		u32 attributes;
+
+		attributes = get_unaligned_le32(t->rx.buf);
+		pi->num_domains = FIELD_GET(GENMASK(15, 0), attributes);
+	}
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static inline int
+scmi_powercap_validate(unsigned int min_val, unsigned int max_val,
+		       unsigned int step_val, bool configurable)
+{
+	if (!min_val || !max_val)
+		return -EPROTO;
+
+	if ((configurable && min_val == max_val) ||
+	    (!configurable && min_val != max_val))
+		return -EPROTO;
+
+	if (min_val != max_val && !step_val)
+		return -EPROTO;
+
+	return 0;
+}
+
+static int
+scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
+				    struct powercap_info *pinfo, u32 domain)
+{
+	int ret;
+	u32 flags;
+	struct scmi_xfer *t;
+	struct scmi_powercap_info *dom_info = pinfo->powercaps + domain;
+	struct scmi_msg_resp_powercap_domain_attributes *resp;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_DOMAIN_ATTRIBUTES,
+				      sizeof(domain), sizeof(*resp), &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(domain, t->tx.buf);
+	resp = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		flags = le32_to_cpu(resp->attributes);
+
+		dom_info->id = domain;
+		dom_info->notify_powercap_cap_change =
+			SUPPORTS_POWERCAP_CAP_CHANGE_NOTIFY(flags);
+		dom_info->notify_powercap_measurement_change =
+			SUPPORTS_POWERCAP_MEASUREMENTS_CHANGE_NOTIFY(flags);
+		dom_info->async_powercap_cap_set =
+			SUPPORTS_ASYNC_POWERCAP_CAP_SET(flags);
+		dom_info->powercap_cap_config =
+			SUPPORTS_POWERCAP_CAP_CONFIGURATION(flags);
+		dom_info->powercap_monitoring =
+			SUPPORTS_POWERCAP_MONITORING(flags);
+		dom_info->powercap_pai_config =
+			SUPPORTS_POWERCAP_PAI_CONFIGURATION(flags);
+		dom_info->powercap_scale_mw =
+			SUPPORTS_POWER_UNITS_MW(flags);
+		dom_info->powercap_scale_uw =
+			SUPPORTS_POWER_UNITS_UW(flags);
+
+		strscpy(dom_info->name, resp->name, SCMI_SHORT_NAME_MAX_SIZE);
+
+		dom_info->min_pai = le32_to_cpu(resp->min_pai);
+		dom_info->max_pai = le32_to_cpu(resp->max_pai);
+		dom_info->pai_step = le32_to_cpu(resp->pai_step);
+		ret = scmi_powercap_validate(dom_info->min_pai,
+					     dom_info->max_pai,
+					     dom_info->pai_step,
+					     dom_info->powercap_pai_config);
+		if (ret) {
+			dev_err(ph->dev,
+				"Platform reported inconsistent PAI config for domain %d - %s\n",
+				dom_info->id, dom_info->name);
+			goto clean;
+		}
+
+		dom_info->min_power_cap = le32_to_cpu(resp->min_power_cap);
+		dom_info->max_power_cap = le32_to_cpu(resp->max_power_cap);
+		dom_info->power_cap_step = le32_to_cpu(resp->power_cap_step);
+		ret = scmi_powercap_validate(dom_info->min_power_cap,
+					     dom_info->max_power_cap,
+					     dom_info->power_cap_step,
+					     dom_info->powercap_cap_config);
+		if (ret) {
+			dev_err(ph->dev,
+				"Platform reported inconsistent CAP config for domain %d - %s\n",
+				dom_info->id, dom_info->name);
+			goto clean;
+		}
+
+		dom_info->sustainable_power =
+			le32_to_cpu(resp->sustainable_power);
+		dom_info->accuracy = le32_to_cpu(resp->accuracy);
+
+		dom_info->parent_id = le32_to_cpu(resp->parent_id);
+		if (dom_info->parent_id != SCMI_POWERCAP_ROOT_ZONE_ID &&
+		    (dom_info->parent_id >= pinfo->num_domains ||
+		     dom_info->parent_id == dom_info->id)) {
+			dev_err(ph->dev,
+				"Platform reported inconsistent parent ID for domain %d - %s\n",
+				dom_info->id, dom_info->name);
+			ret = -ENODEV;
+		}
+	}
+
+clean:
+	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
+		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
+					    domain, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
+
+	return ret;
+}
+
+static int scmi_powercap_num_domains_get(const struct scmi_protocol_handle *ph)
+{
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	return pi->num_domains;
+}
+
+static const struct scmi_powercap_info *
+scmi_powercap_dom_info_get(const struct scmi_protocol_handle *ph, u32 domain_id)
+{
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (domain_id >= pi->num_domains)
+		return NULL;
+
+	return pi->powercaps + domain_id;
+}
+
+static int scmi_powercap_cap_get(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 *power_cap)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!power_cap || domain_id >= pi->num_domains)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_CAP_GET, sizeof(u32),
+				      sizeof(u32), &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(domain_id, t->tx.buf);
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret)
+		*power_cap = get_unaligned_le32(t->rx.buf);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 power_cap,
+				 bool ignore_dresp)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_powercap_set_cap_or_pai *msg;
+	const struct scmi_powercap_info *pc;
+
+	pc = scmi_powercap_dom_info_get(ph, domain_id);
+	if (!pc || !pc->powercap_cap_config || !power_cap ||
+	    power_cap < pc->min_power_cap ||
+	    power_cap > pc->max_power_cap)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_CAP_SET,
+				      sizeof(*msg), 0, &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->domain = cpu_to_le32(domain_id);
+	msg->flags =
+		cpu_to_le32(FIELD_PREP(CAP_SET_ASYNC, !!pc->async_powercap_cap_set) |
+			    FIELD_PREP(CAP_SET_IGNORE_DRESP, !!ignore_dresp));
+	msg->value = cpu_to_le32(power_cap);
+
+	if (!pc->async_powercap_cap_set || ignore_dresp) {
+		ret = ph->xops->do_xfer(ph, t);
+	} else {
+		ret = ph->xops->do_xfer_with_response(ph, t);
+		if (!ret) {
+			struct scmi_msg_resp_powercap_cap_set_complete *resp;
+
+			resp = t->rx.buf;
+			if (le32_to_cpu(resp->domain) == domain_id)
+				dev_dbg(ph->dev,
+					"Powercap ID %d CAP set async to %u\n",
+					domain_id,
+					get_unaligned_le32(&resp->power_cap));
+			else
+				ret = -EPROTO;
+		}
+	}
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_powercap_pai_get(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 *pai)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!pai || domain_id >= pi->num_domains)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_PAI_GET, sizeof(u32),
+				      sizeof(u32), &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(domain_id, t->tx.buf);
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret)
+		*pai = get_unaligned_le32(t->rx.buf);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_powercap_pai_set(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 pai)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_powercap_set_cap_or_pai *msg;
+	const struct scmi_powercap_info *pc;
+
+	pc = scmi_powercap_dom_info_get(ph, domain_id);
+	if (!pc || !pc->powercap_pai_config || !pai ||
+	    pai < pc->min_pai || pai > pc->max_pai)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_PAI_SET,
+				      sizeof(*msg), 0, &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->domain = cpu_to_le32(domain_id);
+	msg->flags = cpu_to_le32(0);
+	msg->value = cpu_to_le32(pai);
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_powercap_measurements_get(const struct scmi_protocol_handle *ph,
+					  u32 domain_id, u32 *average_power,
+					  u32 *pai)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_resp_powercap_meas_get *resp;
+	const struct scmi_powercap_info *pc;
+
+	pc = scmi_powercap_dom_info_get(ph, domain_id);
+	if (!pc || !pc->powercap_monitoring || !pai || !average_power)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, POWERCAP_MEASUREMENTS_GET,
+				      sizeof(u32), sizeof(*resp), &t);
+	if (ret)
+		return ret;
+
+	resp = t->rx.buf;
+	put_unaligned_le32(domain_id, t->tx.buf);
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		*average_power = le32_to_cpu(resp->power);
+		*pai = le32_to_cpu(resp->pai);
+	}
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int
+scmi_powercap_measurements_threshold_get(const struct scmi_protocol_handle *ph,
+					 u32 domain_id, u32 *power_thresh_low,
+					 u32 *power_thresh_high)
+{
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!power_thresh_low || !power_thresh_high ||
+	    domain_id >= pi->num_domains)
+		return -EINVAL;
+
+	*power_thresh_low =  THRESH_LOW(pi, domain_id);
+	*power_thresh_high = THRESH_HIGH(pi, domain_id);
+
+	return 0;
+}
+
+static int
+scmi_powercap_measurements_threshold_set(const struct scmi_protocol_handle *ph,
+					 u32 domain_id, u32 power_thresh_low,
+					 u32 power_thresh_high)
+{
+	int ret = 0;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (domain_id >= pi->num_domains ||
+	    power_thresh_low > power_thresh_high)
+		return -EINVAL;
+
+	/* Anything to do ? */
+	if (THRESH_LOW(pi, domain_id) == power_thresh_low &&
+	    THRESH_HIGH(pi, domain_id) == power_thresh_high)
+		return ret;
+
+	pi->states[domain_id].thresholds =
+		(FIELD_PREP(GENMASK(31, 0), power_thresh_low) |
+		 FIELD_PREP(GENMASK(63, 32), power_thresh_high));
+
+	/* Update thresholds if notification already enabled */
+	if (pi->states[domain_id].meas_notif_enabled)
+		ret = scmi_powercap_notify(ph, domain_id,
+					   POWERCAP_MEASUREMENTS_NOTIFY,
+					   true);
+
+	return ret;
+}
+
+static const struct scmi_powercap_proto_ops powercap_proto_ops = {
+	.num_domains_get = scmi_powercap_num_domains_get,
+	.info_get = scmi_powercap_dom_info_get,
+	.cap_get = scmi_powercap_cap_get,
+	.cap_set = scmi_powercap_cap_set,
+	.pai_get = scmi_powercap_pai_get,
+	.pai_set = scmi_powercap_pai_set,
+	.measurements_get = scmi_powercap_measurements_get,
+	.measurements_threshold_set = scmi_powercap_measurements_threshold_set,
+	.measurements_threshold_get = scmi_powercap_measurements_threshold_get,
+};
+
+static int scmi_powercap_notify(const struct scmi_protocol_handle *ph,
+				u32 domain, int message_id, bool enable)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	switch (message_id) {
+	case POWERCAP_CAP_NOTIFY:
+	{
+		struct scmi_msg_powercap_notify_cap *notify;
+
+		ret = ph->xops->xfer_get_init(ph, message_id,
+					      sizeof(*notify), 0, &t);
+		if (ret)
+			return ret;
+
+		notify = t->tx.buf;
+		notify->domain = cpu_to_le32(domain);
+		notify->notify_enable = cpu_to_le32(enable ? BIT(0) : 0);
+		break;
+	}
+	case POWERCAP_MEASUREMENTS_NOTIFY:
+	{
+		u32 low, high;
+		struct scmi_msg_powercap_notify_thresh *notify;
+
+		/*
+		 * Note that we have to pick the most recently configured
+		 * thresholds to build a proper POWERCAP_MEASUREMENTS_NOTIFY
+		 * enable request and we fail, complaining, if no thresholds
+		 * were ever set, since this is an indication the API has been
+		 * used wrongly.
+		 */
+		ret = scmi_powercap_measurements_threshold_get(ph, domain,
+							       &low, &high);
+		if (ret)
+			return ret;
+
+		if (enable && !low && !high) {
+			dev_err(ph->dev,
+				"Invalid Measurements Notify thresholds: %u/%u\n",
+				low, high);
+			return -EINVAL;
+		}
+
+		ret = ph->xops->xfer_get_init(ph, message_id,
+					      sizeof(*notify), 0, &t);
+		if (ret)
+			return ret;
+
+		notify = t->tx.buf;
+		notify->domain = cpu_to_le32(domain);
+		notify->notify_enable = cpu_to_le32(enable ? BIT(0) : 0);
+		notify->power_thresh_low = cpu_to_le32(low);
+		notify->power_thresh_high = cpu_to_le32(high);
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int
+scmi_powercap_set_notify_enabled(const struct scmi_protocol_handle *ph,
+				 u8 evt_id, u32 src_id, bool enable)
+{
+	int ret, cmd_id;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (evt_id >= ARRAY_SIZE(evt_2_cmd) || src_id >= pi->num_domains)
+		return -EINVAL;
+
+	cmd_id = evt_2_cmd[evt_id];
+	ret = scmi_powercap_notify(ph, src_id, cmd_id, enable);
+	if (ret)
+		pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
+			 evt_id, src_id, ret);
+	else if (cmd_id == POWERCAP_MEASUREMENTS_NOTIFY)
+		/*
+		 * On success save the current notification enabled state, so
+		 * as to be able to properly update the notification thresholds
+		 * when they are modified on a domain for which measurement
+		 * notifications were currently enabled.
+		 *
+		 * This is needed because the SCMI Notification core machinery
+		 * and API does not support passing per-notification custom
+		 * arguments at callback registration time.
+		 *
+		 * Note that this can be done here with a simple flag since the
+		 * SCMI core Notifications code takes care of keeping proper
+		 * per-domain enables refcounting, so that this helper function
+		 * will be called only once (for enables) when the first user
+		 * registers a callback on this domain and once more (disable)
+		 * when the last user de-registers its callback.
+		 */
+		pi->states[src_id].meas_notif_enabled = enable;
+
+	return ret;
+}
+
+static void *
+scmi_powercap_fill_custom_report(const struct scmi_protocol_handle *ph,
+				 u8 evt_id, ktime_t timestamp,
+				 const void *payld, size_t payld_sz,
+				 void *report, u32 *src_id)
+{
+	void *rep = NULL;
+
+	switch (evt_id) {
+	case SCMI_EVENT_POWERCAP_CAP_CHANGED:
+	{
+		const struct scmi_powercap_cap_changed_notify_payld *p = payld;
+		struct scmi_powercap_cap_changed_report *r = report;
+
+		if (sizeof(*p) != payld_sz)
+			break;
+
+		r->timestamp = timestamp;
+		r->agent_id = le32_to_cpu(p->agent_id);
+		r->domain_id = le32_to_cpu(p->domain_id);
+		r->power_cap = le32_to_cpu(p->power_cap);
+		r->pai = le32_to_cpu(p->pai);
+		*src_id = r->domain_id;
+		rep = r;
+		break;
+	}
+	case SCMI_EVENT_POWERCAP_MEASUREMENTS_CHANGED:
+	{
+		const struct scmi_powercap_meas_changed_notify_payld *p = payld;
+		struct scmi_powercap_meas_changed_report *r = report;
+
+		if (sizeof(*p) != payld_sz)
+			break;
+
+		r->timestamp = timestamp;
+		r->agent_id = le32_to_cpu(p->agent_id);
+		r->domain_id = le32_to_cpu(p->domain_id);
+		r->power = le32_to_cpu(p->power);
+		*src_id = r->domain_id;
+		rep = r;
+		break;
+	}
+	default:
+		break;
+	}
+
+	return rep;
+}
+
+static int
+scmi_powercap_get_num_sources(const struct scmi_protocol_handle *ph)
+{
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!pi)
+		return -EINVAL;
+
+	return pi->num_domains;
+}
+
+static const struct scmi_event powercap_events[] = {
+	{
+		.id = SCMI_EVENT_POWERCAP_CAP_CHANGED,
+		.max_payld_sz =
+			sizeof(struct scmi_powercap_cap_changed_notify_payld),
+		.max_report_sz =
+			sizeof(struct scmi_powercap_cap_changed_report),
+	},
+	{
+		.id = SCMI_EVENT_POWERCAP_MEASUREMENTS_CHANGED,
+		.max_payld_sz =
+			sizeof(struct scmi_powercap_meas_changed_notify_payld),
+		.max_report_sz =
+			sizeof(struct scmi_powercap_meas_changed_report),
+	},
+};
+
+static const struct scmi_event_ops powercap_event_ops = {
+	.get_num_sources = scmi_powercap_get_num_sources,
+	.set_notify_enabled = scmi_powercap_set_notify_enabled,
+	.fill_custom_report = scmi_powercap_fill_custom_report,
+};
+
+static const struct scmi_protocol_events powercap_protocol_events = {
+	.queue_sz = SCMI_PROTO_QUEUE_SZ,
+	.ops = &powercap_event_ops,
+	.evts = powercap_events,
+	.num_events = ARRAY_SIZE(powercap_events),
+};
+
+static int
+scmi_powercap_protocol_init(const struct scmi_protocol_handle *ph)
+{
+	int domain, ret;
+	u32 version;
+	struct powercap_info *pinfo;
+
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
+
+	dev_dbg(ph->dev, "Powercap Version %d.%d\n",
+		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
+	if (!pinfo)
+		return -ENOMEM;
+
+	ret = scmi_powercap_attributes_get(ph, pinfo);
+	if (ret)
+		return ret;
+
+	pinfo->powercaps = devm_kcalloc(ph->dev, pinfo->num_domains,
+					sizeof(*pinfo->powercaps),
+					GFP_KERNEL);
+	if (!pinfo->powercaps)
+		return -ENOMEM;
+
+	/*
+	 * Note that any failure in retrieving any domain attribute leads to
+	 * the whole Powercap protocol initialization failure: this way the
+	 * reported Powercap domains are all assured, when accessed, to be well
+	 * formed and correlated by sane parent-child relationship (if any).
+	 */
+	for (domain = 0; domain < pinfo->num_domains; domain++) {
+		ret = scmi_powercap_domain_attributes_get(ph, pinfo, domain);
+		if (ret)
+			return ret;
+	}
+
+	pinfo->states = devm_kcalloc(ph->dev, pinfo->num_domains,
+				     sizeof(*pinfo->states), GFP_KERNEL);
+	if (!pinfo->states)
+		return -ENOMEM;
+
+	pinfo->version = version;
+
+	return ph->set_priv(ph, pinfo);
+}
+
+static const struct scmi_protocol scmi_powercap = {
+	.id = SCMI_PROTOCOL_POWERCAP,
+	.owner = THIS_MODULE,
+	.instance_init = &scmi_powercap_protocol_init,
+	.ops = &powercap_proto_ops,
+	.events = &powercap_protocol_events,
+};
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(powercap, scmi_powercap)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 51c31379f9b3..99d36d503d1e 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -315,5 +315,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(reset);
 DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
 DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
 DECLARE_SCMI_REGISTER_UNREGISTER(system);
+DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
 
 #endif /* _SCMI_PROTOCOLS_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 704111f63993..f709c74030f4 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -560,6 +560,114 @@ struct scmi_voltage_proto_ops {
 			 s32 *volt_uV);
 };
 
+/**
+ * struct scmi_powercap_info  - Describe one available Powercap domain
+ *
+ * @id: Domain ID as advertised by the platform.
+ * @notify_powercap_cap_change: CAP change notification support.
+ * @notify_powercap_measurement_change: MEASUREMENTS change notifications
+ *				       support.
+ * @async_powercap_cap_set: Asynchronous CAP set support.
+ * @powercap_cap_config: CAP configuration support.
+ * @powercap_monitoring: Monitoring (measurements) support.
+ * @powercap_pai_config: PAI configuration support.
+ * @powercap_scale_mw: Domain reports power data in milliwatt units.
+ * @powercap_scale_uw: Domain reports power data in microwatt units.
+ *		       Note that, when both @powercap_scale_mw and
+ *		       @powercap_scale_uw are set to false, the domain
+ *		       reports power data on an abstract linear scale.
+ * @name: name assigned to the Powercap Domain by platform.
+ * @min_pai: Minimum configurable PAI.
+ * @max_pai: Maximum configurable PAI.
+ * @pai_step: Step size between two consecutive PAI values.
+ * @min_power_cap: Minimum configurable CAP.
+ * @max_power_cap: Maximum configurable CAP.
+ * @power_cap_step: Step size between two consecutive CAP values.
+ * @sustainable_power: Maximum sustainable power consumption for this domain
+ *		       under normal conditions.
+ * @accuracy: The accuracy with which the power is measured and reported in
+ *	      integral multiples of 0.001 percent.
+ * @parent_id: Identifier of the containing parent power capping domain, or the
+ *	       value 0xFFFFFFFF if this powercap domain is a root domain not
+ *	       contained in any other domain.
+ */
+struct scmi_powercap_info {
+	unsigned int id;
+	bool notify_powercap_cap_change;
+	bool notify_powercap_measurement_change;
+	bool async_powercap_cap_set;
+	bool powercap_cap_config;
+	bool powercap_monitoring;
+	bool powercap_pai_config;
+	bool powercap_scale_mw;
+	bool powercap_scale_uw;
+	char name[SCMI_MAX_STR_SIZE];
+	unsigned int min_pai;
+	unsigned int max_pai;
+	unsigned int pai_step;
+	unsigned int min_power_cap;
+	unsigned int max_power_cap;
+	unsigned int power_cap_step;
+	unsigned int sustainable_power;
+	unsigned int accuracy;
+#define SCMI_POWERCAP_ROOT_ZONE_ID     0xFFFFFFFFUL
+	unsigned int parent_id;
+};
+
+/**
+ * struct scmi_powercap_proto_ops - represents the various operations provided
+ * by SCMI Powercap Protocol
+ *
+ * @num_domains_get: get the count of powercap domains provided by SCMI.
+ * @info_get: get the information for the specified domain.
+ * @cap_get: get the current CAP value for the specified domain.
+ * @cap_set: set the CAP value for the specified domain to the provided value;
+ *	     if the domain supports setting the CAP with an asynchronous command
+ *	     this request will finally trigger an asynchronous transfer, but, if
+ *	     @ignore_dresp here is set to true, this call will anyway return
+ *	     immediately without waiting for the related delayed response.
+ * @pai_get: get the current PAI value for the specified domain.
+ * @pai_set: set the PAI value for the specified domain to the provided value.
+ * @measurements_get: retrieve the current average power measurements for the
+ *		      specified domain and the related PAI upon which is
+ *		      calculated.
+ * @measurements_threshold_set: set the desired low and high power thresholds
+ *				to be used when registering for notification
+ *				of type POWERCAP_MEASUREMENTS_NOTIFY with this
+ *				powercap domain.
+ *				Note that this must be called at least once
+ *				before registering any callback with the usual
+ *				@scmi_notify_ops; moreover, in case this method
+ *				is called with measurement notifications already
+ *				enabled it will also trigger, transparently, a
+ *				proper update of the power thresholds configured
+ *				in the SCMI backend server.
+ * @measurements_threshold_get: get the currently configured low and high power
+ *				thresholds used when registering callbacks for
+ *				notification POWERCAP_MEASUREMENTS_NOTIFY.
+ */
+struct scmi_powercap_proto_ops {
+	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
+	const struct scmi_powercap_info __must_check *(*info_get)
+		(const struct scmi_protocol_handle *ph, u32 domain_id);
+	int (*cap_get)(const struct scmi_protocol_handle *ph, u32 domain_id,
+		       u32 *power_cap);
+	int (*cap_set)(const struct scmi_protocol_handle *ph, u32 domain_id,
+		       u32 power_cap, bool ignore_dresp);
+	int (*pai_get)(const struct scmi_protocol_handle *ph, u32 domain_id,
+		       u32 *pai);
+	int (*pai_set)(const struct scmi_protocol_handle *ph, u32 domain_id,
+		       u32 pai);
+	int (*measurements_get)(const struct scmi_protocol_handle *ph,
+				u32 domain_id, u32 *average_power, u32 *pai);
+	int (*measurements_threshold_set)(const struct scmi_protocol_handle *ph,
+					  u32 domain_id, u32 power_thresh_low,
+					  u32 power_thresh_high);
+	int (*measurements_threshold_get)(const struct scmi_protocol_handle *ph,
+					  u32 domain_id, u32 *power_thresh_low,
+					  u32 *power_thresh_high);
+};
+
 /**
  * struct scmi_notify_ops  - represents notifications' operations provided by
  * SCMI core
@@ -661,6 +769,7 @@ enum scmi_std_protocol {
 	SCMI_PROTOCOL_SENSOR = 0x15,
 	SCMI_PROTOCOL_RESET = 0x16,
 	SCMI_PROTOCOL_VOLTAGE = 0x17,
+	SCMI_PROTOCOL_POWERCAP = 0x18,
 };
 
 enum scmi_system_events {
@@ -762,6 +871,8 @@ enum scmi_notification_events {
 	SCMI_EVENT_RESET_ISSUED = 0x0,
 	SCMI_EVENT_BASE_ERROR_EVENT = 0x0,
 	SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER = 0x0,
+	SCMI_EVENT_POWERCAP_CAP_CHANGED = 0x0,
+	SCMI_EVENT_POWERCAP_MEASUREMENTS_CHANGED = 0x1,
 };
 
 struct scmi_power_state_changed_report {
@@ -830,4 +941,18 @@ struct scmi_base_error_report {
 	unsigned long long	reports[];
 };
 
+struct scmi_powercap_cap_changed_report {
+	ktime_t		timestamp;
+	unsigned int	agent_id;
+	unsigned int	domain_id;
+	unsigned int	power_cap;
+	unsigned int	pai;
+};
+
+struct scmi_powercap_meas_changed_report {
+	ktime_t		timestamp;
+	unsigned int	agent_id;
+	unsigned int	domain_id;
+	unsigned int	power;
+};
 #endif /* _LINUX_SCMI_PROTOCOL_H */
-- 
2.32.0

[PATCH v3 3/9] firmware: arm_scmi: Generalize FastChannel support

[Cristian Marussi]

Generalize existing FastChannel support used in Perf protocol and make it
available to possibly any protocol refactoring the common code into a
couple of new scmi_proto_helpers_ops routines.

Make Perf protocol FC use this new infrastructure.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++
 drivers/firmware/arm_scmi/perf.c      | 215 ++++++--------------------
 drivers/firmware/arm_scmi/protocols.h |  22 +++
 3 files changed, 231 insertions(+), 171 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6ba1faaf5422..00b7f2aff4ec 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -19,6 +19,7 @@
 #include <linux/export.h>
 #include <linux/idr.h>
 #include <linux/io.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/hashtable.h>
@@ -1259,10 +1260,174 @@ static int scmi_iterator_run(void *iter)
 	return ret;
 }
 
+struct scmi_msg_get_fc_info {
+	__le32 domain;
+	__le32 message_id;
+};
+
+struct scmi_msg_resp_desc_fc {
+	__le32 attr;
+#define SUPPORTS_DOORBELL(x)		((x) & BIT(0))
+#define DOORBELL_REG_WIDTH(x)		FIELD_GET(GENMASK(2, 1), (x))
+	__le32 rate_limit;
+	__le32 chan_addr_low;
+	__le32 chan_addr_high;
+	__le32 chan_size;
+	__le32 db_addr_low;
+	__le32 db_addr_high;
+	__le32 db_set_lmask;
+	__le32 db_set_hmask;
+	__le32 db_preserve_lmask;
+	__le32 db_preserve_hmask;
+};
+
+static void
+scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
+			     u8 describe_id, u32 message_id, u32 valid_size,
+			     u32 domain, void __iomem **p_addr,
+			     struct scmi_fc_db_info **p_db)
+{
+	int ret;
+	u32 flags;
+	u64 phys_addr;
+	u8 size;
+	void __iomem *addr;
+	struct scmi_xfer *t;
+	struct scmi_fc_db_info *db = NULL;
+	struct scmi_msg_get_fc_info *info;
+	struct scmi_msg_resp_desc_fc *resp;
+	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+
+	if (!p_addr) {
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+	ret = ph->xops->xfer_get_init(ph, describe_id,
+				      sizeof(*info), sizeof(*resp), &t);
+	if (ret)
+		goto err_out;
+
+	info = t->tx.buf;
+	info->domain = cpu_to_le32(domain);
+	info->message_id = cpu_to_le32(message_id);
+
+	/*
+	 * Bail out on error leaving fc_info addresses zeroed; this includes
+	 * the case in which the requested domain/message_id does NOT support
+	 * fastchannels at all.
+	 */
+	ret = ph->xops->do_xfer(ph, t);
+	if (ret)
+		goto err_xfer;
+
+	resp = t->rx.buf;
+	flags = le32_to_cpu(resp->attr);
+	size = le32_to_cpu(resp->chan_size);
+	if (size != valid_size) {
+		ret = -EINVAL;
+		goto err_xfer;
+	}
+
+	phys_addr = le32_to_cpu(resp->chan_addr_low);
+	phys_addr |= (u64)le32_to_cpu(resp->chan_addr_high) << 32;
+	addr = devm_ioremap(ph->dev, phys_addr, size);
+	if (!addr) {
+		ret = -EADDRNOTAVAIL;
+		goto err_xfer;
+	}
+
+	*p_addr = addr;
+
+	if (p_db && SUPPORTS_DOORBELL(flags)) {
+		db = devm_kzalloc(ph->dev, sizeof(*db), GFP_KERNEL);
+		if (!db) {
+			ret = -ENOMEM;
+			goto err_db;
+		}
+
+		size = 1 << DOORBELL_REG_WIDTH(flags);
+		phys_addr = le32_to_cpu(resp->db_addr_low);
+		phys_addr |= (u64)le32_to_cpu(resp->db_addr_high) << 32;
+		addr = devm_ioremap(ph->dev, phys_addr, size);
+		if (!addr) {
+			ret = -EADDRNOTAVAIL;
+			goto err_db_mem;
+		}
+
+		db->addr = addr;
+		db->width = size;
+		db->set = le32_to_cpu(resp->db_set_lmask);
+		db->set |= (u64)le32_to_cpu(resp->db_set_hmask) << 32;
+		db->mask = le32_to_cpu(resp->db_preserve_lmask);
+		db->mask |= (u64)le32_to_cpu(resp->db_preserve_hmask) << 32;
+
+		*p_db = db;
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	dev_dbg(ph->dev,
+		"Using valid FC for protocol %X [MSG_ID:%u / RES_ID:%u]\n",
+		pi->proto->id, message_id, domain);
+
+	return;
+
+err_db_mem:
+	devm_kfree(ph->dev, db);
+
+err_db:
+	*p_addr = NULL;
+
+err_xfer:
+	ph->xops->xfer_put(ph, t);
+
+err_out:
+	dev_warn(ph->dev,
+		 "Failed to get FC for protocol %X [MSG_ID:%u / RES_ID:%u] - ret:%d. Using regular messaging.\n",
+		 pi->proto->id, message_id, domain, ret);
+}
+
+#define SCMI_PROTO_FC_RING_DB(w)			\
+do {							\
+	u##w val = 0;					\
+							\
+	if (db->mask)					\
+		val = ioread##w(db->addr) & db->mask;	\
+	iowrite##w((u##w)db->set | val, db->addr);	\
+} while (0)
+
+static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
+{
+	if (!db || !db->addr)
+		return;
+
+	if (db->width == 1)
+		SCMI_PROTO_FC_RING_DB(8);
+	else if (db->width == 2)
+		SCMI_PROTO_FC_RING_DB(16);
+	else if (db->width == 4)
+		SCMI_PROTO_FC_RING_DB(32);
+	else /* db->width == 8 */
+#ifdef CONFIG_64BIT
+		SCMI_PROTO_FC_RING_DB(64);
+#else
+	{
+		u64 val = 0;
+
+		if (db->mask)
+			val = ioread64_hi_lo(db->addr) & db->mask;
+		iowrite64_hi_lo(db->set | val, db->addr);
+	}
+#endif
+}
+
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
 	.iter_response_init = scmi_iterator_init,
 	.iter_response_run = scmi_iterator_run,
+	.fastchannel_init = scmi_common_fastchannel_init,
+	.fastchannel_db_ring = scmi_common_fastchannel_db_ring,
 };
 
 /**
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index bbb0331801ff..521458fda355 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -10,7 +10,6 @@
 #include <linux/bits.h>
 #include <linux/of.h>
 #include <linux/io.h>
-#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
@@ -35,6 +34,12 @@ enum scmi_performance_protocol_cmd {
 	PERF_DOMAIN_NAME_GET = 0xc,
 };
 
+enum {
+	PERF_FC_LEVEL,
+	PERF_FC_LIMIT,
+	PERF_FC_MAX,
+};
+
 struct scmi_opp {
 	u32 perf;
 	u32 power;
@@ -115,43 +120,6 @@ struct scmi_msg_resp_perf_describe_levels {
 	} opp[];
 };
 
-struct scmi_perf_get_fc_info {
-	__le32 domain;
-	__le32 message_id;
-};
-
-struct scmi_msg_resp_perf_desc_fc {
-	__le32 attr;
-#define SUPPORTS_DOORBELL(x)		((x) & BIT(0))
-#define DOORBELL_REG_WIDTH(x)		FIELD_GET(GENMASK(2, 1), (x))
-	__le32 rate_limit;
-	__le32 chan_addr_low;
-	__le32 chan_addr_high;
-	__le32 chan_size;
-	__le32 db_addr_low;
-	__le32 db_addr_high;
-	__le32 db_set_lmask;
-	__le32 db_set_hmask;
-	__le32 db_preserve_lmask;
-	__le32 db_preserve_hmask;
-};
-
-struct scmi_fc_db_info {
-	int width;
-	u64 set;
-	u64 mask;
-	void __iomem *addr;
-};
-
-struct scmi_fc_info {
-	void __iomem *level_set_addr;
-	void __iomem *limit_set_addr;
-	void __iomem *level_get_addr;
-	void __iomem *limit_get_addr;
-	struct scmi_fc_db_info *level_set_db;
-	struct scmi_fc_db_info *limit_set_db;
-};
-
 struct perf_dom_info {
 	bool set_limits;
 	bool set_perf;
@@ -360,40 +328,6 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
 	return ret;
 }
 
-#define SCMI_PERF_FC_RING_DB(w)				\
-do {							\
-	u##w val = 0;					\
-							\
-	if (db->mask)					\
-		val = ioread##w(db->addr) & db->mask;	\
-	iowrite##w((u##w)db->set | val, db->addr);	\
-} while (0)
-
-static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
-{
-	if (!db || !db->addr)
-		return;
-
-	if (db->width == 1)
-		SCMI_PERF_FC_RING_DB(8);
-	else if (db->width == 2)
-		SCMI_PERF_FC_RING_DB(16);
-	else if (db->width == 4)
-		SCMI_PERF_FC_RING_DB(32);
-	else /* db->width == 8 */
-#ifdef CONFIG_64BIT
-		SCMI_PERF_FC_RING_DB(64);
-#else
-	{
-		u64 val = 0;
-
-		if (db->mask)
-			val = ioread64_hi_lo(db->addr) & db->mask;
-		iowrite64_hi_lo(db->set | val, db->addr);
-	}
-#endif
-}
-
 static int scmi_perf_mb_limits_set(const struct scmi_protocol_handle *ph,
 				   u32 domain, u32 max_perf, u32 min_perf)
 {
@@ -426,10 +360,12 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
 	if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3 && !max_perf && !min_perf)
 		return -EINVAL;
 
-	if (dom->fc_info && dom->fc_info->limit_set_addr) {
-		iowrite32(max_perf, dom->fc_info->limit_set_addr);
-		iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
-		scmi_perf_fc_ring_db(dom->fc_info->limit_set_db);
+	if (dom->fc_info && dom->fc_info[PERF_FC_LIMIT].set_addr) {
+		struct scmi_fc_info *fci = &dom->fc_info[PERF_FC_LIMIT];
+
+		iowrite32(max_perf, fci->set_addr);
+		iowrite32(min_perf, fci->set_addr + 4);
+		ph->hops->fastchannel_db_ring(fci->set_db);
 		return 0;
 	}
 
@@ -468,9 +404,11 @@ static int scmi_perf_limits_get(const struct scmi_protocol_handle *ph,
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 	struct perf_dom_info *dom = pi->dom_info + domain;
 
-	if (dom->fc_info && dom->fc_info->limit_get_addr) {
-		*max_perf = ioread32(dom->fc_info->limit_get_addr);
-		*min_perf = ioread32(dom->fc_info->limit_get_addr + 4);
+	if (dom->fc_info && dom->fc_info[PERF_FC_LIMIT].get_addr) {
+		struct scmi_fc_info *fci = &dom->fc_info[PERF_FC_LIMIT];
+
+		*max_perf = ioread32(fci->get_addr);
+		*min_perf = ioread32(fci->get_addr + 4);
 		return 0;
 	}
 
@@ -505,9 +443,11 @@ static int scmi_perf_level_set(const struct scmi_protocol_handle *ph,
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 	struct perf_dom_info *dom = pi->dom_info + domain;
 
-	if (dom->fc_info && dom->fc_info->level_set_addr) {
-		iowrite32(level, dom->fc_info->level_set_addr);
-		scmi_perf_fc_ring_db(dom->fc_info->level_set_db);
+	if (dom->fc_info && dom->fc_info[PERF_FC_LEVEL].set_addr) {
+		struct scmi_fc_info *fci = &dom->fc_info[PERF_FC_LEVEL];
+
+		iowrite32(level, fci->set_addr);
+		ph->hops->fastchannel_db_ring(fci->set_db);
 		return 0;
 	}
 
@@ -542,8 +482,8 @@ static int scmi_perf_level_get(const struct scmi_protocol_handle *ph,
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 	struct perf_dom_info *dom = pi->dom_info + domain;
 
-	if (dom->fc_info && dom->fc_info->level_get_addr) {
-		*level = ioread32(dom->fc_info->level_get_addr);
+	if (dom->fc_info && dom->fc_info[PERF_FC_LEVEL].get_addr) {
+		*level = ioread32(dom->fc_info[PERF_FC_LEVEL].get_addr);
 		return 0;
 	}
 
@@ -572,100 +512,33 @@ static int scmi_perf_level_limits_notify(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
-{
-	if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
-		return true;
-	if ((msg == PERF_LIMITS_GET || msg == PERF_LIMITS_SET) && size == 8)
-		return true;
-	return false;
-}
-
-static void
-scmi_perf_domain_desc_fc(const struct scmi_protocol_handle *ph, u32 domain,
-			 u32 message_id, void __iomem **p_addr,
-			 struct scmi_fc_db_info **p_db)
-{
-	int ret;
-	u32 flags;
-	u64 phys_addr;
-	u8 size;
-	void __iomem *addr;
-	struct scmi_xfer *t;
-	struct scmi_fc_db_info *db;
-	struct scmi_perf_get_fc_info *info;
-	struct scmi_msg_resp_perf_desc_fc *resp;
-
-	if (!p_addr)
-		return;
-
-	ret = ph->xops->xfer_get_init(ph, PERF_DESCRIBE_FASTCHANNEL,
-				      sizeof(*info), sizeof(*resp), &t);
-	if (ret)
-		return;
-
-	info = t->tx.buf;
-	info->domain = cpu_to_le32(domain);
-	info->message_id = cpu_to_le32(message_id);
-
-	ret = ph->xops->do_xfer(ph, t);
-	if (ret)
-		goto err_xfer;
-
-	resp = t->rx.buf;
-	flags = le32_to_cpu(resp->attr);
-	size = le32_to_cpu(resp->chan_size);
-	if (!scmi_perf_fc_size_is_valid(message_id, size))
-		goto err_xfer;
-
-	phys_addr = le32_to_cpu(resp->chan_addr_low);
-	phys_addr |= (u64)le32_to_cpu(resp->chan_addr_high) << 32;
-	addr = devm_ioremap(ph->dev, phys_addr, size);
-	if (!addr)
-		goto err_xfer;
-	*p_addr = addr;
-
-	if (p_db && SUPPORTS_DOORBELL(flags)) {
-		db = devm_kzalloc(ph->dev, sizeof(*db), GFP_KERNEL);
-		if (!db)
-			goto err_xfer;
-
-		size = 1 << DOORBELL_REG_WIDTH(flags);
-		phys_addr = le32_to_cpu(resp->db_addr_low);
-		phys_addr |= (u64)le32_to_cpu(resp->db_addr_high) << 32;
-		addr = devm_ioremap(ph->dev, phys_addr, size);
-		if (!addr)
-			goto err_xfer;
-
-		db->addr = addr;
-		db->width = size;
-		db->set = le32_to_cpu(resp->db_set_lmask);
-		db->set |= (u64)le32_to_cpu(resp->db_set_hmask) << 32;
-		db->mask = le32_to_cpu(resp->db_preserve_lmask);
-		db->mask |= (u64)le32_to_cpu(resp->db_preserve_hmask) << 32;
-		*p_db = db;
-	}
-err_xfer:
-	ph->xops->xfer_put(ph, t);
-}
-
 static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
 				     u32 domain, struct scmi_fc_info **p_fc)
 {
 	struct scmi_fc_info *fc;
 
-	fc = devm_kzalloc(ph->dev, sizeof(*fc), GFP_KERNEL);
+	fc = devm_kcalloc(ph->dev, PERF_FC_MAX, sizeof(*fc), GFP_KERNEL);
 	if (!fc)
 		return;
 
-	scmi_perf_domain_desc_fc(ph, domain, PERF_LEVEL_SET,
-				 &fc->level_set_addr, &fc->level_set_db);
-	scmi_perf_domain_desc_fc(ph, domain, PERF_LEVEL_GET,
-				 &fc->level_get_addr, NULL);
-	scmi_perf_domain_desc_fc(ph, domain, PERF_LIMITS_SET,
-				 &fc->limit_set_addr, &fc->limit_set_db);
-	scmi_perf_domain_desc_fc(ph, domain, PERF_LIMITS_GET,
-				 &fc->limit_get_addr, NULL);
+	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
+				   PERF_LEVEL_SET, 4, domain,
+				   &fc[PERF_FC_LEVEL].set_addr,
+				   &fc[PERF_FC_LEVEL].set_db);
+
+	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
+				   PERF_LEVEL_GET, 4, domain,
+				   &fc[PERF_FC_LEVEL].get_addr, NULL);
+
+	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
+				   PERF_LIMITS_SET, 8, domain,
+				   &fc[PERF_FC_LIMIT].set_addr,
+				   &fc[PERF_FC_LIMIT].set_db);
+
+	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
+				   PERF_LIMITS_GET, 8, domain,
+				   &fc[PERF_FC_LIMIT].get_addr, NULL);
+
 	*p_fc = fc;
 }
 
@@ -789,7 +662,7 @@ static bool scmi_fast_switch_possible(const struct scmi_protocol_handle *ph,
 
 	dom = pi->dom_info + scmi_dev_domain_id(dev);
 
-	return dom->fc_info && dom->fc_info->level_set_addr;
+	return dom->fc_info && dom->fc_info[PERF_FC_LEVEL].set_addr;
 }
 
 static bool scmi_power_scale_mw_get(const struct scmi_protocol_handle *ph)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 99d36d503d1e..2f3bf691db7c 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -215,6 +215,19 @@ struct scmi_iterator_ops {
 				struct scmi_iterator_state *st, void *priv);
 };
 
+struct scmi_fc_db_info {
+	int width;
+	u64 set;
+	u64 mask;
+	void __iomem *addr;
+};
+
+struct scmi_fc_info {
+	void __iomem *set_addr;
+	void __iomem *get_addr;
+	struct scmi_fc_db_info *set_db;
+};
+
 /**
  * struct scmi_proto_helpers_ops  - References to common protocol helpers
  * @extended_name_get: A common helper function to retrieve extended naming
@@ -230,6 +243,9 @@ struct scmi_iterator_ops {
  *			provided in @ops.
  * @iter_response_run: A common helper to trigger the run of a previously
  *		       initialized iterator.
+ * @fastchannel_init: A common helper used to initialize FC descriptors by
+ *		      gathering FC descriptions from the SCMI platform server.
+ * @fastchannel_db_ring: A common helper to ring a FC doorbell.
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
@@ -239,6 +255,12 @@ struct scmi_proto_helpers_ops {
 				    unsigned int max_resources, u8 msg_id,
 				    size_t tx_size, void *priv);
 	int (*iter_response_run)(void *iter);
+	void (*fastchannel_init)(const struct scmi_protocol_handle *ph,
+				 u8 describe_id, u32 message_id,
+				 u32 valid_size, u32 domain,
+				 void __iomem **p_addr,
+				 struct scmi_fc_db_info **p_db);
+	void (*fastchannel_db_ring)(struct scmi_fc_db_info *db);
 };
 
 /**
-- 
2.32.0

[PATCH v3 4/9] firmware: arm_scmi: Add SCMIv3.1 Powercap FastChannels support

[Cristian Marussi]

Add SCMIv3.1 Powercap protocol FastChannel support using common helpers
provided by the SCMI core with scmi_proto_helpers_ops operations.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/powercap.c | 169 +++++++++++++++++++++------
 include/linux/scmi_protocol.h        |   2 +
 2 files changed, 138 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 126855905e2d..ed61df6a68c4 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt) "SCMI Notifications POWERCAP - " fmt
 
 #include <linux/bitfield.h>
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
@@ -27,6 +28,12 @@ enum scmi_powercap_protocol_cmd {
 	POWERCAP_DESCRIBE_FASTCHANNEL = 0xc,
 };
 
+enum {
+	POWERCAP_FC_CAP,
+	POWERCAP_FC_PAI,
+	POWERCAP_FC_MAX,
+};
+
 struct scmi_msg_resp_powercap_domain_attributes {
 	__le32 attributes;
 #define SUPPORTS_POWERCAP_CAP_CHANGE_NOTIFY(x)		((x) & BIT(31))
@@ -36,6 +43,7 @@ struct scmi_msg_resp_powercap_domain_attributes {
 #define SUPPORTS_POWERCAP_CAP_CONFIGURATION(x)		((x) & BIT(27))
 #define SUPPORTS_POWERCAP_MONITORING(x)			((x) & BIT(26))
 #define SUPPORTS_POWERCAP_PAI_CONFIGURATION(x)		((x) & BIT(25))
+#define SUPPORTS_POWERCAP_FASTCHANNELS(x)		((x) & BIT(22))
 #define POWERCAP_POWER_UNIT(x)				\
 		(FIELD_GET(GENMASK(24, 23), (x)))
 #define	SUPPORTS_POWER_UNITS_MW(x)			\
@@ -201,6 +209,8 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
 			SUPPORTS_POWER_UNITS_MW(flags);
 		dom_info->powercap_scale_uw =
 			SUPPORTS_POWER_UNITS_UW(flags);
+		dom_info->fastchannels =
+			SUPPORTS_POWERCAP_FASTCHANNELS(flags);
 
 		strscpy(dom_info->name, resp->name, SCMI_SHORT_NAME_MAX_SIZE);
 
@@ -280,15 +290,11 @@ scmi_powercap_dom_info_get(const struct scmi_protocol_handle *ph, u32 domain_id)
 	return pi->powercaps + domain_id;
 }
 
-static int scmi_powercap_cap_get(const struct scmi_protocol_handle *ph,
-				 u32 domain_id, u32 *power_cap)
+static int scmi_powercap_xfer_cap_get(const struct scmi_protocol_handle *ph,
+				      u32 domain_id, u32 *power_cap)
 {
 	int ret;
 	struct scmi_xfer *t;
-	struct powercap_info *pi = ph->get_priv(ph);
-
-	if (!power_cap || domain_id >= pi->num_domains)
-		return -EINVAL;
 
 	ret = ph->xops->xfer_get_init(ph, POWERCAP_CAP_GET, sizeof(u32),
 				      sizeof(u32), &t);
@@ -305,20 +311,31 @@ static int scmi_powercap_cap_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
-				 u32 domain_id, u32 power_cap,
-				 bool ignore_dresp)
+static int scmi_powercap_cap_get(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 *power_cap)
+{
+	struct scmi_powercap_info *dom;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!power_cap || domain_id >= pi->num_domains)
+		return -EINVAL;
+
+	dom = pi->powercaps + domain_id;
+	if (dom->fc_info && dom->fc_info[POWERCAP_FC_CAP].get_addr) {
+		*power_cap = ioread32(dom->fc_info[POWERCAP_FC_CAP].get_addr);
+		return 0;
+	}
+
+	return scmi_powercap_xfer_cap_get(ph, domain_id, power_cap);
+}
+
+static int scmi_powercap_xfer_cap_set(const struct scmi_protocol_handle *ph,
+				      const struct scmi_powercap_info *pc,
+				      u32 power_cap, bool ignore_dresp)
 {
 	int ret;
 	struct scmi_xfer *t;
 	struct scmi_msg_powercap_set_cap_or_pai *msg;
-	const struct scmi_powercap_info *pc;
-
-	pc = scmi_powercap_dom_info_get(ph, domain_id);
-	if (!pc || !pc->powercap_cap_config || !power_cap ||
-	    power_cap < pc->min_power_cap ||
-	    power_cap > pc->max_power_cap)
-		return -EINVAL;
 
 	ret = ph->xops->xfer_get_init(ph, POWERCAP_CAP_SET,
 				      sizeof(*msg), 0, &t);
@@ -326,7 +343,7 @@ static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
 		return ret;
 
 	msg = t->tx.buf;
-	msg->domain = cpu_to_le32(domain_id);
+	msg->domain = cpu_to_le32(pc->id);
 	msg->flags =
 		cpu_to_le32(FIELD_PREP(CAP_SET_ASYNC, !!pc->async_powercap_cap_set) |
 			    FIELD_PREP(CAP_SET_IGNORE_DRESP, !!ignore_dresp));
@@ -340,10 +357,10 @@ static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
 			struct scmi_msg_resp_powercap_cap_set_complete *resp;
 
 			resp = t->rx.buf;
-			if (le32_to_cpu(resp->domain) == domain_id)
+			if (le32_to_cpu(resp->domain) == pc->id)
 				dev_dbg(ph->dev,
 					"Powercap ID %d CAP set async to %u\n",
-					domain_id,
+					pc->id,
 					get_unaligned_le32(&resp->power_cap));
 			else
 				ret = -EPROTO;
@@ -354,16 +371,35 @@ static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static int scmi_powercap_pai_get(const struct scmi_protocol_handle *ph,
-				 u32 domain_id, u32 *pai)
+static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 power_cap,
+				 bool ignore_dresp)
 {
-	int ret;
-	struct scmi_xfer *t;
-	struct powercap_info *pi = ph->get_priv(ph);
+	const struct scmi_powercap_info *pc;
 
-	if (!pai || domain_id >= pi->num_domains)
+	pc = scmi_powercap_dom_info_get(ph, domain_id);
+	if (!pc || !pc->powercap_cap_config || !power_cap ||
+	    power_cap < pc->min_power_cap ||
+	    power_cap > pc->max_power_cap)
 		return -EINVAL;
 
+	if (pc->fc_info && pc->fc_info[POWERCAP_FC_CAP].set_addr) {
+		struct scmi_fc_info *fci = &pc->fc_info[POWERCAP_FC_CAP];
+
+		iowrite32(power_cap, fci->set_addr);
+		ph->hops->fastchannel_db_ring(fci->set_db);
+		return 0;
+	}
+
+	return scmi_powercap_xfer_cap_set(ph, pc, power_cap, ignore_dresp);
+}
+
+static int scmi_powercap_xfer_pai_get(const struct scmi_protocol_handle *ph,
+				      u32 domain_id, u32 *pai)
+{
+	int ret;
+	struct scmi_xfer *t;
+
 	ret = ph->xops->xfer_get_init(ph, POWERCAP_PAI_GET, sizeof(u32),
 				      sizeof(u32), &t);
 	if (ret)
@@ -379,18 +415,30 @@ static int scmi_powercap_pai_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static int scmi_powercap_pai_set(const struct scmi_protocol_handle *ph,
-				 u32 domain_id, u32 pai)
+static int scmi_powercap_pai_get(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 *pai)
+{
+	struct scmi_powercap_info *dom;
+	struct powercap_info *pi = ph->get_priv(ph);
+
+	if (!pai || domain_id >= pi->num_domains)
+		return -EINVAL;
+
+	dom = pi->powercaps + domain_id;
+	if (dom->fc_info && dom->fc_info[POWERCAP_FC_PAI].get_addr) {
+		*pai = ioread32(dom->fc_info[POWERCAP_FC_PAI].get_addr);
+		return 0;
+	}
+
+	return scmi_powercap_xfer_pai_get(ph, domain_id, pai);
+}
+
+static int scmi_powercap_xfer_pai_set(const struct scmi_protocol_handle *ph,
+				      u32 domain_id, u32 pai)
 {
 	int ret;
 	struct scmi_xfer *t;
 	struct scmi_msg_powercap_set_cap_or_pai *msg;
-	const struct scmi_powercap_info *pc;
-
-	pc = scmi_powercap_dom_info_get(ph, domain_id);
-	if (!pc || !pc->powercap_pai_config || !pai ||
-	    pai < pc->min_pai || pai > pc->max_pai)
-		return -EINVAL;
 
 	ret = ph->xops->xfer_get_init(ph, POWERCAP_PAI_SET,
 				      sizeof(*msg), 0, &t);
@@ -408,6 +456,27 @@ static int scmi_powercap_pai_set(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_powercap_pai_set(const struct scmi_protocol_handle *ph,
+				 u32 domain_id, u32 pai)
+{
+	const struct scmi_powercap_info *pc;
+
+	pc = scmi_powercap_dom_info_get(ph, domain_id);
+	if (!pc || !pc->powercap_pai_config || !pai ||
+	    pai < pc->min_pai || pai > pc->max_pai)
+		return -EINVAL;
+
+	if (pc->fc_info && pc->fc_info[POWERCAP_FC_PAI].set_addr) {
+		struct scmi_fc_info *fci = &pc->fc_info[POWERCAP_FC_PAI];
+
+		iowrite32(pai, fci->set_addr);
+		ph->hops->fastchannel_db_ring(fci->set_db);
+		return 0;
+	}
+
+	return scmi_powercap_xfer_pai_set(ph, domain_id, pai);
+}
+
 static int scmi_powercap_measurements_get(const struct scmi_protocol_handle *ph,
 					  u32 domain_id, u32 *average_power,
 					  u32 *pai)
@@ -497,6 +566,36 @@ static const struct scmi_powercap_proto_ops powercap_proto_ops = {
 	.measurements_threshold_get = scmi_powercap_measurements_threshold_get,
 };
 
+static void scmi_powercap_domain_init_fc(const struct scmi_protocol_handle *ph,
+					 u32 domain, struct scmi_fc_info **p_fc)
+{
+	struct scmi_fc_info *fc;
+
+	fc = devm_kcalloc(ph->dev, POWERCAP_FC_MAX, sizeof(*fc), GFP_KERNEL);
+	if (!fc)
+		return;
+
+	ph->hops->fastchannel_init(ph, POWERCAP_DESCRIBE_FASTCHANNEL,
+				   POWERCAP_CAP_SET, 4, domain,
+				   &fc[POWERCAP_FC_CAP].set_addr,
+				   &fc[POWERCAP_FC_CAP].set_db);
+
+	ph->hops->fastchannel_init(ph, POWERCAP_DESCRIBE_FASTCHANNEL,
+				   POWERCAP_CAP_GET, 4, domain,
+				   &fc[POWERCAP_FC_CAP].get_addr, NULL);
+
+	ph->hops->fastchannel_init(ph, POWERCAP_DESCRIBE_FASTCHANNEL,
+				   POWERCAP_PAI_SET, 4, domain,
+				   &fc[POWERCAP_FC_PAI].set_addr,
+				   &fc[POWERCAP_FC_PAI].set_db);
+
+	ph->hops->fastchannel_init(ph, POWERCAP_DESCRIBE_FASTCHANNEL,
+				   POWERCAP_PAI_GET, 4, domain,
+				   &fc[POWERCAP_FC_PAI].get_addr, NULL);
+
+	*p_fc = fc;
+}
+
 static int scmi_powercap_notify(const struct scmi_protocol_handle *ph,
 				u32 domain, int message_id, bool enable)
 {
@@ -730,6 +829,10 @@ scmi_powercap_protocol_init(const struct scmi_protocol_handle *ph)
 		ret = scmi_powercap_domain_attributes_get(ph, pinfo, domain);
 		if (ret)
 			return ret;
+
+		if (pinfo->powercaps[domain].fastchannels)
+			scmi_powercap_domain_init_fc(ph, domain,
+						     &pinfo->powercaps[domain].fc_info);
 	}
 
 	pinfo->states = devm_kcalloc(ph->dev, pinfo->num_domains,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f709c74030f4..ad9641dbdd25 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -601,6 +601,7 @@ struct scmi_powercap_info {
 	bool powercap_pai_config;
 	bool powercap_scale_mw;
 	bool powercap_scale_uw;
+	bool fastchannels;
 	char name[SCMI_MAX_STR_SIZE];
 	unsigned int min_pai;
 	unsigned int max_pai;
@@ -612,6 +613,7 @@ struct scmi_powercap_info {
 	unsigned int accuracy;
 #define SCMI_POWERCAP_ROOT_ZONE_ID     0xFFFFFFFFUL
 	unsigned int parent_id;
+	struct scmi_fc_info *fc_info;
 };
 
 /**
-- 
2.32.0

[PATCH v3 5/9] firmware: arm_scmi: Make use of FastChannels configurable

[Cristian Marussi]

Add a Kernel configuration entry used to optionally disable, globally, the
usage of SCMI FastChannels even on platforms where they are available.

Make such option default-no to preserve the original SCMI system behaviour
of using any available FC.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- fixed wording in Kconfig
- reverted Kconfig logic _USE_ -> _AVOID_
---
 drivers/firmware/arm_scmi/Kconfig  | 13 +++++++++++++
 drivers/firmware/arm_scmi/driver.c |  6 ++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 1e7b7fec97d9..3fb34db01014 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -42,6 +42,19 @@ config ARM_SCMI_HAVE_MSG
 	  This declares whether a message passing based transport for SCMI is
 	  available.
 
+config ARM_SCMI_AVOID_FASTCHANNELS
+	bool "Avoid using SCMI FastChannels even when available"
+	help
+	  Avoid using SCMI FastChannels even if advertised as available by
+	  the platform.
+
+	  On systems where the SCMI platform advertises the availability of
+	  FastChannels, supported SCMI commands can be issued triggering a
+	  one-way FastChannel request, much more quickly than using a
+	  regular SCMI message transfer.
+	  When set to Y forces the OSPM to use instead regular SCMI message
+	  transfers even if FastChannels are available. If unsure say N.
+
 config ARM_SCMI_TRANSPORT_MAILBOX
 	bool "SCMI transport based on Mailbox"
 	depends on MAILBOX
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 00b7f2aff4ec..76dc82ba04b3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1298,6 +1298,12 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 	struct scmi_msg_resp_desc_fc *resp;
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
 
+	if (IS_ENABLED(CONFIG_ARM_SCMI_AVOID_FASTCHANNELS)) {
+		dev_warn_once(ph->dev,
+			      "FastChannels usage disabled in Kconfig.\n");
+		return;
+	}
+
 	if (!p_addr) {
 		ret = -EINVAL;
 		goto err_out;
-- 
2.32.0

[PATCH v3 6/9] include: trace: Add SCMI FastChannel tracing

[Cristian Marussi]

All the currently defined SCMI events are meant to trace only regular SCMI
transfers based on SCMI messages exchanges; SCMI transactions based on
FastChannels, where used, are completely invisible from the tracing point
of view.

Add support to trace FastChannel transactions; while doing that avoid
exposing full shared memory location addresses.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/trace/events/scmi.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h
index cee4b2b64ae4..fa8879568a37 100644
--- a/include/trace/events/scmi.h
+++ b/include/trace/events/scmi.h
@@ -7,6 +7,31 @@
 
 #include <linux/tracepoint.h>
 
+TRACE_EVENT(scmi_fc_call,
+	TP_PROTO(u8 protocol_id, u8 msg_id, u32 res_id, u32 val1, u32 val2),
+	TP_ARGS(protocol_id, msg_id, res_id, val1, val2),
+
+	TP_STRUCT__entry(
+		__field(u8, protocol_id)
+		__field(u8, msg_id)
+		__field(u32, res_id)
+		__field(u32, val1)
+		__field(u32, val2)
+	),
+
+	TP_fast_assign(
+		__entry->protocol_id = protocol_id;
+		__entry->msg_id = msg_id;
+		__entry->res_id = res_id;
+		__entry->val1 = val1;
+		__entry->val2 = val2;
+	),
+
+	TP_printk("[0x%02X]:[0x%02X]:[%08X]:%u:%u",
+		  __entry->protocol_id, __entry->msg_id,
+		  __entry->res_id, __entry->val1, __entry->val2)
+);
+
 TRACE_EVENT(scmi_xfer_begin,
 	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
 		 bool poll),
-- 
2.32.0

[PATCH v3 7/9] firmware: arm_scmi: Use FastChannel tracing

[Cristian Marussi]

Make use of SCMI FastChannel event tracing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/perf.c     | 10 ++++++++++
 drivers/firmware/arm_scmi/powercap.c | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 521458fda355..64ea2d2f2875 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -16,6 +16,8 @@
 #include <linux/scmi_protocol.h>
 #include <linux/sort.h>
 
+#include <trace/events/scmi.h>
+
 #include "protocols.h"
 #include "notify.h"
 
@@ -363,6 +365,8 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
 	if (dom->fc_info && dom->fc_info[PERF_FC_LIMIT].set_addr) {
 		struct scmi_fc_info *fci = &dom->fc_info[PERF_FC_LIMIT];
 
+		trace_scmi_fc_call(SCMI_PROTOCOL_PERF, PERF_LIMITS_SET,
+				   domain, min_perf, max_perf);
 		iowrite32(max_perf, fci->set_addr);
 		iowrite32(min_perf, fci->set_addr + 4);
 		ph->hops->fastchannel_db_ring(fci->set_db);
@@ -409,6 +413,8 @@ static int scmi_perf_limits_get(const struct scmi_protocol_handle *ph,
 
 		*max_perf = ioread32(fci->get_addr);
 		*min_perf = ioread32(fci->get_addr + 4);
+		trace_scmi_fc_call(SCMI_PROTOCOL_PERF, PERF_LIMITS_GET,
+				   domain, *min_perf, *max_perf);
 		return 0;
 	}
 
@@ -446,6 +452,8 @@ static int scmi_perf_level_set(const struct scmi_protocol_handle *ph,
 	if (dom->fc_info && dom->fc_info[PERF_FC_LEVEL].set_addr) {
 		struct scmi_fc_info *fci = &dom->fc_info[PERF_FC_LEVEL];
 
+		trace_scmi_fc_call(SCMI_PROTOCOL_PERF, PERF_LEVEL_SET,
+				   domain, level, 0);
 		iowrite32(level, fci->set_addr);
 		ph->hops->fastchannel_db_ring(fci->set_db);
 		return 0;
@@ -484,6 +492,8 @@ static int scmi_perf_level_get(const struct scmi_protocol_handle *ph,
 
 	if (dom->fc_info && dom->fc_info[PERF_FC_LEVEL].get_addr) {
 		*level = ioread32(dom->fc_info[PERF_FC_LEVEL].get_addr);
+		trace_scmi_fc_call(SCMI_PROTOCOL_PERF, PERF_LEVEL_GET,
+				   domain, *level, 0);
 		return 0;
 	}
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index ed61df6a68c4..7bd6fab35650 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -12,6 +12,8 @@
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
+#include <trace/events/scmi.h>
+
 #include "protocols.h"
 #include "notify.h"
 
@@ -323,6 +325,8 @@ static int scmi_powercap_cap_get(const struct scmi_protocol_handle *ph,
 	dom = pi->powercaps + domain_id;
 	if (dom->fc_info && dom->fc_info[POWERCAP_FC_CAP].get_addr) {
 		*power_cap = ioread32(dom->fc_info[POWERCAP_FC_CAP].get_addr);
+		trace_scmi_fc_call(SCMI_PROTOCOL_POWERCAP, POWERCAP_CAP_GET,
+				   domain_id, *power_cap, 0);
 		return 0;
 	}
 
@@ -388,6 +392,8 @@ static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
 
 		iowrite32(power_cap, fci->set_addr);
 		ph->hops->fastchannel_db_ring(fci->set_db);
+		trace_scmi_fc_call(SCMI_PROTOCOL_POWERCAP, POWERCAP_CAP_SET,
+				   domain_id, power_cap, 0);
 		return 0;
 	}
 
@@ -427,6 +433,8 @@ static int scmi_powercap_pai_get(const struct scmi_protocol_handle *ph,
 	dom = pi->powercaps + domain_id;
 	if (dom->fc_info && dom->fc_info[POWERCAP_FC_PAI].get_addr) {
 		*pai = ioread32(dom->fc_info[POWERCAP_FC_PAI].get_addr);
+		trace_scmi_fc_call(SCMI_PROTOCOL_POWERCAP, POWERCAP_PAI_GET,
+				   domain_id, *pai, 0);
 		return 0;
 	}
 
@@ -469,6 +477,8 @@ static int scmi_powercap_pai_set(const struct scmi_protocol_handle *ph,
 	if (pc->fc_info && pc->fc_info[POWERCAP_FC_PAI].set_addr) {
 		struct scmi_fc_info *fci = &pc->fc_info[POWERCAP_FC_PAI];
 
+		trace_scmi_fc_call(SCMI_PROTOCOL_POWERCAP, POWERCAP_PAI_SET,
+				   domain_id, pai, 0);
 		iowrite32(pai, fci->set_addr);
 		ph->hops->fastchannel_db_ring(fci->set_db);
 		return 0;
-- 
2.32.0

[PATCH v3 8/9] firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks

[Cristian Marussi]

Add optional .setup and .teardown methods to the scmi_driver descriptor:
such callbacks, if provided, will be called by the SCIM core at driver
registration time, so that, an SCMI driver, registered as usual with the
module_scmi_driver() helper macro, can provide custom callbacks to be
run once for all at module load/unload time to perform specific setup
or teardown operations before/after .probe and .remove steps.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 15 +++++++++++++--
 include/linux/scmi_protocol.h   |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index f6fe723ab869..e95085a66bc4 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -132,12 +132,21 @@ int scmi_driver_register(struct scmi_driver *driver, struct module *owner,
 {
 	int retval;
 
-	if (!driver->probe)
+	if (!driver->probe || !driver->id_table)
 		return -EINVAL;
 
+	if (driver->setup) {
+		retval = driver->setup();
+		if (retval)
+			return retval;
+	}
+
 	retval = scmi_protocol_device_request(driver->id_table);
-	if (retval)
+	if (retval) {
+		if (driver->teardown)
+			driver->teardown();
 		return retval;
+	}
 
 	driver->driver.bus = &scmi_bus_type;
 	driver->driver.name = driver->name;
@@ -156,6 +165,8 @@ void scmi_driver_unregister(struct scmi_driver *driver)
 {
 	driver_unregister(&driver->driver);
 	scmi_protocol_device_unrequest(driver->id_table);
+	if (driver->teardown)
+		driver->teardown();
 }
 EXPORT_SYMBOL_GPL(scmi_driver_unregister);
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index ad9641dbdd25..a922707bdfe8 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -805,6 +805,8 @@ struct scmi_device_id {
 
 struct scmi_driver {
 	const char *name;
+	int (*setup)(void);
+	void (*teardown)(void);
 	int (*probe)(struct scmi_device *sdev);
 	void (*remove)(struct scmi_device *sdev);
 	const struct scmi_device_id *id_table;
-- 
2.32.0

[PATCH v3 9/9] powercap: arm_scmi: Add SCMI Powercap based driver

[Cristian Marussi]

Add a powercap driver that, using the ARM SCMI Protocol to query the SCMI
platform firmware for the list of existing Powercap domains, registers all
of such discovered domains under the new 'arm-scmi' powercap control type.

A new simple powercap zone and constraint is registered for all the SCMI
powercap zones that are found.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- fix sparse warning about missing static on global  *scmi_top_pcntrl;
---
 drivers/powercap/Kconfig             |  13 +
 drivers/powercap/Makefile            |   1 +
 drivers/powercap/arm_scmi_powercap.c | 537 +++++++++++++++++++++++++++
 3 files changed, 551 insertions(+)
 create mode 100644 drivers/powercap/arm_scmi_powercap.c

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 515e3ceb3393..90d33cd1b670 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -44,6 +44,19 @@ config IDLE_INJECT
 	  synchronously on a set of specified CPUs or alternatively
 	  on a per CPU basis.
 
+config ARM_SCMI_POWERCAP
+	tristate "ARM SCMI Powercap driver"
+	depends on ARM_SCMI_PROTOCOL
+	help
+	  This enables support for the ARM Powercap based on ARM SCMI
+	  Powercap protocol.
+
+	  ARM SCMI Powercap protocol allows power limits to be enforced
+	  and monitored against the SCMI Powercap domains advertised as
+	  available by the SCMI platform firmware.
+
+	  When compiled as module it will be called arm_scmi_powercap.ko.
+
 config DTPM
 	bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
 	depends on OF
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index 494617cdad88..4474201b4aa7 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
 obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
 obj-$(CONFIG_IDLE_INJECT) += idle_inject.o
+obj-$(CONFIG_ARM_SCMI_POWERCAP) += arm_scmi_powercap.o
diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
new file mode 100644
index 000000000000..36f6dc211fbb
--- /dev/null
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -0,0 +1,537 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SCMI Powercap support.
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/math.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/powercap.h>
+#include <linux/scmi_protocol.h>
+
+#define to_scmi_powercap_zone(z)		\
+	container_of(z, struct scmi_powercap_zone, zone)
+
+static const struct scmi_powercap_proto_ops *powercap_ops;
+
+struct scmi_powercap_zone {
+	unsigned int height;
+	struct device *dev;
+	struct scmi_protocol_handle *ph;
+	const struct scmi_powercap_info *info;
+	struct scmi_powercap_zone *spzones;
+	struct powercap_zone zone;
+	struct list_head node;
+};
+
+struct scmi_powercap_root {
+	unsigned int num_zones;
+	struct scmi_powercap_zone *spzones;
+	struct list_head *registered_zones;
+};
+
+static struct powercap_control_type *scmi_top_pcntrl;
+
+static int scmi_powercap_zone_release(struct powercap_zone *pz)
+{
+	return 0;
+}
+
+static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
+						u64 *max_power_range_uw)
+{
+	*max_power_range_uw = (u64)U32_MAX;
+	return 0;
+}
+
+static int scmi_powercap_get_power_uw(struct powercap_zone *pz,
+				      u64 *power_uw)
+{
+	int ret;
+	u32 avg_power, pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_monitoring)
+		return -EINVAL;
+
+	ret = powercap_ops->measurements_get(spz->ph, spz->info->id, &avg_power,
+					     &pai);
+	if (ret)
+		return ret;
+
+	if (spz->info->powercap_scale_mw)
+		*power_uw = (u64)(avg_power * 1000);
+	else
+		*power_uw = (u64)avg_power;
+
+	return 0;
+}
+
+static const struct powercap_zone_ops zone_ops = {
+	.get_max_power_range_uw = scmi_powercap_get_max_power_range_uw,
+	.get_power_uw = scmi_powercap_get_power_uw,
+	.release = scmi_powercap_zone_release,
+};
+
+static inline int
+scmi_powercap_normalize_cap(const struct scmi_powercap_info *info,
+			    u64 power_limit_uw, u32 *normalized)
+{
+	u64 req_power;
+
+	if (info->powercap_scale_mw)
+		req_power = DIV_ROUND_UP_ULL(power_limit_uw, 1000);
+	else
+		req_power = power_limit_uw;
+
+	if (req_power > info->max_power_cap)
+		*normalized = info->max_power_cap;
+	else if (req_power < info->min_power_cap)
+		*normalized = info->min_power_cap;
+	else
+		*normalized = (u32)req_power;
+
+	*normalized = rounddown(*normalized, info->power_cap_step);
+
+	return 0;
+}
+
+static int scmi_powercap_set_power_limit_uw(struct powercap_zone *pz, int cid,
+					    u64 power_uw)
+{
+	int ret;
+	u32 power;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_cap_config)
+		return -EINVAL;
+
+	ret = scmi_powercap_normalize_cap(spz->info, power_uw, &power);
+	if (ret)
+		return ret;
+
+	return powercap_ops->cap_set(spz->ph, spz->info->id, power, false);
+}
+
+static int scmi_powercap_get_power_limit_uw(struct powercap_zone *pz, int cid,
+					    u64 *power_limit_uw)
+{
+	int ret;
+	u32 power;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	ret = powercap_ops->cap_get(spz->ph, spz->info->id, &power);
+	if (ret)
+		return ret;
+
+	if (spz->info->powercap_scale_mw)
+		*power_limit_uw = power * 1000;
+	else
+		*power_limit_uw = power;
+
+	return 0;
+}
+
+static inline int
+scmi_powercap_normalize_time(const struct scmi_powercap_info *info,
+			     u64 time_us, u32 *normalized)
+{
+	if (time_us > info->max_pai)
+		*normalized = info->max_pai;
+	else if (time_us < info->min_pai)
+		*normalized = info->min_pai;
+	else
+		*normalized = (u32)time_us;
+
+	*normalized = rounddown(*normalized, info->pai_step);
+
+	return 0;
+}
+
+static int scmi_powercap_set_time_window_us(struct powercap_zone *pz, int cid,
+					    u64 time_window_us)
+{
+	int ret;
+	u32 pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_pai_config)
+		return -EINVAL;
+
+	ret = scmi_powercap_normalize_time(spz->info, time_window_us, &pai);
+	if (ret)
+		return ret;
+
+	return powercap_ops->pai_set(spz->ph, spz->info->id, pai);
+}
+
+static int scmi_powercap_get_time_window_us(struct powercap_zone *pz, int cid,
+					    u64 *time_window_us)
+{
+	int ret;
+	u32 pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	ret = powercap_ops->pai_get(spz->ph, spz->info->id, &pai);
+	if (ret)
+		return ret;
+
+	*time_window_us = (u64)pai;
+
+	return 0;
+}
+
+static int scmi_powercap_get_max_power_uw(struct powercap_zone *pz, int cid,
+					  u64 *max_power_uw)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (spz->info->powercap_scale_uw)
+		*max_power_uw = (u64)spz->info->max_power_cap;
+	else
+		*max_power_uw = (u64)(spz->info->max_power_cap * 1000);
+
+	return 0;
+}
+
+static int scmi_powercap_get_min_power_uw(struct powercap_zone *pz, int cid,
+					  u64 *min_power_uw)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (spz->info->powercap_scale_uw)
+		*min_power_uw = (u64)spz->info->min_power_cap;
+	else
+		*min_power_uw = (u64)(spz->info->min_power_cap * 1000);
+
+	return 0;
+}
+
+static int scmi_powercap_get_max_time_window_us(struct powercap_zone *pz,
+						int cid, u64 *time_window_us)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	*time_window_us = (u64)spz->info->max_pai;
+
+	return 0;
+}
+
+static int scmi_powercap_get_min_time_window_us(struct powercap_zone *pz,
+						int cid, u64 *time_window_us)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	*time_window_us = (u64)spz->info->min_pai;
+
+	return 0;
+}
+
+static const char *scmi_powercap_get_name(struct powercap_zone *pz, int cid)
+{
+	return "SCMI power-cap";
+}
+
+static const struct powercap_zone_constraint_ops constraint_ops  = {
+	.set_power_limit_uw = scmi_powercap_set_power_limit_uw,
+	.get_power_limit_uw = scmi_powercap_get_power_limit_uw,
+	.set_time_window_us = scmi_powercap_set_time_window_us,
+	.get_time_window_us = scmi_powercap_get_time_window_us,
+	.get_max_power_uw = scmi_powercap_get_max_power_uw,
+	.get_min_power_uw = scmi_powercap_get_min_power_uw,
+	.get_max_time_window_us = scmi_powercap_get_max_time_window_us,
+	.get_min_time_window_us = scmi_powercap_get_min_time_window_us,
+	.get_name = scmi_powercap_get_name,
+};
+
+static void scmi_powercap_unregister_all_zones(struct scmi_powercap_root *pr)
+{
+	int i;
+
+	/* Un-register children zones first starting from the leaves */
+	for (i = pr->num_zones - 1; i >= 0; i--) {
+		if (!list_empty(&pr->registered_zones[i])) {
+			struct scmi_powercap_zone *spz;
+
+			list_for_each_entry(spz, &pr->registered_zones[i], node)
+				powercap_unregister_zone(scmi_top_pcntrl,
+							 &spz->zone);
+		}
+	}
+}
+
+static inline bool
+scmi_powercap_is_zone_registered(struct scmi_powercap_zone *spz)
+{
+	return !list_empty(&spz->node);
+}
+
+static inline unsigned int
+scmi_powercap_get_zone_height(struct scmi_powercap_zone *spz)
+{
+	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
+		return 0;
+
+	return spz->spzones[spz->info->parent_id].height + 1;
+}
+
+static inline struct scmi_powercap_zone *
+scmi_powercap_get_parent_zone(struct scmi_powercap_zone *spz)
+{
+	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
+		return NULL;
+
+	return &spz->spzones[spz->info->parent_id];
+}
+
+/**
+ * scmi_powercap_register_zone  - Register an SCMI powercap zone recursively
+ *
+ * @pr: A reference to the root powercap zones descriptors
+ * @spz: A reference to the SCMI powercap zone to register
+ *
+ * When registering SCMI powercap zones with the powercap framework we should
+ * take care to always register zones starting from the root ones and to
+ * deregister starting from the leaves.
+ *
+ * Unfortunately we cannot assume that the array of available SCMI powercap
+ * zones provided by the SCMI platform firmware is built to comply with such
+ * requirement.
+ *
+ * This function, given an SCMI powercap zone to register, takes care to walk
+ * the SCMI powercap zones tree up to the root looking recursively for
+ * unregistered parent zones before regsitering the provided zone; at the same
+ * time each registered zone height in such a tree is accounted for and each
+ * zone, once registered, is stored in the @registered_zones array that is
+ * indexed by zone height: this way will be trivial, at unregister time, to walk
+ * the @registered_zones array backward and unregister all the zones starting
+ * from the leaves, removing children zones before parents.
+ *
+ * While doing this, we prune away any zone marked as invalid (like the ones
+ * sporting an SCMI abstract power scale) as long as they are positioned as
+ * leaves in the SCMI powercap zones hierarchy: any non-leaf invalid zone causes
+ * the entire process to fail since we cannot assume the correctness of an SCMI
+ * powercap zones hierarchy if some of the internal nodes are missing.
+ *
+ * Note that the array of SCMI powercap zones as returned by the SCMI platform
+ * is known to be sane, i.e. zones relationships have been validated at the
+ * protocol layer.
+ *
+ * Return: 0 on Success
+ */
+static int scmi_powercap_register_zone(struct scmi_powercap_root *pr,
+				       struct scmi_powercap_zone *spz)
+{
+	int ret = 0;
+	struct scmi_powercap_zone *parent;
+
+	if (!spz->info)
+		return ret;
+
+	parent = scmi_powercap_get_parent_zone(spz);
+	if (parent && !scmi_powercap_is_zone_registered(parent)) {
+		/*
+		 * Bail out if a parent domain was marked as unsupported:
+		 * only domains participating as leaves can be skipped.
+		 */
+		if (!parent->info)
+			return -ENODEV;
+
+		ret = scmi_powercap_register_zone(pr, parent);
+		if (ret)
+			return ret;
+	}
+
+	if (!scmi_powercap_is_zone_registered(spz)) {
+		struct powercap_zone *z;
+
+		z = powercap_register_zone(&spz->zone,
+					   scmi_top_pcntrl,
+					   spz->info->name,
+					   parent ? &parent->zone : NULL,
+					   &zone_ops, 1, &constraint_ops);
+		if (!IS_ERR(z)) {
+			spz->height = scmi_powercap_get_zone_height(spz);
+			list_add(&spz->node,
+				 &pr->registered_zones[spz->height]);
+			dev_dbg(spz->dev,
+				"Registered node %s - parent %s - height:%d\n",
+				spz->info->name,
+				parent ? parent->info->name : "ROOT",
+				spz->height);
+			ret = 0;
+		} else {
+			ret = PTR_ERR(z);
+			dev_err(spz->dev,
+				"Error registering node:%s - parent:%s - h:%d - ret:%d\n",
+				 spz->info->name,
+				 parent ? parent->info->name : "ROOT",
+				 spz->height, ret);
+		}
+	}
+
+	return ret;
+}
+
+static int scmi_powercap_probe(struct scmi_device *sdev)
+{
+	int ret, i;
+	struct scmi_powercap_root *pr;
+	struct scmi_powercap_zone *spz;
+	struct scmi_protocol_handle *ph;
+	struct device *dev = &sdev->dev;
+	const struct scmi_handle *handle = sdev->handle;
+
+	if (!handle)
+		return -ENODEV;
+
+	powercap_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_POWERCAP,
+						 &ph);
+	if (IS_ERR(powercap_ops))
+		return PTR_ERR(powercap_ops);
+
+	pr = devm_kzalloc(dev, sizeof(*pr), GFP_KERNEL);
+	if (!pr)
+		return -ENOMEM;
+
+	pr->num_zones = powercap_ops->num_domains_get(ph);
+	if (pr->num_zones < 0) {
+		dev_err(dev, "number of powercap domains not found\n");
+		return pr->num_zones;
+	}
+
+	pr->spzones = devm_kcalloc(dev, pr->num_zones,
+				   sizeof(*pr->spzones), GFP_KERNEL);
+	if (!pr->spzones)
+		return -ENOMEM;
+
+	/* Allocate for worst possible scenario of maximum tree height. */
+	pr->registered_zones = devm_kcalloc(dev, pr->num_zones,
+					    sizeof(*pr->registered_zones),
+					    GFP_KERNEL);
+	if (!pr->registered_zones)
+		return -ENOMEM;
+
+	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
+		/*
+		 * Powercap domains are validate by the protocol layer, i.e.
+		 * when only non-NULL domains are returned here, whose
+		 * parent_id is assured to point to another valid domain.
+		 */
+		spz->info = powercap_ops->info_get(ph, i);
+
+		spz->dev = dev;
+		spz->ph = ph;
+		spz->spzones = pr->spzones;
+		INIT_LIST_HEAD(&spz->node);
+		INIT_LIST_HEAD(&pr->registered_zones[i]);
+
+		/*
+		 * Forcibly skip powercap domains using an abstract scale.
+		 * Note that only leaves domains can be skipped, so this could
+		 * lead later to a global failure.
+		 */
+		if (!spz->info->powercap_scale_uw &&
+		    !spz->info->powercap_scale_mw) {
+			dev_warn(dev,
+				 "Abstract power scale not supported. Skip %s.\n",
+				 spz->info->name);
+			spz->info = NULL;
+			continue;
+		}
+	}
+
+	/*
+	 * Scan array of retrieved SCMI powercap domains and register them
+	 * recursively starting from the root domains.
+	 */
+	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
+		ret = scmi_powercap_register_zone(pr, spz);
+		if (ret) {
+			dev_err(dev,
+				"Failed to register powercap zone %s - ret:%d\n",
+				spz->info->name, ret);
+			scmi_powercap_unregister_all_zones(pr);
+			return ret;
+		}
+	}
+
+	dev_set_drvdata(dev, pr);
+
+	dev_info(dev, "Registered %d SCMI Powercap domains !\n", pr->num_zones);
+
+	return ret;
+}
+
+static void scmi_powercap_remove(struct scmi_device *sdev)
+{
+	struct device *dev = &sdev->dev;
+	struct scmi_powercap_root *pr = dev_get_drvdata(dev);
+
+	scmi_powercap_unregister_all_zones(pr);
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_POWERCAP, "powercap" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static int scmi_powercap_setup(void)
+{
+	scmi_top_pcntrl = powercap_register_control_type(NULL, "arm-scmi", NULL);
+	if (!scmi_top_pcntrl)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void scmi_powercap_teardown(void)
+{
+	powercap_unregister_control_type(scmi_top_pcntrl);
+}
+
+static struct scmi_driver scmi_powercap_driver = {
+	.name = "scmi-powercap",
+	.setup = scmi_powercap_setup,
+	.teardown = scmi_powercap_teardown,
+	.probe = scmi_powercap_probe,
+	.remove = scmi_powercap_remove,
+	.id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_powercap_driver);
+
+MODULE_AUTHOR("Cristian Marussi <cristian.marussi@arm.com>");
+MODULE_DESCRIPTION("ARM SCMI Powercap driver");
+MODULE_LICENSE("GPL");
-- 
2.32.0

Re: [PATCH v3 5/9] firmware: arm_scmi: Make use of FastChannels configurable

[Sudeep Holla]

On Mon, Jun 27, 2022 at 01:30:34PM +0100, Cristian Marussi wrote:
> Add a Kernel configuration entry used to optionally disable, globally, the
> usage of SCMI FastChannels even on platforms where they are available.
> 
> Make such option default-no to preserve the original SCMI system behaviour
> of using any available FC.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v2 --> v3
> - fixed wording in Kconfig
> - reverted Kconfig logic _USE_ -> _AVOID_
> ---
>  drivers/firmware/arm_scmi/Kconfig  | 13 +++++++++++++
>  drivers/firmware/arm_scmi/driver.c |  6 ++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 1e7b7fec97d9..3fb34db01014 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -42,6 +42,19 @@ config ARM_SCMI_HAVE_MSG
>  	  This declares whether a message passing based transport for SCMI is
>  	  available.
>  
> +config ARM_SCMI_AVOID_FASTCHANNELS
> +	bool "Avoid using SCMI FastChannels even when available"

Without default, won't this prompt user now ? I would have explicit default n
if we are adding this. But why do we need this is my question ? This would
be a quirk IMO on systems where FC is broken. I don't want people to enable
this during testing and ship f/w with FC broken(or not developed yet).

We can add this if some platforms really need this as a quirk in the future.
Thoughts ?

-- 
Regards,
Sudeep

Re: [PATCH v3 8/9] firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks

[Sudeep Holla]

On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote:
> Add optional .setup and .teardown methods to the scmi_driver descriptor:
> such callbacks, if provided, will be called by the SCIM core at driver
> registration time, so that, an SCMI driver, registered as usual with the
> module_scmi_driver() helper macro, can provide custom callbacks to be
> run once for all at module load/unload time to perform specific setup
> or teardown operations before/after .probe and .remove steps.
>

What can't the driver call this setup/teardown on its own before/after
calling scmi_driver_register/unregister ?

Based on the usage in 9/9, I guess it is mainly to use the
module_scmi_driver ? If so, I would avoid using that or have another
macro to manage this setup/teardown(once there are multiple users for that).
IMO, it doesn't make sense to add callbacks to do things that are outside
the scope of scmi drivers. No ?

-- 
Regards,
Sudeep

Re: [PATCH v3 5/9] firmware: arm_scmi: Make use of FastChannels configurable

[Cristian Marussi]

On Fri, Jul 01, 2022 at 03:03:07PM +0100, Sudeep Holla wrote:
> On Mon, Jun 27, 2022 at 01:30:34PM +0100, Cristian Marussi wrote:
> > Add a Kernel configuration entry used to optionally disable, globally, the
> > usage of SCMI FastChannels even on platforms where they are available.
> > 
> > Make such option default-no to preserve the original SCMI system behaviour
> > of using any available FC.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v2 --> v3
> > - fixed wording in Kconfig
> > - reverted Kconfig logic _USE_ -> _AVOID_
> > ---
> >  drivers/firmware/arm_scmi/Kconfig  | 13 +++++++++++++
> >  drivers/firmware/arm_scmi/driver.c |  6 ++++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 1e7b7fec97d9..3fb34db01014 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -42,6 +42,19 @@ config ARM_SCMI_HAVE_MSG
> >  	  This declares whether a message passing based transport for SCMI is
> >  	  available.
> >  
> > +config ARM_SCMI_AVOID_FASTCHANNELS
> > +	bool "Avoid using SCMI FastChannels even when available"
> 
> Without default, won't this prompt user now ? I would have explicit default n
> if we are adding this. But why do we need this is my question ? This would
> be a quirk IMO on systems where FC is broken. I don't want people to enable
> this during testing and ship f/w with FC broken(or not developed yet).
> 
> We can add this if some platforms really need this as a quirk in the future.
>

Ah ok an explicit default no is better indeed to avoid prompting if you
do not defconfig (I missed this difference between default implicit NO
and explicit NO) ...

... regarding why, I am using personally this indeed for testing with or
without FCs without having to change the installed FW blob, BUT the reason
for upstreaming such an option is that FC support is indeed optional by the
spec so I thought it would have been acceptabel that you could want to
configure a platform NOT to use them even though the FW implementation you
are using, maybe across multiple platforms, supports it.

Thanks,
Cristian

Re: [PATCH v3 5/9] firmware: arm_scmi: Make use of FastChannels configurable

[Sudeep Holla]

On Fri, Jul 01, 2022 at 03:47:20PM +0100, Cristian Marussi wrote:

[...]

> ... regarding why, I am using personally this indeed for testing with or
> without FCs without having to change the installed FW blob, BUT the reason
> for upstreaming such an option is that FC support is indeed optional by the
> spec so I thought it would have been acceptabel that you could want to
> configure a platform NOT to use them even though the FW implementation you
> are using, maybe across multiple platforms, supports it.

Yes it is optional. But we use only if F/W advertises that it is available,
so no harm if it is not implemented right ? I don't believe it will save
space in the way config is used and I don't want to push all the code
under the config too.

-- 
Regards,
Sudeep

Re: [PATCH v3 5/9] firmware: arm_scmi: Make use of FastChannels configurable

[Cristian Marussi]

On Fri, Jul 01, 2022 at 03:55:08PM +0100, Sudeep Holla wrote:
> On Fri, Jul 01, 2022 at 03:47:20PM +0100, Cristian Marussi wrote:
> 
> [...]
> 
> > ... regarding why, I am using personally this indeed for testing with or
> > without FCs without having to change the installed FW blob, BUT the reason
> > for upstreaming such an option is that FC support is indeed optional by the
> > spec so I thought it would have been acceptabel that you could want to
> > configure a platform NOT to use them even though the FW implementation you
> > are using, maybe across multiple platforms, supports it.
> 
> Yes it is optional. But we use only if F/W advertises that it is available,
> so no harm if it is not implemented right ? I don't believe it will save
> space in the way config is used and I don't want to push all the code
> under the config too.

What I meant was to be able to use the same FW blob_X with valid and
advertised FCs support on multiple platforms, but having the possibility
to configure some of these not to use it even if advertised as
available. Indeed there is no space saving at all that was ot the idea.

Thanks
Cristian

Re: [PATCH v3 8/9] firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks

[Cristian Marussi]

On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote:
> On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote:
> > Add optional .setup and .teardown methods to the scmi_driver descriptor:
> > such callbacks, if provided, will be called by the SCIM core at driver
> > registration time, so that, an SCMI driver, registered as usual with the
> > module_scmi_driver() helper macro, can provide custom callbacks to be
> > run once for all at module load/unload time to perform specific setup
> > or teardown operations before/after .probe and .remove steps.
> >
> 
> What can't the driver call this setup/teardown on its own before/after
> calling scmi_driver_register/unregister ?

> 
> Based on the usage in 9/9, I guess it is mainly to use the
> module_scmi_driver ? If so, I would avoid using that or have another
> macro to manage this setup/teardown(once there are multiple users for that).
> IMO, it doesn't make sense to add callbacks to do things that are outside
> the scope of scmi drivers. No ?
>

This is exactly what I was doing in fact :D at first ... defining a normal
init/exit from where I called what I needed at first and then ivoke the
scmi_driver_register()...so bypassing/not using the module_scmi-driver macro
indeed...then I realized I needed something similar also for the SCMI Test
driver, so I tried to unify; in both cases indeed the required ops to be
done before the scmi_driver_register are NOT scmi related things.

So I can drop this if you prefer and use bare module_init/exit that
calls scmi_driver_register() after having setup what needed for the
specific driver initialization (before probe)...I was not really
convinced it was worth this level of unification.

Thanks,
Cristian

Re: [PATCH v3 8/9] firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks

[Sudeep Holla]

On Fri, Jul 01, 2022 at 04:09:05PM +0100, Cristian Marussi wrote:
> On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote:
> > On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote:
> > > Add optional .setup and .teardown methods to the scmi_driver descriptor:
> > > such callbacks, if provided, will be called by the SCIM core at driver
> > > registration time, so that, an SCMI driver, registered as usual with the
> > > module_scmi_driver() helper macro, can provide custom callbacks to be
> > > run once for all at module load/unload time to perform specific setup
> > > or teardown operations before/after .probe and .remove steps.
> > >
> > 
> > What can't the driver call this setup/teardown on its own before/after
> > calling scmi_driver_register/unregister ?
> 
> > 
> > Based on the usage in 9/9, I guess it is mainly to use the
> > module_scmi_driver ? If so, I would avoid using that or have another
> > macro to manage this setup/teardown(once there are multiple users for that).
> > IMO, it doesn't make sense to add callbacks to do things that are outside
> > the scope of scmi drivers. No ?
> >
> 
> This is exactly what I was doing in fact :D at first ... defining a normal
> init/exit from where I called what I needed at first and then ivoke the
> scmi_driver_register()...so bypassing/not using the module_scmi-driver macro
> indeed...then I realized I needed something similar also for the SCMI Test
> driver, so I tried to unify; in both cases indeed the required ops to be
> done before the scmi_driver_register are NOT scmi related things.
> 
> So I can drop this if you prefer and use bare module_init/exit that
> calls scmi_driver_register() after having setup what needed for the
> specific driver initialization (before probe)...I was not really
> convinced it was worth this level of unification.
> 

We can add macro for that too if there is another user for this. i.e.
if and when we merge the test code using something similar, we can
wrap them in a macro module_scmi_driver_setup_teardown(driver, setup, teardown)
and simplify things for the users.

-- 
Regards,
Sudeep

Re: [PATCH v3 8/9] firmware: arm_scmi: Add scmi_driver optional setup/teardown callbacks

[Cristian Marussi]

On Fri, Jul 01, 2022 at 04:18:29PM +0100, Sudeep Holla wrote:
> On Fri, Jul 01, 2022 at 04:09:05PM +0100, Cristian Marussi wrote:
> > On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote:
> > > On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote:
> > > > Add optional .setup and .teardown methods to the scmi_driver descriptor:
> > > > such callbacks, if provided, will be called by the SCIM core at driver
> > > > registration time, so that, an SCMI driver, registered as usual with the
> > > > module_scmi_driver() helper macro, can provide custom callbacks to be
> > > > run once for all at module load/unload time to perform specific setup
> > > > or teardown operations before/after .probe and .remove steps.
> > > >
> > > 
> > > What can't the driver call this setup/teardown on its own before/after
> > > calling scmi_driver_register/unregister ?
> > 
> > > 
> > > Based on the usage in 9/9, I guess it is mainly to use the
> > > module_scmi_driver ? If so, I would avoid using that or have another
> > > macro to manage this setup/teardown(once there are multiple users for that).
> > > IMO, it doesn't make sense to add callbacks to do things that are outside
> > > the scope of scmi drivers. No ?
> > >
> > 
> > This is exactly what I was doing in fact :D at first ... defining a normal
> > init/exit from where I called what I needed at first and then ivoke the
> > scmi_driver_register()...so bypassing/not using the module_scmi-driver macro
> > indeed...then I realized I needed something similar also for the SCMI Test
> > driver, so I tried to unify; in both cases indeed the required ops to be
> > done before the scmi_driver_register are NOT scmi related things.
> > 
> > So I can drop this if you prefer and use bare module_init/exit that
> > calls scmi_driver_register() after having setup what needed for the
> > specific driver initialization (before probe)...I was not really
> > convinced it was worth this level of unification.
> > 
> 
> We can add macro for that too if there is another user for this. i.e.
> if and when we merge the test code using something similar, we can
> wrap them in a macro module_scmi_driver_setup_teardown(driver, setup, teardown)
> and simplify things for the users.
> 
Ok, thanks I'll drop this for this series.

Thanks,
Cristian