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

[PATCH v2 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 fd7c58ee3026 ("firmware: arm_scmi: Fix locking in notifications")

Any feedback welcome,

Thanks,

Cristian

---
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           | 382 +++++++++++++++
 drivers/regulator/Kconfig                     |   9 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/scmi-regulator.c            | 453 ++++++++++++++++++
 include/linux/scmi_protocol.h                 |  64 +++
 9 files changed, 956 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/voltage.c
 create mode 100644 drivers/regulator/scmi-regulator.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 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>
---
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 | 382 ++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h       |  64 +++++
 5 files changed, 450 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..a20da5128de1
--- /dev/null
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -0,0 +1,382 @@
+// 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_levels)
+{
+	bool segmented;
+
+	segmented = SUPPORTS_SEGMENTED_LEVELS(flags);
+	/* segmented levels array's entries must be multiple-of-3 */
+	if (!num_levels || (segmented && num_levels % 3))
+		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);
+		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",
+					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 (!v->negative_volts_allowed && 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 on memory errors, just skip domain on any
+		 * other error.
+		 */
+		if (!ret)
+			vinfo->domains[dom] = v;
+		else if (ret == -ENOMEM)
+			break;
+
+		scmi_reset_rx_to_maxsz(handle, td);
+	}
+
+	scmi_xfer_put(handle, tl);
+outd:
+	scmi_xfer_put(handle, td);
+
+	return ret;
+}
+
+static inline 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 *
+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..032ad9bb2a53 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 {
+	u32 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;
+	u32 attributes;
+	char name[SCMI_MAX_STR_SIZE];
+	u16 num_levels;
+	s32 *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 *(*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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Etienne Carriere]

Hello Cristian,

Thanks for the update.
Some minor comments below.
FYI I successfully tested this series.

Regards,
Etienne

On Thu, 15 Oct 2020 at 17:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> Add SCMI Voltage Domain protocol support.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> 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 | 382 ++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h       |  64 +++++
>  5 files changed, 450 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..a20da5128de1
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -0,0 +1,382 @@
> +// 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_levels)
> +{
> +       bool segmented;
> +
> +       segmented = SUPPORTS_SEGMENTED_LEVELS(flags);
> +       /* segmented levels array's entries must be multiple-of-3 */
> +       if (!num_levels || (segmented && num_levels % 3))
> +               return -EINVAL;

I think segment levels are described by a single triplet.
If right, should test (!num_levels || (segmented && num_levels == 3)).


> +
> +       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);
> +               if (ret)
> +                       continue;

Continue and break and report the error code?
Seems you prefer to abort only on local error (ENOMEM), not comm error.
Maybe state with nice inline comment as done below (/* Skip domain on error */).

> +
> +               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",

Missing '\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 (!v->negative_volts_allowed && val < 0)

could skip testing v->negative_volts_allowed.
        if (val < 0)
                v->negative_volts_allowed = true;


> +                                       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 on memory errors, just skip domain on any
> +                * other error.
> +                */
> +               if (!ret)
> +                       vinfo->domains[dom] = v;
> +               else if (ret == -ENOMEM)
> +                       break;
> +
> +               scmi_reset_rx_to_maxsz(handle, td);
> +       }
> +
> +       scmi_xfer_put(handle, tl);
> +outd:
> +       scmi_xfer_put(handle, td);
> +
> +       return ret;
> +}
> +
> +static inline int __scmi_voltage_get_u32(const struct scmi_handle *handle,

inline needed ?

> +                                        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 *
> +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..032ad9bb2a53 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 {
> +       u32 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;
> +       u32 attributes;
> +       char name[SCMI_MAX_STR_SIZE];
> +       u16 num_levels;
> +       s32 *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 *(*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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Cristian Marussi]

On Fri, Oct 16, 2020 at 03:36:51PM +0200, Etienne Carriere wrote:
> Hello Cristian,
> 
> Thanks for the update.
> Some minor comments below.
> FYI I successfully tested this series.
> 

Hi Etienne

thanks for the review and for testing !

> Regards,
> Etienne
> 
> On Thu, 15 Oct 2020 at 17:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > Add SCMI Voltage Domain protocol support.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > 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 | 382 ++++++++++++++++++++++++++++
> >  include/linux/scmi_protocol.h       |  64 +++++
> >  5 files changed, 450 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..a20da5128de1
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/voltage.c
> > @@ -0,0 +1,382 @@
> > +// 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_levels)
> > +{
> > +       bool segmented;
> > +
> > +       segmented = SUPPORTS_SEGMENTED_LEVELS(flags);
> > +       /* segmented levels array's entries must be multiple-of-3 */
> > +       if (!num_levels || (segmented && num_levels % 3))
> > +               return -EINVAL;
> 
> I think segment levels are described by a single triplet.
> If right, should test (!num_levels || (segmented && num_levels == 3)).
> 
> 

You're right, multiple triplets were initially described but dropped
from the final spec, so my bad I forgot to update this. (same goes with
the correspondent regulator support for multiple linear mappings as you
spotted there).

I'll fix in V3 like

if (!num_levels || (segmented && num_levels != 3))
	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);
> > +               if (ret)
> > +                       continue;
> 
> Continue and break and report the error code?
> Seems you prefer to abort only on local error (ENOMEM), not comm error.
> Maybe state with nice inline comment as done below (/* Skip domain on error */).
> 

Ok I'll do.

> > +
> > +               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",
> 
> Missing '\n'.
> 

Right.

> > +                                       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 (!v->negative_volts_allowed && val < 0)
> 
> could skip testing v->negative_volts_allowed.
>         if (val < 0)
>                 v->negative_volts_allowed = true;
> 

Ok.
> 
> > +                                       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 on memory errors, just skip domain on any
> > +                * other error.
> > +                */
> > +               if (!ret)
> > +                       vinfo->domains[dom] = v;
> > +               else if (ret == -ENOMEM)
> > +                       break;
> > +
> > +               scmi_reset_rx_to_maxsz(handle, td);
> > +       }
> > +
> > +       scmi_xfer_put(handle, tl);
> > +outd:
> > +       scmi_xfer_put(handle, td);
> > +
> > +       return ret;
> > +}
> > +
> > +static inline int __scmi_voltage_get_u32(const struct scmi_handle *handle,
> 
> inline needed ?
> 

Mmm, probably not.


Thanks

Cristian

> > +                                        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 *
> > +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..032ad9bb2a53 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 {
> > +       u32 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;
> > +       u32 attributes;
> > +       char name[SCMI_MAX_STR_SIZE];
> > +       u16 num_levels;
> > +       s32 *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 *(*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
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 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>
----
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 | 453 +++++++++++++++++++++++++++++
 3 files changed, 463 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..e4e7d0345723
--- /dev/null
+++ b/drivers/regulator/scmi-regulator.c
@@ -0,0 +1,453 @@
+// 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)
+{
+	int ret;
+	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;
+
+	ret = handle->voltage_ops->level_set(handle, sreg->id, 0x0, volt_uV);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+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_range_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_range,
+	.map_voltage = regulator_map_voltage_linear_range,
+};
+
+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)
+{
+	/*
+	 * Note that SCMI voltage domains describable by linear ranges
+	 * (segments) {low, high, step} are guaranteed to come in triplets by
+	 * the SCMI Voltage Domain protocol support itself.
+	 */
+	if (vinfo->num_levels == 3) {
+		s32 delta_uV;
+
+		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;
+		}
+	} else {
+		/* Multiple linear mappings. */
+		int i, num_ranges, last_max = -1;
+		struct linear_range *lr;
+
+		num_ranges = vinfo->num_levels / 3;
+		lr = devm_kcalloc(&sreg->sdev->dev, num_ranges,
+				  sizeof(*lr), GFP_KERNEL);
+		if (!lr)
+			return -ENOMEM;
+
+		sreg->desc.n_linear_ranges = num_ranges;
+		sreg->desc.linear_ranges = lr;
+		for (i = 0; num_ranges; num_ranges--, i += 3, lr++) {
+			s32 delta_uV;
+
+			lr->min =
+				vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_LOW];
+			lr->step =
+				vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_STEP];
+			delta_uV =
+			    vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_HIGH] -
+			    lr->min;
+			if (delta_uV <= 0 || !(delta_uV / lr->step)) {
+				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;
+			}
+			lr->max_sel = delta_uV / lr->step - 1;
+			lr->min_sel = last_max + 1;
+			last_max = lr->max_sel;
+		}
+		sreg->desc.n_voltages = last_max + 1;
+		sreg->desc.ops = &scmi_reg_range_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)
+		return -ENODEV;
+
+	if (!vinfo->num_levels)
+		return -EINVAL;
+
+	/*
+	 * 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_info(&sdev->dev,
+		 "Found valid SCMI Regulator -- OF node [%d] -> %s\n",
+		 dom, np->full_name);
+
+	return ret;
+}
+
+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
+	 */
+	for (d = 0; d < num_doms; d++) {
+		struct scmi_regulator *sreg = rinfo->sregv[d];
+
+		if (!sreg)
+			continue;
+		ret = scmi_regulator_common_init(sreg);
+		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](%s)\n",
+			 sreg->desc.name, sreg->id, sreg->desc.name);
+	}
+
+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] regulator: add SCMI driver

[Etienne Carriere]

Hi Cristian,

Some minor comments...

Etienne


On Thu, 15 Oct 2020 at 17:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> Add a simple regulator based on SCMI Voltage Domain Protocol.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ----
> 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 | 453 +++++++++++++++++++++++++++++
>  3 files changed, 463 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..e4e7d0345723
> --- /dev/null
> +++ b/drivers/regulator/scmi-regulator.c
> @@ -0,0 +1,453 @@
> +// 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)
> +{
> +       int ret;
> +       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;
> +
> +       ret = handle->voltage_ops->level_set(handle, sreg->id, 0x0, volt_uV);
> +       if (ret)
> +               return ret;
> +
> +       return ret;

    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_range_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_range,
> +       .map_voltage = regulator_map_voltage_linear_range,
> +};
> +
> +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)
> +{
> +       /*
> +        * Note that SCMI voltage domains describable by linear ranges
> +        * (segments) {low, high, step} are guaranteed to come in triplets by
> +        * the SCMI Voltage Domain protocol support itself.
> +        */
> +       if (vinfo->num_levels == 3) {
> +               s32 delta_uV;
> +
> +               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;
> +               }
> +       } else {
> +               /* Multiple linear mappings. */

I don't see this multi-linear mapping in the v3.0 beta of the spec.
Is it something planned?

> +               int i, num_ranges, last_max = -1;
> +               struct linear_range *lr;
> +
> +               num_ranges = vinfo->num_levels / 3;
> +               lr = devm_kcalloc(&sreg->sdev->dev, num_ranges,
> +                                 sizeof(*lr), GFP_KERNEL);
> +               if (!lr)
> +                       return -ENOMEM;
> +
> +               sreg->desc.n_linear_ranges = num_ranges;
> +               sreg->desc.linear_ranges = lr;
> +               for (i = 0; num_ranges; num_ranges--, i += 3, lr++) {
> +                       s32 delta_uV;
> +
> +                       lr->min =
> +                               vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_LOW];
> +                       lr->step =
> +                               vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_STEP];
> +                       delta_uV =
> +                           vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_HIGH] -
> +                           lr->min;
> +                       if (delta_uV <= 0 || !(delta_uV / lr->step)) {
> +                               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;
> +                       }
> +                       lr->max_sel = delta_uV / lr->step - 1;
> +                       lr->min_sel = last_max + 1;
> +                       last_max = lr->max_sel;
> +               }
> +               sreg->desc.n_voltages = last_max + 1;
> +               sreg->desc.ops = &scmi_reg_range_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)
> +               return -ENODEV;
> +
> +       if (!vinfo->num_levels)
> +               return -EINVAL;
> +
> +       /*
> +        * 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_info(&sdev->dev,
> +                "Found valid SCMI Regulator -- OF node [%d] -> %s\n",
> +                dom, np->full_name);
> +
> +       return ret;

suggest return 0; here.

> +}
> +
> +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
> +        */
> +       for (d = 0; d < num_doms; d++) {
> +               struct scmi_regulator *sreg = rinfo->sregv[d];
> +
> +               if (!sreg)
> +                       continue;
> +               ret = scmi_regulator_common_init(sreg);
> +               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](%s)\n",
> +                        sreg->desc.name, sreg->id, sreg->desc.name);

Maybe not display sreg->desc.name twice.

> +       }
> +
> +       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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] regulator: add SCMI driver

[Cristian Marussi]

On Fri, Oct 16, 2020 at 04:09:42PM +0200, Etienne Carriere wrote:
> Hi Cristian,
> 
> Some minor comments...
> 
> Etienne
> 
> 
> On Thu, 15 Oct 2020 at 17:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > Add a simple regulator based on SCMI Voltage Domain Protocol.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ----
> > 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 | 453 +++++++++++++++++++++++++++++
> >  3 files changed, 463 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..e4e7d0345723
> > --- /dev/null
> > +++ b/drivers/regulator/scmi-regulator.c
> > @@ -0,0 +1,453 @@
> > +// 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)
> > +{
> > +       int ret;
> > +       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;
> > +
> > +       ret = handle->voltage_ops->level_set(handle, sreg->id, 0x0, volt_uV);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return ret;
> 
>     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_range_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_range,
> > +       .map_voltage = regulator_map_voltage_linear_range,
> > +};
> > +
> > +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)
> > +{
> > +       /*
> > +        * Note that SCMI voltage domains describable by linear ranges
> > +        * (segments) {low, high, step} are guaranteed to come in triplets by
> > +        * the SCMI Voltage Domain protocol support itself.
> > +        */
> > +       if (vinfo->num_levels == 3) {
> > +               s32 delta_uV;
> > +
> > +               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;
> > +               }
> > +       } else {
> > +               /* Multiple linear mappings. */
> 
> I don't see this multi-linear mapping in the v3.0 beta of the spec.
> Is it something planned?
> 

No, it was indeed something included in previous spec iterations (multiple triplets)
but now removed, as said in previous patch, I forgot to remove this
redundant support. I'll do in V3.

> > +               int i, num_ranges, last_max = -1;
> > +               struct linear_range *lr;
> > +
> > +               num_ranges = vinfo->num_levels / 3;
> > +               lr = devm_kcalloc(&sreg->sdev->dev, num_ranges,
> > +                                 sizeof(*lr), GFP_KERNEL);
> > +               if (!lr)
> > +                       return -ENOMEM;
> > +
> > +               sreg->desc.n_linear_ranges = num_ranges;
> > +               sreg->desc.linear_ranges = lr;
> > +               for (i = 0; num_ranges; num_ranges--, i += 3, lr++) {
> > +                       s32 delta_uV;
> > +
> > +                       lr->min =
> > +                               vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_LOW];
> > +                       lr->step =
> > +                               vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_STEP];
> > +                       delta_uV =
> > +                           vinfo->levels_uv[i + SCMI_VOLTAGE_SEGMENT_HIGH] -
> > +                           lr->min;
> > +                       if (delta_uV <= 0 || !(delta_uV / lr->step)) {
> > +                               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;
> > +                       }
> > +                       lr->max_sel = delta_uV / lr->step - 1;
> > +                       lr->min_sel = last_max + 1;
> > +                       last_max = lr->max_sel;
> > +               }
> > +               sreg->desc.n_voltages = last_max + 1;
> > +               sreg->desc.ops = &scmi_reg_range_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)
> > +               return -ENODEV;
> > +
> > +       if (!vinfo->num_levels)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * 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_info(&sdev->dev,
> > +                "Found valid SCMI Regulator -- OF node [%d] -> %s\n",
> > +                dom, np->full_name);
> > +
> > +       return ret;
> 
> suggest return 0; here.

Ok.

> 
> > +}
> > +
> > +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
> > +        */
> > +       for (d = 0; d < num_doms; d++) {
> > +               struct scmi_regulator *sreg = rinfo->sregv[d];
> > +
> > +               if (!sreg)
> > +                       continue;
> > +               ret = scmi_regulator_common_init(sreg);
> > +               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](%s)\n",
> > +                        sreg->desc.name, sreg->id, sreg->desc.name);
> 
> Maybe not display sreg->desc.name twice.
> 

Yes does not make any sense indeed now that I have only one name o_O

> > +       }
> > +
> > +       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
> >

Thanks

Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 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>
---
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..6d3d0bdab322 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_cpu: regulator_scmi_cpu@0 {
+					reg = <0x0>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				regulator_gpu: regulator_scmi_gpu@9 {
+					reg = <0x9>;
+					regulator-min-microvolt = <500000>;
+					regulator-max-microvolt = <4200000>;
+				};
+
+				...
+			};
+		};
 	};
 };
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel