[PATCH v2 0/5] cmd: add scmi command

[PATCH v2 0/5] cmd: add scmi command

[AKASHI Takahiro]

"Scmi" command will be re-introduced per Michal's request.
The functionality is the same as I put it in my patch set of adding
SCMI base protocol support, but made some tweak to make UT, "ut dm
scmi_cmd," more flexible and tolerable when enabling/disabling a specific
SCMI protocol for test purpose.

Each commit may have some change history inherited from the preceding
patch series.

Test
====
The patch series was tested on the following platforms:
* sandbox

Prerequisite:
=============
* This patch series is based on the latest master.

Changes:
========
v2(Nov 13, 2023)
* localize global variables to avoid pytest errors.

AKASHI Takahiro (5):
  test: dm: skip scmi tests against disabled protocols
  firmware: scmi: support protocols on sandbox only if enabled
  cmd: add scmi command for SCMI firmware
  doc: cmd: add documentation for scmi
  test: dm: add scmi command test

 cmd/Kconfig                                  |   9 +
 cmd/Makefile                                 |   1 +
 cmd/scmi.c                                   | 384 +++++++++++++++++++
 configs/sandbox_defconfig                    |   1 +
 doc/usage/cmd/scmi.rst                       | 126 ++++++
 doc/usage/index.rst                          |   1 +
 drivers/firmware/scmi/sandbox-scmi_agent.c   |  27 +-
 drivers/firmware/scmi/sandbox-scmi_devices.c |  78 ++--
 test/dm/scmi.c                               |  93 +++++
 9 files changed, 687 insertions(+), 33 deletions(-)
 create mode 100644 cmd/scmi.c
 create mode 100644 doc/usage/cmd/scmi.rst

-- 
2.34.1

[PATCH v2 1/5] test: dm: skip scmi tests against disabled protocols

[AKASHI Takahiro]

This is a precautionary change to make scmi tests workable whether or not
a specific protocol be enabled.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/dm/scmi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/dm/scmi.c b/test/dm/scmi.c
index da45314f2e4c..2f63f2da16fb 100644
--- a/test/dm/scmi.c
+++ b/test/dm/scmi.c
@@ -217,6 +217,9 @@ static int dm_test_scmi_power_domains(struct unit_test_state *uts)
 	u8 *name;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
+		return 0;
+
 	/* preparation */
 	ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
 	ut_assertnonnull(agent);
@@ -317,6 +320,9 @@ static int dm_test_scmi_clocks(struct unit_test_state *uts)
 	int ret_dev;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_CLK_SCMI))
+		return 0;
+
 	ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
 	if (ret)
 		return ret;
@@ -382,6 +388,9 @@ static int dm_test_scmi_resets(struct unit_test_state *uts)
 	struct udevice *agent_dev, *reset_dev, *dev = NULL;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_RESET_SCMI))
+		return 0;
+
 	ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
 	if (ret)
 		return ret;
@@ -418,6 +427,9 @@ static int dm_test_scmi_voltage_domains(struct unit_test_state *uts)
 	struct udevice *dev;
 	struct udevice *regul0_dev;
 
+	if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
+		return 0;
+
 	ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
 
 	scmi_devices = sandbox_scmi_devices_ctx(dev);
-- 
2.34.1

[PATCH v2 2/5] firmware: scmi: support protocols on sandbox only if enabled

[AKASHI Takahiro]

This change will be useful when we manually test SCMI on sandbox
by enabling/disabling a specific SCMI protocol.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/firmware/scmi/sandbox-scmi_agent.c   | 27 ++++++-
 drivers/firmware/scmi/sandbox-scmi_devices.c | 78 ++++++++++++--------
 2 files changed, 72 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
index d13180962662..1fc9a0f4ea7e 100644
--- a/drivers/firmware/scmi/sandbox-scmi_agent.c
+++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
@@ -66,10 +66,18 @@ struct scmi_channel {
 };
 
 static u8 protocols[] = {
+#if IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)
 	SCMI_PROTOCOL_ID_POWER_DOMAIN,
+#endif
+#if IS_ENABLED(CONFIG_CLK_SCMI)
 	SCMI_PROTOCOL_ID_CLOCK,
+#endif
+#if IS_ENABLED(CONFIG_RESET_SCMI)
 	SCMI_PROTOCOL_ID_RESET_DOMAIN,
+#endif
+#if IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)
 	SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
+#endif
 };
 
 #define NUM_PROTOCOLS ARRAY_SIZE(protocols)
@@ -1160,6 +1168,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 		}
 		break;
 	case SCMI_PROTOCOL_ID_POWER_DOMAIN:
+		if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
+			goto not_supported;
+
 		switch (msg->message_id) {
 		case SCMI_PROTOCOL_VERSION:
 			return sandbox_scmi_pwd_protocol_version(dev, msg);
@@ -1180,6 +1191,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 		}
 		break;
 	case SCMI_PROTOCOL_ID_CLOCK:
+		if (!IS_ENABLED(CONFIG_CLK_SCMI))
+			goto not_supported;
+
 		switch (msg->message_id) {
 		case SCMI_PROTOCOL_ATTRIBUTES:
 			return sandbox_scmi_clock_protocol_attribs(dev, msg);
@@ -1196,6 +1210,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 		}
 		break;
 	case SCMI_PROTOCOL_ID_RESET_DOMAIN:
+		if (!IS_ENABLED(CONFIG_RESET_SCMI))
+			goto not_supported;
+
 		switch (msg->message_id) {
 		case SCMI_RESET_DOMAIN_ATTRIBUTES:
 			return sandbox_scmi_rd_attribs(dev, msg);
@@ -1206,6 +1223,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 		}
 		break;
 	case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
+		if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
+			goto not_supported;
+
 		switch (msg->message_id) {
 		case SCMI_VOLTAGE_DOMAIN_ATTRIBUTES:
 			return sandbox_scmi_voltd_attribs(dev, msg);
@@ -1224,8 +1244,7 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 	case SCMI_PROTOCOL_ID_SYSTEM:
 	case SCMI_PROTOCOL_ID_PERF:
 	case SCMI_PROTOCOL_ID_SENSOR:
-		*(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
-		return 0;
+		goto not_supported;
 	default:
 		break;
 	}
@@ -1239,6 +1258,10 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 	/* Intentionnaly report unhandled IDs through the SCMI return code */
 	*(u32 *)msg->out_msg = SCMI_PROTOCOL_ERROR;
 	return 0;
+
+not_supported:
+	*(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
+	return 0;
 }
 
 static int sandbox_scmi_test_remove(struct udevice *dev)
diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
index facb5b06ffb5..0519cf889aa9 100644
--- a/drivers/firmware/scmi/sandbox-scmi_devices.c
+++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
@@ -62,12 +62,13 @@ static int sandbox_scmi_devices_remove(struct udevice *dev)
 	if (!devices)
 		return 0;
 
-	for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
-		int ret2 = reset_free(devices->reset + n);
+	if (IS_ENABLED(CONFIG_RESET_SCMI))
+		for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
+			int ret2 = reset_free(devices->reset + n);
 
-		if (ret2 && !ret)
-			ret = ret2;
-	}
+			if (ret2 && !ret)
+				ret = ret2;
+		}
 
 	return ret;
 }
@@ -89,39 +90,53 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
 		.regul_count = SCMI_TEST_DEVICES_VOLTD_COUNT,
 	};
 
-	ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
-	if (ret) {
-		dev_err(dev, "%s: Failed on power domain\n", __func__);
-		return ret;
-	}
-
-	for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
-		ret = clk_get_by_index(dev, n, priv->devices.clk + n);
+	if (IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)) {
+		ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
 		if (ret) {
-			dev_err(dev, "%s: Failed on clk %zu\n", __func__, n);
+			dev_err(dev, "%s: Failed on power domain\n", __func__);
 			return ret;
 		}
 	}
 
-	for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
-		ret = reset_get_by_index(dev, n, priv->devices.reset + n);
-		if (ret) {
-			dev_err(dev, "%s: Failed on reset %zu\n", __func__, n);
-			goto err_reset;
+	if (IS_ENABLED(CONFIG_CLK_SCMI)) {
+		for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
+			ret = clk_get_by_index(dev, n, priv->devices.clk + n);
+			if (ret) {
+				dev_err(dev, "%s: Failed on clk %zu\n",
+					__func__, n);
+				return ret;
+			}
 		}
 	}
 
-	for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
-		char name[32];
-
-		ret = snprintf(name, sizeof(name), "regul%zu-supply", n);
-		assert(ret >= 0 && ret < sizeof(name));
+	if (IS_ENABLED(CONFIG_RESET_SCMI)) {
+		for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
+			ret = reset_get_by_index(dev, n,
+						 priv->devices.reset + n);
+			if (ret) {
+				dev_err(dev, "%s: Failed on reset %zu\n",
+					__func__, n);
+				goto err_reset;
+			}
+		}
+	}
 
-		ret = device_get_supply_regulator(dev, name,
-						  priv->devices.regul + n);
-		if (ret) {
-			dev_err(dev, "%s: Failed on voltd %zu\n", __func__, n);
-			goto err_regul;
+	if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
+		for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
+			char name[32];
+
+			ret = snprintf(name, sizeof(name), "regul%zu-supply",
+				       n);
+			assert(ret >= 0 && ret < sizeof(name));
+
+			ret = device_get_supply_regulator(dev, name,
+							  priv->devices.regul
+								+ n);
+			if (ret) {
+				dev_err(dev, "%s: Failed on voltd %zu\n",
+					__func__, n);
+				goto err_regul;
+			}
 		}
 	}
 
@@ -130,8 +145,9 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
 err_regul:
 	n = SCMI_TEST_DEVICES_RD_COUNT;
 err_reset:
-	for (; n > 0; n--)
-		reset_free(priv->devices.reset + n - 1);
+	if (IS_ENABLED(CONFIG_RESET_SCMI))
+		for (; n > 0; n--)
+			reset_free(priv->devices.reset + n - 1);
 
 	return ret;
 }
-- 
2.34.1

[PATCH v2 3/5] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

This command, "scmi", may provide a command line interface to various SCMI
protocols. It supports at least initially SCMI base protocol and is
intended mainly for debug purpose.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
v8 (actually v2 as SCMI cmd)
* localize global variables to avoid pytest errors
v3
* describe that arguments are in hex at a help message
* modify the code for dynamically allocated agent names
v2
* remove sub command category, 'scmi base', for simplicity
---
 cmd/Kconfig  |   9 ++
 cmd/Makefile |   1 +
 cmd/scmi.c   | 384 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 394 insertions(+)
 create mode 100644 cmd/scmi.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index df6d71c103f9..ca9f742dcf78 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2559,6 +2559,15 @@ config CMD_CROS_EC
 	  a number of sub-commands for performing EC tasks such as
 	  updating its flash, accessing a small saved context area
 	  and talking to the I2C bus behind the EC (if there is one).
+
+config CMD_SCMI
+	bool "Enable scmi command"
+	depends on SCMI_FIRMWARE
+	default n
+	help
+	  This command provides user interfaces to several SCMI (System
+	  Control and Management Interface) protocols available on Arm
+	  platforms to manage system resources.
 endmenu
 
 menu "Filesystem commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 9a6790cc1708..320f0b5266eb 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -159,6 +159,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
 obj-$(CONFIG_CMD_NVME) += nvme.o
 obj-$(CONFIG_SANDBOX) += sb.o
 obj-$(CONFIG_CMD_SF) += sf.o
+obj-$(CONFIG_CMD_SCMI) += scmi.o
 obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
 obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
 obj-$(CONFIG_CMD_SEAMA) += seama.o
diff --git a/cmd/scmi.c b/cmd/scmi.c
new file mode 100644
index 000000000000..664062c4eff5
--- /dev/null
+++ b/cmd/scmi.c
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  SCMI (System Control and Management Interface) utility command
+ *
+ *  Copyright (c) 2023 Linaro Limited
+ *		Author: AKASHI Takahiro
+ */
+
+#include <command.h>
+#include <exports.h>
+#include <scmi_agent.h>
+#include <scmi_agent-uclass.h>
+#include <stdlib.h>
+#include <asm/types.h>
+#include <dm/device.h>
+#include <dm/uclass.h> /* uclass_get_device */
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+
+struct {
+	enum scmi_std_protocol id;
+	const char *name;
+} protocol_name[] = {
+	{SCMI_PROTOCOL_ID_BASE, "Base"},
+	{SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
+	{SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
+	{SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
+	{SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
+	{SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
+	{SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
+	{SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
+};
+
+/**
+ * get_agent() - get SCMI agent device
+ *
+ * Return:	Pointer to SCMI agent device on success, NULL on failure
+ */
+static struct udevice *get_agent(void)
+{
+	struct udevice *agent;
+
+	if (uclass_get_device(UCLASS_SCMI_AGENT, 0, &agent)) {
+		printf("Cannot find any SCMI agent\n");
+		return NULL;
+	}
+
+	return agent;
+}
+
+/**
+ * get_base_proto() - get SCMI base protocol device
+ * @agent:	SCMI agent device
+ *
+ * Return:	Pointer to SCMI base protocol device on success,
+ *		NULL on failure
+ */
+static struct udevice *get_base_proto(struct udevice *agent)
+{
+	struct udevice *base_proto;
+
+	if (!agent) {
+		agent = get_agent();
+		if (!agent)
+			return NULL;
+	}
+
+	base_proto = scmi_get_protocol(agent, SCMI_PROTOCOL_ID_BASE);
+	if (!base_proto) {
+		printf("SCMI base protocol not found\n");
+		return NULL;
+	}
+
+	return base_proto;
+}
+
+/**
+ * get_proto_name() - get the name of SCMI protocol
+ *
+ * @id:		SCMI Protocol ID
+ *
+ * Get the printable name of the protocol, @id
+ *
+ * Return:	Name string on success, NULL on failure
+ */
+static const char *get_proto_name(enum scmi_std_protocol id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
+		if (id == protocol_name[i].id)
+			return protocol_name[i].name;
+
+	return NULL;
+}
+
+/**
+ * do_scmi_info() - get the information of SCMI services
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Get the information of SCMI services using various interfaces
+ * provided by the Base protocol.
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_info(struct cmd_tbl *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	struct udevice *agent, *base_proto;
+	u32 agent_id, num_protocols;
+	u8 *agent_name, *protocols;
+	int i, ret;
+
+	if (argc != 1)
+		return CMD_RET_USAGE;
+
+	agent = get_agent();
+	if (!agent)
+		return CMD_RET_FAILURE;
+	base_proto = get_base_proto(agent);
+	if (!base_proto)
+		return CMD_RET_FAILURE;
+
+	printf("SCMI device: %s\n", agent->name);
+	printf("  protocol version: 0x%x\n", scmi_version(agent));
+	printf("  # of agents: %d\n", scmi_num_agents(agent));
+	for (i = 0; i < scmi_num_agents(agent); i++) {
+		ret = scmi_base_discover_agent(base_proto, i, &agent_id,
+					       &agent_name);
+		if (ret) {
+			if (ret != -EOPNOTSUPP)
+				printf("base_discover_agent() failed for id: %d (%d)\n",
+				       i, ret);
+			break;
+		}
+		printf("    %c%2d: %s\n", i == scmi_agent_id(agent) ? '>' : ' ',
+		       i, agent_name);
+		free(agent_name);
+	}
+	printf("  # of protocols: %d\n", scmi_num_protocols(agent));
+	num_protocols = scmi_num_protocols(agent);
+	protocols = scmi_protocols(agent);
+	if (protocols)
+		for (i = 0; i < num_protocols; i++)
+			printf("      %s\n", get_proto_name(protocols[i]));
+	printf("  vendor: %s\n", scmi_vendor(agent));
+	printf("  sub vendor: %s\n", scmi_sub_vendor(agent));
+	printf("  impl version: 0x%x\n", scmi_impl_version(agent));
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_set_dev() - set access permission to device
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Set access permission to device with SCMI_BASE_SET_DEVICE_PERMISSIONS
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
+			   char * const argv[])
+{
+	u32 agent_id, device_id, flags, attributes;
+	char *end;
+	struct udevice *base_proto;
+	int ret;
+
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	device_id = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[3], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	base_proto = get_base_proto(NULL);
+	if (!base_proto)
+		return CMD_RET_FAILURE;
+
+	ret = scmi_base_protocol_message_attrs(base_proto,
+					       SCMI_BASE_SET_DEVICE_PERMISSIONS,
+					       &attributes);
+	if (ret) {
+		printf("This operation is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = scmi_base_set_device_permissions(base_proto, agent_id,
+					       device_id, flags);
+	if (ret) {
+		printf("%s access to device:%u failed (%d)\n",
+		       flags ? "Allowing" : "Denying", device_id, ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_set_proto() - set protocol permission to device
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Set protocol permission to device with SCMI_BASE_SET_PROTOCOL_PERMISSIONS
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_set_proto(struct cmd_tbl *cmdtp, int flag, int argc,
+			     char * const argv[])
+{
+	u32 agent_id, device_id, protocol_id, flags, attributes;
+	char *end;
+	struct udevice *base_proto;
+	int ret;
+
+	if (argc != 5)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	device_id = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	protocol_id = simple_strtoul(argv[3], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[4], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	base_proto = get_base_proto(NULL);
+	if (!base_proto)
+		return CMD_RET_FAILURE;
+
+	ret = scmi_base_protocol_message_attrs(base_proto,
+					       SCMI_BASE_SET_PROTOCOL_PERMISSIONS,
+					       &attributes);
+	if (ret) {
+		printf("This operation is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = scmi_base_set_protocol_permissions(base_proto, agent_id,
+						 device_id, protocol_id,
+						 flags);
+	if (ret) {
+		printf("%s access to protocol:0x%x on device:%u failed (%d)\n",
+		       flags ? "Allowing" : "Denying", protocol_id, device_id,
+		       ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_reset() - reset platform resource settings
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Reset platform resource settings with BASE_RESET_AGENT_CONFIGURATION
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_reset(struct cmd_tbl *cmdtp, int flag, int argc,
+			 char * const argv[])
+{
+	u32 agent_id, flags, attributes;
+	char *end;
+	struct udevice *base_proto;
+	int ret;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	base_proto = get_base_proto(NULL);
+	if (!base_proto)
+		return CMD_RET_FAILURE;
+
+	ret = scmi_base_protocol_message_attrs(base_proto,
+					       SCMI_BASE_RESET_AGENT_CONFIGURATION,
+					       &attributes);
+	if (ret) {
+		printf("Reset is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = scmi_base_reset_agent_configuration(base_proto, agent_id, flags);
+	if (ret) {
+		printf("Reset failed (%d)\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static struct cmd_tbl cmd_scmi_sub[] = {
+	U_BOOT_CMD_MKENT(info, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_info, "", ""),
+	U_BOOT_CMD_MKENT(perm_dev, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_set_dev, "", ""),
+	U_BOOT_CMD_MKENT(perm_proto, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_set_proto, "", ""),
+	U_BOOT_CMD_MKENT(reset, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_reset, "", ""),
+};
+
+/**
+ * do_scmi() - SCMI utility
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Provide user interfaces to SCMI protocols.
+ *
+ * Return:	CMD_RET_SUCCESS on success,
+ *		CMD_RET_USAGE or CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi(struct cmd_tbl *cmdtp, int flag,
+		   int argc, char *const argv[])
+{
+	struct cmd_tbl *cp;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	cp = find_cmd_tbl(argv[0], cmd_scmi_sub, ARRAY_SIZE(cmd_scmi_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+static char scmi_help_text[] =
+	" - SCMI utility\n"
+	" info - get the info of SCMI services\n"
+	" perm_dev <agent-id in hex> <device-id in hex> <flags in hex>\n"
+	"   - set access permission to device\n"
+	" perm_proto <agent-id in hex> <device-id in hex> <protocol-id in hex> <flags in hex>\n"
+	"   - set protocol permission to device\n"
+	" reset <agent-id in hex> <flags in hex>\n"
+	"   - reset platform resource settings\n"
+	"";
+
+U_BOOT_CMD(scmi, CONFIG_SYS_MAXARGS, 0, do_scmi, "SCMI utility",
+	   scmi_help_text);
-- 
2.34.1

[PATCH v2 4/5] doc: cmd: add documentation for scmi

[AKASHI Takahiro]

This is a help text for scmi command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
v6
* add the manual to doc/usage/index.rst
v4
* s/tranport/transport/
v2
* add more descriptions about SCMI
---
 doc/usage/cmd/scmi.rst | 126 +++++++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst    |   1 +
 2 files changed, 127 insertions(+)
 create mode 100644 doc/usage/cmd/scmi.rst

diff --git a/doc/usage/cmd/scmi.rst b/doc/usage/cmd/scmi.rst
new file mode 100644
index 000000000000..9ea7e0e41dad
--- /dev/null
+++ b/doc/usage/cmd/scmi.rst
@@ -0,0 +1,126 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+scmi command
+============
+
+Synopsis
+--------
+
+::
+
+    scmi info
+    scmi perm_dev <agent id> <device id> <flags>
+    scmi perm_proto <agent id> <device id> <protocol id> <flags>
+    scmi reset <agent id> <flags>
+
+Description
+-----------
+
+Arm System Control and Management Interface (SCMI hereafter) is a set of
+standardised interfaces to manage system resources, like clocks, power
+domains, pin controls, reset and so on, in a system-wide manner.
+
+An entity which provides those services is called a SCMI firmware (or
+SCMI server if you like) may be placed/implemented by EL3 software or
+by a dedicated system control processor (SCP) or else.
+
+A user of SCMI interfaces, including U-Boot, is called a SCMI agent and
+may issues commands, which are defined in each protocol for specific system
+resources, to SCMI server via a communication channel, called a transport.
+Those interfaces are independent from the server's implementation thanks to
+a transport layer.
+
+For more details, see the `SCMI specification`_.
+
+While most of system resources managed under SCMI protocols are implemented
+and handled as standard U-Boot devices, for example clk_scmi, scmi command
+provides additional management functionality against SCMI server.
+
+scmi info
+~~~~~~~~~
+    Show base information about SCMI server and supported protocols
+
+scmi perm_dev
+~~~~~~~~~~~~~
+    Allow or deny access permission to the device
+
+scmi perm_proto
+~~~~~~~~~~~~~~~
+    Allow or deny access to the protocol on the device
+
+scmi reset
+~~~~~~~~~~
+    Reset the already-configured permissions against the device
+
+Parameters are used as follows:
+
+<agent id>
+    SCMI Agent ID, hex value
+
+<device id>
+    SCMI Device ID, hex value
+
+    Please note that what a device means is not defined
+    in the specification.
+
+<protocol id>
+    SCMI Protocol ID, hex value
+
+    It must not be 0x10 (base protocol)
+
+<flags>
+    Flags to control the action, hex value
+
+    0 to deny, 1 to allow. The other values are reserved and allowed
+    values may depend on the implemented version of SCMI server in
+    the future. See SCMI specification for more details.
+
+Example
+-------
+
+Obtain basic information about SCMI server:
+
+::
+
+    => scmi info
+    SCMI device: scmi
+      protocol version: 0x20000
+      # of agents: 3
+          0: platform
+        > 1: OSPM
+          2: PSCI
+      # of protocols: 4
+          Power domain management
+          Performance domain management
+          Clock management
+          Sensor management
+      vendor: Linaro
+      sub vendor: PMWG
+      impl version: 0x20b0000
+
+Ask for access permission to device#0:
+
+::
+
+    => scmi perm_dev 1 0 1
+
+Reset configurations with all access permission settings retained:
+
+::
+
+    => scmi reset 1 0
+
+Configuration
+-------------
+
+The scmi command is only available if CONFIG_CMD_SCMI=y.
+Default n because this command is mainly for debug purpose.
+
+Return value
+------------
+
+The return value ($?) is set to 0 if the operation succeeded,
+1 if the operation failed or -1 if the operation failed due to
+a syntax error.
+
+.. _`SCMI specification`: https://developer.arm.com/documentation/den0056/e/?lang=en
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index d8e23fcacffb..1a626c03c237 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -94,6 +94,7 @@ Shell commands
    cmd/rng
    cmd/saves
    cmd/sbi
+   cmd/scmi
    cmd/scp03
    cmd/seama
    cmd/setexpr
-- 
2.34.1

[PATCH v2 5/5] test: dm: add scmi command test

[AKASHI Takahiro]

In this test, "scmi" command is tested against different sub-commands.
Please note that scmi command is for debug purpose and is not intended
in production system.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
v7
* make test assertions more flexible depending on the number of provided
  protocols
v4
* move 'base'-related changes to the prior commit
* add CONFIG_CMD_SCMI to sandbox_defconfig
v3
* change char to u8 in vendor/agent names
v2
* use helper functions, removing direct uses of ops
---
 configs/sandbox_defconfig |  1 +
 test/dm/scmi.c            | 81 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index bc5bcb2a6237..c550af93b0ca 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -121,6 +121,7 @@ CONFIG_CMD_REGULATOR=y
 CONFIG_CMD_AES=y
 CONFIG_CMD_TPM=y
 CONFIG_CMD_TPM_TEST=y
+CONFIG_CMD_SCMI=y
 CONFIG_CMD_BTRFS=y
 CONFIG_CMD_CBFS=y
 CONFIG_CMD_CRAMFS=y
diff --git a/test/dm/scmi.c b/test/dm/scmi.c
index 2f63f2da16fb..2bcf7ac6fcc3 100644
--- a/test/dm/scmi.c
+++ b/test/dm/scmi.c
@@ -19,6 +19,7 @@
 #include <scmi_agent.h>
 #include <scmi_agent-uclass.h>
 #include <scmi_protocols.h>
+#include <vsprintf.h>
 #include <asm/scmi_test.h>
 #include <dm/device-internal.h>
 #include <dm/test.h>
@@ -206,6 +207,86 @@ static int dm_test_scmi_base(struct unit_test_state *uts)
 
 DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT);
 
+static int dm_test_scmi_cmd(struct unit_test_state *uts)
+{
+	struct udevice *agent_dev;
+	int num_proto = 0;
+	char cmd_out[30];
+
+	if (!IS_ENABLED(CONFIG_CMD_SCMI))
+		return 0;
+
+	/* preparation */
+	ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
+					      &agent_dev));
+	ut_assertnonnull(agent_dev);
+
+	/*
+	 * Estimate the number of provided protocols.
+	 * This estimation is correct as far as a corresponding
+	 * protocol support is added to sandbox fake serer.
+	 */
+	if (IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
+		num_proto++;
+	if (IS_ENABLED(CONFIG_CLK_SCMI))
+		num_proto++;
+	if (IS_ENABLED(CONFIG_RESET_SCMI))
+		num_proto++;
+	if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
+		num_proto++;
+
+	/* scmi info */
+	ut_assertok(run_command("scmi info", 0));
+
+	ut_assert_nextline("SCMI device: scmi");
+	snprintf(cmd_out, 30, "  protocol version: 0x%x",
+		 SCMI_BASE_PROTOCOL_VERSION);
+	ut_assert_nextline(cmd_out);
+	ut_assert_nextline("  # of agents: 2");
+	ut_assert_nextline("      0: platform");
+	ut_assert_nextline("    > 1: OSPM");
+	snprintf(cmd_out, 30, "  # of protocols: %d", num_proto);
+	ut_assert_nextline(cmd_out);
+	if (IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
+		ut_assert_nextline("      Power domain management");
+	if (IS_ENABLED(CONFIG_CLK_SCMI))
+		ut_assert_nextline("      Clock management");
+	if (IS_ENABLED(CONFIG_RESET_SCMI))
+		ut_assert_nextline("      Reset domain management");
+	if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
+		ut_assert_nextline("      Voltage domain management");
+	ut_assert_nextline("  vendor: U-Boot");
+	ut_assert_nextline("  sub vendor: Sandbox");
+	ut_assert_nextline("  impl version: 0x1");
+
+	ut_assert_console_end();
+
+	/* scmi perm_dev */
+	ut_assertok(run_command("scmi perm_dev 1 0 1", 0));
+	ut_assert_console_end();
+
+	ut_assert(run_command("scmi perm_dev 1 0 0", 0));
+	ut_assert_nextline("Denying access to device:0 failed (-13)");
+	ut_assert_console_end();
+
+	/* scmi perm_proto */
+	ut_assertok(run_command("scmi perm_proto 1 0 14 1", 0));
+	ut_assert_console_end();
+
+	ut_assert(run_command("scmi perm_proto 1 0 14 0", 0));
+	ut_assert_nextline("Denying access to protocol:0x14 on device:0 failed (-13)");
+	ut_assert_console_end();
+
+	/* scmi reset */
+	ut_assert(run_command("scmi reset 1 1", 0));
+	ut_assert_nextline("Reset failed (-13)");
+	ut_assert_console_end();
+
+	return 0;
+}
+
+DM_TEST(dm_test_scmi_cmd, UT_TESTF_SCAN_FDT);
+
 static int dm_test_scmi_power_domains(struct unit_test_state *uts)
 {
 	struct sandbox_scmi_agent *agent;
-- 
2.34.1

Re: [PATCH v2 1/5] test: dm: skip scmi tests against disabled protocols

[Simon Glass]

Hi AKASHI,

On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This is a precautionary change to make scmi tests workable whether or not
> a specific protocol be enabled.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  test/dm/scmi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> index da45314f2e4c..2f63f2da16fb 100644
> --- a/test/dm/scmi.c
> +++ b/test/dm/scmi.c
> @@ -217,6 +217,9 @@ static int dm_test_scmi_power_domains(struct unit_test_state *uts)
>         u8 *name;
>         int ret;
>
> +       if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
> +               return 0;

-EAGAIN to skip a test

Please update a comment if this needs to be documented better

> +
>         /* preparation */
>         ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
>         ut_assertnonnull(agent);
> @@ -317,6 +320,9 @@ static int dm_test_scmi_clocks(struct unit_test_state *uts)
>         int ret_dev;
>         int ret;
>
> +       if (!IS_ENABLED(CONFIG_CLK_SCMI))
> +               return 0;
> +
>         ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
>         if (ret)
>                 return ret;
> @@ -382,6 +388,9 @@ static int dm_test_scmi_resets(struct unit_test_state *uts)
>         struct udevice *agent_dev, *reset_dev, *dev = NULL;
>         int ret;
>
> +       if (!IS_ENABLED(CONFIG_RESET_SCMI))
> +               return 0;
> +
>         ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
>         if (ret)
>                 return ret;
> @@ -418,6 +427,9 @@ static int dm_test_scmi_voltage_domains(struct unit_test_state *uts)
>         struct udevice *dev;
>         struct udevice *regul0_dev;
>
> +       if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
> +               return 0;
> +
>         ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
>
>         scmi_devices = sandbox_scmi_devices_ctx(dev);
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 2/5] firmware: scmi: support protocols on sandbox only if enabled

[Simon Glass]

Hi AKASHI,

On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This change will be useful when we manually test SCMI on sandbox
> by enabling/disabling a specific SCMI protocol.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/firmware/scmi/sandbox-scmi_agent.c   | 27 ++++++-
>  drivers/firmware/scmi/sandbox-scmi_devices.c | 78 ++++++++++++--------
>  2 files changed, 72 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
> index d13180962662..1fc9a0f4ea7e 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> @@ -66,10 +66,18 @@ struct scmi_channel {
>  };
>
>  static u8 protocols[] = {
> +#if IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)
>         SCMI_PROTOCOL_ID_POWER_DOMAIN,

Is this better? Perhaps not!

CONFIG_IS_ENABLED(SCMI_POWER_DOMAIN, (SCMI_PROTOCOL_ID_POWER_DOMAIN,))

> +#endif
> +#if IS_ENABLED(CONFIG_CLK_SCMI)
>         SCMI_PROTOCOL_ID_CLOCK,
> +#endif
> +#if IS_ENABLED(CONFIG_RESET_SCMI)
>         SCMI_PROTOCOL_ID_RESET_DOMAIN,
> +#endif
> +#if IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)
>         SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
> +#endif
>  };
>
>  #define NUM_PROTOCOLS ARRAY_SIZE(protocols)
> @@ -1160,6 +1168,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>                 }
>                 break;
>         case SCMI_PROTOCOL_ID_POWER_DOMAIN:
> +               if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
> +                       goto not_supported;
> +
>                 switch (msg->message_id) {
>                 case SCMI_PROTOCOL_VERSION:
>                         return sandbox_scmi_pwd_protocol_version(dev, msg);
> @@ -1180,6 +1191,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>                 }
>                 break;
>         case SCMI_PROTOCOL_ID_CLOCK:
> +               if (!IS_ENABLED(CONFIG_CLK_SCMI))
> +                       goto not_supported;

How about putting this all in a function and avoiding the goto?

> +
>                 switch (msg->message_id) {
>                 case SCMI_PROTOCOL_ATTRIBUTES:
>                         return sandbox_scmi_clock_protocol_attribs(dev, msg);
> @@ -1196,6 +1210,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>                 }
>                 break;
>         case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> +               if (!IS_ENABLED(CONFIG_RESET_SCMI))
> +                       goto not_supported;
> +
>                 switch (msg->message_id) {
>                 case SCMI_RESET_DOMAIN_ATTRIBUTES:
>                         return sandbox_scmi_rd_attribs(dev, msg);
> @@ -1206,6 +1223,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>                 }
>                 break;
>         case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> +               if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
> +                       goto not_supported;
> +
>                 switch (msg->message_id) {
>                 case SCMI_VOLTAGE_DOMAIN_ATTRIBUTES:
>                         return sandbox_scmi_voltd_attribs(dev, msg);
> @@ -1224,8 +1244,7 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>         case SCMI_PROTOCOL_ID_SYSTEM:
>         case SCMI_PROTOCOL_ID_PERF:
>         case SCMI_PROTOCOL_ID_SENSOR:
> -               *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> -               return 0;
> +               goto not_supported;
>         default:
>                 break;
>         }
> @@ -1239,6 +1258,10 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>         /* Intentionnaly report unhandled IDs through the SCMI return code */
>         *(u32 *)msg->out_msg = SCMI_PROTOCOL_ERROR;
>         return 0;
> +
> +not_supported:
> +       *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> +       return 0;
>  }
>
>  static int sandbox_scmi_test_remove(struct udevice *dev)
> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
> index facb5b06ffb5..0519cf889aa9 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> @@ -62,12 +62,13 @@ static int sandbox_scmi_devices_remove(struct udevice *dev)
>         if (!devices)
>                 return 0;
>
> -       for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> -               int ret2 = reset_free(devices->reset + n);
> +       if (IS_ENABLED(CONFIG_RESET_SCMI))
> +               for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> +                       int ret2 = reset_free(devices->reset + n);
>
> -               if (ret2 && !ret)
> -                       ret = ret2;
> -       }
> +                       if (ret2 && !ret)
> +                               ret = ret2;
> +               }
>
>         return ret;
>  }
> @@ -89,39 +90,53 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
>                 .regul_count = SCMI_TEST_DEVICES_VOLTD_COUNT,
>         };
>
> -       ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
> -       if (ret) {
> -               dev_err(dev, "%s: Failed on power domain\n", __func__);
> -               return ret;
> -       }
> -
> -       for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
> -               ret = clk_get_by_index(dev, n, priv->devices.clk + n);
> +       if (IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)) {
> +               ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
>                 if (ret) {
> -                       dev_err(dev, "%s: Failed on clk %zu\n", __func__, n);
> +                       dev_err(dev, "%s: Failed on power domain\n", __func__);
>                         return ret;
>                 }
>         }
>
> -       for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> -               ret = reset_get_by_index(dev, n, priv->devices.reset + n);
> -               if (ret) {
> -                       dev_err(dev, "%s: Failed on reset %zu\n", __func__, n);
> -                       goto err_reset;
> +       if (IS_ENABLED(CONFIG_CLK_SCMI)) {
> +               for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
> +                       ret = clk_get_by_index(dev, n, priv->devices.clk + n);
> +                       if (ret) {
> +                               dev_err(dev, "%s: Failed on clk %zu\n",
> +                                       __func__, n);
> +                               return ret;
> +                       }
>                 }
>         }
>
> -       for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
> -               char name[32];
> -
> -               ret = snprintf(name, sizeof(name), "regul%zu-supply", n);
> -               assert(ret >= 0 && ret < sizeof(name));
> +       if (IS_ENABLED(CONFIG_RESET_SCMI)) {
> +               for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> +                       ret = reset_get_by_index(dev, n,
> +                                                priv->devices.reset + n);
> +                       if (ret) {
> +                               dev_err(dev, "%s: Failed on reset %zu\n",
> +                                       __func__, n);
> +                               goto err_reset;
> +                       }
> +               }
> +       }
>
> -               ret = device_get_supply_regulator(dev, name,
> -                                                 priv->devices.regul + n);
> -               if (ret) {
> -                       dev_err(dev, "%s: Failed on voltd %zu\n", __func__, n);
> -                       goto err_regul;
> +       if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
> +               for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
> +                       char name[32];
> +
> +                       ret = snprintf(name, sizeof(name), "regul%zu-supply",
> +                                      n);
> +                       assert(ret >= 0 && ret < sizeof(name));
> +
> +                       ret = device_get_supply_regulator(dev, name,
> +                                                         priv->devices.regul
> +                                                               + n);
> +                       if (ret) {
> +                               dev_err(dev, "%s: Failed on voltd %zu\n",
> +                                       __func__, n);
> +                               goto err_regul;
> +                       }
>                 }
>         }
>
> @@ -130,8 +145,9 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
>  err_regul:
>         n = SCMI_TEST_DEVICES_RD_COUNT;
>  err_reset:
> -       for (; n > 0; n--)
> -               reset_free(priv->devices.reset + n - 1);
> +       if (IS_ENABLED(CONFIG_RESET_SCMI))
> +               for (; n > 0; n--)
> +                       reset_free(priv->devices.reset + n - 1);
>
>         return ret;
>  }
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 1/5] test: dm: skip scmi tests against disabled protocols

[AKASHI Takahiro]

On Mon, Nov 13, 2023 at 11:01:18AM -0700, Simon Glass wrote:
> Hi AKASHI,
> 
> On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This is a precautionary change to make scmi tests workable whether or not
> > a specific protocol be enabled.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  test/dm/scmi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > index da45314f2e4c..2f63f2da16fb 100644
> > --- a/test/dm/scmi.c
> > +++ b/test/dm/scmi.c
> > @@ -217,6 +217,9 @@ static int dm_test_scmi_power_domains(struct unit_test_state *uts)
> >         u8 *name;
> >         int ret;
> >
> > +       if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
> > +               return 0;
> 
> -EAGAIN to skip a test

Ah, I didn't notice such a common practice as it is rarely seen
under ut. Will fix.

-Takahiro Akashi

> Please update a comment if this needs to be documented better
> 
> > +
> >         /* preparation */
> >         ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
> >         ut_assertnonnull(agent);
> > @@ -317,6 +320,9 @@ static int dm_test_scmi_clocks(struct unit_test_state *uts)
> >         int ret_dev;
> >         int ret;
> >
> > +       if (!IS_ENABLED(CONFIG_CLK_SCMI))
> > +               return 0;
> > +
> >         ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
> >         if (ret)
> >                 return ret;
> > @@ -382,6 +388,9 @@ static int dm_test_scmi_resets(struct unit_test_state *uts)
> >         struct udevice *agent_dev, *reset_dev, *dev = NULL;
> >         int ret;
> >
> > +       if (!IS_ENABLED(CONFIG_RESET_SCMI))
> > +               return 0;
> > +
> >         ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
> >         if (ret)
> >                 return ret;
> > @@ -418,6 +427,9 @@ static int dm_test_scmi_voltage_domains(struct unit_test_state *uts)
> >         struct udevice *dev;
> >         struct udevice *regul0_dev;
> >
> > +       if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
> > +               return 0;
> > +
> >         ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
> >
> >         scmi_devices = sandbox_scmi_devices_ctx(dev);
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon

Re: [PATCH v2 2/5] firmware: scmi: support protocols on sandbox only if enabled

[AKASHI Takahiro]

On Mon, Nov 13, 2023 at 11:01:20AM -0700, Simon Glass wrote:
> Hi AKASHI,
> 
> On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This change will be useful when we manually test SCMI on sandbox
> > by enabling/disabling a specific SCMI protocol.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/firmware/scmi/sandbox-scmi_agent.c   | 27 ++++++-
> >  drivers/firmware/scmi/sandbox-scmi_devices.c | 78 ++++++++++++--------
> >  2 files changed, 72 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
> > index d13180962662..1fc9a0f4ea7e 100644
> > --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> > +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> > @@ -66,10 +66,18 @@ struct scmi_channel {
> >  };
> >
> >  static u8 protocols[] = {
> > +#if IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)
> >         SCMI_PROTOCOL_ID_POWER_DOMAIN,
> 
> Is this better? Perhaps not!
> 
> CONFIG_IS_ENABLED(SCMI_POWER_DOMAIN, (SCMI_PROTOCOL_ID_POWER_DOMAIN,))

Ah, good notation.

> > +#endif
> > +#if IS_ENABLED(CONFIG_CLK_SCMI)
> >         SCMI_PROTOCOL_ID_CLOCK,
> > +#endif
> > +#if IS_ENABLED(CONFIG_RESET_SCMI)
> >         SCMI_PROTOCOL_ID_RESET_DOMAIN,
> > +#endif
> > +#if IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)
> >         SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
> > +#endif
> >  };
> >
> >  #define NUM_PROTOCOLS ARRAY_SIZE(protocols)
> > @@ -1160,6 +1168,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >                 }
> >                 break;
> >         case SCMI_PROTOCOL_ID_POWER_DOMAIN:
> > +               if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
> > +                       goto not_supported;
> > +
> >                 switch (msg->message_id) {
> >                 case SCMI_PROTOCOL_VERSION:
> >                         return sandbox_scmi_pwd_protocol_version(dev, msg);
> > @@ -1180,6 +1191,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >                 }
> >                 break;
> >         case SCMI_PROTOCOL_ID_CLOCK:
> > +               if (!IS_ENABLED(CONFIG_CLK_SCMI))
> > +                       goto not_supported;
> 
> How about putting this all in a function and avoiding the goto?

Okay, will do.

Thanks,
-Takahiro Akashi

> > +
> >                 switch (msg->message_id) {
> >                 case SCMI_PROTOCOL_ATTRIBUTES:
> >                         return sandbox_scmi_clock_protocol_attribs(dev, msg);
> > @@ -1196,6 +1210,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >                 }
> >                 break;
> >         case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> > +               if (!IS_ENABLED(CONFIG_RESET_SCMI))
> > +                       goto not_supported;
> > +
> >                 switch (msg->message_id) {
> >                 case SCMI_RESET_DOMAIN_ATTRIBUTES:
> >                         return sandbox_scmi_rd_attribs(dev, msg);
> > @@ -1206,6 +1223,9 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >                 }
> >                 break;
> >         case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> > +               if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
> > +                       goto not_supported;
> > +
> >                 switch (msg->message_id) {
> >                 case SCMI_VOLTAGE_DOMAIN_ATTRIBUTES:
> >                         return sandbox_scmi_voltd_attribs(dev, msg);
> > @@ -1224,8 +1244,7 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >         case SCMI_PROTOCOL_ID_SYSTEM:
> >         case SCMI_PROTOCOL_ID_PERF:
> >         case SCMI_PROTOCOL_ID_SENSOR:
> > -               *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> > -               return 0;
> > +               goto not_supported;
> >         default:
> >                 break;
> >         }
> > @@ -1239,6 +1258,10 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >         /* Intentionnaly report unhandled IDs through the SCMI return code */
> >         *(u32 *)msg->out_msg = SCMI_PROTOCOL_ERROR;
> >         return 0;
> > +
> > +not_supported:
> > +       *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> > +       return 0;
> >  }
> >
> >  static int sandbox_scmi_test_remove(struct udevice *dev)
> > diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
> > index facb5b06ffb5..0519cf889aa9 100644
> > --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> > +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> > @@ -62,12 +62,13 @@ static int sandbox_scmi_devices_remove(struct udevice *dev)
> >         if (!devices)
> >                 return 0;
> >
> > -       for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> > -               int ret2 = reset_free(devices->reset + n);
> > +       if (IS_ENABLED(CONFIG_RESET_SCMI))
> > +               for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> > +                       int ret2 = reset_free(devices->reset + n);
> >
> > -               if (ret2 && !ret)
> > -                       ret = ret2;
> > -       }
> > +                       if (ret2 && !ret)
> > +                               ret = ret2;
> > +               }
> >
> >         return ret;
> >  }
> > @@ -89,39 +90,53 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
> >                 .regul_count = SCMI_TEST_DEVICES_VOLTD_COUNT,
> >         };
> >
> > -       ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
> > -       if (ret) {
> > -               dev_err(dev, "%s: Failed on power domain\n", __func__);
> > -               return ret;
> > -       }
> > -
> > -       for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
> > -               ret = clk_get_by_index(dev, n, priv->devices.clk + n);
> > +       if (IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)) {
> > +               ret = power_domain_get_by_index(dev, priv->devices.pwdom, 0);
> >                 if (ret) {
> > -                       dev_err(dev, "%s: Failed on clk %zu\n", __func__, n);
> > +                       dev_err(dev, "%s: Failed on power domain\n", __func__);
> >                         return ret;
> >                 }
> >         }
> >
> > -       for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> > -               ret = reset_get_by_index(dev, n, priv->devices.reset + n);
> > -               if (ret) {
> > -                       dev_err(dev, "%s: Failed on reset %zu\n", __func__, n);
> > -                       goto err_reset;
> > +       if (IS_ENABLED(CONFIG_CLK_SCMI)) {
> > +               for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
> > +                       ret = clk_get_by_index(dev, n, priv->devices.clk + n);
> > +                       if (ret) {
> > +                               dev_err(dev, "%s: Failed on clk %zu\n",
> > +                                       __func__, n);
> > +                               return ret;
> > +                       }
> >                 }
> >         }
> >
> > -       for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
> > -               char name[32];
> > -
> > -               ret = snprintf(name, sizeof(name), "regul%zu-supply", n);
> > -               assert(ret >= 0 && ret < sizeof(name));
> > +       if (IS_ENABLED(CONFIG_RESET_SCMI)) {
> > +               for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> > +                       ret = reset_get_by_index(dev, n,
> > +                                                priv->devices.reset + n);
> > +                       if (ret) {
> > +                               dev_err(dev, "%s: Failed on reset %zu\n",
> > +                                       __func__, n);
> > +                               goto err_reset;
> > +                       }
> > +               }
> > +       }
> >
> > -               ret = device_get_supply_regulator(dev, name,
> > -                                                 priv->devices.regul + n);
> > -               if (ret) {
> > -                       dev_err(dev, "%s: Failed on voltd %zu\n", __func__, n);
> > -                       goto err_regul;
> > +       if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
> > +               for (n = 0; n < SCMI_TEST_DEVICES_VOLTD_COUNT; n++) {
> > +                       char name[32];
> > +
> > +                       ret = snprintf(name, sizeof(name), "regul%zu-supply",
> > +                                      n);
> > +                       assert(ret >= 0 && ret < sizeof(name));
> > +
> > +                       ret = device_get_supply_regulator(dev, name,
> > +                                                         priv->devices.regul
> > +                                                               + n);
> > +                       if (ret) {
> > +                               dev_err(dev, "%s: Failed on voltd %zu\n",
> > +                                       __func__, n);
> > +                               goto err_regul;
> > +                       }
> >                 }
> >         }
> >
> > @@ -130,8 +145,9 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
> >  err_regul:
> >         n = SCMI_TEST_DEVICES_RD_COUNT;
> >  err_reset:
> > -       for (; n > 0; n--)
> > -               reset_free(priv->devices.reset + n - 1);
> > +       if (IS_ENABLED(CONFIG_RESET_SCMI))
> > +               for (; n > 0; n--)
> > +                       reset_free(priv->devices.reset + n - 1);
> >
> >         return ret;
> >  }
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon