[PATCH v3 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator

[PATCH v3 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator

[Cristian Marussi]

Hi,

this series introduces the support for the new SCMI Voltage Domain Protocol
defined by the upcoming SCMIv3.0 specification, whose BETA release is
available at [1].

Afterwards, a new generic SCMI Regulator driver is developed on top of the
new SCMI VD Protocol.

The series is currently based on for-next/scmi [2] on top of:

commit b9ceca6be432 ("firmware: arm_scmi: Fix duplicate workqueue name")

Any feedback welcome,

Thanks,

Cristian

---

v2 --> v3
- DT bindings
  - avoid awkard examples based on _cpu/_gpu regulators
- SCMI Regulator
  - remove multiple linear mappings support
  - removed duplicated voltage name printout
  - added a few comments
  - simplified return path in scmi_reg_set_voltage_sel()
- VD Protocol
  - restrict segmented voltage domain descriptors to one triplet
  - removed unneeded inline
  - free allocated resources for invalid voltage domain
  - added __must_check to info_get voltage operations
  - added a few comments
  - removed fixed size typing from struct voltage_info
    
v1 --> v2
- rebased on for-next/scmi v5.10
- DT bindings
  - removed any reference to negative voltages
- SCMI Regulator
  - removed duplicate regulator naming
  - removed redundant .get/set_voltage ops: only _sel variants implemented
  - removed condexpr on fail path to increase readability
- VD Protocol
  - fix voltage levels query loop to reload full cmd description
    between iterations as reported by Etienne Carriere
  - ensure transport rx buffer is properly sized calli scmi_reset_rx_to_maxsz
    between transfers

[1]:https://developer.arm.com/documentation/den0056/c/
[2]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi


Cristian Marussi (4):
  firmware: arm_scmi: Add Voltage Domain Support
  firmware: arm_scmi: add SCMI Voltage Domain devname
  regulator: add SCMI driver
  dt-bindings: arm: add support for SCMI Regulators

 .../devicetree/bindings/arm/arm,scmi.txt      |  42 ++
 drivers/firmware/arm_scmi/Makefile            |   2 +-
 drivers/firmware/arm_scmi/common.h            |   1 +
 drivers/firmware/arm_scmi/driver.c            |   3 +
 drivers/firmware/arm_scmi/voltage.c           | 398 +++++++++++++++++
 drivers/regulator/Kconfig                     |   9 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/scmi-regulator.c            | 403 ++++++++++++++++++
 include/linux/scmi_protocol.h                 |  64 +++
 9 files changed, 922 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/voltage.c
 create mode 100644 drivers/regulator/scmi-regulator.c

-- 
2.17.1

[PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Cristian Marussi]

Add SCMI Voltage Domain protocol support.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- restrict segmented voltage domain descriptors to one triplet
- removed unneeded inline
- free allocated resources for invalid voltage domain
- added __must_check to info_get voltage operations
- added a few comments
- removed fixed size typing from struct voltage_info

v1 --> v2
- fix voltage levels query loop to reload full cmd description
  between iterations as reported by Etienne Carriere
- ensure rx buffer is properly sized calli scmi_reset_rx_to_maxsz
  between transfers
---
 drivers/firmware/arm_scmi/Makefile  |   2 +-
 drivers/firmware/arm_scmi/common.h  |   1 +
 drivers/firmware/arm_scmi/driver.c  |   2 +
 drivers/firmware/arm_scmi/voltage.c | 398 ++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h       |  64 +++++
 5 files changed, 466 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/voltage.c

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index bc0d54f8e861..6a2ef63306d6 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o
 scmi-transport-y = shmem.o
 scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
 scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.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/common.h b/drivers/firmware/arm_scmi/common.h
index 65063fa948d4..c0fb45e7c3e8 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -169,6 +169,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(perf);
 DECLARE_SCMI_REGISTER_UNREGISTER(power);
 DECLARE_SCMI_REGISTER_UNREGISTER(reset);
 DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
+DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
 DECLARE_SCMI_REGISTER_UNREGISTER(system);
 
 #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3dfd8b6a0ebf..ada35e63feae 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -946,6 +946,7 @@ static int __init scmi_driver_init(void)
 	scmi_power_register();
 	scmi_reset_register();
 	scmi_sensors_register();
+	scmi_voltage_register();
 	scmi_system_register();
 
 	return platform_driver_register(&scmi_driver);
@@ -961,6 +962,7 @@ static void __exit scmi_driver_exit(void)
 	scmi_power_unregister();
 	scmi_reset_unregister();
 	scmi_sensors_unregister();
+	scmi_voltage_unregister();
 	scmi_system_unregister();
 
 	platform_driver_unregister(&scmi_driver);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
new file mode 100644
index 000000000000..2c5a3c8b8649
--- /dev/null
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Voltage Protocol
+ *
+ * Copyright (C) 2020 ARM Ltd.
+ */
+
+#include <linux/scmi_protocol.h>
+
+#include "common.h"
+
+#define VOLTAGE_DOMS_NUM_MASK		GENMASK(15, 0)
+#define REMAINING_LEVELS_MASK		GENMASK(31, 16)
+#define RETURNED_LEVELS_MASK		GENMASK(11, 0)
+
+enum scmi_voltage_protocol_cmd {
+	VOLTAGE_DOMAIN_ATTRIBUTES = 0x3,
+	VOLTAGE_DESCRIBE_LEVELS = 0x4,
+	VOLTAGE_CONFIG_SET = 0x5,
+	VOLTAGE_CONFIG_GET = 0x6,
+	VOLTAGE_LEVEL_SET = 0x7,
+	VOLTAGE_LEVEL_GET = 0x8,
+};
+
+struct scmi_msg_resp_protocol_attributes {
+	__le32 attr;
+#define NUM_VOLTAGE_DOMAINS(x)	(u16)(FIELD_GET(VOLTAGE_DOMS_NUM_MASK, (x)))
+};
+
+struct scmi_msg_resp_domain_attributes {
+	__le32 attr;
+	u8 name[SCMI_MAX_STR_SIZE];
+};
+
+struct scmi_msg_cmd_describe_levels {
+	__le32 domain_id;
+	__le32 level_index;
+};
+
+struct scmi_msg_resp_describe_levels {
+	__le32 flags;
+#define NUM_REMAINING_LEVELS(f)	(u16)(FIELD_GET(REMAINING_LEVELS_MASK, (f)))
+#define NUM_RETURNED_LEVELS(f)	(u16)(FIELD_GET(RETURNED_LEVELS_MASK, (f)))
+#define SUPPORTS_SEGMENTED_LEVELS(f)	((f) & BIT(12))
+	__le32 voltage[];
+};
+
+struct scmi_msg_cmd_config_set {
+	__le32 domain_id;
+	__le32 config;
+};
+
+struct scmi_msg_cmd_level_set {
+	__le32 domain_id;
+	__le32 flags;
+	__le32 voltage_level;
+};
+
+struct voltage_info {
+	u32 version;
+	u16 num_domains;
+	struct scmi_voltage_info **domains;
+};
+
+static int scmi_protocol_attributes_get(const struct scmi_handle *handle,
+					struct voltage_info *vinfo)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_resp_protocol_attributes *resp;
+
+	ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
+				 SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t);
+	if (ret)
+		return ret;
+
+	resp = t->rx.buf;
+	ret = scmi_do_xfer(handle, t);
+	if (!ret)
+		vinfo->num_domains =
+			NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr));
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static inline int scmi_init_voltage_levels(struct device *dev,
+					   struct scmi_voltage_info *v,
+					   u32 flags, u32 num_returned,
+					   u32 num_remaining)
+{
+	bool segmented;
+	u32 num_levels;
+
+	segmented = SUPPORTS_SEGMENTED_LEVELS(flags);
+	num_levels = num_returned + num_remaining;
+	/*
+	 * segmented levels entries are represented by a single triplet
+	 * returned all in one go.
+	 */
+	if (!num_levels ||
+	    (segmented && (num_remaining || num_returned != 3))) {
+		dev_err(dev,
+			"Invalid level descriptor(%d/%d/%d) for voltage dom %d\n",
+			num_levels, num_returned, num_remaining, v->id);
+		return -EINVAL;
+	}
+
+	v->levels_uv = devm_kcalloc(dev, num_levels, sizeof(u32), GFP_KERNEL);
+	if (!v->levels_uv)
+		return -ENOMEM;
+
+	v->num_levels = num_levels;
+	v->segmented = segmented;
+
+	return 0;
+}
+
+static int scmi_voltage_descriptors_get(const struct scmi_handle *handle,
+					struct voltage_info *vinfo)
+{
+	int ret, dom;
+	struct scmi_xfer *td, *tl;
+	struct device *dev = handle->dev;
+	struct scmi_msg_resp_domain_attributes *resp_dom;
+	struct scmi_msg_resp_describe_levels *resp_levels;
+
+	ret = scmi_xfer_get_init(handle, VOLTAGE_DOMAIN_ATTRIBUTES,
+				 SCMI_PROTOCOL_VOLTAGE, sizeof(__le32),
+				 sizeof(*resp_dom), &td);
+	if (ret)
+		return ret;
+	resp_dom = td->rx.buf;
+
+	ret = scmi_xfer_get_init(handle, VOLTAGE_DESCRIBE_LEVELS,
+				 SCMI_PROTOCOL_VOLTAGE, sizeof(__le64), 0, &tl);
+	if (ret)
+		goto outd;
+	resp_levels = tl->rx.buf;
+
+	for (dom = 0; dom < vinfo->num_domains; dom++) {
+		u32 desc_index = 0;
+		u16 num_returned = 0, num_remaining = 0;
+		struct scmi_msg_cmd_describe_levels *cmd;
+		struct scmi_voltage_info *v;
+
+		/* Retrieve domain attributes at first ... */
+		put_unaligned_le32(dom, td->tx.buf);
+		ret = scmi_do_xfer(handle, td);
+		/* Skip domain on comms error */
+		if (ret)
+			continue;
+
+		v = devm_kzalloc(dev, sizeof(*v), GFP_KERNEL);
+		if (!v) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		v->id = dom;
+		v->attributes = le32_to_cpu(resp_dom->attr);
+		strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
+
+		cmd = tl->tx.buf;
+		/* ...then retrieve domain levels descriptions */
+		do {
+			u32 flags;
+			int cnt;
+
+			cmd->domain_id = cpu_to_le32(v->id);
+			cmd->level_index = desc_index;
+			ret = scmi_do_xfer(handle, tl);
+			if (ret)
+				break;
+
+			flags = le32_to_cpu(resp_levels->flags);
+			num_returned = NUM_RETURNED_LEVELS(flags);
+			num_remaining = NUM_REMAINING_LEVELS(flags);
+
+			/* Allocate space for num_levels if not already done */
+			if (!v->num_levels) {
+				ret = scmi_init_voltage_levels(dev, v, flags,
+							       num_returned,
+							       num_remaining);
+				if (ret)
+					break;
+			}
+
+			if (desc_index + num_returned > v->num_levels) {
+				dev_err(handle->dev,
+					"No. of voltage levels can't exceed %d\n",
+					v->num_levels);
+				ret = -EINVAL;
+				break;
+			}
+
+			for (cnt = 0; cnt < num_returned; cnt++) {
+				s32 val;
+
+				val =
+				    (s32)le32_to_cpu(resp_levels->voltage[cnt]);
+				v->levels_uv[desc_index + cnt] = val;
+				if (val < 0)
+					v->negative_volts_allowed = true;
+			}
+
+			desc_index += num_returned;
+
+			scmi_reset_rx_to_maxsz(handle, tl);
+			/* check both to avoid infinite loop due to buggy fw */
+		} while (num_returned && num_remaining);
+
+		/*
+		 * Bail out only on memory errors, just skip domain and free
+		 * on any other error.
+		 */
+		if (!ret) {
+			vinfo->domains[dom] = v;
+		} else if (ret != -ENOMEM) {
+			devm_kfree(dev, v->levels_uv);
+			devm_kfree(dev, v);
+		} else {
+			break;
+		}
+
+		scmi_reset_rx_to_maxsz(handle, td);
+	}
+
+	scmi_xfer_put(handle, tl);
+outd:
+	scmi_xfer_put(handle, td);
+
+	return ret;
+}
+
+static int __scmi_voltage_get_u32(const struct scmi_handle *handle,
+				  u8 cmd_id, u32 domain_id, u32 *value)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct voltage_info *vinfo = handle->voltage_priv;
+
+	if (domain_id >= vinfo->num_domains)
+		return -EINVAL;
+
+	ret = scmi_xfer_get_init(handle, cmd_id,
+				 SCMI_PROTOCOL_VOLTAGE,
+				 sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(domain_id, t->tx.buf);
+	ret = scmi_do_xfer(handle, t);
+	if (!ret)
+		*value = get_unaligned_le32(t->rx.buf);
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_voltage_config_set(const struct scmi_handle *handle,
+				   u32 domain_id, u32 config)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct voltage_info *vinfo = handle->voltage_priv;
+	struct scmi_msg_cmd_config_set *cmd;
+
+	if (domain_id >= vinfo->num_domains)
+		return -EINVAL;
+
+	ret = scmi_xfer_get_init(handle, VOLTAGE_CONFIG_SET,
+				 SCMI_PROTOCOL_VOLTAGE,
+				 sizeof(*cmd), 0, &t);
+	if (ret)
+		return ret;
+
+	cmd = t->tx.buf;
+	cmd->domain_id = cpu_to_le32(domain_id);
+	cmd->config = cpu_to_le32(config & GENMASK(3, 0));
+
+	ret = scmi_do_xfer(handle, t);
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_voltage_config_get(const struct scmi_handle *handle,
+				   u32 domain_id, u32 *config)
+{
+	return __scmi_voltage_get_u32(handle, VOLTAGE_CONFIG_GET,
+				      domain_id, config);
+}
+
+static int scmi_voltage_level_set(const struct scmi_handle *handle,
+				  u32 domain_id, u32 flags, s32 volt_uV)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct voltage_info *vinfo = handle->voltage_priv;
+	struct scmi_msg_cmd_level_set *cmd;
+
+	if (domain_id >= vinfo->num_domains)
+		return -EINVAL;
+
+	ret = scmi_xfer_get_init(handle, VOLTAGE_LEVEL_SET,
+				 SCMI_PROTOCOL_VOLTAGE,
+				 sizeof(*cmd), 0, &t);
+	if (ret)
+		return ret;
+
+	cmd = t->tx.buf;
+	cmd->domain_id = cpu_to_le32(domain_id);
+	cmd->flags = cpu_to_le32(flags);
+	cmd->voltage_level = cpu_to_le32(volt_uV);
+
+	ret = scmi_do_xfer(handle, t);
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_voltage_level_get(const struct scmi_handle *handle,
+				  u32 domain_id, s32 *volt_uV)
+{
+	return __scmi_voltage_get_u32(handle, VOLTAGE_LEVEL_GET,
+				      domain_id, (u32 *)volt_uV);
+}
+
+static const struct scmi_voltage_info * __must_check
+scmi_voltage_info_get(const struct scmi_handle *handle, u32 domain_id)
+{
+	struct voltage_info *vinfo = handle->voltage_priv;
+
+	if (domain_id >= vinfo->num_domains)
+		return NULL;
+
+	return vinfo->domains[domain_id];
+}
+
+static int scmi_voltage_domains_num_get(const struct scmi_handle *handle)
+{
+	struct voltage_info *vinfo = handle->voltage_priv;
+
+	return vinfo->num_domains;
+}
+
+static struct scmi_voltage_ops voltage_ops = {
+	.num_domains_get = scmi_voltage_domains_num_get,
+	.info_get = scmi_voltage_info_get,
+	.config_set = scmi_voltage_config_set,
+	.config_get = scmi_voltage_config_get,
+	.level_set = scmi_voltage_level_set,
+	.level_get = scmi_voltage_level_get,
+};
+
+static int scmi_voltage_protocol_init(struct scmi_handle *handle)
+{
+	int ret;
+	u32 version;
+	struct voltage_info *vinfo;
+
+	ret = scmi_version_get(handle, SCMI_PROTOCOL_VOLTAGE, &version);
+	if (ret)
+		return ret;
+
+	dev_dbg(handle->dev, "Voltage Version %d.%d\n",
+		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	vinfo = devm_kzalloc(handle->dev, sizeof(*vinfo), GFP_KERNEL);
+	if (!vinfo)
+		return -ENOMEM;
+	vinfo->version = version;
+
+	ret = scmi_protocol_attributes_get(handle, vinfo);
+	if (ret)
+		return ret;
+
+	if (vinfo->num_domains) {
+		vinfo->domains = devm_kcalloc(handle->dev, vinfo->num_domains,
+					      sizeof(vinfo->domains),
+					      GFP_KERNEL);
+		if (!vinfo->domains)
+			return -ENOMEM;
+		ret = scmi_voltage_descriptors_get(handle, vinfo);
+		if (ret)
+			return ret;
+	} else {
+		dev_warn(handle->dev, "No Voltage domains found.\n");
+	}
+
+	handle->voltage_ops = &voltage_ops;
+	handle->voltage_priv = vinfo;
+
+	return 0;
+}
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_VOLTAGE, voltage)
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 9cd312a1ff92..181fdebc2793 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -209,6 +209,64 @@ struct scmi_reset_ops {
 	int (*deassert)(const struct scmi_handle *handle, u32 domain);
 };
 
+/**
+ * struct scmi_voltage_info - describe one available SCMI Voltage Domain
+ *
+ * @id: the domain ID as advertised by the platform
+ * @segmented: defines the layout of the entries of array @levels_uv.
+ *	       - when True the entries are to be interpreted as triplets,
+ *	         each defining a segment representing a range of equally
+ *	         space voltages: <lowest_volts>, <highest_volt>, <step_uV>
+ *	       - when False the entries simply represent a single discrete
+ *	         supported voltage level
+ * @negative_volts_allowed: True if any of the entries of @levels_uv represent
+ *			    a negative voltage.
+ * @attributes: represents Voltage Domain advertised attributes
+ * @name: name assigned to the Voltage Domain by platform
+ * @num_levels: number of total entries in @levels_uv.
+ * @levels_uv: array of entries describing the available voltage levels for
+ *	       this domain.
+ */
+struct scmi_voltage_info {
+	unsigned int id;
+	bool segmented;
+#define SCMI_VOLTAGE_SEGMENT_LOW	0
+#define SCMI_VOLTAGE_SEGMENT_HIGH	1
+#define SCMI_VOLTAGE_SEGMENT_STEP	2
+	bool negative_volts_allowed;
+	unsigned int attributes;
+	char name[SCMI_MAX_STR_SIZE];
+	unsigned int num_levels;
+	int *levels_uv;
+};
+
+/**
+ * struct scmi_voltage_ops - represents the various operations provided
+ * by SCMI Voltage Protocol
+ *
+ * @num_domains_get: get the count of voltage domains provided by SCMI
+ * @info_get: get the information of the specified domain
+ * @config_set: set the config for the specified domain
+ * @config_get: get the config of the specified domain
+ * @level_set: set the voltage level for the specified domain
+ * @level_get: get the voltage level of the specified domain
+ */
+struct scmi_voltage_ops {
+	int (*num_domains_get)(const struct scmi_handle *handle);
+	const struct scmi_voltage_info __must_check *(*info_get)
+		(const struct scmi_handle *handle, u32 domain_id);
+	int (*config_set)(const struct scmi_handle *handle, u32 domain_id,
+			  u32 config);
+#define	SCMI_VOLTAGE_ARCH_STATE_OFF		0x0
+#define	SCMI_VOLTAGE_ARCH_STATE_ON		0x7
+	int (*config_get)(const struct scmi_handle *handle, u32 domain_id,
+			  u32 *config);
+	int (*level_set)(const struct scmi_handle *handle, u32 domain_id,
+			 u32 flags, s32 volt_uV);
+	int (*level_get)(const struct scmi_handle *handle, u32 domain_id,
+			 s32 *volt_uV);
+};
+
 /**
  * struct scmi_notify_ops  - represents notifications' operations provided by
  * SCMI core
@@ -262,6 +320,7 @@ struct scmi_notify_ops {
  * @clk_ops: pointer to set of clock protocol operations
  * @sensor_ops: pointer to set of sensor protocol operations
  * @reset_ops: pointer to set of reset protocol operations
+ * @voltage_ops: pointer to set of voltage protocol operations
  * @notify_ops: pointer to set of notifications related operations
  * @perf_priv: pointer to private data structure specific to performance
  *	protocol(for internal use only)
@@ -273,6 +332,8 @@ struct scmi_notify_ops {
  *	protocol(for internal use only)
  * @reset_priv: pointer to private data structure specific to reset
  *	protocol(for internal use only)
+ * @voltage_priv: pointer to private data structure specific to voltage
+ *	protocol(for internal use only)
  * @notify_priv: pointer to private data structure specific to notifications
  *	(for internal use only)
  */
@@ -284,6 +345,7 @@ struct scmi_handle {
 	const struct scmi_power_ops *power_ops;
 	const struct scmi_sensor_ops *sensor_ops;
 	const struct scmi_reset_ops *reset_ops;
+	const struct scmi_voltage_ops *voltage_ops;
 	const struct scmi_notify_ops *notify_ops;
 	/* for protocol internal use */
 	void *perf_priv;
@@ -291,6 +353,7 @@ struct scmi_handle {
 	void *power_priv;
 	void *sensor_priv;
 	void *reset_priv;
+	void *voltage_priv;
 	void *notify_priv;
 	void *system_priv;
 };
@@ -303,6 +366,7 @@ enum scmi_std_protocol {
 	SCMI_PROTOCOL_CLOCK = 0x14,
 	SCMI_PROTOCOL_SENSOR = 0x15,
 	SCMI_PROTOCOL_RESET = 0x16,
+	SCMI_PROTOCOL_VOLTAGE = 0x17,
 };
 
 enum scmi_system_events {
-- 
2.17.1

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

Hi Cristian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on soc/for-next]
[also build test WARNING on next-20201026]
[cannot apply to regulator/for-next robh/for-next rockchip/for-next keystone/next arm64/for-next/core linux/master linux-rpi/for-rpi-next at91/at91-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Add-support-for-SCMIv3-0-Voltage-Domain-Protocol-and-SCMI-Regulator/20201027-043404
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: arm-randconfig-c004-20201026 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"coccinelle warnings: (new ones prefixed by >>)"
>> drivers/firmware/arm_scmi/voltage.c:381:11-17: ERROR: application of sizeof to pointer

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31699 bytes --]

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Cristian Marussi]

On Tue, Oct 27, 2020 at 02:04:39PM +0800, kernel test robot wrote:
> Hi Cristian,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on soc/for-next]
> [also build test WARNING on next-20201026]
> [cannot apply to regulator/for-next robh/for-next rockchip/for-next keystone/next arm64/for-next/core linux/master linux-rpi/for-rpi-next at91/at91-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Add-support-for-SCMIv3-0-Voltage-Domain-Protocol-and-SCMI-Regulator/20201027-043404
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
> config: arm-randconfig-c004-20201026 (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> "coccinelle warnings: (new ones prefixed by >>)"
> >> drivers/firmware/arm_scmi/voltage.c:381:11-17: ERROR: application of sizeof to pointer
> 
This is a false positive because the intention here was in fact to
allocate an array of pointers with devm_kcalloc().
Maybe I could explicitly use a sizeof(void *) to avoid fooling
coccicheck ?

Thanks

Cristian

> Please review and possibly fold the followup patch.
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[PATCH] firmware: arm_scmi: fix noderef.cocci warnings

[kernel test robot]

From: kernel test robot <lkp@intel.com>

drivers/firmware/arm_scmi/voltage.c:381:11-17: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: scripts/coccinelle/misc/noderef.cocci

CC: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Add-support-for-SCMIv3-0-Voltage-Domain-Protocol-and-SCMI-Regulator/20201027-043404
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next

 voltage.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -378,7 +378,7 @@ static int scmi_voltage_protocol_init(st
 
 	if (vinfo->num_domains) {
 		vinfo->domains = devm_kcalloc(handle->dev, vinfo->num_domains,
-					      sizeof(vinfo->domains),
+					      sizeof(*vinfo->domains),
 					      GFP_KERNEL);
 		if (!vinfo->domains)
 			return -ENOMEM;

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Etienne Carriere]

Hi Cristian,

Some remaining minor comments see below.
FYI I've successfully tested this series (the 4 patches).

Regards,
Etienne

On Mon, 26 Oct 2020 at 21:32, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> Add SCMI Voltage Domain protocol support.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v2 --> v3
> - restrict segmented voltage domain descriptors to one triplet
> - removed unneeded inline
> - free allocated resources for invalid voltage domain
> - added __must_check to info_get voltage operations
> - added a few comments
> - removed fixed size typing from struct voltage_info
>
> v1 --> v2
> - fix voltage levels query loop to reload full cmd description
>   between iterations as reported by Etienne Carriere
> - ensure rx buffer is properly sized calli scmi_reset_rx_to_maxsz
>   between transfers
> ---
>  drivers/firmware/arm_scmi/Makefile  |   2 +-
>  drivers/firmware/arm_scmi/common.h  |   1 +
>  drivers/firmware/arm_scmi/driver.c  |   2 +
>  drivers/firmware/arm_scmi/voltage.c | 398 ++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h       |  64 +++++
>  5 files changed, 466 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/arm_scmi/voltage.c
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index bc0d54f8e861..6a2ef63306d6 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o
>  scmi-transport-y = shmem.o
>  scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
>  scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.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/common.h b/drivers/firmware/arm_scmi/common.h
> index 65063fa948d4..c0fb45e7c3e8 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -169,6 +169,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(perf);
>  DECLARE_SCMI_REGISTER_UNREGISTER(power);
>  DECLARE_SCMI_REGISTER_UNREGISTER(reset);
>  DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> +DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
>  DECLARE_SCMI_REGISTER_UNREGISTER(system);
>
>  #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 3dfd8b6a0ebf..ada35e63feae 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -946,6 +946,7 @@ static int __init scmi_driver_init(void)
>         scmi_power_register();
>         scmi_reset_register();
>         scmi_sensors_register();
> +       scmi_voltage_register();
>         scmi_system_register();
>
>         return platform_driver_register(&scmi_driver);
> @@ -961,6 +962,7 @@ static void __exit scmi_driver_exit(void)
>         scmi_power_unregister();
>         scmi_reset_unregister();
>         scmi_sensors_unregister();
> +       scmi_voltage_unregister();
>         scmi_system_unregister();
>
>         platform_driver_unregister(&scmi_driver);
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> new file mode 100644
> index 000000000000..2c5a3c8b8649
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Voltage Protocol
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + */
> +
> +#include <linux/scmi_protocol.h>
> +
> +#include "common.h"
> +
> +#define VOLTAGE_DOMS_NUM_MASK          GENMASK(15, 0)
> +#define REMAINING_LEVELS_MASK          GENMASK(31, 16)
> +#define RETURNED_LEVELS_MASK           GENMASK(11, 0)
> +
> +enum scmi_voltage_protocol_cmd {
> +       VOLTAGE_DOMAIN_ATTRIBUTES = 0x3,
> +       VOLTAGE_DESCRIBE_LEVELS = 0x4,
> +       VOLTAGE_CONFIG_SET = 0x5,
> +       VOLTAGE_CONFIG_GET = 0x6,
> +       VOLTAGE_LEVEL_SET = 0x7,
> +       VOLTAGE_LEVEL_GET = 0x8,
> +};
> +
> +struct scmi_msg_resp_protocol_attributes {
> +       __le32 attr;
> +#define NUM_VOLTAGE_DOMAINS(x) (u16)(FIELD_GET(VOLTAGE_DOMS_NUM_MASK, (x)))
> +};
> +
> +struct scmi_msg_resp_domain_attributes {
> +       __le32 attr;
> +       u8 name[SCMI_MAX_STR_SIZE];
> +};
> +
> +struct scmi_msg_cmd_describe_levels {
> +       __le32 domain_id;
> +       __le32 level_index;
> +};
> +
> +struct scmi_msg_resp_describe_levels {
> +       __le32 flags;
> +#define NUM_REMAINING_LEVELS(f)        (u16)(FIELD_GET(REMAINING_LEVELS_MASK, (f)))
> +#define NUM_RETURNED_LEVELS(f) (u16)(FIELD_GET(RETURNED_LEVELS_MASK, (f)))
> +#define SUPPORTS_SEGMENTED_LEVELS(f)   ((f) & BIT(12))
> +       __le32 voltage[];
> +};
> +
> +struct scmi_msg_cmd_config_set {
> +       __le32 domain_id;
> +       __le32 config;
> +};
> +
> +struct scmi_msg_cmd_level_set {
> +       __le32 domain_id;
> +       __le32 flags;
> +       __le32 voltage_level;
> +};
> +
> +struct voltage_info {
> +       u32 version;
> +       u16 num_domains;

Could be an unsigned int.

> +       struct scmi_voltage_info **domains;
> +};
> +
> +static int scmi_protocol_attributes_get(const struct scmi_handle *handle,
> +                                       struct voltage_info *vinfo)
> +{
> +       int ret;
> +       struct scmi_xfer *t;
> +       struct scmi_msg_resp_protocol_attributes *resp;
> +
> +       ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
> +                                SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t);
> +       if (ret)
> +               return ret;
> +
> +       resp = t->rx.buf;
> +       ret = scmi_do_xfer(handle, t);
> +       if (!ret)
> +               vinfo->num_domains =
> +                       NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr));
> +
> +       scmi_xfer_put(handle, t);
> +       return ret;
> +}
> +
> +static inline int scmi_init_voltage_levels(struct device *dev,

Should remove this inline attribute.

> +                                          struct scmi_voltage_info *v,
> +                                          u32 flags, u32 num_returned,
> +                                          u32 num_remaining)
> +{
> +       bool segmented;
> +       u32 num_levels;
> +
> +       segmented = SUPPORTS_SEGMENTED_LEVELS(flags);
> +       num_levels = num_returned + num_remaining;
> +       /*
> +        * segmented levels entries are represented by a single triplet
> +        * returned all in one go.
> +        */
> +       if (!num_levels ||
> +           (segmented && (num_remaining || num_returned != 3))) {
> +               dev_err(dev,
> +                       "Invalid level descriptor(%d/%d/%d) for voltage dom %d\n",
> +                       num_levels, num_returned, num_remaining, v->id);
> +               return -EINVAL;
> +       }
> +
> +       v->levels_uv = devm_kcalloc(dev, num_levels, sizeof(u32), GFP_KERNEL);
> +       if (!v->levels_uv)
> +               return -ENOMEM;
> +
> +       v->num_levels = num_levels;
> +       v->segmented = segmented;
> +
> +       return 0;
> +}
> +
> +static int scmi_voltage_descriptors_get(const struct scmi_handle *handle,
> +                                       struct voltage_info *vinfo)
> +{
> +       int ret, dom;
> +       struct scmi_xfer *td, *tl;
> +       struct device *dev = handle->dev;
> +       struct scmi_msg_resp_domain_attributes *resp_dom;
> +       struct scmi_msg_resp_describe_levels *resp_levels;
> +
> +       ret = scmi_xfer_get_init(handle, VOLTAGE_DOMAIN_ATTRIBUTES,
> +                                SCMI_PROTOCOL_VOLTAGE, sizeof(__le32),
> +                                sizeof(*resp_dom), &td);
> +       if (ret)
> +               return ret;
> +       resp_dom = td->rx.buf;
> +
> +       ret = scmi_xfer_get_init(handle, VOLTAGE_DESCRIBE_LEVELS,
> +                                SCMI_PROTOCOL_VOLTAGE, sizeof(__le64), 0, &tl);
> +       if (ret)
> +               goto outd;
> +       resp_levels = tl->rx.buf;
> +
> +       for (dom = 0; dom < vinfo->num_domains; dom++) {
> +               u32 desc_index = 0;
> +               u16 num_returned = 0, num_remaining = 0;
> +               struct scmi_msg_cmd_describe_levels *cmd;
> +               struct scmi_voltage_info *v;
> +
> +               /* Retrieve domain attributes at first ... */
> +               put_unaligned_le32(dom, td->tx.buf);
> +               ret = scmi_do_xfer(handle, td);
> +               /* Skip domain on comms error */
> +               if (ret)
> +                       continue;
> +
> +               v = devm_kzalloc(dev, sizeof(*v), GFP_KERNEL);
> +               if (!v) {
> +                       ret = -ENOMEM;
> +                       break;
> +               }
> +
> +               v->id = dom;
> +               v->attributes = le32_to_cpu(resp_dom->attr);
> +               strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
> +
> +               cmd = tl->tx.buf;
> +               /* ...then retrieve domain levels descriptions */
> +               do {
> +                       u32 flags;
> +                       int cnt;
> +
> +                       cmd->domain_id = cpu_to_le32(v->id);
> +                       cmd->level_index = desc_index;
> +                       ret = scmi_do_xfer(handle, tl);
> +                       if (ret)
> +                               break;
> +
> +                       flags = le32_to_cpu(resp_levels->flags);
> +                       num_returned = NUM_RETURNED_LEVELS(flags);
> +                       num_remaining = NUM_REMAINING_LEVELS(flags);
> +
> +                       /* Allocate space for num_levels if not already done */
> +                       if (!v->num_levels) {
> +                               ret = scmi_init_voltage_levels(dev, v, flags,
> +                                                              num_returned,
> +                                                              num_remaining);
> +                               if (ret)
> +                                       break;
> +                       }
> +
> +                       if (desc_index + num_returned > v->num_levels) {
> +                               dev_err(handle->dev,
> +                                       "No. of voltage levels can't exceed %d\n",
> +                                       v->num_levels);
> +                               ret = -EINVAL;
> +                               break;
> +                       }
> +
> +                       for (cnt = 0; cnt < num_returned; cnt++) {
> +                               s32 val;
> +
> +                               val =
> +                                   (s32)le32_to_cpu(resp_levels->voltage[cnt]);
> +                               v->levels_uv[desc_index + cnt] = val;
> +                               if (val < 0)
> +                                       v->negative_volts_allowed = true;
> +                       }
> +
> +                       desc_index += num_returned;
> +
> +                       scmi_reset_rx_to_maxsz(handle, tl);
> +                       /* check both to avoid infinite loop due to buggy fw */
> +               } while (num_returned && num_remaining);
> +
> +               /*
> +                * Bail out only on memory errors, just skip domain and free
> +                * on any other error.
> +                */
> +               if (!ret) {
> +                       vinfo->domains[dom] = v;
> +               } else if (ret != -ENOMEM) {
> +                       devm_kfree(dev, v->levels_uv);
> +                       devm_kfree(dev, v);
> +               } else {
> +                       break;
> +               }
> +
> +               scmi_reset_rx_to_maxsz(handle, td);
> +       }
> +
> +       scmi_xfer_put(handle, tl);
> +outd:
> +       scmi_xfer_put(handle, td);
> +
> +       return ret;
> +}
> +
> +static int __scmi_voltage_get_u32(const struct scmi_handle *handle,
> +                                 u8 cmd_id, u32 domain_id, u32 *value)
> +{
> +       int ret;
> +       struct scmi_xfer *t;
> +       struct voltage_info *vinfo = handle->voltage_priv;
> +
> +       if (domain_id >= vinfo->num_domains)
> +               return -EINVAL;
> +
> +       ret = scmi_xfer_get_init(handle, cmd_id,
> +                                SCMI_PROTOCOL_VOLTAGE,
> +                                sizeof(__le32), 0, &t);
> +       if (ret)
> +               return ret;
> +
> +       put_unaligned_le32(domain_id, t->tx.buf);
> +       ret = scmi_do_xfer(handle, t);
> +       if (!ret)
> +               *value = get_unaligned_le32(t->rx.buf);
> +
> +       scmi_xfer_put(handle, t);
> +       return ret;
> +}
> +
> +static int scmi_voltage_config_set(const struct scmi_handle *handle,
> +                                  u32 domain_id, u32 config)
> +{
> +       int ret;
> +       struct scmi_xfer *t;
> +       struct voltage_info *vinfo = handle->voltage_priv;
> +       struct scmi_msg_cmd_config_set *cmd;
> +
> +       if (domain_id >= vinfo->num_domains)
> +               return -EINVAL;
> +
> +       ret = scmi_xfer_get_init(handle, VOLTAGE_CONFIG_SET,
> +                                SCMI_PROTOCOL_VOLTAGE,
> +                                sizeof(*cmd), 0, &t);
> +       if (ret)
> +               return ret;
> +
> +       cmd = t->tx.buf;
> +       cmd->domain_id = cpu_to_le32(domain_id);
> +       cmd->config = cpu_to_le32(config & GENMASK(3, 0));
> +
> +       ret = scmi_do_xfer(handle, t);
> +
> +       scmi_xfer_put(handle, t);
> +       return ret;
> +}
> +
> +static int scmi_voltage_config_get(const struct scmi_handle *handle,
> +                                  u32 domain_id, u32 *config)
> +{
> +       return __scmi_voltage_get_u32(handle, VOLTAGE_CONFIG_GET,
> +                                     domain_id, config);
> +}
> +
> +static int scmi_voltage_level_set(const struct scmi_handle *handle,
> +                                 u32 domain_id, u32 flags, s32 volt_uV)
> +{
> +       int ret;
> +       struct scmi_xfer *t;
> +       struct voltage_info *vinfo = handle->voltage_priv;
> +       struct scmi_msg_cmd_level_set *cmd;
> +
> +       if (domain_id >= vinfo->num_domains)
> +               return -EINVAL;
> +
> +       ret = scmi_xfer_get_init(handle, VOLTAGE_LEVEL_SET,
> +                                SCMI_PROTOCOL_VOLTAGE,
> +                                sizeof(*cmd), 0, &t);
> +       if (ret)
> +               return ret;
> +
> +       cmd = t->tx.buf;
> +       cmd->domain_id = cpu_to_le32(domain_id);
> +       cmd->flags = cpu_to_le32(flags);
> +       cmd->voltage_level = cpu_to_le32(volt_uV);
> +
> +       ret = scmi_do_xfer(handle, t);
> +
> +       scmi_xfer_put(handle, t);
> +       return ret;
> +}
> +
> +static int scmi_voltage_level_get(const struct scmi_handle *handle,
> +                                 u32 domain_id, s32 *volt_uV)
> +{
> +       return __scmi_voltage_get_u32(handle, VOLTAGE_LEVEL_GET,
> +                                     domain_id, (u32 *)volt_uV);
> +}
> +
> +static const struct scmi_voltage_info * __must_check
> +scmi_voltage_info_get(const struct scmi_handle *handle, u32 domain_id)
> +{
> +       struct voltage_info *vinfo = handle->voltage_priv;
> +
> +       if (domain_id >= vinfo->num_domains)
> +               return NULL;
> +
> +       return vinfo->domains[domain_id];
> +}
> +
> +static int scmi_voltage_domains_num_get(const struct scmi_handle *handle)
> +{
> +       struct voltage_info *vinfo = handle->voltage_priv;
> +
> +       return vinfo->num_domains;
> +}
> +
> +static struct scmi_voltage_ops voltage_ops = {
> +       .num_domains_get = scmi_voltage_domains_num_get,
> +       .info_get = scmi_voltage_info_get,
> +       .config_set = scmi_voltage_config_set,
> +       .config_get = scmi_voltage_config_get,
> +       .level_set = scmi_voltage_level_set,
> +       .level_get = scmi_voltage_level_get,
> +};
> +
> +static int scmi_voltage_protocol_init(struct scmi_handle *handle)
> +{
> +       int ret;
> +       u32 version;
> +       struct voltage_info *vinfo;
> +
> +       ret = scmi_version_get(handle, SCMI_PROTOCOL_VOLTAGE, &version);
> +       if (ret)
> +               return ret;
> +
> +       dev_dbg(handle->dev, "Voltage Version %d.%d\n",
> +               PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> +       vinfo = devm_kzalloc(handle->dev, sizeof(*vinfo), GFP_KERNEL);
> +       if (!vinfo)
> +               return -ENOMEM;
> +       vinfo->version = version;
> +
> +       ret = scmi_protocol_attributes_get(handle, vinfo);
> +       if (ret)
> +               return ret;
> +
> +       if (vinfo->num_domains) {
> +               vinfo->domains = devm_kcalloc(handle->dev, vinfo->num_domains,
> +                                             sizeof(vinfo->domains),
> +                                             GFP_KERNEL);
> +               if (!vinfo->domains)
> +                       return -ENOMEM;
> +               ret = scmi_voltage_descriptors_get(handle, vinfo);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               dev_warn(handle->dev, "No Voltage domains found.\n");
> +       }
> +
> +       handle->voltage_ops = &voltage_ops;
> +       handle->voltage_priv = vinfo;
> +
> +       return 0;
> +}
> +
> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_VOLTAGE, voltage)
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 9cd312a1ff92..181fdebc2793 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -209,6 +209,64 @@ struct scmi_reset_ops {
>         int (*deassert)(const struct scmi_handle *handle, u32 domain);
>  };
>
> +/**
> + * struct scmi_voltage_info - describe one available SCMI Voltage Domain
> + *
> + * @id: the domain ID as advertised by the platform
> + * @segmented: defines the layout of the entries of array @levels_uv.
> + *            - when True the entries are to be interpreted as triplets,
> + *              each defining a segment representing a range of equally
> + *              space voltages: <lowest_volts>, <highest_volt>, <step_uV>
> + *            - when False the entries simply represent a single discrete
> + *              supported voltage level
> + * @negative_volts_allowed: True if any of the entries of @levels_uv represent
> + *                         a negative voltage.
> + * @attributes: represents Voltage Domain advertised attributes
> + * @name: name assigned to the Voltage Domain by platform
> + * @num_levels: number of total entries in @levels_uv.
> + * @levels_uv: array of entries describing the available voltage levels for
> + *            this domain.
> + */
> +struct scmi_voltage_info {
> +       unsigned int id;
> +       bool segmented;
> +#define SCMI_VOLTAGE_SEGMENT_LOW       0
> +#define SCMI_VOLTAGE_SEGMENT_HIGH      1
> +#define SCMI_VOLTAGE_SEGMENT_STEP      2

Maybe move these macros before 'int *level_us;' as these are indices
in that array.

> +       bool negative_volts_allowed;
> +       unsigned int attributes;
> +       char name[SCMI_MAX_STR_SIZE];
> +       unsigned int num_levels;
> +       int *levels_uv;
> +};
> +
> +/**
> + * struct scmi_voltage_ops - represents the various operations provided
> + * by SCMI Voltage Protocol
> + *
> + * @num_domains_get: get the count of voltage domains provided by SCMI
> + * @info_get: get the information of the specified domain
> + * @config_set: set the config for the specified domain
> + * @config_get: get the config of the specified domain
> + * @level_set: set the voltage level for the specified domain
> + * @level_get: get the voltage level of the specified domain
> + */
> +struct scmi_voltage_ops {
> +       int (*num_domains_get)(const struct scmi_handle *handle);
> +       const struct scmi_voltage_info __must_check *(*info_get)
> +               (const struct scmi_handle *handle, u32 domain_id);
> +       int (*config_set)(const struct scmi_handle *handle, u32 domain_id,
> +                         u32 config);
> +#define        SCMI_VOLTAGE_ARCH_STATE_OFF             0x0
> +#define        SCMI_VOLTAGE_ARCH_STATE_ON              0x7
> +       int (*config_get)(const struct scmi_handle *handle, u32 domain_id,
> +                         u32 *config);
> +       int (*level_set)(const struct scmi_handle *handle, u32 domain_id,
> +                        u32 flags, s32 volt_uV);
> +       int (*level_get)(const struct scmi_handle *handle, u32 domain_id,
> +                        s32 *volt_uV);
> +};
> +
>  /**
>   * struct scmi_notify_ops  - represents notifications' operations provided by
>   * SCMI core
> @@ -262,6 +320,7 @@ struct scmi_notify_ops {
>   * @clk_ops: pointer to set of clock protocol operations
>   * @sensor_ops: pointer to set of sensor protocol operations
>   * @reset_ops: pointer to set of reset protocol operations
> + * @voltage_ops: pointer to set of voltage protocol operations
>   * @notify_ops: pointer to set of notifications related operations
>   * @perf_priv: pointer to private data structure specific to performance
>   *     protocol(for internal use only)
> @@ -273,6 +332,8 @@ struct scmi_notify_ops {
>   *     protocol(for internal use only)
>   * @reset_priv: pointer to private data structure specific to reset
>   *     protocol(for internal use only)
> + * @voltage_priv: pointer to private data structure specific to voltage
> + *     protocol(for internal use only)
>   * @notify_priv: pointer to private data structure specific to notifications
>   *     (for internal use only)
>   */
> @@ -284,6 +345,7 @@ struct scmi_handle {
>         const struct scmi_power_ops *power_ops;
>         const struct scmi_sensor_ops *sensor_ops;
>         const struct scmi_reset_ops *reset_ops;
> +       const struct scmi_voltage_ops *voltage_ops;
>         const struct scmi_notify_ops *notify_ops;
>         /* for protocol internal use */
>         void *perf_priv;
> @@ -291,6 +353,7 @@ struct scmi_handle {
>         void *power_priv;
>         void *sensor_priv;
>         void *reset_priv;
> +       void *voltage_priv;
>         void *notify_priv;
>         void *system_priv;
>  };
> @@ -303,6 +366,7 @@ enum scmi_std_protocol {
>         SCMI_PROTOCOL_CLOCK = 0x14,
>         SCMI_PROTOCOL_SENSOR = 0x15,
>         SCMI_PROTOCOL_RESET = 0x16,
> +       SCMI_PROTOCOL_VOLTAGE = 0x17,
>  };
>
>  enum scmi_system_events {
> --
> 2.17.1
>

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Cristian Marussi]

On Wed, Oct 28, 2020 at 03:29:57PM +0100, Etienne Carriere wrote:
> Hi Cristian,
> 
> Some remaining minor comments see below.
> FYI I've successfully tested this series (the 4 patches).
> 

Hi

Thanks a lot !

Replies inline down below.

[snip]
> > +struct scmi_msg_cmd_config_set {
> > +       __le32 domain_id;
> > +       __le32 config;
> > +};
> > +
> > +struct scmi_msg_cmd_level_set {
> > +       __le32 domain_id;
> > +       __le32 flags;
> > +       __le32 voltage_level;
> > +};
> > +
> > +struct voltage_info {
> > +       u32 version;
> > +       u16 num_domains;
> 
> Could be an unsigned int.
> 

I tend to use fixed size types matching the protocols messages sizing on
the internal while exposing non-fixed size types in scmi_protocol.h like
in scmi_voltage_info, but these indeed are values exposed directly to
the user afterwards. Any other reason to prefer non-fixed size here ?

> > +       struct scmi_voltage_info **domains;
> > +};
> > +
> > +static int scmi_protocol_attributes_get(const struct scmi_handle *handle,
> > +                                       struct voltage_info *vinfo)
> > +{
> > +       int ret;
> > +       struct scmi_xfer *t;
> > +       struct scmi_msg_resp_protocol_attributes *resp;
> > +
> > +       ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
> > +                                SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t);
> > +       if (ret)
> > +               return ret;
> > +
> > +       resp = t->rx.buf;
> > +       ret = scmi_do_xfer(handle, t);
> > +       if (!ret)
> > +               vinfo->num_domains =
> > +                       NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr));
> > +
> > +       scmi_xfer_put(handle, t);
> > +       return ret;
> > +}
> > +
> > +static inline int scmi_init_voltage_levels(struct device *dev,
> 
> Should remove this inline attribute.
> 

Ah right, I removed one and left this. I'll do.

> > +                                          struct scmi_voltage_info *v,
> > +                                          u32 flags, u32 num_returned,
> > +                                          u32 num_remaining)

[snip]

> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_VOLTAGE, voltage)
> > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > index 9cd312a1ff92..181fdebc2793 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -209,6 +209,64 @@ struct scmi_reset_ops {
> >         int (*deassert)(const struct scmi_handle *handle, u32 domain);
> >  };
> >
> > +/**
> > + * struct scmi_voltage_info - describe one available SCMI Voltage Domain
> > + *
> > + * @id: the domain ID as advertised by the platform
> > + * @segmented: defines the layout of the entries of array @levels_uv.
> > + *            - when True the entries are to be interpreted as triplets,
> > + *              each defining a segment representing a range of equally
> > + *              space voltages: <lowest_volts>, <highest_volt>, <step_uV>
> > + *            - when False the entries simply represent a single discrete
> > + *              supported voltage level
> > + * @negative_volts_allowed: True if any of the entries of @levels_uv represent
> > + *                         a negative voltage.
> > + * @attributes: represents Voltage Domain advertised attributes
> > + * @name: name assigned to the Voltage Domain by platform
> > + * @num_levels: number of total entries in @levels_uv.
> > + * @levels_uv: array of entries describing the available voltage levels for
> > + *            this domain.
> > + */
> > +struct scmi_voltage_info {
> > +       unsigned int id;
> > +       bool segmented;
> > +#define SCMI_VOLTAGE_SEGMENT_LOW       0
> > +#define SCMI_VOLTAGE_SEGMENT_HIGH      1
> > +#define SCMI_VOLTAGE_SEGMENT_STEP      2
> 
> Maybe move these macros before 'int *level_us;' as these are indices
> in that array.
> 

Right, I'll do.

Thanks

Cristian

> > +       bool negative_volts_allowed;
> > +       unsigned int attributes;
> > +       char name[SCMI_MAX_STR_SIZE];
> > +       unsigned int num_levels;
> > +       int *levels_uv;
> > +};
> > +
> > +/**
> > + * struct scmi_voltage_ops - represents the various operations provided
> > + * by SCMI Voltage Protocol
> > + *
> > + * @num_domains_get: get the count of voltage domains provided by SCMI
> > + * @info_get: get the information of the specified domain
> > + * @config_set: set the config for the specified domain
> > + * @config_get: get the config of the specified domain
> > + * @level_set: set the voltage level for the specified domain
> > + * @level_get: get the voltage level of the specified domain
> > + */
> > +struct scmi_voltage_ops {
> > +       int (*num_domains_get)(const struct scmi_handle *handle);
> > +       const struct scmi_voltage_info __must_check *(*info_get)
> > +               (const struct scmi_handle *handle, u32 domain_id);
> > +       int (*config_set)(const struct scmi_handle *handle, u32 domain_id,
> > +                         u32 config);
> > +#define        SCMI_VOLTAGE_ARCH_STATE_OFF             0x0
> > +#define        SCMI_VOLTAGE_ARCH_STATE_ON              0x7
> > +       int (*config_get)(const struct scmi_handle *handle, u32 domain_id,
> > +                         u32 *config);
> > +       int (*level_set)(const struct scmi_handle *handle, u32 domain_id,
> > +                        u32 flags, s32 volt_uV);
> > +       int (*level_get)(const struct scmi_handle *handle, u32 domain_id,
> > +                        s32 *volt_uV);
> > +};
> > +
> >  /**
> >   * struct scmi_notify_ops  - represents notifications' operations provided by
> >   * SCMI core
> > @@ -262,6 +320,7 @@ struct scmi_notify_ops {
> >   * @clk_ops: pointer to set of clock protocol operations
> >   * @sensor_ops: pointer to set of sensor protocol operations
> >   * @reset_ops: pointer to set of reset protocol operations
> > + * @voltage_ops: pointer to set of voltage protocol operations
> >   * @notify_ops: pointer to set of notifications related operations
> >   * @perf_priv: pointer to private data structure specific to performance
> >   *     protocol(for internal use only)
> > @@ -273,6 +332,8 @@ struct scmi_notify_ops {
> >   *     protocol(for internal use only)
> >   * @reset_priv: pointer to private data structure specific to reset
> >   *     protocol(for internal use only)
> > + * @voltage_priv: pointer to private data structure specific to voltage
> > + *     protocol(for internal use only)
> >   * @notify_priv: pointer to private data structure specific to notifications
> >   *     (for internal use only)
> >   */
> > @@ -284,6 +345,7 @@ struct scmi_handle {
> >         const struct scmi_power_ops *power_ops;
> >         const struct scmi_sensor_ops *sensor_ops;
> >         const struct scmi_reset_ops *reset_ops;
> > +       const struct scmi_voltage_ops *voltage_ops;
> >         const struct scmi_notify_ops *notify_ops;
> >         /* for protocol internal use */
> >         void *perf_priv;
> > @@ -291,6 +353,7 @@ struct scmi_handle {
> >         void *power_priv;
> >         void *sensor_priv;
> >         void *reset_priv;
> > +       void *voltage_priv;
> >         void *notify_priv;
> >         void *system_priv;
> >  };
> > @@ -303,6 +366,7 @@ enum scmi_std_protocol {
> >         SCMI_PROTOCOL_CLOCK = 0x14,
> >         SCMI_PROTOCOL_SENSOR = 0x15,
> >         SCMI_PROTOCOL_RESET = 0x16,
> > +       SCMI_PROTOCOL_VOLTAGE = 0x17,
> >  };
> >
> >  enum scmi_system_events {
> > --
> > 2.17.1
> >

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Etienne Carriere]

On Wed, 28 Oct 2020 at 21:44, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Wed, Oct 28, 2020 at 03:29:57PM +0100, Etienne Carriere wrote:
> > Hi Cristian,
> >
> > Some remaining minor comments see below.
> > FYI I've successfully tested this series (the 4 patches).
> >
>
> Hi
>
> Thanks a lot !
>
> Replies inline down below.
>
> [snip]
> > > +struct scmi_msg_cmd_config_set {
> > > +       __le32 domain_id;
> > > +       __le32 config;
> > > +};
> > > +
> > > +struct scmi_msg_cmd_level_set {
> > > +       __le32 domain_id;
> > > +       __le32 flags;
> > > +       __le32 voltage_level;
> > > +};
> > > +
> > > +struct voltage_info {
> > > +       u32 version;
> > > +       u16 num_domains;
> >
> > Could be an unsigned int.
> >
>
> I tend to use fixed size types matching the protocols messages sizing on
> the internal while exposing non-fixed size types in scmi_protocol.h like
> in scmi_voltage_info, but these indeed are values exposed directly to
> the user afterwards. Any other reason to prefer non-fixed size here ?

I think fixed size should be used where really needed.
Since in this structure you're quite abstracted from the protocol, a generic
type is preferred I think.

Regards,
Etienne

>
> > > +       struct scmi_voltage_info **domains;
> > > +};
> > > +
> > > +static int scmi_protocol_attributes_get(const struct scmi_handle *handle,
> > > +                                       struct voltage_info *vinfo)
> > > +{
> > > +       int ret;
> > > +       struct scmi_xfer *t;
> > > +       struct scmi_msg_resp_protocol_attributes *resp;
> > > +
> > > +       ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
> > > +                                SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       resp = t->rx.buf;
> > > +       ret = scmi_do_xfer(handle, t);
> > > +       if (!ret)
> > > +               vinfo->num_domains =
> > > +                       NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr));
> > > +
> > > +       scmi_xfer_put(handle, t);
> > > +       return ret;
> > > +}
> > > +
> > > +static inline int scmi_init_voltage_levels(struct device *dev,
> >
> > Should remove this inline attribute.
> >
>
> Ah right, I removed one and left this. I'll do.
>
> > > +                                          struct scmi_voltage_info *v,
> > > +                                          u32 flags, u32 num_returned,
> > > +                                          u32 num_remaining)
>
> [snip]
>
> > > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_VOLTAGE, voltage)
> > > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > > index 9cd312a1ff92..181fdebc2793 100644
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -209,6 +209,64 @@ struct scmi_reset_ops {
> > >         int (*deassert)(const struct scmi_handle *handle, u32 domain);
> > >  };
> > >
> > > +/**
> > > + * struct scmi_voltage_info - describe one available SCMI Voltage Domain
> > > + *
> > > + * @id: the domain ID as advertised by the platform
> > > + * @segmented: defines the layout of the entries of array @levels_uv.
> > > + *            - when True the entries are to be interpreted as triplets,
> > > + *              each defining a segment representing a range of equally
> > > + *              space voltages: <lowest_volts>, <highest_volt>, <step_uV>
> > > + *            - when False the entries simply represent a single discrete
> > > + *              supported voltage level
> > > + * @negative_volts_allowed: True if any of the entries of @levels_uv represent
> > > + *                         a negative voltage.
> > > + * @attributes: represents Voltage Domain advertised attributes
> > > + * @name: name assigned to the Voltage Domain by platform
> > > + * @num_levels: number of total entries in @levels_uv.
> > > + * @levels_uv: array of entries describing the available voltage levels for
> > > + *            this domain.
> > > + */
> > > +struct scmi_voltage_info {
> > > +       unsigned int id;
> > > +       bool segmented;
> > > +#define SCMI_VOLTAGE_SEGMENT_LOW       0
> > > +#define SCMI_VOLTAGE_SEGMENT_HIGH      1
> > > +#define SCMI_VOLTAGE_SEGMENT_STEP      2
> >
> > Maybe move these macros before 'int *level_us;' as these are indices
> > in that array.
> >
>
> Right, I'll do.
>
> Thanks
>
> Cristian
>
> > > +       bool negative_volts_allowed;
> > > +       unsigned int attributes;
> > > +       char name[SCMI_MAX_STR_SIZE];
> > > +       unsigned int num_levels;
> > > +       int *levels_uv;
> > > +};
> > > +
> > > +/**
> > > + * struct scmi_voltage_ops - represents the various operations provided
> > > + * by SCMI Voltage Protocol
> > > + *
> > > + * @num_domains_get: get the count of voltage domains provided by SCMI
> > > + * @info_get: get the information of the specified domain
> > > + * @config_set: set the config for the specified domain
> > > + * @config_get: get the config of the specified domain
> > > + * @level_set: set the voltage level for the specified domain
> > > + * @level_get: get the voltage level of the specified domain
> > > + */
> > > +struct scmi_voltage_ops {
> > > +       int (*num_domains_get)(const struct scmi_handle *handle);
> > > +       const struct scmi_voltage_info __must_check *(*info_get)
> > > +               (const struct scmi_handle *handle, u32 domain_id);
> > > +       int (*config_set)(const struct scmi_handle *handle, u32 domain_id,
> > > +                         u32 config);
> > > +#define        SCMI_VOLTAGE_ARCH_STATE_OFF             0x0
> > > +#define        SCMI_VOLTAGE_ARCH_STATE_ON              0x7
> > > +       int (*config_get)(const struct scmi_handle *handle, u32 domain_id,
> > > +                         u32 *config);
> > > +       int (*level_set)(const struct scmi_handle *handle, u32 domain_id,
> > > +                        u32 flags, s32 volt_uV);
> > > +       int (*level_get)(const struct scmi_handle *handle, u32 domain_id,
> > > +                        s32 *volt_uV);
> > > +};
> > > +
> > >  /**
> > >   * struct scmi_notify_ops  - represents notifications' operations provided by
> > >   * SCMI core
> > > @@ -262,6 +320,7 @@ struct scmi_notify_ops {
> > >   * @clk_ops: pointer to set of clock protocol operations
> > >   * @sensor_ops: pointer to set of sensor protocol operations
> > >   * @reset_ops: pointer to set of reset protocol operations
> > > + * @voltage_ops: pointer to set of voltage protocol operations
> > >   * @notify_ops: pointer to set of notifications related operations
> > >   * @perf_priv: pointer to private data structure specific to performance
> > >   *     protocol(for internal use only)
> > > @@ -273,6 +332,8 @@ struct scmi_notify_ops {
> > >   *     protocol(for internal use only)
> > >   * @reset_priv: pointer to private data structure specific to reset
> > >   *     protocol(for internal use only)
> > > + * @voltage_priv: pointer to private data structure specific to voltage
> > > + *     protocol(for internal use only)
> > >   * @notify_priv: pointer to private data structure specific to notifications
> > >   *     (for internal use only)
> > >   */
> > > @@ -284,6 +345,7 @@ struct scmi_handle {
> > >         const struct scmi_power_ops *power_ops;
> > >         const struct scmi_sensor_ops *sensor_ops;
> > >         const struct scmi_reset_ops *reset_ops;
> > > +       const struct scmi_voltage_ops *voltage_ops;
> > >         const struct scmi_notify_ops *notify_ops;
> > >         /* for protocol internal use */
> > >         void *perf_priv;
> > > @@ -291,6 +353,7 @@ struct scmi_handle {
> > >         void *power_priv;
> > >         void *sensor_priv;
> > >         void *reset_priv;
> > > +       void *voltage_priv;
> > >         void *notify_priv;
> > >         void *system_priv;
> > >  };
> > > @@ -303,6 +366,7 @@ enum scmi_std_protocol {
> > >         SCMI_PROTOCOL_CLOCK = 0x14,
> > >         SCMI_PROTOCOL_SENSOR = 0x15,
> > >         SCMI_PROTOCOL_RESET = 0x16,
> > > +       SCMI_PROTOCOL_VOLTAGE = 0x17,
> > >  };
> > >
> > >  enum scmi_system_events {
> > > --
> > > 2.17.1
> > >

Re: [PATCH v3 1/4] firmware: arm_scmi: Add Voltage Domain Support

[Cristian Marussi]

On Thu, Oct 29, 2020 at 12:21:56PM +0100, Etienne Carriere wrote:
> On Wed, 28 Oct 2020 at 21:44, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > On Wed, Oct 28, 2020 at 03:29:57PM +0100, Etienne Carriere wrote:
> > > Hi Cristian,
> > >
> > > Some remaining minor comments see below.
> > > FYI I've successfully tested this series (the 4 patches).
> > >
> >
> > Hi
> >
> > Thanks a lot !
> >
> > Replies inline down below.
> >
> > [snip]
> > > > +struct scmi_msg_cmd_config_set {
> > > > +       __le32 domain_id;
> > > > +       __le32 config;
> > > > +};
> > > > +
> > > > +struct scmi_msg_cmd_level_set {
> > > > +       __le32 domain_id;
> > > > +       __le32 flags;
> > > > +       __le32 voltage_level;
> > > > +};
> > > > +
> > > > +struct voltage_info {
> > > > +       u32 version;
> > > > +       u16 num_domains;
> > >
> > > Could be an unsigned int.
> > >
> >
> > I tend to use fixed size types matching the protocols messages sizing on
> > the internal while exposing non-fixed size types in scmi_protocol.h like
> > in scmi_voltage_info, but these indeed are values exposed directly to
> > the user afterwards. Any other reason to prefer non-fixed size here ?
> 
> I think fixed size should be used where really needed.
> Since in this structure you're quite abstracted from the protocol, a generic
> type is preferred I think.
> 

Ok, I'll fix in V4.

Thanks

Cristian

> Regards,
> Etienne
> 
> >
> > > > +       struct scmi_voltage_info **domains;
> > > > +};
> > > > +
> > > > +static int scmi_protocol_attributes_get(const struct scmi_handle *handle,
> > > > +                                       struct voltage_info *vinfo)
> > > > +{
> > > > +       int ret;
> > > > +       struct scmi_xfer *t;
> > > > +       struct scmi_msg_resp_protocol_attributes *resp;
> > > > +
> > > > +       ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
> > > > +                                SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       resp = t->rx.buf;
> > > > +       ret = scmi_do_xfer(handle, t);
> > > > +       if (!ret)
> > > > +               vinfo->num_domains =
> > > > +                       NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr));
> > > > +
> > > > +       scmi_xfer_put(handle, t);
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static inline int scmi_init_voltage_levels(struct device *dev,
> > >
> > > Should remove this inline attribute.
> > >
> >
> > Ah right, I removed one and left this. I'll do.
> >
> > > > +                                          struct scmi_voltage_info *v,
> > > > +                                          u32 flags, u32 num_returned,
> > > > +                                          u32 num_remaining)
> >
> > [snip]
> >
> > > > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_VOLTAGE, voltage)
> > > > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > > > index 9cd312a1ff92..181fdebc2793 100644
> > > > --- a/include/linux/scmi_protocol.h
> > > > +++ b/include/linux/scmi_protocol.h
> > > > @@ -209,6 +209,64 @@ struct scmi_reset_ops {
> > > >         int (*deassert)(const struct scmi_handle *handle, u32 domain);
> > > >  };
> > > >
> > > > +/**
> > > > + * struct scmi_voltage_info - describe one available SCMI Voltage Domain
> > > > + *
> > > > + * @id: the domain ID as advertised by the platform
> > > > + * @segmented: defines the layout of the entries of array @levels_uv.
> > > > + *            - when True the entries are to be interpreted as triplets,
> > > > + *              each defining a segment representing a range of equally
> > > > + *              space voltages: <lowest_volts>, <highest_volt>, <step_uV>
> > > > + *            - when False the entries simply represent a single discrete
> > > > + *              supported voltage level
> > > > + * @negative_volts_allowed: True if any of the entries of @levels_uv represent
> > > > + *                         a negative voltage.
> > > > + * @attributes: represents Voltage Domain advertised attributes
> > > > + * @name: name assigned to the Voltage Domain by platform
> > > > + * @num_levels: number of total entries in @levels_uv.
> > > > + * @levels_uv: array of entries describing the available voltage levels for
> > > > + *            this domain.
> > > > + */
> > > > +struct scmi_voltage_info {
> > > > +       unsigned int id;
> > > > +       bool segmented;
> > > > +#define SCMI_VOLTAGE_SEGMENT_LOW       0
> > > > +#define SCMI_VOLTAGE_SEGMENT_HIGH      1
> > > > +#define SCMI_VOLTAGE_SEGMENT_STEP      2
> > >
> > > Maybe move these macros before 'int *level_us;' as these are indices
> > > in that array.
> > >
> >
> > Right, I'll do.
> >
> > Thanks
> >
> > Cristian
> >
> > > > +       bool negative_volts_allowed;
> > > > +       unsigned int attributes;
> > > > +       char name[SCMI_MAX_STR_SIZE];
> > > > +       unsigned int num_levels;
> > > > +       int *levels_uv;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct scmi_voltage_ops - represents the various operations provided
> > > > + * by SCMI Voltage Protocol
> > > > + *
> > > > + * @num_domains_get: get the count of voltage domains provided by SCMI
> > > > + * @info_get: get the information of the specified domain
> > > > + * @config_set: set the config for the specified domain
> > > > + * @config_get: get the config of the specified domain
> > > > + * @level_set: set the voltage level for the specified domain
> > > > + * @level_get: get the voltage level of the specified domain
> > > > + */
> > > > +struct scmi_voltage_ops {
> > > > +       int (*num_domains_get)(const struct scmi_handle *handle);
> > > > +       const struct scmi_voltage_info __must_check *(*info_get)
> > > > +               (const struct scmi_handle *handle, u32 domain_id);
> > > > +       int (*config_set)(const struct scmi_handle *handle, u32 domain_id,
> > > > +                         u32 config);
> > > > +#define        SCMI_VOLTAGE_ARCH_STATE_OFF             0x0
> > > > +#define        SCMI_VOLTAGE_ARCH_STATE_ON              0x7
> > > > +       int (*config_get)(const struct scmi_handle *handle, u32 domain_id,
> > > > +                         u32 *config);
> > > > +       int (*level_set)(const struct scmi_handle *handle, u32 domain_id,
> > > > +                        u32 flags, s32 volt_uV);
> > > > +       int (*level_get)(const struct scmi_handle *handle, u32 domain_id,
> > > > +                        s32 *volt_uV);
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct scmi_notify_ops  - represents notifications' operations provided by
> > > >   * SCMI core
> > > > @@ -262,6 +320,7 @@ struct scmi_notify_ops {
> > > >   * @clk_ops: pointer to set of clock protocol operations
> > > >   * @sensor_ops: pointer to set of sensor protocol operations
> > > >   * @reset_ops: pointer to set of reset protocol operations
> > > > + * @voltage_ops: pointer to set of voltage protocol operations
> > > >   * @notify_ops: pointer to set of notifications related operations
> > > >   * @perf_priv: pointer to private data structure specific to performance
> > > >   *     protocol(for internal use only)
> > > > @@ -273,6 +332,8 @@ struct scmi_notify_ops {
> > > >   *     protocol(for internal use only)
> > > >   * @reset_priv: pointer to private data structure specific to reset
> > > >   *     protocol(for internal use only)
> > > > + * @voltage_priv: pointer to private data structure specific to voltage
> > > > + *     protocol(for internal use only)
> > > >   * @notify_priv: pointer to private data structure specific to notifications
> > > >   *     (for internal use only)
> > > >   */
> > > > @@ -284,6 +345,7 @@ struct scmi_handle {
> > > >         const struct scmi_power_ops *power_ops;
> > > >         const struct scmi_sensor_ops *sensor_ops;
> > > >         const struct scmi_reset_ops *reset_ops;
> > > > +       const struct scmi_voltage_ops *voltage_ops;
> > > >         const struct scmi_notify_ops *notify_ops;
> > > >         /* for protocol internal use */
> > > >         void *perf_priv;
> > > > @@ -291,6 +353,7 @@ struct scmi_handle {
> > > >         void *power_priv;
> > > >         void *sensor_priv;
> > > >         void *reset_priv;
> > > > +       void *voltage_priv;
> > > >         void *notify_priv;
> > > >         void *system_priv;
> > > >  };
> > > > @@ -303,6 +366,7 @@ enum scmi_std_protocol {
> > > >         SCMI_PROTOCOL_CLOCK = 0x14,
> > > >         SCMI_PROTOCOL_SENSOR = 0x15,
> > > >         SCMI_PROTOCOL_RESET = 0x16,
> > > > +       SCMI_PROTOCOL_VOLTAGE = 0x17,
> > > >  };
> > > >
> > > >  enum scmi_system_events {
> > > > --
> > > > 2.17.1
> > > >

[PATCH v3 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname

[Cristian Marussi]

Add SCMI Voltage Domain device name to the core list of supported protocol
devices.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ada35e63feae..5392e1fc6b4e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -743,6 +743,7 @@ static struct scmi_prot_devnames devnames[] = {
 	{ SCMI_PROTOCOL_CLOCK,  { "clocks" },},
 	{ SCMI_PROTOCOL_SENSOR, { "hwmon" },},
 	{ SCMI_PROTOCOL_RESET,  { "reset" },},
+	{ SCMI_PROTOCOL_VOLTAGE,  { "regulator" },},
 };
 
 static inline void
-- 
2.17.1

[PATCH v3 3/4] regulator: add SCMI driver

[Cristian Marussi]

Add a simple regulator based on SCMI Voltage Domain Protocol.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
----
v2 --> v3
- remove multiple linear mappings support
- removed duplicated voltage name printout
- added a few comments
- simplified return path in scmi_reg_set_voltage_sel()

v1 --> v2
- removed duplicate regulator naming
- removed redundant .get/set_voltage ops: only _sel variants implemented
- removed condexpr on fail path to increase readability

v0 --> v1
- fixed init_data constraint parsing
- fixes for v5.8 (linear_range.h)
- fixed commit message content and subject line format
- factored out SCMI core specific changes to distinct patch
- reworked Kconfig and Makefile to keep proper alphabetic order
- fixed SPDX comment style
- removed unneeded inline functions
- reworked conditionals for legibility
- fixed some return paths to properly report SCMI original errors codes
- added some more descriptive error messages when fw returns invalid ranges
- removed unneeded explicit devm_regulator_unregister from .remove()
---
 drivers/regulator/Kconfig          |   9 +
 drivers/regulator/Makefile         |   1 +
 drivers/regulator/scmi-regulator.c | 403 +++++++++++++++++++++++++++++
 3 files changed, 413 insertions(+)
 create mode 100644 drivers/regulator/scmi-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index de17ef7e18f0..6d3a10cb9833 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -155,6 +155,15 @@ config REGULATOR_ARIZONA_MICSUPP
 	  and Wolfson Microelectronic Arizona codecs
 	  devices.
 
+config REGULATOR_ARM_SCMI
+	tristate "SCMI based regulator driver"
+	depends on ARM_SCMI_PROTOCOL && OF
+	help
+	  This adds the regulator driver support for ARM platforms using SCMI
+	  protocol for device voltage management.
+	  This driver uses SCMI Message Protocol driver to interact with the
+	  firmware providing the device Voltage functionality.
+
 config REGULATOR_AS3711
 	tristate "AS3711 PMIC"
 	depends on MFD_AS3711
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d8d3ecf526a8..0532a7393d5d 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
 obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 obj-$(CONFIG_REGULATOR_ARIZONA_LDO1) += arizona-ldo1.o
 obj-$(CONFIG_REGULATOR_ARIZONA_MICSUPP) += arizona-micsupp.o
+obj-$(CONFIG_REGULATOR_ARM_SCMI) += scmi-regulator.o
 obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
new file mode 100644
index 000000000000..e8d75e737bae
--- /dev/null
+++ b/drivers/regulator/scmi-regulator.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// System Control and Management Interface (SCMI) based regulator driver
+//
+// Copyright (C) 2020 ARM Ltd.
+//
+// Implements a regulator driver on top of the SCMI Voltage Protocol.
+//
+// The ARM SCMI Protocol aims in general to hide as much as possible all the
+// underlying operational details while providing an abstracted interface for
+// its users to operate upon: as a consequence the resulting operational
+// capabilities and configurability of this regulator device are much more
+// limited than the ones usually available on a standard physical regulator.
+//
+// The supported SCMI regulator ops are restricted to the bare minimum:
+//
+//  - 'status_ops': enable/disable/is_enabled
+//  - 'voltage_ops': get_voltage_sel/set_voltage_sel
+//		     list_voltage/map_voltage
+//
+// Each SCMI regulator instance is associated, through the means of a proper DT
+// entry description, to a specific SCMI Voltage Domain.
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/linear_range.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct scmi_regulator {
+	u32 id;
+	struct scmi_device *sdev;
+	struct regulator_dev *rdev;
+	struct device_node *of_node;
+	struct regulator_desc desc;
+	struct regulator_config conf;
+};
+
+struct scmi_regulator_info {
+	int num_doms;
+	struct scmi_regulator **sregv;
+};
+
+static int scmi_reg_enable(struct regulator_dev *rdev)
+{
+	struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+	const struct scmi_handle *handle = sreg->sdev->handle;
+
+	return handle->voltage_ops->config_set(handle, sreg->id,
+					       SCMI_VOLTAGE_ARCH_STATE_ON);
+}
+
+static int scmi_reg_disable(struct regulator_dev *rdev)
+{
+	struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+	const struct scmi_handle *handle = sreg->sdev->handle;
+
+	return handle->voltage_ops->config_set(handle, sreg->id,
+					       SCMI_VOLTAGE_ARCH_STATE_OFF);
+}
+
+static int scmi_reg_is_enabled(struct regulator_dev *rdev)
+{
+	int ret;
+	u32 config;
+	struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+	const struct scmi_handle *handle = sreg->sdev->handle;
+
+	ret = handle->voltage_ops->config_get(handle, sreg->id,
+					      &config);
+	if (ret) {
+		dev_err(&sreg->sdev->dev,
+			"Error %d reading regulator %s status.\n",
+			ret, sreg->desc.name);
+		return 0;
+	}
+
+	return config & SCMI_VOLTAGE_ARCH_STATE_ON;
+}
+
+static int scmi_reg_get_voltage_sel(struct regulator_dev *rdev)
+{
+	int ret;
+	s32 volt_uV;
+	struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+	const struct scmi_handle *handle = sreg->sdev->handle;
+
+	ret = handle->voltage_ops->level_get(handle, sreg->id, &volt_uV);
+	if (ret)
+		return ret;
+
+	return sreg->desc.ops->map_voltage(rdev, volt_uV, volt_uV);
+}
+
+static int scmi_reg_set_voltage_sel(struct regulator_dev *rdev,
+				    unsigned int selector)
+{
+	s32 volt_uV;
+	struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+	const struct scmi_handle *handle = sreg->sdev->handle;
+
+	volt_uV = sreg->desc.ops->list_voltage(rdev, selector);
+	if (volt_uV <= 0)
+		return -EINVAL;
+
+	return handle->voltage_ops->level_set(handle, sreg->id, 0x0, volt_uV);
+}
+
+static const struct regulator_ops scmi_reg_fixed_ops = {
+	.enable = scmi_reg_enable,
+	.disable = scmi_reg_disable,
+	.is_enabled = scmi_reg_is_enabled,
+};
+
+static const struct regulator_ops scmi_reg_linear_ops = {
+	.enable = scmi_reg_enable,
+	.disable = scmi_reg_disable,
+	.is_enabled = scmi_reg_is_enabled,
+	.get_voltage_sel = scmi_reg_get_voltage_sel,
+	.set_voltage_sel = scmi_reg_set_voltage_sel,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+};
+
+static const struct regulator_ops scmi_reg_discrete_ops = {
+	.enable = scmi_reg_enable,
+	.disable = scmi_reg_disable,
+	.is_enabled = scmi_reg_is_enabled,
+	.get_voltage_sel = scmi_reg_get_voltage_sel,
+	.set_voltage_sel = scmi_reg_set_voltage_sel,
+	.list_voltage = regulator_list_voltage_table,
+	.map_voltage = regulator_map_voltage_iterate,
+};
+
+static int
+scmi_config_linear_regulator_mappings(struct scmi_regulator *sreg,
+				      const struct scmi_voltage_info *vinfo)
+{
+	s32 delta_uV;
+
+	/*
+	 * Note that SCMI voltage domains describable by linear ranges
+	 * (segments) {low, high, step} are guaranteed to come in one single
+	 * triplet by the SCMI Voltage Domain protocol support itself.
+	 */
+
+	delta_uV = (vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_HIGH] -
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_LOW]);
+	/* Rule out buggy negative-intervals answers from fw */
+	if (delta_uV < 0) {
+		dev_err(&sreg->sdev->dev,
+			"Invalid volt-range %d-%duV for domain %d\n",
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_LOW],
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_HIGH],
+			sreg->id);
+		return -EINVAL;
+	}
+
+	if (!delta_uV) {
+		/* Just one fixed voltage exposed by SCMI */
+		sreg->desc.fixed_uV =
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_LOW];
+		sreg->desc.n_voltages = 1;
+		sreg->desc.ops = &scmi_reg_fixed_ops;
+	} else {
+		/* One simple linear mapping. */
+		sreg->desc.min_uV =
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_LOW];
+		sreg->desc.uV_step =
+			vinfo->levels_uv[SCMI_VOLTAGE_SEGMENT_STEP];
+		sreg->desc.linear_min_sel = 0;
+		sreg->desc.n_voltages = delta_uV / sreg->desc.uV_step;
+		sreg->desc.ops = &scmi_reg_linear_ops;
+	}
+
+	return 0;
+}
+
+static int
+scmi_config_discrete_regulator_mappings(struct scmi_regulator *sreg,
+					const struct scmi_voltage_info *vinfo)
+{
+	/* Discrete non linear levels are mapped to volt_table */
+	sreg->desc.n_voltages = vinfo->num_levels;
+	if (sreg->desc.n_voltages > 1) {
+		sreg->desc.volt_table = (const unsigned int *)vinfo->levels_uv;
+		sreg->desc.ops = &scmi_reg_discrete_ops;
+	} else {
+		sreg->desc.fixed_uV = vinfo->levels_uv[0];
+		sreg->desc.ops = &scmi_reg_fixed_ops;
+	}
+
+	return 0;
+}
+
+static int scmi_regulator_common_init(struct scmi_regulator *sreg)
+{
+	int ret;
+	const struct scmi_handle *handle = sreg->sdev->handle;
+	struct device *dev = &sreg->sdev->dev;
+	const struct scmi_voltage_info *vinfo;
+
+	vinfo = handle->voltage_ops->info_get(handle, sreg->id);
+	if (!vinfo) {
+		dev_warn(dev, "Skipping invalid voltage domain %d\n",
+			 sreg->id);
+		return -ENODEV;
+	}
+
+	/*
+	 * Regulator framework does not fully support negative voltages
+	 * so we discard any voltage domain reported as supporting negative
+	 * voltages: as a consequence each levels_uv entry is guaranteed to
+	 * be non-negative from here on.
+	 */
+	if (vinfo->negative_volts_allowed) {
+		dev_warn(dev, "Negative voltages NOT supported...skip %s\n",
+			 sreg->of_node->full_name);
+		return -EOPNOTSUPP;
+	}
+
+	sreg->desc.name = devm_kasprintf(dev, GFP_KERNEL, "%s", vinfo->name);
+	if (!sreg->desc.name)
+		return -ENOMEM;
+
+	sreg->desc.id = sreg->id;
+	sreg->desc.type = REGULATOR_VOLTAGE;
+	sreg->desc.owner = THIS_MODULE;
+	sreg->desc.of_match = sreg->of_node->name;
+	sreg->desc.regulators_node = "regulators";
+	if (vinfo->segmented)
+		ret = scmi_config_linear_regulator_mappings(sreg, vinfo);
+	else
+		ret = scmi_config_discrete_regulator_mappings(sreg, vinfo);
+	if (ret)
+		return ret;
+
+	sreg->conf.dev = dev;
+	sreg->conf.driver_data = sreg;
+
+	return 0;
+}
+
+static int process_scmi_regulator_of_node(struct scmi_device *sdev,
+					  struct device_node *np,
+					  struct scmi_regulator_info *rinfo)
+{
+	u32 dom, ret;
+
+	ret = of_property_read_u32(np, "reg", &dom);
+	if (ret)
+		return ret;
+
+	if (dom >= rinfo->num_doms)
+		return -ENODEV;
+
+	if (rinfo->sregv[dom]) {
+		dev_err(&sdev->dev,
+			"SCMI Voltage Domain %d already in use. Skipping: %s\n",
+			dom, np->full_name);
+		return -EINVAL;
+	}
+
+	rinfo->sregv[dom] = devm_kzalloc(&sdev->dev,
+					 sizeof(struct scmi_regulator),
+					 GFP_KERNEL);
+	if (!rinfo->sregv[dom])
+		return -ENOMEM;
+
+	rinfo->sregv[dom]->id = dom;
+	rinfo->sregv[dom]->sdev = sdev;
+	/* get hold of good nodes */
+	of_node_get(np);
+	rinfo->sregv[dom]->of_node = np;
+	dev_dbg(&sdev->dev,
+		"Found SCMI Regulator entry -- OF node [%d] -> %s\n",
+		dom, np->full_name);
+
+	return 0;
+}
+
+static int scmi_regulator_probe(struct scmi_device *sdev)
+{
+	int d, ret, num_doms;
+	struct device_node *np, *child;
+	const struct scmi_handle *handle = sdev->handle;
+	struct scmi_regulator_info *rinfo;
+
+	if (!handle || !handle->voltage_ops)
+		return -ENODEV;
+
+	num_doms = handle->voltage_ops->num_domains_get(handle);
+	if (num_doms <= 0) {
+		if (!num_doms) {
+			dev_err(&sdev->dev,
+				"number of voltage domains invalid\n");
+			num_doms = -EINVAL;
+		} else {
+			dev_err(&sdev->dev,
+				"failed to get voltage domains - err:%d\n",
+				num_doms);
+		}
+
+		return num_doms;
+	}
+
+	rinfo = devm_kzalloc(&sdev->dev, sizeof(*rinfo), GFP_KERNEL);
+	if (!rinfo)
+		return -ENOMEM;
+
+	/* Allocate pointers' array for all possible domains */
+	rinfo->sregv = devm_kcalloc(&sdev->dev, num_doms,
+				    sizeof(rinfo->sregv), GFP_KERNEL);
+	if (!rinfo->sregv)
+		return -ENOMEM;
+
+	rinfo->num_doms = num_doms;
+	/*
+	 * Start collecting into rinfo->sregv possibly good SCMI Regulators as
+	 * described by a well-formed DT entry and associated with an existing
+	 * plausible SCMI Voltage Domain number, all belonging to this SCMI
+	 * platform instance node (handle->dev->of_node).
+	 */
+	np = of_find_node_by_name(handle->dev->of_node, "regulators");
+	for_each_child_of_node(np, child) {
+		ret = process_scmi_regulator_of_node(sdev, child, rinfo);
+		/* abort on any mem issue */
+		if (ret == -ENOMEM)
+			return ret;
+	}
+
+	/*
+	 * Register a regulator for each valid regulator-DT-entry that we
+	 * can successfully reach via SCMI and has a valid associated voltage
+	 * domain.
+	 */
+	for (d = 0; d < num_doms; d++) {
+		struct scmi_regulator *sreg = rinfo->sregv[d];
+
+		/* Skip empty slots */
+		if (!sreg)
+			continue;
+		ret = scmi_regulator_common_init(sreg);
+		/* Skip invalid voltage domains */
+		if (ret)
+			continue;
+		sreg->rdev = devm_regulator_register(&sdev->dev, &sreg->desc,
+						     &sreg->conf);
+		if (IS_ERR(sreg->rdev)) {
+			sreg->rdev = NULL;
+			continue;
+		}
+
+		dev_info(&sdev->dev,
+			 "Regulator %s registered for domain [%d]\n",
+			 sreg->desc.name, sreg->id);
+	}
+
+	dev_set_drvdata(&sdev->dev, rinfo);
+
+	return 0;
+}
+
+static void scmi_regulator_remove(struct scmi_device *sdev)
+{
+	int d;
+	struct scmi_regulator_info *rinfo;
+
+	rinfo = dev_get_drvdata(&sdev->dev);
+	if (!rinfo)
+		return;
+
+	for (d = 0; d < rinfo->num_doms; d++) {
+		if (!rinfo->sregv[d])
+			continue;
+		of_node_put(rinfo->sregv[d]->of_node);
+	}
+}
+
+static const struct scmi_device_id scmi_regulator_id_table[] = {
+	{ SCMI_PROTOCOL_VOLTAGE,  "regulator" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_regulator_id_table);
+
+static struct scmi_driver scmi_drv = {
+	.name		= "scmi-regulator",
+	.probe		= scmi_regulator_probe,
+	.remove		= scmi_regulator_remove,
+	.id_table	= scmi_regulator_id_table,
+};
+
+module_scmi_driver(scmi_drv);
+
+MODULE_AUTHOR("Cristian Marussi <cristian.marussi@arm.com>");
+MODULE_DESCRIPTION("ARM SCMI regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v3 4/4] dt-bindings: arm: add support for SCMI Regulators

[Cristian Marussi]

Add devicetree bindings to support regulators based on SCMI Voltage
Domain Protocol.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- avoid awkard examples based on _cpu/_gpu regulators
v1 --> v2
- removed any reference to negative voltages
---
 .../devicetree/bindings/arm/arm,scmi.txt      | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 55deb68230eb..0cef83a60f03 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -62,6 +62,28 @@ Required properties:
  - #power-domain-cells : Should be 1. Contains the device or the power
 			 domain ID value used by SCMI commands.
 
+Regulator bindings for the SCMI Regulator based on SCMI Message Protocol
+------------------------------------------------------------
+
+An SCMI Regulator is permanently bound to a well defined SCMI Voltage Domain,
+and should be always positioned as a root regulator.
+It does not support any current operation.
+
+This binding uses the common regulator binding[6].
+
+SCMI Regulators are grouped under a 'regulators' node which in turn is a child
+of the SCMI Voltage protocol node inside the desired SCMI instance node.
+
+Required properties:
+ - reg : shall identify an existent SCMI Voltage Domain.
+
+Optional properties:
+ - all of the other standard regulator bindings as in [6]: note that, since
+   the SCMI Protocol itself aims in fact to hide away many of the operational
+   capabilities usually exposed by the properties of a standard regulator,
+   most of the usual regulator bindings could have just no effect in the
+   context of this SCMI regulator.
+
 Sensor bindings for the sensors based on SCMI Message Protocol
 --------------------------------------------------------------
 SCMI provides an API to access the various sensors on the SoC.
@@ -105,6 +127,7 @@ Required sub-node properties:
 [3] Documentation/devicetree/bindings/thermal/thermal*.yaml
 [4] Documentation/devicetree/bindings/sram/sram.yaml
 [5] Documentation/devicetree/bindings/reset/reset.txt
+[6] Documentation/devicetree/bindings/regulator/regulator.yaml
 
 Example:
 
@@ -169,6 +192,25 @@ firmware {
 			reg = <0x16>;
 			#reset-cells = <1>;
 		};
+
+		scmi_voltage: protocol@17 {
+			reg = <0x17>;
+
+			regulators {
+				regulator_devX: regulator_scmi_devX@0 {
+					reg = <0x0>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				regulator_devY: regulator_scmi_devY@9 {
+					reg = <0x9>;
+					regulator-min-microvolt = <500000>;
+					regulator-max-microvolt = <4200000>;
+				};
+
+				...
+			};
+		};
 	};
 };
 
-- 
2.17.1

Re: [PATCH v3 4/4] dt-bindings: arm: add support for SCMI Regulators

[Rob Herring]

On Mon, Oct 26, 2020 at 08:31:48PM +0000, Cristian Marussi wrote:
> Add devicetree bindings to support regulators based on SCMI Voltage
> Domain Protocol.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v2 --> v3
> - avoid awkard examples based on _cpu/_gpu regulators
> v1 --> v2
> - removed any reference to negative voltages
> ---
>  .../devicetree/bindings/arm/arm,scmi.txt      | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> index 55deb68230eb..0cef83a60f03 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> @@ -62,6 +62,28 @@ Required properties:
>   - #power-domain-cells : Should be 1. Contains the device or the power
>  			 domain ID value used by SCMI commands.
>  
> +Regulator bindings for the SCMI Regulator based on SCMI Message Protocol
> +------------------------------------------------------------
> +
> +An SCMI Regulator is permanently bound to a well defined SCMI Voltage Domain,
> +and should be always positioned as a root regulator.
> +It does not support any current operation.
> +
> +This binding uses the common regulator binding[6].
> +
> +SCMI Regulators are grouped under a 'regulators' node which in turn is a child
> +of the SCMI Voltage protocol node inside the desired SCMI instance node.
> +
> +Required properties:
> + - reg : shall identify an existent SCMI Voltage Domain.
> +
> +Optional properties:
> + - all of the other standard regulator bindings as in [6]: note that, since
> +   the SCMI Protocol itself aims in fact to hide away many of the operational
> +   capabilities usually exposed by the properties of a standard regulator,
> +   most of the usual regulator bindings could have just no effect in the
> +   context of this SCMI regulator.

You can't have it both ways... You should list out which ones apply.

I'm a bit worried that now we're changing CPUs (at least?) from clocks 
to 'performance domains' while at the same time here we're adding 
low level, virtual regulators. Are we going to end up wanting something 
more abstract here too?

> +
>  Sensor bindings for the sensors based on SCMI Message Protocol
>  --------------------------------------------------------------
>  SCMI provides an API to access the various sensors on the SoC.
> @@ -105,6 +127,7 @@ Required sub-node properties:
>  [3] Documentation/devicetree/bindings/thermal/thermal*.yaml
>  [4] Documentation/devicetree/bindings/sram/sram.yaml
>  [5] Documentation/devicetree/bindings/reset/reset.txt
> +[6] Documentation/devicetree/bindings/regulator/regulator.yaml
>  
>  Example:
>  
> @@ -169,6 +192,25 @@ firmware {
>  			reg = <0x16>;
>  			#reset-cells = <1>;
>  		};
> +
> +		scmi_voltage: protocol@17 {
> +			reg = <0x17>;
> +
> +			regulators {
> +				regulator_devX: regulator_scmi_devX@0 {

Node names should be generic:

regulator@0

> +					reg = <0x0>;
> +					regulator-max-microvolt = <3300000>;
> +				};
> +
> +				regulator_devY: regulator_scmi_devY@9 {
> +					reg = <0x9>;
> +					regulator-min-microvolt = <500000>;
> +					regulator-max-microvolt = <4200000>;
> +				};
> +
> +				...
> +			};
> +		};
>  	};
>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 4/4] dt-bindings: arm: add support for SCMI Regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On Fri, Oct 30, 2020 at 01:55:14PM -0500, Rob Herring wrote:

> I'm a bit worried that now we're changing CPUs (at least?) from clocks 
> to 'performance domains' while at the same time here we're adding 
> low level, virtual regulators. Are we going to end up wanting something 
> more abstract here too?

My understanding is that this is aimed at systems which have done the
more abstract thing where regulators just aren't visible at all to the
kernel but then find that they actually need to control some of the
regulators explicitly for things like MMC so need a mechanism for the
firmware to expose select regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/4] dt-bindings: arm: add support for SCMI Regulators

[Sudeep Holla]

On Fri, Oct 30, 2020 at 07:06:07PM +0000, Mark Brown wrote:
> On Fri, Oct 30, 2020 at 01:55:14PM -0500, Rob Herring wrote:
>
> > I'm a bit worried that now we're changing CPUs (at least?) from clocks
> > to 'performance domains' while at the same time here we're adding
> > low level, virtual regulators. Are we going to end up wanting something
> > more abstract here too?

Valid concern and I too am with the same opinion. However as Mark Brown
points out this was added to just satisfy some exiting consumers that
rely on regulators.

I had or still argue that we need a way to check if this is not getting
misused with devices like CPUs. I was thinking of some check with DT where
such properties are not allowed in certain device type.

>
> My understanding is that this is aimed at systems which have done the
> more abstract thing where regulators just aren't visible at all to the
> kernel but then find that they actually need to control some of the
> regulators explicitly for things like MMC so need a mechanism for the
> firmware to expose select regulators.

Thanks Mark for the explaining this, saved me time 😄.

-- 
Regards,
Sudeep

Re: [PATCH v3 4/4] dt-bindings: arm: add support for SCMI Regulators

[Cristian Marussi]

Hi Rob

thanks for the review.

On Fri, Oct 30, 2020 at 01:55:14PM -0500, Rob Herring wrote:
> On Mon, Oct 26, 2020 at 08:31:48PM +0000, Cristian Marussi wrote:
> > Add devicetree bindings to support regulators based on SCMI Voltage
> > Domain Protocol.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v2 --> v3
> > - avoid awkard examples based on _cpu/_gpu regulators
> > v1 --> v2
> > - removed any reference to negative voltages
> > ---
> >  .../devicetree/bindings/arm/arm,scmi.txt      | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > index 55deb68230eb..0cef83a60f03 100644
> > --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > @@ -62,6 +62,28 @@ Required properties:
> >   - #power-domain-cells : Should be 1. Contains the device or the power
> >  			 domain ID value used by SCMI commands.
> >  
> > +Regulator bindings for the SCMI Regulator based on SCMI Message Protocol
> > +------------------------------------------------------------
> > +
> > +An SCMI Regulator is permanently bound to a well defined SCMI Voltage Domain,
> > +and should be always positioned as a root regulator.
> > +It does not support any current operation.
> > +
> > +This binding uses the common regulator binding[6].
> > +
> > +SCMI Regulators are grouped under a 'regulators' node which in turn is a child
> > +of the SCMI Voltage protocol node inside the desired SCMI instance node.
> > +
> > +Required properties:
> > + - reg : shall identify an existent SCMI Voltage Domain.
> > +
> > +Optional properties:
> > + - all of the other standard regulator bindings as in [6]: note that, since
> > +   the SCMI Protocol itself aims in fact to hide away many of the operational
> > +   capabilities usually exposed by the properties of a standard regulator,
> > +   most of the usual regulator bindings could have just no effect in the
> > +   context of this SCMI regulator.
> 
> You can't have it both ways... You should list out which ones apply.
> 

I'm double checking now every regulator binding that can apply, and I'll
append here the full list of supported standard regulator bindings.

> I'm a bit worried that now we're changing CPUs (at least?) from clocks 
> to 'performance domains' while at the same time here we're adding 
> low level, virtual regulators. Are we going to end up wanting something 
> more abstract here too?
> 

As said already in a different thread by Mark and Sudeep, this was needed
to accomodate some driver like MMC which expects to be able to fine tune
some regulator voltages at some point.

The SCMI abstraction exposed by the SCMI Power protocol indeed already
provided a coarse grain tuning in term of abstract power levels and
on/off ops but it was not able to provide fine grained voltage tuning
in a clean way (without horribly abusing IMP DEFINED values), so
Voltage Domain protocol with a regulator on top of it was added.
(and I think anyway partners were already starting thinking about
developing their own custom protocols and regulators to bypass these
limitations...so providing a unified solution seemed sensible)

Anyway consider that everything remains still in control of the SCMI fw
which has the last word and can anyway drop your 'fine' tuned request
if deemed unreasonable/invalid.

> > +
> >  Sensor bindings for the sensors based on SCMI Message Protocol
> >  --------------------------------------------------------------
> >  SCMI provides an API to access the various sensors on the SoC.
> > @@ -105,6 +127,7 @@ Required sub-node properties:
> >  [3] Documentation/devicetree/bindings/thermal/thermal*.yaml
> >  [4] Documentation/devicetree/bindings/sram/sram.yaml
> >  [5] Documentation/devicetree/bindings/reset/reset.txt
> > +[6] Documentation/devicetree/bindings/regulator/regulator.yaml
> >  
> >  Example:
> >  
> > @@ -169,6 +192,25 @@ firmware {
> >  			reg = <0x16>;
> >  			#reset-cells = <1>;
> >  		};
> > +
> > +		scmi_voltage: protocol@17 {
> > +			reg = <0x17>;
> > +
> > +			regulators {
> > +				regulator_devX: regulator_scmi_devX@0 {
> 
> Node names should be generic:
> 
> regulator@0
> 

Yes, I was not sure if it this kind of naming for reg-based property was
preferable of mandatory and so, since supporting using a <common-nam>@<unit>
format in this driver needs a small patch in the regulator core framework
(at least it seems so to me...), I wanted to be sure this was needed.

I'll repost soon with naming fixed and with any additional patch added.

Thanks

Cristian

> > +					reg = <0x0>;
> > +					regulator-max-microvolt = <3300000>;
> > +				};
> > +
> > +				regulator_devY: regulator_scmi_devY@9 {
> > +					reg = <0x9>;
> > +					regulator-min-microvolt = <500000>;
> > +					regulator-max-microvolt = <4200000>;
> > +				};
> > +
> > +				...
> > +			};
> > +		};
> >  	};
> >  };
> >  
> > -- 
> > 2.17.1
> >