* [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator
@ 2020-10-05 22:26 Cristian Marussi
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-05 22:26 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, lukasz.luba, james.quinlan, Jonathan.Cameron,
broonie, robh, satyakim, etienne.carriere, 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 66d90f6ecee7 ("firmware: arm_scmi: Enable building as a single
module")
Any feedback welcome,
Thanks,
Cristian
[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 | 44 ++
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 | 378 ++++++++++++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/scmi-regulator.c | 488 ++++++++++++++++++
include/linux/scmi_protocol.h | 64 +++
9 files changed, 989 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/arm_scmi/voltage.c
create mode 100644 drivers/regulator/scmi-regulator.c
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
@ 2020-10-05 22:26 ` Cristian Marussi
2020-10-08 10:10 ` Etienne Carriere
2020-10-05 22:26 ` [PATCH 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname Cristian Marussi
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2020-10-05 22:26 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, lukasz.luba, james.quinlan, Jonathan.Cameron,
broonie, robh, satyakim, etienne.carriere, cristian.marussi
Add SCMI Voltage Domain protocol support.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
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 | 378 ++++++++++++++++++++++++++++
include/linux/scmi_protocol.h | 64 +++++
5 files changed, 446 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 37fb583f1bf5..eaa4d0e1174f 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -167,6 +167,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 c5dea87edf8f..d7f50bf5f958 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -938,6 +938,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);
@@ -953,6 +954,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..94221d979b80
--- /dev/null
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -0,0 +1,378 @@
+// 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;
+ cmd->domain_id = cpu_to_le32(v->id);
+ /* ...then retrieve domain levels descriptions */
+ do {
+ u32 flags;
+ int cnt;
+
+ 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;
+ /* 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_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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
@ 2020-10-05 22:26 ` Cristian Marussi
2020-10-05 22:26 ` [PATCH 3/4] regulator: add SCMI driver Cristian Marussi
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-05 22:26 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, lukasz.luba, james.quinlan, Jonathan.Cameron,
broonie, robh, satyakim, etienne.carriere, 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 d7f50bf5f958..03f1152f255e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -735,6 +735,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] regulator: add SCMI driver
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
2020-10-05 22:26 ` [PATCH 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname Cristian Marussi
@ 2020-10-05 22:26 ` Cristian Marussi
2020-10-06 10:56 ` Mark Brown
2020-10-05 22:26 ` [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators Cristian Marussi
2020-10-06 0:38 ` [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Florian Fainelli
4 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2020-10-05 22:26 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, lukasz.luba, james.quinlan, Jonathan.Cameron,
broonie, robh, satyakim, etienne.carriere, cristian.marussi
Add a simple regulator based on SCMI Voltage Domain Protocol.
Supported operations are:
- .enable / .disable / .is_enabled: routed via SCMI Voltage Domain Protocol
- .get_voltage / .set_voltage: routed via SCMI Voltage Domain Protocol
- .get_voltage_sel/.set_voltage_sel: using regulator framework helpers
- .list_voltage/.map_voltage: using regulator frameowrk helpers
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
----
v2 --> v3
- fixed init_data constraint parsing
- fixes for v5.8 (linear_range.h)
v1 --> v2
- 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 | 488 +++++++++++++++++++++++++++++
3 files changed, 498 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..041c6a9dd4df
--- /dev/null
+++ b/drivers/regulator/scmi-regulator.c
@@ -0,0 +1,488 @@
+// 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/set_voltage
+// 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;
+ const char *name;
+ 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->name);
+ return 0;
+ }
+
+ return config & SCMI_VOLTAGE_ARCH_STATE_ON;
+}
+
+static int scmi_reg_get_voltage(struct regulator_dev *rdev)
+{
+ int ret;
+ u32 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, (s32 *)&volt_uV);
+ if (ret)
+ return ret;
+
+ return volt_uV;
+}
+
+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(struct regulator_dev *rdev, int min_uV,
+ int max_uV, unsigned int *selector)
+{
+ int ret;
+ struct scmi_regulator *sreg = rdev_get_drvdata(rdev);
+ const struct scmi_handle *handle = sreg->sdev->handle;
+
+ /*
+ * This should have been filtered out by non-negative
+ * constraints...just in case.
+ */
+ if (min_uV < 0 || max_uV < 0) {
+ dev_warn(&sreg->sdev->dev, "Negative voltages NOT supported\n");
+ return -EINVAL;
+ }
+
+ ret = handle->voltage_ops->level_set(handle, sreg->id, 0x0, min_uV);
+ if (ret)
+ return ret;
+
+ if (selector)
+ *selector = sreg->desc.ops->map_voltage(rdev, min_uV, min_uV);
+
+ return ret;
+}
+
+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 = scmi_reg_get_voltage,
+ .set_voltage = scmi_reg_set_voltage,
+ .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 = scmi_reg_get_voltage,
+ .set_voltage = scmi_reg_set_voltage,
+ .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->name = devm_kasprintf(dev, GFP_KERNEL, "%s", vinfo->name);
+ sreg->desc.name = devm_kasprintf(dev, GFP_KERNEL,
+ "Vscmi.%s", sreg->name);
+ if (!sreg->name || !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) {
+ dev_err(&sdev->dev, "number of voltage domains invalid\n");
+ return num_doms ?: -EINVAL;
+ }
+
+ 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->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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
` (2 preceding siblings ...)
2020-10-05 22:26 ` [PATCH 3/4] regulator: add SCMI driver Cristian Marussi
@ 2020-10-05 22:26 ` Cristian Marussi
2020-10-06 10:59 ` Mark Brown
2020-10-06 0:38 ` [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Florian Fainelli
4 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2020-10-05 22:26 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, lukasz.luba, james.quinlan, Jonathan.Cameron,
broonie, robh, satyakim, etienne.carriere, cristian.marussi
Add devicetree bindings to support regulators based on SCMI Voltage
Domain Protocol.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
.../devicetree/bindings/arm/arm,scmi.txt | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 55deb68230eb..c6a008737631 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -62,6 +62,30 @@ 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 negative voltages nor 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:
+ - regulator-min-microvolt: when defined should be non-negative.
+ - regulator-max-microvolt: when defined should be non-negative.
+ - 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 +129,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 +194,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
` (3 preceding siblings ...)
2020-10-05 22:26 ` [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators Cristian Marussi
@ 2020-10-06 0:38 ` Florian Fainelli
2020-10-09 12:42 ` Cristian Marussi
4 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2020-10-06 0:38 UTC (permalink / raw)
To: Cristian Marussi, linux-kernel, linux-arm-kernel
Cc: robh, satyakim, sudeep.holla, broonie, james.quinlan,
Jonathan.Cameron, etienne.carriere, lukasz.luba
On 10/5/2020 3:26 PM, Cristian Marussi wrote:
> 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 66d90f6ecee7 ("firmware: arm_scmi: Enable building as a single
> module")
>
> Any feedback welcome,
Well, this is just great! We were right about to develop a proprietary
SCMI protocol in order to control a locked down PMIC accessible behind a
secured firmware. We would have done essentially just that since the use
case is to control the various regulators exposed by this PMIC over
SCMI. Thanks a lot!
>
> Thanks,
>
> Cristian
>
> [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 | 44 ++
> 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 | 378 ++++++++++++++
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/scmi-regulator.c | 488 ++++++++++++++++++
> include/linux/scmi_protocol.h | 64 +++
> 9 files changed, 989 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/arm_scmi/voltage.c
> create mode 100644 drivers/regulator/scmi-regulator.c
>
--
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] regulator: add SCMI driver
2020-10-05 22:26 ` [PATCH 3/4] regulator: add SCMI driver Cristian Marussi
@ 2020-10-06 10:56 ` Mark Brown
2020-10-15 15:38 ` Cristian Marussi
0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2020-10-06 10:56 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, sudeep.holla, lukasz.luba,
james.quinlan, Jonathan.Cameron, robh, satyakim,
etienne.carriere
[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]
On Mon, Oct 05, 2020 at 11:26:22PM +0100, Cristian Marussi wrote:
> - .get_voltage / .set_voltage: routed via SCMI Voltage Domain Protocol
> - .get_voltage_sel/.set_voltage_sel: using regulator framework helpers
You should not be implementing both of these interfaces, pick one. It
looks like the direct voltage operations are the redundant ones here,
while the protocol uses actual voltages to communicate with the firmware
which makes the direct voltage operations a better fit it seems like the
expectation is that only a limited set of voltages is supported (as is
normal for the underlying physical regulators) so you want selectors.
> + sreg->name = devm_kasprintf(dev, GFP_KERNEL, "%s", vinfo->name);
> + sreg->desc.name = devm_kasprintf(dev, GFP_KERNEL,
> + "Vscmi.%s", sreg->name);
> + if (!sreg->name || !sreg->desc.name)
> + return -ENOMEM;
Why are we using different names here?
> + num_doms = handle->voltage_ops->num_domains_get(handle);
> + if (num_doms <= 0) {
> + dev_err(&sdev->dev, "number of voltage domains invalid\n");
> + return num_doms ?: -EINVAL;
Please write normal conditional statements to improve legibility.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators
2020-10-05 22:26 ` [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators Cristian Marussi
@ 2020-10-06 10:59 ` Mark Brown
2020-10-07 8:06 ` Cristian Marussi
0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2020-10-06 10:59 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, sudeep.holla, lukasz.luba,
james.quinlan, Jonathan.Cameron, robh, satyakim,
etienne.carriere
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
On Mon, Oct 05, 2020 at 11:26:23PM +0100, Cristian Marussi wrote:
> +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 negative voltages nor any current operation.
Support for negative voltages is an implementation detail of Linux, IIRC
there's nothing in the generic regulator binding preventing it and even
if there were updates to the generic binding are what are relevant here
- they could start working with no updates to this binding.
> +Optional properties:
> + - regulator-min-microvolt: when defined should be non-negative.
> + - regulator-max-microvolt: when defined should be non-negative.
These are covered by the generic regulator binding, no need to duplicate
the documentation here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators
2020-10-06 10:59 ` Mark Brown
@ 2020-10-07 8:06 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-07 8:06 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, linux-arm-kernel, sudeep.holla, lukasz.luba,
james.quinlan, Jonathan.Cameron, robh, satyakim,
etienne.carriere
Hi Mark
thanks for reviewing this series.
On Tue, Oct 06, 2020 at 11:59:22AM +0100, Mark Brown wrote:
> On Mon, Oct 05, 2020 at 11:26:23PM +0100, Cristian Marussi wrote:
>
> > +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 negative voltages nor any current operation.
>
> Support for negative voltages is an implementation detail of Linux, IIRC
> there's nothing in the generic regulator binding preventing it and even
> if there were updates to the generic binding are what are relevant here
> - they could start working with no updates to this binding.
>
> > +Optional properties:
> > + - regulator-min-microvolt: when defined should be non-negative.
> > + - regulator-max-microvolt: when defined should be non-negative.
>
> These are covered by the generic regulator binding, no need to duplicate
> the documentation here.
Right I'll drop this references to negative voltages in the bindings.
Anyway, the underlying SCMI Voltage Domain protocol do support negatives
and just expose a negative_volts_allowed flags in the Voltage descriptor
if the domain is advertised by fw as supporting negatives.
Based on that, since the regulator framework as of now cannot report
negative voltages (since the .get ops return an int which is used also
for reporting negative error codes), the SCMI regulator driver in this
series just checks for that flag on every found Voltage Domain and refuse
to handle it like:
if (vinfo->negative_volts_allowed) {
+ dev_warn(dev, "Negative voltages NOT supported...skip %s\n",
+ sreg->of_node->full_name);
+ return -EOPNOTSUPP;
+ }
So the driver itself will need to be patched in this regards the day the
regulator framework should support negatives, it won't just work.
(but I suppose this is not a problem since it will need to be changed anyway
to use properly this possible new API handling negatives.)
Thanks
Cristian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
@ 2020-10-08 10:10 ` Etienne Carriere
2020-10-09 12:37 ` Cristian Marussi
0 siblings, 1 reply; 13+ messages in thread
From: Etienne Carriere @ 2020-10-08 10:10 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Sudeep Holla, lukasz.luba, Jim Quinlan, Jonathan.Cameron,
broonie, Rob Herring, satyakim
Hello Christian,
I faced an issue with this change, see below.
On Tue, 6 Oct 2020 at 00:26, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> Add SCMI Voltage Domain protocol support.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> 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 | 378 ++++++++++++++++++++++++++++
> include/linux/scmi_protocol.h | 64 +++++
> 5 files changed, 446 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 37fb583f1bf5..eaa4d0e1174f 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -167,6 +167,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 c5dea87edf8f..d7f50bf5f958 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -938,6 +938,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);
> @@ -953,6 +954,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..94221d979b80
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -0,0 +1,378 @@
> +// 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;
> + cmd->domain_id = cpu_to_le32(v->id);
> + /* ...then retrieve domain levels descriptions */
> + do {
> + u32 flags;
> + int cnt;
> +
Here domain_id must be reloaded in the message payload.
+ cmd->domain_id = cpu_to_le32(v->id);
When the SCMI shared memory is used as a unique buffer for
tx and rx messages, cmd refers to the memory storing previous
message sent on the SCMI channel.
In the loop we're in, the previous response has scratched
previous outgoing message including domain_id value hence
the full message sent to the SCMI server should be reloaded
before scmi_do_xfer() is called.
Regards,
etienne
> + 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;
> + /* 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_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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support
2020-10-08 10:10 ` Etienne Carriere
@ 2020-10-09 12:37 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-09 12:37 UTC (permalink / raw)
To: Etienne Carriere
Cc: linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Sudeep Holla, lukasz.luba, Jim Quinlan, Jonathan.Cameron,
broonie, Rob Herring, satyakim
On Thu, Oct 08, 2020 at 12:10:25PM +0200, Etienne Carriere wrote:
> Hello Christian,
>
> I faced an issue with this change, see below.
>
Hi Etienne,
thanks for the review.
[snip]
> > +
> > +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;
> > + cmd->domain_id = cpu_to_le32(v->id);
> > + /* ...then retrieve domain levels descriptions */
> > + do {
> > + u32 flags;
> > + int cnt;
> > +
>
> Here domain_id must be reloaded in the message payload.
>
> + cmd->domain_id = cpu_to_le32(v->id);
>
> When the SCMI shared memory is used as a unique buffer for
> tx and rx messages, cmd refers to the memory storing previous
> message sent on the SCMI channel.
> In the loop we're in, the previous response has scratched
> previous outgoing message including domain_id value hence
> the full message sent to the SCMI server should be reloaded
> before scmi_do_xfer() is called.
>
Ah right.. I forgot you already pointed out this on another thread.
My bad, I'll fix in v2.
Thanks
Cristian
> Regards,
> etienne
>
> > + 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;
> > + /* 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_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
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator
2020-10-06 0:38 ` [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Florian Fainelli
@ 2020-10-09 12:42 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-09 12:42 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, linux-arm-kernel, robh, satyakim, sudeep.holla,
broonie, james.quinlan, Jonathan.Cameron, etienne.carriere,
lukasz.luba
On Mon, Oct 05, 2020 at 05:38:52PM -0700, Florian Fainelli wrote:
>
>
> On 10/5/2020 3:26 PM, Cristian Marussi wrote:
> >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 66d90f6ecee7 ("firmware: arm_scmi: Enable building as a single
> > module")
> >
> >Any feedback welcome,
>
> Well, this is just great! We were right about to develop a proprietary SCMI
> protocol in order to control a locked down PMIC accessible behind a secured
> firmware. We would have done essentially just that since the use case is to
> control the various regulators exposed by this PMIC over SCMI. Thanks a lot!
>
You're welcome :D ... any feedback from your independent testing will be
appreciated.
Thanks
Cristian
> >
> >Thanks,
> >
> >Cristian
> >
> >[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 | 44 ++
> > 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 | 378 ++++++++++++++
> > drivers/regulator/Kconfig | 9 +
> > drivers/regulator/Makefile | 1 +
> > drivers/regulator/scmi-regulator.c | 488 ++++++++++++++++++
> > include/linux/scmi_protocol.h | 64 +++
> > 9 files changed, 989 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/firmware/arm_scmi/voltage.c
> > create mode 100644 drivers/regulator/scmi-regulator.c
> >
>
> --
> Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] regulator: add SCMI driver
2020-10-06 10:56 ` Mark Brown
@ 2020-10-15 15:38 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2020-10-15 15:38 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, linux-arm-kernel, sudeep.holla, lukasz.luba,
james.quinlan, Jonathan.Cameron, robh, satyakim,
etienne.carriere
Hi
sorry for the late reply.
On Tue, Oct 06, 2020 at 11:56:37AM +0100, Mark Brown wrote:
> On Mon, Oct 05, 2020 at 11:26:22PM +0100, Cristian Marussi wrote:
>
> > - .get_voltage / .set_voltage: routed via SCMI Voltage Domain Protocol
> > - .get_voltage_sel/.set_voltage_sel: using regulator framework helpers
>
> You should not be implementing both of these interfaces, pick one. It
> looks like the direct voltage operations are the redundant ones here,
> while the protocol uses actual voltages to communicate with the firmware
> which makes the direct voltage operations a better fit it seems like the
> expectation is that only a limited set of voltages is supported (as is
> normal for the underlying physical regulators) so you want selectors.
>
I'm dropping non _sel methods in V2.
> > + sreg->name = devm_kasprintf(dev, GFP_KERNEL, "%s", vinfo->name);
> > + sreg->desc.name = devm_kasprintf(dev, GFP_KERNEL,
> > + "Vscmi.%s", sreg->name);
> > + if (!sreg->name || !sreg->desc.name)
> > + return -ENOMEM;
>
> Why are we using different names here?
>
Not really a good reason...dropping internal name and "Vscmi" prefix
in V2.
> > + num_doms = handle->voltage_ops->num_domains_get(handle);
> > + if (num_doms <= 0) {
> > + dev_err(&sdev->dev, "number of voltage domains invalid\n");
> > + return num_doms ?: -EINVAL;
>
> Please write normal conditional statements to improve legibility.
Ok.
Thanks
Cristian
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-10-15 15:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
2020-10-08 10:10 ` Etienne Carriere
2020-10-09 12:37 ` Cristian Marussi
2020-10-05 22:26 ` [PATCH 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname Cristian Marussi
2020-10-05 22:26 ` [PATCH 3/4] regulator: add SCMI driver Cristian Marussi
2020-10-06 10:56 ` Mark Brown
2020-10-15 15:38 ` Cristian Marussi
2020-10-05 22:26 ` [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators Cristian Marussi
2020-10-06 10:59 ` Mark Brown
2020-10-07 8:06 ` Cristian Marussi
2020-10-06 0:38 ` [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Florian Fainelli
2020-10-09 12:42 ` Cristian Marussi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).