[PATCH 00/10] firmware: scmi: add SCMI base protocol support

[PATCH 00/10] firmware: scmi: add SCMI base protocol support

[AKASHI Takahiro]

This patch series allows users to access SCMI base protocol provided by
SCMI server (platform). It will also be utilized in separate patches
in the future to add sanity/validity checks for other protocols.
See SCMI specification document v3.2 beta[1] for more details about SCMI
base protocol.

What is currently not implemented is
- SCMI_BASE_NOTIFY_ERRORS command and notification callback mechanism

This feature won't be very useful in the current U-Boot environment.

[1] https://developer.arm.com/documentation/den0056/e/?lang=en


Test
====
The patch series was tested on the following platforms:
* sandbox
* qemu-arm64 with OPTEE as SCMI server


Prerequisite:
=============
* This patch series is based on v2023.07-rc and relies on the following
  patch:

[2] https://lists.denx.de/pipermail/u-boot/2023-June/520083.html


Patches:
========
Patch#1-#4: Add SCMI base protocol driver
Patch#5-#7: Add SCMI base protocol device unit test
Patch#8-#10: Add scmi command


Change history:
===============
v1 (Jun, 28, 2023)
* initial release

AKASHI Takahiro (10):
  firmware: scmi: implement SCMI base protocol
  firmware: scmi: framework for installing additional protocols
  firmware: scmi: install base protocol to SCMI agent
  sandbox: remove SCMI base node definition from test.dts
  firmware: scmi: fake base protocol commands on sandbox
  test: dm: simplify SCMI unit test on sandbox
  test: dm: add SCMI base protocol test
  cmd: add scmi command for SCMI firmware
  doc: cmd: add documentation for scmi
  test: dm: add scmi command test

 arch/sandbox/dts/test.dts                  |   4 -
 arch/sandbox/include/asm/scmi_test.h       |   7 +-
 cmd/Kconfig                                |   8 +
 cmd/Makefile                               |   1 +
 cmd/scmi.c                                 | 373 +++++++++++++++
 doc/usage/cmd/scmi.rst                     |  98 ++++
 drivers/firmware/scmi/Makefile             |   1 +
 drivers/firmware/scmi/base.c               | 523 +++++++++++++++++++++
 drivers/firmware/scmi/sandbox-scmi_agent.c | 379 ++++++++++++++-
 drivers/firmware/scmi/scmi_agent-uclass.c  | 195 +++++++-
 include/dm/uclass-id.h                     |   1 +
 include/scmi_agent-uclass.h                |  84 ++++
 include/scmi_agent.h                       |  24 +
 include/scmi_protocols.h                   | 201 +++++++-
 test/dm/scmi.c                             | 233 +++++++--
 15 files changed, 2057 insertions(+), 75 deletions(-)
 create mode 100644 cmd/scmi.c
 create mode 100644 doc/usage/cmd/scmi.rst
 create mode 100644 drivers/firmware/scmi/base.c

-- 
2.41.0

[PATCH 01/10] firmware: scmi: implement SCMI base protocol

[AKASHI Takahiro]

SCMI base protocol is mandatory according to the SCMI specification.

With this patch, SCMI base protocol can be accessed via SCMI transport
layers. All the commands, except SCMI_BASE_NOTIFY_ERRORS, are supported.
This is because U-Boot doesn't support interrupts and the current transport
layers are not able to handle asynchronous messages properly.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/firmware/scmi/Makefile |   1 +
 drivers/firmware/scmi/base.c   | 517 +++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h         |   1 +
 include/scmi_protocols.h       | 201 ++++++++++++-
 4 files changed, 718 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/scmi/base.c

diff --git a/drivers/firmware/scmi/Makefile b/drivers/firmware/scmi/Makefile
index b2ff483c75a1..1a23d4981709 100644
--- a/drivers/firmware/scmi/Makefile
+++ b/drivers/firmware/scmi/Makefile
@@ -1,4 +1,5 @@
 obj-y	+= scmi_agent-uclass.o
+obj-y	+= base.o
 obj-y	+= smt.o
 obj-$(CONFIG_SCMI_AGENT_SMCCC)		+= smccc_agent.o
 obj-$(CONFIG_SCMI_AGENT_MAILBOX)	+= mailbox_agent.o
diff --git a/drivers/firmware/scmi/base.c b/drivers/firmware/scmi/base.c
new file mode 100644
index 000000000000..04018eb6ecf3
--- /dev/null
+++ b/drivers/firmware/scmi/base.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * SCMI Base protocol as U-Boot device
+ *
+ * Copyright (C) 2023 Linaro Limited
+ *		author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <scmi_agent.h>
+#include <scmi_protocols.h>
+#include <stdlib.h>
+#include <asm/types.h>
+#include <dm/device_compat.h>
+#include <linux/kernel.h>
+
+/**
+ * struct scmi_base_priv - Private data for SCMI base protocol
+ * @channel: Reference to the SCMI channel to use
+ */
+struct scmi_base_priv {
+	struct scmi_channel *channel;
+};
+
+/**
+ * scmi_protocol_version - get protocol version
+ * @dev:	Device
+ * @version:	Pointer to protocol version
+ *
+ * Obtain the protocol version number in @version.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_protocol_version(struct udevice *dev, u32 *version)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_protocol_version_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_PROTOCOL_VERSION,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	*version = out.version;
+
+	return 0;
+}
+
+/**
+ * scmi_protocol_attrs - get protocol attributes
+ * @dev:		Device
+ * @num_agents:		Number of agents
+ * @num_protocols:	Number of protocols
+ *
+ * Obtain the protocol attributes, the number of agents and the number
+ * of protocols, in @num_agents and @num_protocols respectively, that
+ * the device provides.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_protocol_attrs(struct udevice *dev, u32 *num_agents,
+			       u32 *num_protocols)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_protocol_attrs_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_PROTOCOL_ATTRIBUTES,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	*num_agents = SCMI_PROTOCOL_ATTRS_NUM_AGENTS(out.attributes);
+	*num_protocols = SCMI_PROTOCOL_ATTRS_NUM_PROTOCOLS(out.attributes);
+
+	return 0;
+}
+
+/**
+ * scmi_protocol_message_attrs - get message-specific attributes
+ * @dev:		Device
+ * @message_id:		Identifier of message
+ * @attributes:		Message-specific attributes
+ *
+ * Obtain the message-specific attributes in @attributes.
+ * This command succeeds if the message is implemented and available.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_protocol_message_attrs(struct udevice *dev, u32 message_id,
+				       u32 *attributes)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_protocol_msg_attrs_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_PROTOCOL_MESSAGE_ATTRIBUTES,
+		.in_msg = (u8 *)&message_id,
+		.in_msg_sz = sizeof(message_id),
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	*attributes = out.attributes;
+
+	return 0;
+}
+
+/**
+ * scmi_base_discover_vendor - get vendor name
+ * @dev:	Device
+ * @vendor:	Vendor name
+ *
+ * Obtain the vendor's name in @vendor.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_discover_vendor_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_DISCOVER_VENDOR,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	strcpy(vendor, out.vendor_identifier);
+
+	return 0;
+}
+
+/**
+ * scmi_base_discover_sub_vendor - get sub-vendor name
+ * @dev:	Device
+ * @sub_vendor:	Sub-vendor name
+ *
+ * Obtain the sub-vendor's name in @sub_vendor.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_discover_sub_vendor(struct udevice *dev, u8 *sub_vendor)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_discover_vendor_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_DISCOVER_SUB_VENDOR,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	strcpy(sub_vendor, out.vendor_identifier);
+
+	return 0;
+}
+
+/**
+ * scmi_base_discover_impl_version - get implementation version
+ * @dev:		Device
+ * @impl_version:	Pointer to implementation version
+ *
+ * Obtain the implementation version number in @impl_version.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_discover_impl_version(struct udevice *dev, u32 *impl_version)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_discover_impl_version_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_DISCOVER_IMPL_VERSION,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	*impl_version = out.impl_version;
+
+	return 0;
+}
+
+/**
+ * scmi_base_discover_list_protocols - get list of protocols
+ * @dev:	Device
+ * @protocols:	Pointer to array of protocols
+ *
+ * Obtain the list of protocols provided in @protocols.
+ * The number of elements in @protocols always match to the number of
+ * protocols returned by smci_protocol_attrs() when this function succeeds.
+ * It is a caller's responsibility to free @protocols.
+ *
+ * Return: the number of protocols in @protocols on success, error code otherwise
+ */
+static int scmi_base_discover_list_protocols(struct udevice *dev, u8 **protocols)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_discover_list_protocols_out out;
+	int cur;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_DISCOVER_LIST_PROTOCOLS,
+		.in_msg = (u8 *)&cur,
+		.in_msg_sz = sizeof(cur),
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	u32 num_agents, num_protocols;
+	u8 *buf;
+	int i, ret;
+
+	ret = scmi_protocol_attrs(dev, &num_agents, &num_protocols);
+	if (ret)
+		return ret;
+
+	buf = calloc(sizeof(u8), num_protocols);
+	if (!buf)
+		return -ENOMEM;
+
+	cur = 0;
+	do {
+		ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+		if (ret)
+			goto err;
+		if (out.status) {
+			ret = scmi_to_linux_errno(out.status);
+			goto err;
+		}
+
+		for (i = 0; i < out.num_protocols; i++, cur++)
+			buf[cur] = out.protocols[i / 4] >> ((i % 4) * 8);
+	} while (cur < num_protocols);
+
+	*protocols = buf;
+
+	return num_protocols;
+err:
+	free(buf);
+
+	return ret;
+}
+
+/**
+ * scmi_base_discover_agent - identify agent
+ * @dev:		Device
+ * @agent_id:		Identifier of agent
+ * @ret_agent_id:	Pointer to identifier of agent
+ * @name:		Agent name
+ *
+ * Obtain the agent's name in @name. If @agent_id is equal to 0xffffffff,
+ * this function returns the caller's agent id in @ret_agent_id.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_discover_agent(struct udevice *dev, u32 agent_id,
+				    u32 *ret_agent_id, u8 *name)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_discover_agent_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_DISCOVER_AGENT,
+		.in_msg = (u8 *)&agent_id,
+		.in_msg_sz = sizeof(agent_id),
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (out.status)
+		return scmi_to_linux_errno(out.status);
+
+	strcpy(name, out.name);
+	*ret_agent_id = out.agent_id;
+
+	return 0;
+}
+
+/**
+ * scmi_base_notify_errors - configure error notification
+ * @dev:	Device
+ * @enable:	Operation
+ *
+ * This function is not yet implemented.
+ *
+ * Return: always -EOPNOTSUPP
+ */
+static int scmi_base_notify_errors(struct udevice *dev, u32 enable)
+{
+	return -EOPNOTSUPP;
+}
+
+/**
+ * scmi_base_set_device_permissions - configure access permission to device
+ * @dev:	Device
+ * @agent_id:	Identifier of agent
+ * @device_id:	Identifier of device to access
+ * @flags:	A set of flags
+ *
+ * Ask for allowing or denying access permission to the device, @device_id.
+ * The meaning of @flags is defined in SCMI specification.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_set_device_permissions(struct udevice *dev, u32 agent_id,
+					    u32 device_id, u32 flags)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_set_device_permissions_in in = {
+		.agent_id = agent_id,
+		.device_id = device_id,
+		.flags = flags,
+	};
+	s32 status;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_SET_DEVICE_PERMISSIONS,
+		.in_msg = (u8 *)&in,
+		.in_msg_sz = sizeof(in),
+		.out_msg = (u8 *)&status,
+		.out_msg_sz = sizeof(status),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (status)
+		return scmi_to_linux_errno(status);
+
+	return 0;
+}
+
+/**
+ * scmi_base_set_protocol_permissions - configure access permission to protocol
+					on device
+ * @dev:	Device
+ * @agent_id:	Identifier of agent
+ * @device_id:	Identifier of device to access
+ * @command_id:	Identifier of command
+ * @flags:	A set of flags
+ *
+ * Ask for allowing or denying access permission to the protocol, @command_id,
+ * on the device, @device_id.
+ * The meaning of @flags is defined in SCMI specification.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_set_protocol_permissions(struct udevice *dev, u32 agent_id,
+					      u32 device_id, u32 command_id,
+					      u32 flags)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_set_protocol_permissions_in in = {
+		.agent_id = agent_id,
+		.device_id = device_id,
+		.command_id = command_id,
+		.flags = flags,
+	};
+	s32 status;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_SET_PROTOCOL_PERMISSIONS,
+		.in_msg = (u8 *)&in,
+		.in_msg_sz = sizeof(in),
+		.out_msg = (u8 *)&status,
+		.out_msg_sz = sizeof(status),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (status)
+		return scmi_to_linux_errno(status);
+
+	return 0;
+}
+
+/**
+ * scmi_base_reset_agent_configuration - reset resource settings
+ * @dev:	Device
+ * @agent_id:	Identifier of agent
+ * @flags:	A set of flags
+ *
+ * Ask for allowing or denying access permission to the protocol, @command_id,
+ * on the device, @device_id.
+ * The meaning of @flags is defined in SCMI specification.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_reset_agent_configuration(struct udevice *dev, u32 agent_id,
+					       u32 flags)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	struct scmi_base_reset_agent_configuration_in in = {
+		.agent_id = agent_id,
+		.flags = flags,
+	};
+	s32 status;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_BASE,
+		.message_id = SCMI_BASE_RESET_AGENT_CONFIGURATION,
+		.in_msg = (u8 *)&in,
+		.in_msg_sz = sizeof(in),
+		.out_msg = (u8 *)&status,
+		.out_msg_sz = sizeof(status),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, priv->channel, &msg);
+	if (ret)
+		return ret;
+	if (status)
+		return scmi_to_linux_errno(status);
+
+	return 0;
+}
+
+/**
+ * scmi_base_probe - probe base protocol device
+ * @dev:	Device
+ *
+ * Probe the device for SCMI base protocol and initialize the private data.
+ *
+ * Return: 0 on success, error code otherwise
+ */
+static int scmi_base_probe(struct udevice *dev)
+{
+	struct scmi_base_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = devm_scmi_of_get_channel(dev, &priv->channel);
+	if (ret) {
+		dev_err(dev, "get_channel failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+struct scmi_base_ops scmi_base_ops = {
+	/* Commands */
+	.protocol_version = scmi_protocol_version,
+	.protocol_attrs = scmi_protocol_attrs,
+	.protocol_message_attrs = scmi_protocol_message_attrs,
+	.base_discover_vendor = scmi_base_discover_vendor,
+	.base_discover_sub_vendor = scmi_base_discover_sub_vendor,
+	.base_discover_impl_version = scmi_base_discover_impl_version,
+	.base_discover_list_protocols = scmi_base_discover_list_protocols,
+	.base_discover_agent = scmi_base_discover_agent,
+	.base_notify_errors = scmi_base_notify_errors,
+	.base_set_device_permissions = scmi_base_set_device_permissions,
+	.base_set_protocol_permissions = scmi_base_set_protocol_permissions,
+	.base_reset_agent_configuration = scmi_base_reset_agent_configuration,
+};
+
+U_BOOT_DRIVER(scmi_base_drv) = {
+	.id = UCLASS_SCMI_BASE,
+	.name = "scmi_base_drv",
+	.ops = &scmi_base_ops,
+	.probe = scmi_base_probe,
+	.priv_auto = sizeof(struct scmi_base_priv *),
+};
+
+UCLASS_DRIVER(scmi_base) = {
+	.id		= UCLASS_SCMI_BASE,
+	.name		= "scmi_base",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 307ad6931ca7..f7a110852321 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -116,6 +116,7 @@ enum uclass_id {
 	UCLASS_RNG,		/* Random Number Generator */
 	UCLASS_RTC,		/* Real time clock device */
 	UCLASS_SCMI_AGENT,	/* Interface with an SCMI server */
+	UCLASS_SCMI_BASE,	/* Interface for SCMI Base protocol */
 	UCLASS_SCSI,		/* SCSI device */
 	UCLASS_SERIAL,		/* Serial UART */
 	UCLASS_SIMPLE_BUS,	/* Bus with child devices */
diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
index a220cb2a91ad..769041654534 100644
--- a/include/scmi_protocols.h
+++ b/include/scmi_protocols.h
@@ -15,6 +15,8 @@
  * https://developer.arm.com/docs/den0056/b
  */
 
+#define SCMI_BASE_NAME_LENGTH_MAX 16
+
 enum scmi_std_protocol {
 	SCMI_PROTOCOL_ID_BASE = 0x10,
 	SCMI_PROTOCOL_ID_POWER_DOMAIN = 0x11,
@@ -41,12 +43,207 @@ enum scmi_status_code {
 };
 
 /*
- * Generic message IDs
+ * SCMI Base Protocol
  */
-enum scmi_discovery_id {
+#define SCMI_BASE_PROTOCOL_VERSION 0x20000
+
+enum scmi_base_message_id {
 	SCMI_PROTOCOL_VERSION = 0x0,
 	SCMI_PROTOCOL_ATTRIBUTES = 0x1,
 	SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
+	SCMI_BASE_DISCOVER_VENDOR = 0x3,
+	SCMI_BASE_DISCOVER_SUB_VENDOR = 0x4,
+	SCMI_BASE_DISCOVER_IMPL_VERSION = 0x5,
+	SCMI_BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
+	SCMI_BASE_DISCOVER_AGENT = 0x7,
+	SCMI_BASE_NOTIFY_ERRORS = 0x8,
+	SCMI_BASE_SET_DEVICE_PERMISSIONS = 0x9,
+	SCMI_BASE_SET_PROTOCOL_PERMISSIONS = 0xa,
+	SCMI_BASE_RESET_AGENT_CONFIGURATION = 0xb,
+};
+
+/**
+ * struct scmi_protocol_version_out - Response for SCMI_PROTOCOL_VERSION
+ *					command
+ * @status:	SCMI command status
+ * @version:	Protocol version
+ */
+struct scmi_protocol_version_out {
+	s32 status;
+	u32 version;
+};
+
+/**
+ * struct scmi_protocol_attrs_out - Response for SCMI_PROTOCOL_ATTRIBUTES
+ *					command
+ * @status:	SCMI command status
+ * @attributes:	Protocol attributes or implementation details
+ */
+struct scmi_protocol_attrs_out {
+	s32 status;
+	u32 attributes;
+};
+
+#define SCMI_PROTOCOL_ATTRS_NUM_AGENTS(attributes) \
+				(((attributes) & GENMASK(15, 8)) >> 8)
+#define SCMI_PROTOCOL_ATTRS_NUM_PROTOCOLS(attributes) \
+				((attributes) & GENMASK(7, 0))
+
+/**
+ * struct scmi_protocol_msg_attrs_out - Response for
+ *					SCMI_PROTOCOL_MESSAGE_ATTRIBUTES command
+ * @status:	SCMI command status
+ * @attributes:	Message-specific attributes
+ */
+struct scmi_protocol_msg_attrs_out {
+	s32 status;
+	u32 attributes;
+};
+
+/**
+ * struct scmi_base_discover_vendor_out - Response for
+ *					  SCMI_BASE_DISCOVER_VENDOR or
+ *					  SCMI_BASE_DISCOVER_SUB_VENDOR command
+ * @status:		SCMI command status
+ * @vendor_identifier:	Identifier of vendor or sub-vendor in string
+ */
+struct scmi_base_discover_vendor_out {
+	s32 status;
+	u8 vendor_identifier[SCMI_BASE_NAME_LENGTH_MAX];
+};
+
+/**
+ * struct scmi_base_discover_impl_version_out - Response for
+ *					SCMI_BASE_DISCOVER_IMPL_VERSION command
+ * @status:		SCMI command status
+ * @impl_version:	Vendor-specific implementation version
+ */
+struct scmi_base_discover_impl_version_out {
+	s32 status;
+	u32 impl_version;
+};
+
+/**
+ * struct scmi_base_discover_list_protocols_out - Response for
+ *				SCMI_BASE_DISCOVER_LIST_PROTOCOLS command
+ * @status:		SCMI command status
+ * @num_protocols:	Number of protocols in @protocol
+ * @protocols:		Array of packed protocol ID's
+ */
+struct scmi_base_discover_list_protocols_out {
+	s32 status;
+	u32 num_protocols;
+	u32 protocols[3];
+};
+
+/**
+ * struct scmi_base_discover_agent_out - Response for
+ *					 SCMI_BASE_DISCOVER_AGENT command
+ * @status:	SCMI command status
+ * @agent_id:	Identifier of agent
+ * @name:	Name of agent in string
+ */
+struct scmi_base_discover_agent_out {
+	s32 status;
+	u32 agent_id;
+	u8 name[SCMI_BASE_NAME_LENGTH_MAX];
+};
+
+#define SCMI_BASE_NOTIFY_ERRORS_ENABLE BIT(0)
+
+/**
+ * struct scmi_base_set_device_permissions_in - Parameters for
+ *					SCMI_BASE_SET_DEVICE_PERMISSIONS command
+ * @agent_id:	Identifier of agent
+ * @device_id:	Identifier of device
+ * @flags:	A set of flags
+ */
+struct scmi_base_set_device_permissions_in {
+	u32 agent_id;
+	u32 device_id;
+	u32 flags;
+};
+
+#define SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS BIT(0)
+
+/**
+ * struct scmi_base_set_protocol_permissions_in - Parameters for
+ *				SCMI_BASE_SET_PROTOCOL_PERMISSIONS command
+ * @agent_id:		Identifier of agent
+ * @device_id:		Identifier of device
+ * @command_id:		Identifier of command
+ * @flags:		A set of flags
+ */
+struct scmi_base_set_protocol_permissions_in {
+	u32 agent_id;
+	u32 device_id;
+	u32 command_id;
+	u32 flags;
+};
+
+#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_COMMAND GENMASK(7, 0)
+#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS BIT(0)
+
+/**
+ * struct scmi_base_reset_agent_configuration_in - Parameters for
+ *				SCMI_BASE_RESET_AGENT_CONFIGURATION command
+ * @agent_id:	Identifier of agent
+ * @flags:	A set of flags
+ */
+struct scmi_base_reset_agent_configuration_in {
+	u32 agent_id;
+	u32 flags;
+};
+
+#define SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS BIT(0)
+
+/**
+ * struct scmi_base_ops - SCMI base protocol interfaces
+ * @protocol_version:			Function pointer for
+ *						PROTOCOL_VERSION
+ * @protocol_attrs:			Function pointer for
+ *						PROTOCOL_ATTRIBUTES
+ * @protocol_message_attrs:		Function pointer for
+ *						PROTOCOL_MESSAGE_ATTRIBUTES
+ * @base_discover_vendor:		Function pointer for
+ *						BASE_DISCOVER_VENDOR
+ * @base_discover_sub_vendor:		Function pointer for
+ *						BASE_DISCOVER_SUB_VENDOR
+ * @base_discover_impl_version:		Function pointer for
+ *						BASE_DISCOVER_IMPLEMENTATION_VERSION
+ * @base_discover_list_protocols:	Function pointer for
+ *						BASE_DISCOVER_LIST_PROTOCOLS
+ * @base_discover_agent:		Function pointer for
+ *						BASE_DISCOVER_AGENT
+ * @base_notify_errors:			Function pointer for
+ *						BASE_NOTIFY_ERRORS
+ * @base_set_device_permissions:	Function pointer for
+ *						BASE_SET_DEVICE_PROTOCOLS
+ * @base_set_protocol_permissions:	Function pointer for
+ *						BASE_SET_PROTOCOL_PERMISSIONS
+ * @base_reset_agent_configuration:	Function pointer for
+ *						BASE_RESET_AGENT_CONFIGURATION
+ */
+struct scmi_base_ops {
+	int (*protocol_version)(struct udevice *dev, u32 *version);
+	int (*protocol_attrs)(struct udevice *dev, u32 *num_agents,
+			      u32 *num_protocols);
+	int (*protocol_message_attrs)(struct udevice *dev, u32 message_id,
+				      u32 *attributes);
+	int (*base_discover_vendor)(struct udevice *dev, u8 *vendor);
+	int (*base_discover_sub_vendor)(struct udevice *dev, u8 *sub_vendor);
+	int (*base_discover_impl_version)(struct udevice *dev, u32 *impl_version);
+	int (*base_discover_list_protocols)(struct udevice *dev, u8 **protocols);
+	int (*base_discover_agent)(struct udevice *dev, u32 agent_id,
+				   u32 *ret_agent_id, u8 *name);
+	int (*base_notify_errors)(struct udevice *dev, u32 enable);
+	int (*base_set_device_permissions)(struct udevice *dev, u32 agent_id,
+					   u32 device_id, u32 flags);
+	int (*base_set_protocol_permissions)(struct udevice *dev, u32 agent_id,
+					     u32 device_id, u32 command_id,
+					     u32 flags);
+	int (*base_reset_agent_configuration)(struct udevice *dev, u32 agent_id,
+					      u32 flags);
 };
 
 /*
-- 
2.41.0

[PATCH 02/10] firmware: scmi: framework for installing additional protocols

[AKASHI Takahiro]

This framework allows SCMI protocols to be installed and bound to the agent
so that the agent can manage and utilize them later.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/firmware/scmi/scmi_agent-uclass.c | 105 +++++++++++++++++++++-
 include/scmi_agent-uclass.h               |  12 +++
 include/scmi_agent.h                      |  14 +++
 3 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c
index 02de692d66f3..6fd948ae6ddf 100644
--- a/drivers/firmware/scmi/scmi_agent-uclass.c
+++ b/drivers/firmware/scmi/scmi_agent-uclass.c
@@ -37,6 +37,86 @@ static const struct error_code scmi_linux_errmap[] = {
 	{ .scmi = SCMI_PROTOCOL_ERROR, .errno = -EPROTO, },
 };
 
+/*
+ * NOTE: The only one instance should exist according to
+ * the current specification and device tree bindings.
+ */
+struct udevice *scmi_agent;
+
+struct udevice *scmi_get_protocol(struct udevice *dev,
+				  enum scmi_std_protocol id)
+{
+	struct scmi_agent_priv *priv;
+	struct udevice *proto;
+
+	priv = dev_get_uclass_plat(dev);
+	if (!priv) {
+		dev_err(dev, "No priv data found\n");
+		return NULL;
+	}
+
+	switch (id) {
+	case SCMI_PROTOCOL_ID_CLOCK:
+		proto = priv->clock_dev;
+		break;
+	case SCMI_PROTOCOL_ID_RESET_DOMAIN:
+		proto = priv->resetdom_dev;
+		break;
+	case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
+		proto = priv->voltagedom_dev;
+		break;
+	default:
+		dev_err(dev, "Protocol not supported\n");
+		proto = NULL;
+		break;
+	}
+	if (proto && device_probe(proto))
+		dev_err(dev, "Probe failed\n");
+
+	return proto;
+}
+
+/**
+ * scmi_add_protocol - add protocol to agent
+ * @dev:	SCMI device
+ * @proto_id:	Identifier of protocol
+ * @proto:	Device of protocol instance
+ *
+ * Associate the protocol instance, @proto, to the agent, @dev,
+ * for later use.
+ *
+ * Return:	0 on success, error code otherwise
+ */
+static int scmi_add_protocol(struct udevice *dev,
+			     enum scmi_std_protocol proto_id,
+			     struct udevice *proto)
+{
+	struct scmi_agent_priv *priv;
+
+	priv = dev_get_uclass_plat(dev);
+	if (!priv) {
+		dev_err(dev, "No priv data found\n");
+		return -ENODEV;
+	}
+
+	switch (proto_id) {
+	case SCMI_PROTOCOL_ID_CLOCK:
+		priv->clock_dev = proto;
+		break;
+	case SCMI_PROTOCOL_ID_RESET_DOMAIN:
+		priv->resetdom_dev = proto;
+		break;
+	case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
+		priv->voltagedom_dev = proto;
+		break;
+	default:
+		dev_err(dev, "Protocol not supported\n");
+		return -EPROTO;
+	}
+
+	return 0;
+}
+
 int scmi_to_linux_errno(s32 scmi_code)
 {
 	int n;
@@ -61,9 +141,15 @@ static int scmi_bind_protocols(struct udevice *dev)
 	int ret = 0;
 	ofnode node;
 	const char *name;
+	struct driver *drv;
+	struct udevice *proto;
+
+	if (scmi_agent) {
+		dev_err(dev, "SCMI agent already exists: %s\n", dev->name);
+		return -EBUSY;
+	}
 
 	dev_for_each_subnode(node, dev) {
-		struct driver *drv = NULL;
 		u32 protocol_id;
 
 		if (!ofnode_is_enabled(node))
@@ -72,6 +158,7 @@ static int scmi_bind_protocols(struct udevice *dev)
 		if (ofnode_read_u32(node, "reg", &protocol_id))
 			continue;
 
+		drv = NULL;
 		name = ofnode_get_name(node);
 		switch (protocol_id) {
 		case SCMI_PROTOCOL_ID_CLOCK:
@@ -102,11 +189,22 @@ static int scmi_bind_protocols(struct udevice *dev)
 			continue;
 		}
 
-		ret = device_bind(dev, drv, name, NULL, node, NULL);
-		if (ret)
+		ret = device_bind(dev, drv, name, NULL, node, &proto);
+		if (ret) {
+			dev_err(dev, "failed to bind %s protocol\n", drv->name);
 			break;
+		}
+		ret = scmi_add_protocol(dev, protocol_id, proto);
+		if (ret) {
+			dev_err(dev, "failed to add protocol: %s, ret: %d\n",
+				proto->name, ret);
+			break;
+		}
 	}
 
+	if (!ret)
+		scmi_agent = dev;
+
 	return ret;
 }
 
@@ -168,4 +266,5 @@ UCLASS_DRIVER(scmi_agent) = {
 	.id		= UCLASS_SCMI_AGENT,
 	.name		= "scmi_agent",
 	.post_bind	= scmi_bind_protocols,
+	.per_device_plat_auto = sizeof(struct scmi_agent_priv),
 };
diff --git a/include/scmi_agent-uclass.h b/include/scmi_agent-uclass.h
index b1c93532c0ea..6c3d1321b1aa 100644
--- a/include/scmi_agent-uclass.h
+++ b/include/scmi_agent-uclass.h
@@ -9,6 +9,18 @@ struct udevice;
 struct scmi_msg;
 struct scmi_channel;
 
+/**
+ * struct scmi_agent_priv - private data maintained by agent instance
+ * @clock_dev:	SCMI clock protocol device
+ * @clock_dev:	SCMI reset domain protocol device
+ * @clock_dev:	SCMI voltage domain protocol device
+ */
+struct scmi_agent_priv {
+	struct udevice *clock_dev;
+	struct udevice *resetdom_dev;
+	struct udevice *voltagedom_dev;
+};
+
 /**
  * struct scmi_transport_ops - The functions that a SCMI transport layer must implement.
  */
diff --git a/include/scmi_agent.h b/include/scmi_agent.h
index ee6286366df7..f77c10f7abfd 100644
--- a/include/scmi_agent.h
+++ b/include/scmi_agent.h
@@ -11,6 +11,7 @@
 #define SCMI_AGENT_H
 
 #include <asm/types.h>
+#include <scmi_protocols.h>
 
 struct udevice;
 struct scmi_channel;
@@ -69,6 +70,19 @@ int devm_scmi_of_get_channel(struct udevice *dev, struct scmi_channel **channel)
 int devm_scmi_process_msg(struct udevice *dev, struct scmi_channel *channel,
 			  struct scmi_msg *msg);
 
+/**
+ * scmi_get_protocol() - get protocol instance
+ *
+ * @dev:	SCMI device
+ * @id:		Identifier of protocol
+ *
+ * Obtain the device instance for given protocol ID, @id.
+ *
+ * Return:	Pointer to the device if found, null otherwise
+ */
+struct udevice *scmi_get_protocol(struct udevice *dev,
+				  enum scmi_std_protocol id);
+
 /**
  * scmi_to_linux_errno() - Convert an SCMI error code into a Linux errno code
  *
-- 
2.41.0

[PATCH 03/10] firmware: scmi: install base protocol to SCMI agent

[AKASHI Takahiro]

SCMI base protocol is mandatory, and once SCMI node is found in a device
tree, the protocol handle (udevice) is unconditionally installed to
the agent. Then basic information will be retrieved from SCMI server via
the protocol and saved into the agent instance's local storage.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/firmware/scmi/base.c              |   6 ++
 drivers/firmware/scmi/scmi_agent-uclass.c | 104 ++++++++++++++++++++++
 include/scmi_agent-uclass.h               |  78 +++++++++++++++-
 include/scmi_agent.h                      |  10 +++
 4 files changed, 195 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/scmi/base.c b/drivers/firmware/scmi/base.c
index 04018eb6ecf3..224e0ca7a915 100644
--- a/drivers/firmware/scmi/base.c
+++ b/drivers/firmware/scmi/base.c
@@ -484,6 +484,12 @@ static int scmi_base_probe(struct udevice *dev)
 		return ret;
 	}
 
+	ret = scmi_fill_base_info(dev->parent);
+	if (ret) {
+		dev_err(dev, "get information failed\n");
+		return ret;
+	}
+
 	return ret;
 }
 
diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c
index 6fd948ae6ddf..ec15580cc947 100644
--- a/drivers/firmware/scmi/scmi_agent-uclass.c
+++ b/drivers/firmware/scmi/scmi_agent-uclass.c
@@ -56,6 +56,9 @@ struct udevice *scmi_get_protocol(struct udevice *dev,
 	}
 
 	switch (id) {
+	case SCMI_PROTOCOL_ID_BASE:
+		proto = priv->base_dev;
+		break;
 	case SCMI_PROTOCOL_ID_CLOCK:
 		proto = priv->clock_dev;
 		break;
@@ -100,6 +103,9 @@ static int scmi_add_protocol(struct udevice *dev,
 	}
 
 	switch (proto_id) {
+	case SCMI_PROTOCOL_ID_BASE:
+		priv->base_dev = proto;
+		break;
 	case SCMI_PROTOCOL_ID_CLOCK:
 		priv->clock_dev = proto;
 		break;
@@ -149,6 +155,20 @@ static int scmi_bind_protocols(struct udevice *dev)
 		return -EBUSY;
 	}
 
+	drv = DM_DRIVER_GET(scmi_base_drv);
+	name = "scmi-base.0";
+	ret = device_bind(dev, drv, name, NULL, ofnode_null(), &proto);
+	if (ret) {
+		dev_err(dev, "failed to bind base protocol\n");
+		return ret;
+	}
+	ret = scmi_add_protocol(dev, SCMI_PROTOCOL_ID_BASE, proto);
+	if (ret) {
+		dev_err(dev, "failed to add protocol: %s, ret: %d\n",
+			proto->name, ret);
+		return ret;
+	}
+
 	dev_for_each_subnode(node, dev) {
 		u32 protocol_id;
 
@@ -262,6 +282,90 @@ int devm_scmi_process_msg(struct udevice *dev, struct scmi_channel *channel,
 	return -EPROTONOSUPPORT;
 }
 
+int scmi_fill_base_info(struct udevice *dev)
+{
+	struct scmi_agent_priv *priv;
+	struct udevice *base;
+	const struct scmi_base_ops *ops;
+	int ret;
+
+	priv = dev_get_uclass_plat(dev);
+
+	base = priv->base_dev;
+	if (!base) {
+		dev_err(dev, "Base protocol not found\n");
+		return -EPROTO;
+	}
+
+	ops = dev_get_driver_ops(base);
+	if (!ops) {
+		dev_err(base, "Operations in Base protocol not found\n");
+		return -EPROTO;
+	}
+	ret = (*ops->protocol_version)(base, &priv->version);
+	if (ret) {
+		dev_err(base, "protocol_version() failed (%d)\n", ret);
+		return ret;
+	}
+	/* check for required version */
+	if (priv->version < SCMI_BASE_PROTOCOL_VERSION) {
+		dev_err(base, "base protocol version (%d) lower than expected\n",
+			priv->version);
+		return -EPROTO;
+	}
+
+	ret = (*ops->protocol_attrs)(base, &priv->num_agents,
+				     &priv->num_protocols);
+	if (ret) {
+		dev_err(base, "protocol_attrs() failed (%d)\n", ret);
+		return ret;
+	}
+	ret = (*ops->base_discover_vendor)(base, priv->vendor);
+	if (ret) {
+		dev_err(base, "base_discover_vendor() failed (%d)\n", ret);
+		return ret;
+	}
+	ret = (*ops->base_discover_sub_vendor)(base, priv->sub_vendor);
+	if (ret) {
+		if (ret != -EOPNOTSUPP) {
+			dev_err(base, "base_discover_sub_vendor() failed (%d)\n",
+				ret);
+			return ret;
+		}
+		strcpy(priv->sub_vendor, "NA");
+	}
+	ret = (*ops->base_discover_impl_version)(base,
+						 &priv->impl_version);
+	if (ret) {
+		dev_err(base, "base_discover_impl_version() failed (%d)\n",
+			ret);
+		return ret;
+	}
+
+	priv->agent_id = 0xffffffff; /* to avoid false claim */
+	ret = (*ops->base_discover_agent)(base, 0xffffffff,
+					  &priv->agent_id, priv->agent_name);
+	if (ret) {
+		if (ret != -EOPNOTSUPP) {
+			dev_err(base,
+				"base_discover_agent() failed for myself (%d)\n",
+				ret);
+			return ret;
+		}
+		priv->agent_name[0] = '\0';
+	}
+
+	ret = (*ops->base_discover_list_protocols)(base,
+						    &priv->protocols);
+	if (ret != priv->num_protocols) {
+		dev_err(base, "base_discover_list_protocols() failed (%d)\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 UCLASS_DRIVER(scmi_agent) = {
 	.id		= UCLASS_SCMI_AGENT,
 	.name		= "scmi_agent",
diff --git a/include/scmi_agent-uclass.h b/include/scmi_agent-uclass.h
index 6c3d1321b1aa..8ead08b38fb1 100644
--- a/include/scmi_agent-uclass.h
+++ b/include/scmi_agent-uclass.h
@@ -5,22 +5,94 @@
 #ifndef _SCMI_AGENT_UCLASS_H
 #define _SCMI_AGENT_UCLASS_H
 
+#include <scmi_protocols.h>
+
 struct udevice;
 struct scmi_msg;
 struct scmi_channel;
 
 /**
  * struct scmi_agent_priv - private data maintained by agent instance
- * @clock_dev:	SCMI clock protocol device
- * @clock_dev:	SCMI reset domain protocol device
- * @clock_dev:	SCMI voltage domain protocol device
+ * @version:		Version
+ * @num_agents:		Number of agents
+ * @num_protocols:	Number of protocols
+ * @impl_version:	Implementation version
+ * @protocols:		Array of protocol IDs
+ * @vendor:		Vendor name
+ * @sub_vendor:		Sub-vendor name
+ * @agent_name:		Agent name
+ * agent_id:		Identifier of agent
+ * @base_dev:		SCMI base protocol device
+ * @clock_dev:		SCMI block protocol device
+ * @clock_dev:		SCMI reset domain protocol device
+ * @clock_dev:		SCMI voltage domain protocol device
  */
 struct scmi_agent_priv {
+	u32 version;
+	u32 num_agents;
+	u32 num_protocols;
+	u32 impl_version;
+	u8 *protocols;
+	u8 vendor[SCMI_BASE_NAME_LENGTH_MAX];
+	u8 sub_vendor[SCMI_BASE_NAME_LENGTH_MAX];
+	u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX];
+	u32 agent_id;
+	struct udevice *base_dev;
 	struct udevice *clock_dev;
 	struct udevice *resetdom_dev;
 	struct udevice *voltagedom_dev;
 };
 
+static inline u32 scmi_version(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->version;
+}
+
+static inline u32 scmi_num_agents(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_agents;
+}
+
+static inline u32 scmi_num_protocols(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_protocols;
+}
+
+static inline u32 scmi_impl_version(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->impl_version;
+}
+
+static inline u8 *scmi_protocols(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->protocols;
+}
+
+static inline u8 *scmi_vendor(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->vendor;
+}
+
+static inline u8 *scmi_sub_vendor(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->sub_vendor;
+}
+
+static inline u8 *scmi_agent_name(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_name;
+}
+
+static inline u32 scmi_agent_id(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_id;
+}
+
+static inline struct udevice *scmi_base_dev(struct udevice *dev)
+{
+	return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->base_dev;
+}
+
 /**
  * struct scmi_transport_ops - The functions that a SCMI transport layer must implement.
  */
diff --git a/include/scmi_agent.h b/include/scmi_agent.h
index f77c10f7abfd..07e99f19726e 100644
--- a/include/scmi_agent.h
+++ b/include/scmi_agent.h
@@ -91,4 +91,14 @@ struct udevice *scmi_get_protocol(struct udevice *dev,
  */
 int scmi_to_linux_errno(s32 scmi_errno);
 
+/**
+ * scmi_fill_base_info - fill information about services by SCMI server
+ * @dev:        SCMI device
+ *
+ * Fill basic information about services provided by SCMI server
+ *
+ * Return:      0 on success, error code otherwise
+ */
+int scmi_fill_base_info(struct udevice *dev);
+
 #endif /* SCMI_H */
-- 
2.41.0

[PATCH 04/10] sandbox: remove SCMI base node definition from test.dts

[AKASHI Takahiro]

SCMI base protocol is mandatory and doesn't need to be listed in a device
tree.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/sandbox/dts/test.dts | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index ff9f9222e6f9..2aad94681148 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -682,10 +682,6 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			protocol@10 {
-				reg = <0x10>;
-			};
-
 			clk_scmi: protocol@14 {
 				reg = <0x14>;
 				#clock-cells = <1>;
-- 
2.41.0

[PATCH 05/10] firmware: scmi: fake base protocol commands on sandbox

[AKASHI Takahiro]

This is a simple implementation of SCMI base protocol for sandbox.
The main use is in SCMI unit test.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/firmware/scmi/sandbox-scmi_agent.c | 359 ++++++++++++++++++++-
 1 file changed, 358 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
index 031882998dfa..1f0261ea5c94 100644
--- a/drivers/firmware/scmi/sandbox-scmi_agent.c
+++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
@@ -14,11 +14,14 @@
 #include <asm/io.h>
 #include <asm/scmi_test.h>
 #include <dm/device_compat.h>
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
 
 /*
  * The sandbox SCMI agent driver simulates to some extend a SCMI message
  * processing. It simulates few of the SCMI services for some of the
  * SCMI protocols embedded in U-Boot. Currently:
+ * - SCMI base protocol
  * - SCMI clock protocol emulates an agent exposing 2 clocks
  * - SCMI reset protocol emulates an agent exposing a reset controller
  * - SCMI voltage domain protocol emulates an agent exposing 2 regulators
@@ -33,6 +36,21 @@
  * various uclass devices, as clocks and reset controllers.
  */
 
+#define SANDBOX_SCMI_BASE_PROTOCOL_VERSION SCMI_BASE_PROTOCOL_VERSION
+#define SANDBOX_SCMI_VENDOR "U-Boot"
+#define SANDBOX_SCMI_SUB_VENDOR "Sandbox"
+#define SANDBOX_SCMI_IMPL_VERSION 0x1
+#define SANDBOX_SCMI_AGENT_NAME "OSPM"
+#define SANDBOX_SCMI_PLATFORM_NAME "platform"
+
+static u8 protocols[] = {
+	SCMI_PROTOCOL_ID_CLOCK,
+	SCMI_PROTOCOL_ID_RESET_DOMAIN,
+	SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
+};
+
+#define NUM_PROTOCOLS ARRAY_SIZE(protocols)
+
 static struct sandbox_scmi_clk scmi_clk[] = {
 	{ .rate = 333 },
 	{ .rate = 200 },
@@ -114,6 +132,316 @@ static struct sandbox_scmi_voltd *get_scmi_voltd_state(uint domain_id)
  * Sandbox SCMI agent ops
  */
 
+/* Base Protocol */
+
+/**
+ * sandbox_scmi_base_protocol_version - implement SCMI_BASE_PROTOCOL_VERSION
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_PROTOCOL_VERSION command.
+ */
+static int sandbox_scmi_base_protocol_version(struct udevice *dev,
+					      struct scmi_msg *msg)
+{
+	struct scmi_protocol_version_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_protocol_version_out *)msg->out_msg;
+	out->version = SANDBOX_SCMI_BASE_PROTOCOL_VERSION;
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_protocol_attrs - implement SCMI_BASE_PROTOCOL_ATTRIBUTES
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_PROTOCOL_ATTRIBUTES command.
+ */
+static int sandbox_scmi_base_protocol_attrs(struct udevice *dev,
+					    struct scmi_msg *msg)
+{
+	struct scmi_protocol_attrs_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_protocol_attrs_out *)msg->out_msg;
+	out->attributes = FIELD_PREP(0xff00, 2) | NUM_PROTOCOLS;
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_message_attrs - implement
+ *					SCMI_BASE_PROTOCOL_MESSAGE_ATTRIBUTES
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_PROTOCOL_MESSAGE_ATTRIBUTES command.
+ */
+static int sandbox_scmi_base_message_attrs(struct udevice *dev,
+					   struct scmi_msg *msg)
+{
+	u32 message_id;
+	struct scmi_protocol_msg_attrs_out *out = NULL;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(message_id) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	message_id = *(u32 *)msg->in_msg;
+	out = (struct scmi_protocol_msg_attrs_out *)msg->out_msg;
+
+	if (message_id >= SCMI_PROTOCOL_VERSION &&
+	    message_id <= SCMI_BASE_RESET_AGENT_CONFIGURATION &&
+	    message_id != SCMI_BASE_NOTIFY_ERRORS) {
+		out->attributes = 0;
+		out->status = SCMI_SUCCESS;
+	} else {
+		out->status = SCMI_NOT_FOUND;
+	}
+
+		return 0;
+}
+
+/**
+ * sandbox_scmi_base_discover_vendor - implement SCMI_BASE_DISCOVER_VENDOR
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_DISCOVER_VENDOR command
+ */
+static int sandbox_scmi_base_discover_vendor(struct udevice *dev,
+					     struct scmi_msg *msg)
+{
+	struct scmi_base_discover_vendor_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_base_discover_vendor_out *)msg->out_msg;
+	strcpy(out->vendor_identifier, SANDBOX_SCMI_VENDOR);
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_discover_sub_vendor - implement
+ *						SCMI_BASE_DISCOVER_SUB_VENDOR
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_DISCOVER_SUB_VENDOR command
+ */
+static int sandbox_scmi_base_discover_sub_vendor(struct udevice *dev,
+						 struct scmi_msg *msg)
+{
+	struct scmi_base_discover_vendor_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_base_discover_vendor_out *)msg->out_msg;
+	strcpy(out->vendor_identifier, SANDBOX_SCMI_SUB_VENDOR);
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_discover_impl_version - implement
+ *				SCMI_BASE_DISCOVER_IMPLEMENTATION_VERSION
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_DISCOVER_IMPLEMENTATION_VERSION command
+ */
+static int sandbox_scmi_base_discover_impl_version(struct udevice *dev,
+						   struct scmi_msg *msg)
+{
+	struct scmi_base_discover_impl_version_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_base_discover_impl_version_out *)msg->out_msg;
+	out->impl_version = SANDBOX_SCMI_IMPL_VERSION;
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_discover_list_protocols - implement
+ *					SCMI_BASE_DISCOVER_LIST_PROTOCOLS
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_DISCOVER_LIST_PROTOCOLS command
+ */
+static int sandbox_scmi_base_discover_list_protocols(struct udevice *dev,
+						     struct scmi_msg *msg)
+{
+	struct scmi_base_discover_list_protocols_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_base_discover_list_protocols_out *)msg->out_msg;
+	memcpy(out->protocols, protocols, sizeof(protocols));
+	out->num_protocols = NUM_PROTOCOLS;
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_discover_agent - implement SCMI_BASE_DISCOVER_AGENT
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_DISCOVER_AGENT command
+ */
+static int sandbox_scmi_base_discover_agent(struct udevice *dev,
+					    struct scmi_msg *msg)
+{
+	u32 agent_id;
+	struct scmi_base_discover_agent_out *out = NULL;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(agent_id) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	agent_id = *(u32 *)msg->in_msg;
+	out = (struct scmi_base_discover_agent_out *)msg->out_msg;
+	out->status = SCMI_SUCCESS;
+	if (agent_id == 0xffffffff || agent_id == 1) {
+		out->agent_id = 1;
+		strcpy(out->name, SANDBOX_SCMI_AGENT_NAME);
+	} else if (!agent_id) {
+		out->agent_id = agent_id;
+		strcpy(out->name, SANDBOX_SCMI_PLATFORM_NAME);
+	} else {
+		out->status = SCMI_NOT_FOUND;
+	}
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_set_device_permissions - implement
+ *						SCMI_BASE_SET_DEVICE_PERMISSIONS
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_SET_DEVICE_PERMISSIONS command
+ */
+static int sandbox_scmi_base_set_device_permissions(struct udevice *dev,
+						    struct scmi_msg *msg)
+{
+	struct scmi_base_set_device_permissions_in *in = NULL;
+	u32 *status;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*status))
+		return -EINVAL;
+
+	in = (struct scmi_base_set_device_permissions_in *)msg->in_msg;
+	status = (u32 *)msg->out_msg;
+
+	if (in->agent_id != 1 || in->device_id != 0)
+		*status = SCMI_NOT_FOUND;
+	else if (in->flags & ~SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS)
+		*status = SCMI_INVALID_PARAMETERS;
+	else if (in->flags & SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS)
+		*status = SCMI_SUCCESS;
+	else
+		/* unset not allowed */
+		*status = SCMI_DENIED;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_set_protocol_permissions - implement
+ *					SCMI_BASE_SET_PROTOCOL_PERMISSIONS
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_SET_PROTOCOL_PERMISSIONS command
+ */
+static int sandbox_scmi_base_set_protocol_permissions(struct udevice *dev,
+						      struct scmi_msg *msg)
+{
+	struct scmi_base_set_protocol_permissions_in *in = NULL;
+	u32 *status;
+	int i;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*status))
+		return -EINVAL;
+
+	in = (struct scmi_base_set_protocol_permissions_in *)msg->in_msg;
+	status = (u32 *)msg->out_msg;
+
+	for (i = 0; i < ARRAY_SIZE(protocols); i++)
+		if (protocols[i] == in->command_id)
+			break;
+	if (in->agent_id != 1 || in->device_id != 0 ||
+	    i == ARRAY_SIZE(protocols))
+		*status = SCMI_NOT_FOUND;
+	else if (in->flags & ~SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS)
+		*status = SCMI_INVALID_PARAMETERS;
+	else if (in->flags & SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS)
+		*status = SCMI_SUCCESS;
+	else
+		/* unset not allowed */
+		*status = SCMI_DENIED;
+
+	return 0;
+}
+
+/**
+ * sandbox_scmi_base_reset_agent_configuration - implement
+ *					SCMI_BASE_RESET_AGENT_CONFIGURATION
+ * @udevice:	SCMI device
+ * @msg:	SCMI message
+ *
+ * Implement SCMI_BASE_RESET_AGENT_CONFIGURATION command
+ */
+static int sandbox_scmi_base_reset_agent_configuration(struct udevice *dev,
+						       struct scmi_msg *msg)
+{
+	struct scmi_base_reset_agent_configuration_in *in = NULL;
+	u32 *status;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*status))
+		return -EINVAL;
+
+	in = (struct scmi_base_reset_agent_configuration_in *)msg->in_msg;
+	status = (u32 *)msg->out_msg;
+
+	if (in->agent_id != 1)
+		*status = SCMI_NOT_FOUND;
+	else if (in->flags & ~SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS)
+		*status = SCMI_INVALID_PARAMETERS;
+	else
+		*status = SCMI_DENIED;
+
+	return 0;
+}
+
+/* Clock Protocol */
+
 static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev,
 					       struct scmi_msg *msg)
 {
@@ -475,6 +803,36 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 					 struct scmi_msg *msg)
 {
 	switch (msg->protocol_id) {
+	case SCMI_PROTOCOL_ID_BASE:
+		switch (msg->message_id) {
+		case SCMI_PROTOCOL_VERSION:
+			return sandbox_scmi_base_protocol_version(dev, msg);
+		case SCMI_PROTOCOL_ATTRIBUTES:
+			return sandbox_scmi_base_protocol_attrs(dev, msg);
+		case SCMI_PROTOCOL_MESSAGE_ATTRIBUTES:
+			return sandbox_scmi_base_message_attrs(dev, msg);
+		case SCMI_BASE_DISCOVER_VENDOR:
+			return sandbox_scmi_base_discover_vendor(dev, msg);
+		case SCMI_BASE_DISCOVER_SUB_VENDOR:
+			return sandbox_scmi_base_discover_sub_vendor(dev, msg);
+		case SCMI_BASE_DISCOVER_IMPL_VERSION:
+			return sandbox_scmi_base_discover_impl_version(dev, msg);
+		case SCMI_BASE_DISCOVER_LIST_PROTOCOLS:
+			return sandbox_scmi_base_discover_list_protocols(dev, msg);
+		case SCMI_BASE_DISCOVER_AGENT:
+			return sandbox_scmi_base_discover_agent(dev, msg);
+		case SCMI_BASE_NOTIFY_ERRORS:
+			break;
+		case SCMI_BASE_SET_DEVICE_PERMISSIONS:
+			return sandbox_scmi_base_set_device_permissions(dev, msg);
+		case SCMI_BASE_SET_PROTOCOL_PERMISSIONS:
+			return sandbox_scmi_base_set_protocol_permissions(dev, msg);
+		case SCMI_BASE_RESET_AGENT_CONFIGURATION:
+			return sandbox_scmi_base_reset_agent_configuration(dev, msg);
+		default:
+			break;
+		}
+		break;
 	case SCMI_PROTOCOL_ID_CLOCK:
 		switch (msg->message_id) {
 		case SCMI_PROTOCOL_ATTRIBUTES:
@@ -517,7 +875,6 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 			break;
 		}
 		break;
-	case SCMI_PROTOCOL_ID_BASE:
 	case SCMI_PROTOCOL_ID_POWER_DOMAIN:
 	case SCMI_PROTOCOL_ID_SYSTEM:
 	case SCMI_PROTOCOL_ID_PERF:
-- 
2.41.0

[PATCH 06/10] test: dm: simplify SCMI unit test on sandbox

[AKASHI Takahiro]

Adding SCMI base protocol makes it inconvenient to hold the agent instance
(udevice) locally since the agent device will be re-created per each test.
Just remove it and simply the test flows.
The test scenario is not changed at all.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/sandbox/include/asm/scmi_test.h       |  7 ++-
 drivers/firmware/scmi/sandbox-scmi_agent.c | 20 +------
 drivers/firmware/scmi/scmi_agent-uclass.c  | 14 -----
 test/dm/scmi.c                             | 64 +++++++---------------
 4 files changed, 26 insertions(+), 79 deletions(-)

diff --git a/arch/sandbox/include/asm/scmi_test.h b/arch/sandbox/include/asm/scmi_test.h
index c72ec1e1cb25..2718336a9a50 100644
--- a/arch/sandbox/include/asm/scmi_test.h
+++ b/arch/sandbox/include/asm/scmi_test.h
@@ -89,10 +89,11 @@ struct sandbox_scmi_devices {
 
 #ifdef CONFIG_SCMI_FIRMWARE
 /**
- * sandbox_scmi_service_ctx - Get the simulated SCMI services context
+ * sandbox_scmi_agent_ctx - Get the simulated SCMI agent context
+ * @dev:	Reference to the test agent
  * @return:	Reference to backend simulated resources state
  */
-struct sandbox_scmi_service *sandbox_scmi_service_ctx(void);
+struct sandbox_scmi_agent *sandbox_scmi_agent_ctx(struct udevice *dev);
 
 /**
  * sandbox_scmi_devices_ctx - Get references to devices accessed through SCMI
@@ -101,7 +102,7 @@ struct sandbox_scmi_service *sandbox_scmi_service_ctx(void);
  */
 struct sandbox_scmi_devices *sandbox_scmi_devices_ctx(struct udevice *dev);
 #else
-static inline struct sandbox_scmi_service *sandbox_scmi_service_ctx(void)
+static struct sandbox_scmi_agent *sandbox_scmi_agent_ctx(struct udevice *dev)
 {
 	return NULL;
 }
diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
index 1f0261ea5c94..ab8afb01de40 100644
--- a/drivers/firmware/scmi/sandbox-scmi_agent.c
+++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
@@ -66,11 +66,9 @@ static struct sandbox_scmi_voltd scmi_voltd[] = {
 	{ .id = 1, .voltage_uv = 1800000 },
 };
 
-static struct sandbox_scmi_service sandbox_scmi_service_state;
-
-struct sandbox_scmi_service *sandbox_scmi_service_ctx(void)
+struct sandbox_scmi_agent *sandbox_scmi_agent_ctx(struct udevice *dev)
 {
-	return &sandbox_scmi_service_state;
+	return dev_get_priv(dev);
 }
 
 static void debug_print_agent_state(struct udevice *dev, char *str)
@@ -898,16 +896,8 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
 
 static int sandbox_scmi_test_remove(struct udevice *dev)
 {
-	struct sandbox_scmi_agent *agent = dev_get_priv(dev);
-
-	if (agent != sandbox_scmi_service_ctx()->agent)
-		return -EINVAL;
-
 	debug_print_agent_state(dev, "removed");
 
-	/* We only need to dereference the agent in the context */
-	sandbox_scmi_service_ctx()->agent = NULL;
-
 	return 0;
 }
 
@@ -915,9 +905,6 @@ static int sandbox_scmi_test_probe(struct udevice *dev)
 {
 	struct sandbox_scmi_agent *agent = dev_get_priv(dev);
 
-	if (sandbox_scmi_service_ctx()->agent)
-		return -EINVAL;
-
 	*agent = (struct sandbox_scmi_agent){
 		.clk = scmi_clk,
 		.clk_count = ARRAY_SIZE(scmi_clk),
@@ -929,9 +916,6 @@ static int sandbox_scmi_test_probe(struct udevice *dev)
 
 	debug_print_agent_state(dev, "probed");
 
-	/* Save reference for tests purpose */
-	sandbox_scmi_service_ctx()->agent = agent;
-
 	return 0;
 };
 
diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c
index ec15580cc947..4526f75486ed 100644
--- a/drivers/firmware/scmi/scmi_agent-uclass.c
+++ b/drivers/firmware/scmi/scmi_agent-uclass.c
@@ -37,12 +37,6 @@ static const struct error_code scmi_linux_errmap[] = {
 	{ .scmi = SCMI_PROTOCOL_ERROR, .errno = -EPROTO, },
 };
 
-/*
- * NOTE: The only one instance should exist according to
- * the current specification and device tree bindings.
- */
-struct udevice *scmi_agent;
-
 struct udevice *scmi_get_protocol(struct udevice *dev,
 				  enum scmi_std_protocol id)
 {
@@ -150,11 +144,6 @@ static int scmi_bind_protocols(struct udevice *dev)
 	struct driver *drv;
 	struct udevice *proto;
 
-	if (scmi_agent) {
-		dev_err(dev, "SCMI agent already exists: %s\n", dev->name);
-		return -EBUSY;
-	}
-
 	drv = DM_DRIVER_GET(scmi_base_drv);
 	name = "scmi-base.0";
 	ret = device_bind(dev, drv, name, NULL, ofnode_null(), &proto);
@@ -222,9 +211,6 @@ static int scmi_bind_protocols(struct udevice *dev)
 		}
 	}
 
-	if (!ret)
-		scmi_agent = dev;
-
 	return ret;
 }
 
diff --git a/test/dm/scmi.c b/test/dm/scmi.c
index d87e2731ce42..881be3171b7c 100644
--- a/test/dm/scmi.c
+++ b/test/dm/scmi.c
@@ -23,22 +23,11 @@
 #include <power/regulator.h>
 #include <test/ut.h>
 
-static int ut_assert_scmi_state_preprobe(struct unit_test_state *uts)
-{
-	struct sandbox_scmi_service *scmi_ctx = sandbox_scmi_service_ctx();
-
-	ut_assertnonnull(scmi_ctx);
-	ut_assertnull(scmi_ctx->agent);
-
-	return 0;
-}
-
 static int ut_assert_scmi_state_postprobe(struct unit_test_state *uts,
+					  struct sandbox_scmi_agent *agent,
 					  struct udevice *dev)
 {
 	struct sandbox_scmi_devices *scmi_devices;
-	struct sandbox_scmi_service *scmi_ctx;
-	struct sandbox_scmi_agent *agent;
 
 	/* Device references to check context against test sequence */
 	scmi_devices = sandbox_scmi_devices_ctx(dev);
@@ -48,10 +37,6 @@ static int ut_assert_scmi_state_postprobe(struct unit_test_state *uts,
 	ut_asserteq(2, scmi_devices->regul_count);
 
 	/* State of the simulated SCMI server exposed */
-	scmi_ctx = sandbox_scmi_service_ctx();
-	ut_assertnonnull(scmi_ctx);
-	agent = scmi_ctx->agent;
-	ut_assertnonnull(agent);
 	ut_asserteq(3, agent->clk_count);
 	ut_assertnonnull(agent->clk);
 	ut_asserteq(1, agent->reset_count);
@@ -63,27 +48,32 @@ static int ut_assert_scmi_state_postprobe(struct unit_test_state *uts,
 }
 
 static int load_sandbox_scmi_test_devices(struct unit_test_state *uts,
+					  struct sandbox_scmi_agent **ctx,
 					  struct udevice **dev)
 {
-	int ret;
+	struct udevice *agent_dev;
 
-	ret = ut_assert_scmi_state_preprobe(uts);
-	if (ret)
-		return ret;
+	ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
+					      &agent_dev));
+	ut_assertnonnull(agent_dev);
+
+	*ctx = sandbox_scmi_agent_ctx(agent_dev);
+	ut_assertnonnull(*ctx);
 
+	/* probe */
 	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "sandbox_scmi",
 					      dev));
 	ut_assertnonnull(*dev);
 
-	return ut_assert_scmi_state_postprobe(uts, *dev);
+	return ut_assert_scmi_state_postprobe(uts, *ctx, *dev);
 }
 
 static int release_sandbox_scmi_test_devices(struct unit_test_state *uts,
 					     struct udevice *dev)
 {
+	/* un-probe */
 	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 
-	/* Not sure test devices are fully removed, agent may not be visible */
 	return 0;
 }
 
@@ -93,10 +83,11 @@ static int release_sandbox_scmi_test_devices(struct unit_test_state *uts,
  */
 static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
 {
+	struct sandbox_scmi_agent *ctx;
 	struct udevice *dev = NULL;
 	int ret;
 
-	ret = load_sandbox_scmi_test_devices(uts, &dev);
+	ret = load_sandbox_scmi_test_devices(uts, &ctx, &dev);
 	if (!ret)
 		ret = release_sandbox_scmi_test_devices(uts, dev);
 
@@ -106,23 +97,18 @@ DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
 
 static int dm_test_scmi_clocks(struct unit_test_state *uts)
 {
-	struct sandbox_scmi_devices *scmi_devices;
-	struct sandbox_scmi_service *scmi_ctx;
 	struct sandbox_scmi_agent *agent;
+	struct sandbox_scmi_devices *scmi_devices;
 	struct udevice *dev;
 	int ret_dev;
 	int ret;
 
-	ret = load_sandbox_scmi_test_devices(uts, &dev);
+	ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
 	if (ret)
 		return ret;
 
 	scmi_devices = sandbox_scmi_devices_ctx(dev);
 	ut_assertnonnull(scmi_devices);
-	scmi_ctx = sandbox_scmi_service_ctx();
-	ut_assertnonnull(scmi_ctx);
-	agent = scmi_ctx->agent;
-	ut_assertnonnull(agent);
 
 	/* Test SCMI clocks rate manipulation */
 	ut_asserteq(333, agent->clk[0].rate);
@@ -169,22 +155,17 @@ DM_TEST(dm_test_scmi_clocks, UT_TESTF_SCAN_FDT);
 
 static int dm_test_scmi_resets(struct unit_test_state *uts)
 {
-	struct sandbox_scmi_devices *scmi_devices;
-	struct sandbox_scmi_service *scmi_ctx;
 	struct sandbox_scmi_agent *agent;
+	struct sandbox_scmi_devices *scmi_devices;
 	struct udevice *dev = NULL;
 	int ret;
 
-	ret = load_sandbox_scmi_test_devices(uts, &dev);
+	ret = load_sandbox_scmi_test_devices(uts, &agent, &dev);
 	if (ret)
 		return ret;
 
 	scmi_devices = sandbox_scmi_devices_ctx(dev);
 	ut_assertnonnull(scmi_devices);
-	scmi_ctx = sandbox_scmi_service_ctx();
-	ut_assertnonnull(scmi_ctx);
-	agent = scmi_ctx->agent;
-	ut_assertnonnull(agent);
 
 	/* Test SCMI resect controller manipulation */
 	ut_assert(!agent->reset[0].asserted);
@@ -201,21 +182,16 @@ DM_TEST(dm_test_scmi_resets, UT_TESTF_SCAN_FDT);
 
 static int dm_test_scmi_voltage_domains(struct unit_test_state *uts)
 {
-	struct sandbox_scmi_devices *scmi_devices;
-	struct sandbox_scmi_service *scmi_ctx;
 	struct sandbox_scmi_agent *agent;
+	struct sandbox_scmi_devices *scmi_devices;
 	struct dm_regulator_uclass_plat *uc_pdata;
 	struct udevice *dev;
 	struct udevice *regul0_dev;
 
-	ut_assertok(load_sandbox_scmi_test_devices(uts, &dev));
+	ut_assertok(load_sandbox_scmi_test_devices(uts, &agent, &dev));
 
 	scmi_devices = sandbox_scmi_devices_ctx(dev);
 	ut_assertnonnull(scmi_devices);
-	scmi_ctx = sandbox_scmi_service_ctx();
-	ut_assertnonnull(scmi_ctx);
-	agent = scmi_ctx->agent;
-	ut_assertnonnull(agent);
 
 	/* Set/Get an SCMI voltage domain level */
 	regul0_dev = scmi_devices->regul[0];
-- 
2.41.0

[PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

Added is a new unit test for SCMI base protocol, which will exercise all
the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
  $ ut dm scmi_base
It is assumed that test.dtb is used as sandbox's device tree.

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

diff --git a/test/dm/scmi.c b/test/dm/scmi.c
index 881be3171b7c..563017bb63e0 100644
--- a/test/dm/scmi.c
+++ b/test/dm/scmi.c
@@ -16,6 +16,9 @@
 #include <clk.h>
 #include <dm.h>
 #include <reset.h>
+#include <scmi_agent.h>
+#include <scmi_agent-uclass.h>
+#include <scmi_protocols.h>
 #include <asm/scmi_test.h>
 #include <dm/device-internal.h>
 #include <dm/test.h>
@@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
 
+static int dm_test_scmi_base(struct unit_test_state *uts)
+{
+	struct udevice *agent_dev, *base;
+	struct scmi_agent_priv *priv;
+	const struct scmi_base_ops *ops;
+	u32 version, num_agents, num_protocols, impl_version;
+	u32 attributes, agent_id;
+	char vendor[SCMI_BASE_NAME_LENGTH_MAX],
+	     agent_name[SCMI_BASE_NAME_LENGTH_MAX];
+	u8 *protocols;
+	int ret;
+
+	/* preparation */
+	ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
+					      &agent_dev));
+	ut_assertnonnull(agent_dev);
+	ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
+	ut_assertnonnull(base = scmi_get_protocol(agent_dev,
+						  SCMI_PROTOCOL_ID_BASE));
+	ut_assertnonnull(ops = dev_get_driver_ops(base));
+
+	/* version */
+	ret = (*ops->protocol_version)(base, &version);
+	ut_assertok(ret);
+	ut_asserteq(priv->version, version);
+
+	/* protocol attributes */
+	ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols);
+	ut_assertok(ret);
+	ut_asserteq(priv->num_agents, num_agents);
+	ut_asserteq(priv->num_protocols, num_protocols);
+
+	/* discover vendor */
+	ret = (*ops->base_discover_vendor)(base, vendor);
+	ut_assertok(ret);
+	ut_asserteq_str(priv->vendor, vendor);
+
+	/* message attributes */
+	ret = (*ops->protocol_message_attrs)(base,
+					     SCMI_BASE_DISCOVER_SUB_VENDOR,
+					     &attributes);
+	ut_assertok(ret);
+	ut_assertok(attributes);
+
+	/* discover sub vendor */
+	ret = (*ops->base_discover_sub_vendor)(base, vendor);
+	ut_assertok(ret);
+	ut_asserteq_str(priv->sub_vendor, vendor);
+
+	/* impl version */
+	ret = (*ops->base_discover_impl_version)(base, &impl_version);
+	ut_assertok(ret);
+	ut_asserteq(priv->impl_version, impl_version);
+
+	/* discover agent (my self) */
+	ret = (*ops->base_discover_agent)(base, 0xffffffff,
+					  &agent_id, agent_name);
+	ut_assertok(ret);
+	ut_asserteq(priv->agent_id, agent_id);
+	ut_asserteq_str(priv->agent_name, agent_name);
+
+	/* discover protocols */
+	ret = (*ops->base_discover_list_protocols)(base, &protocols);
+	ut_asserteq(num_protocols, ret);
+	ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols);
+	free(protocols);
+
+	/*
+	 * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting
+	 * access and protocol permissions, but doesn't allow unsetting them nor
+	 * resetting the configurations.
+	 */
+	/* set device permissions */
+	ret = (*ops->base_set_device_permissions)(base, agent_id, 0,
+						  SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
+	ut_assertok(ret); /* SCMI_SUCCESS */
+	ret = (*ops->base_set_device_permissions)(base, agent_id, 1,
+						  SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
+	ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
+	ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0);
+	ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
+
+	/* set protocol permissions */
+	ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
+						    SCMI_PROTOCOL_ID_CLOCK,
+						    SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
+	ut_assertok(ret); /* SCMI_SUCCESS */
+	ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1,
+						    SCMI_PROTOCOL_ID_CLOCK,
+						    SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
+	ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
+	ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
+						    SCMI_PROTOCOL_ID_CLOCK, 0);
+	ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
+
+	/* reset agent configuration */
+	ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
+	ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
+	ret = (*ops->base_reset_agent_configuration)(base, agent_id,
+						     SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS);
+	ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
+	ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
+	ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
+
+	return 0;
+}
+
+DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT);
+
 static int dm_test_scmi_clocks(struct unit_test_state *uts)
 {
 	struct sandbox_scmi_agent *agent;
-- 
2.41.0

[PATCH 08/10] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

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

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Kconfig  |   8 ++
 cmd/Makefile |   1 +
 cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 382 insertions(+)
 create mode 100644 cmd/scmi.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 02e54f1e50fe..065470a76295 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2504,6 +2504,14 @@ 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 protocols,
+	  including Base protocol.
 endmenu
 
 menu "Filesystem commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 6c37521b4e2b..826e0b74b587 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -156,6 +156,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..c97f77e97d95
--- /dev/null
+++ b/cmd/scmi.c
@@ -0,0 +1,373 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  SCMI utility command
+ *
+ *  Copyright (c) 2023 Linaro Limited
+ *		Author: AKASHI Takahiro
+ */
+
+#include <common.h>
+#include <command.h>
+#include <dm/device.h>
+#include <dm/uclass.h> /* uclass_get_device */
+#include <exports.h>
+#include <scmi_agent.h>
+#include <scmi_agent-uclass.h>
+#include <asm/types.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+
+static struct udevice *agent;
+static struct udevice *base_proto;
+static const struct scmi_base_ops *ops;
+
+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"},
+};
+
+/**
+ * scmi_convert_id_to_string() - get the name of SCMI protocol
+ *
+ * @id:		Protocol ID
+ *
+ * Get the printable name of the protocol, @id
+ *
+ * Return:	Name string on success, NULL on failure
+ */
+static const char *scmi_convert_id_to_string(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_base_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_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
+			     char * const argv[])
+{
+	u32 agent_id, num_protocols;
+	u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
+	int i, ret;
+
+	if (argc != 1)
+		return CMD_RET_USAGE;
+
+	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 = (*ops->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);
+	}
+	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",
+			       scmi_convert_id_to_string(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_base_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_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
+				char * const argv[])
+{
+	u32 agent_id, device_id, flags, attributes;
+	char *end;
+	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;
+
+	ret = (*ops->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 = (*ops->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_base_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_base_set_proto(struct cmd_tbl *cmdtp, int flag, int argc,
+				  char * const argv[])
+{
+	u32 agent_id, device_id, command_id, flags, attributes;
+	char *end;
+	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;
+
+	command_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;
+
+	ret = (*ops->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 = (*ops->base_set_protocol_permissions)(base_proto, agent_id,
+						    device_id, command_id,
+						    flags);
+	if (ret) {
+		printf("%s access to protocol:0x%x on device:%u failed (%d)\n",
+		       flags ? "Allowing" : "Denying", command_id, device_id,
+		       ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_base_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_base_reset(struct cmd_tbl *cmdtp, int flag, int argc,
+			      char * const argv[])
+{
+	u32 agent_id, flags, attributes;
+	char *end;
+	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;
+
+	ret = (*ops->protocol_message_attrs)(base_proto,
+					     SCMI_BASE_RESET_AGENT_CONFIGURATION,
+					     &attributes);
+	if (ret) {
+		printf("Reset is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = (*ops->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_base_sub[] = {
+	U_BOOT_CMD_MKENT(info, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_info, "", ""),
+	U_BOOT_CMD_MKENT(perm_dev, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_set_dev, "", ""),
+	U_BOOT_CMD_MKENT(perm_proto, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_set_proto, "", ""),
+	U_BOOT_CMD_MKENT(reset, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_reset, "", ""),
+};
+
+/**
+ * do_scmi_base() - handle SCMI base protocol
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Provide user interfaces to SCMI base protocol.
+ *
+ * Return:	CMD_RET_SUCCESS on success,
+ *		CMD_RET_USAGE or CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base(struct cmd_tbl *cmdtp, int flag,
+			int argc, char *const argv[])
+{
+	struct cmd_tbl *cp;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	if (!base_proto)
+		base_proto = scmi_get_protocol(agent, SCMI_PROTOCOL_ID_BASE);
+	if (!base_proto) {
+		printf("SCMI base protocol not found\n");
+		return CMD_RET_FAILURE;
+	}
+	ops = dev_get_driver_ops(base_proto);
+
+	cp = find_cmd_tbl(argv[0], cmd_scmi_base_sub,
+			  ARRAY_SIZE(cmd_scmi_base_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+static struct cmd_tbl cmd_scmi_sub[] = {
+	U_BOOT_CMD_MKENT(base, CONFIG_SYS_MAXARGS, 1, do_scmi_base, "", ""),
+};
+
+/**
+ * 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++;
+
+	if (!agent) {
+		if (uclass_get_device(UCLASS_SCMI_AGENT, 0, &agent)) {
+			printf("Cannot find any SCMI agent\n");
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	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"
+	" base info - get the info of SCMI services\n"
+	" base perm_dev <agent id> <device id> <flags>\n"
+	"   - set access permission to device\n"
+	" base perm_proto <agent id> <device id> <command id> <flags>\n"
+	"   - set protocol permission to device\n"
+	" base reset <agent id> <flags>\n"
+	"   - reset platform resource settings\n"
+	"";
+
+U_BOOT_CMD(scmi, CONFIG_SYS_MAXARGS, 0, do_scmi, "SCMI utility",
+	   scmi_help_text);
-- 
2.41.0

[PATCH 09/10] 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>
---
 doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 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..20cdae4b877d
--- /dev/null
+++ b/doc/usage/cmd/scmi.rst
@@ -0,0 +1,98 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+scmi command
+============
+
+Synopsis
+--------
+
+::
+
+    scmi base info
+    scmi base perm_dev <agent id> <device id> <flags>
+    scmi base perm_proto <agent id> <device id> <command id> <flags>
+    scmi base reset <agent id> <flags>
+
+Description
+-----------
+
+The scmi command is used to access and operate on SCMI server.
+
+scmi base info
+~~~~~~~~~~~~~~
+    Show base information about SCMI server and supported protocols
+
+scmi base perm_dev
+~~~~~~~~~~~~~~~~~~
+    Allow or deny access permission to the device
+
+scmi base perm_proto
+~~~~~~~~~~~~~~~~~~~~
+    Allow or deny access to the protocol on the device
+
+scmi base reset
+~~~~~~~~~~~~~~~
+    Reset the existing configurations
+
+Parameters are used as follows:
+
+<agent id>
+    Agent ID
+
+<device id>
+    Device ID
+
+<command id>
+    Protocol ID, should not be 0x10 (base protocol)
+
+<flags>
+    Flags to control the action. See SCMI specification for
+    defined values.
+
+Example
+-------
+
+Obtain basic information about SCMI server:
+
+::
+
+    => scmi base 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 base perm_dev 1 0 1
+
+Reset configurations with all access permission settings retained:
+
+::
+
+    => scmi base 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.
-- 
2.41.0

[PATCH 10/10] test: dm: add scmi command test

[AKASHI Takahiro]

In this test, "scmi" with different sub-commands is tested.
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>
---
 test/dm/scmi.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/test/dm/scmi.c b/test/dm/scmi.c
index 563017bb63e0..b40c84011295 100644
--- a/test/dm/scmi.c
+++ b/test/dm/scmi.c
@@ -207,6 +207,63 @@ 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, *base;
+	struct scmi_agent_priv *priv;
+
+	/* preparation */
+	ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
+					      &agent_dev));
+	ut_assertnonnull(agent_dev);
+	ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
+	ut_assertnonnull(base = scmi_get_protocol(agent_dev,
+						  SCMI_PROTOCOL_ID_BASE));
+
+	/* scmi base info */
+	ut_assertok(run_command("scmi base info", 0));
+
+	ut_assert_nextline("SCMI device: scmi");
+	ut_assert_nextline("  protocol version: 0x20000");
+	ut_assert_nextline("  # of agents: 2");
+	ut_assert_nextline("      0: platform");
+	ut_assert_nextline("    > 1: OSPM");
+	ut_assert_nextline("  # of protocols: 3");
+	ut_assert_nextline("      Clock management");
+	ut_assert_nextline("      Reset domain management");
+	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 base perm_dev */
+	ut_assertok(run_command("scmi base perm_dev 1 0 1", 0));
+	ut_assert_console_end();
+
+	ut_assert(run_command("scmi base perm_dev 1 0 0", 0));
+	ut_assert_nextline("Denying access to device:0 failed (-13)");
+	ut_assert_console_end();
+
+	/* scmi base perm_proto */
+	ut_assertok(run_command("scmi base perm_proto 1 0 14 1", 0));
+	ut_assert_console_end();
+
+	ut_assert(run_command("scmi base 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 base reset */
+	ut_assert(run_command("scmi base 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_clocks(struct unit_test_state *uts)
 {
 	struct sandbox_scmi_agent *agent;
-- 
2.41.0

Re: [PATCH 01/10] firmware: scmi: implement SCMI base protocol

[Simon Glass]

Hi AKASHI,

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> SCMI base protocol is mandatory according to the SCMI specification.
>
> With this patch, SCMI base protocol can be accessed via SCMI transport
> layers. All the commands, except SCMI_BASE_NOTIFY_ERRORS, are supported.
> This is because U-Boot doesn't support interrupts and the current transport
> layers are not able to handle asynchronous messages properly.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/firmware/scmi/Makefile |   1 +
>  drivers/firmware/scmi/base.c   | 517 +++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h         |   1 +
>  include/scmi_protocols.h       | 201 ++++++++++++-
>  4 files changed, 718 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/firmware/scmi/base.c
>
> diff --git a/drivers/firmware/scmi/Makefile b/drivers/firmware/scmi/Makefile
> index b2ff483c75a1..1a23d4981709 100644
> --- a/drivers/firmware/scmi/Makefile
> +++ b/drivers/firmware/scmi/Makefile
> @@ -1,4 +1,5 @@
>  obj-y  += scmi_agent-uclass.o
> +obj-y  += base.o
>  obj-y  += smt.o
>  obj-$(CONFIG_SCMI_AGENT_SMCCC)         += smccc_agent.o
>  obj-$(CONFIG_SCMI_AGENT_MAILBOX)       += mailbox_agent.o

[..]

> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 307ad6931ca7..f7a110852321 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -116,6 +116,7 @@ enum uclass_id {
>         UCLASS_RNG,             /* Random Number Generator */
>         UCLASS_RTC,             /* Real time clock device */
>         UCLASS_SCMI_AGENT,      /* Interface with an SCMI server */
> +       UCLASS_SCMI_BASE,       /* Interface for SCMI Base protocol */
>         UCLASS_SCSI,            /* SCSI device */
>         UCLASS_SERIAL,          /* Serial UART */
>         UCLASS_SIMPLE_BUS,      /* Bus with child devices */
> diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> index a220cb2a91ad..769041654534 100644
> --- a/include/scmi_protocols.h
> +++ b/include/scmi_protocols.h
> @@ -15,6 +15,8 @@
>   * https://developer.arm.com/docs/den0056/b
>   */
>
> +#define SCMI_BASE_NAME_LENGTH_MAX 16
> +
>  enum scmi_std_protocol {
>         SCMI_PROTOCOL_ID_BASE = 0x10,
>         SCMI_PROTOCOL_ID_POWER_DOMAIN = 0x11,
> @@ -41,12 +43,207 @@ enum scmi_status_code {
>  };
>
>  /*
> - * Generic message IDs
> + * SCMI Base Protocol
>   */
> -enum scmi_discovery_id {
> +#define SCMI_BASE_PROTOCOL_VERSION 0x20000
> +
> +enum scmi_base_message_id {
>         SCMI_PROTOCOL_VERSION = 0x0,
>         SCMI_PROTOCOL_ATTRIBUTES = 0x1,
>         SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> +       SCMI_BASE_DISCOVER_VENDOR = 0x3,
> +       SCMI_BASE_DISCOVER_SUB_VENDOR = 0x4,
> +       SCMI_BASE_DISCOVER_IMPL_VERSION = 0x5,
> +       SCMI_BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
> +       SCMI_BASE_DISCOVER_AGENT = 0x7,
> +       SCMI_BASE_NOTIFY_ERRORS = 0x8,
> +       SCMI_BASE_SET_DEVICE_PERMISSIONS = 0x9,
> +       SCMI_BASE_SET_PROTOCOL_PERMISSIONS = 0xa,
> +       SCMI_BASE_RESET_AGENT_CONFIGURATION = 0xb,
> +};
> +
> +/**
> + * struct scmi_protocol_version_out - Response for SCMI_PROTOCOL_VERSION
> + *                                     command
> + * @status:    SCMI command status
> + * @version:   Protocol version
> + */
> +struct scmi_protocol_version_out {
> +       s32 status;
> +       u32 version;
> +};
> +
> +/**
> + * struct scmi_protocol_attrs_out - Response for SCMI_PROTOCOL_ATTRIBUTES
> + *                                     command
> + * @status:    SCMI command status
> + * @attributes:        Protocol attributes or implementation details
> + */
> +struct scmi_protocol_attrs_out {
> +       s32 status;
> +       u32 attributes;
> +};
> +
> +#define SCMI_PROTOCOL_ATTRS_NUM_AGENTS(attributes) \
> +                               (((attributes) & GENMASK(15, 8)) >> 8)
> +#define SCMI_PROTOCOL_ATTRS_NUM_PROTOCOLS(attributes) \
> +                               ((attributes) & GENMASK(7, 0))
> +
> +/**
> + * struct scmi_protocol_msg_attrs_out - Response for
> + *                                     SCMI_PROTOCOL_MESSAGE_ATTRIBUTES command
> + * @status:    SCMI command status
> + * @attributes:        Message-specific attributes
> + */
> +struct scmi_protocol_msg_attrs_out {
> +       s32 status;
> +       u32 attributes;
> +};
> +
> +/**
> + * struct scmi_base_discover_vendor_out - Response for
> + *                                       SCMI_BASE_DISCOVER_VENDOR or
> + *                                       SCMI_BASE_DISCOVER_SUB_VENDOR command
> + * @status:            SCMI command status
> + * @vendor_identifier: Identifier of vendor or sub-vendor in string
> + */
> +struct scmi_base_discover_vendor_out {
> +       s32 status;
> +       u8 vendor_identifier[SCMI_BASE_NAME_LENGTH_MAX];
> +};
> +
> +/**
> + * struct scmi_base_discover_impl_version_out - Response for
> + *                                     SCMI_BASE_DISCOVER_IMPL_VERSION command
> + * @status:            SCMI command status
> + * @impl_version:      Vendor-specific implementation version
> + */
> +struct scmi_base_discover_impl_version_out {
> +       s32 status;
> +       u32 impl_version;
> +};
> +
> +/**
> + * struct scmi_base_discover_list_protocols_out - Response for
> + *                             SCMI_BASE_DISCOVER_LIST_PROTOCOLS command
> + * @status:            SCMI command status
> + * @num_protocols:     Number of protocols in @protocol
> + * @protocols:         Array of packed protocol ID's
> + */
> +struct scmi_base_discover_list_protocols_out {
> +       s32 status;
> +       u32 num_protocols;
> +       u32 protocols[3];
> +};
> +
> +/**
> + * struct scmi_base_discover_agent_out - Response for
> + *                                      SCMI_BASE_DISCOVER_AGENT command
> + * @status:    SCMI command status
> + * @agent_id:  Identifier of agent
> + * @name:      Name of agent in string
> + */
> +struct scmi_base_discover_agent_out {
> +       s32 status;
> +       u32 agent_id;
> +       u8 name[SCMI_BASE_NAME_LENGTH_MAX];
> +};
> +
> +#define SCMI_BASE_NOTIFY_ERRORS_ENABLE BIT(0)
> +
> +/**
> + * struct scmi_base_set_device_permissions_in - Parameters for
> + *                                     SCMI_BASE_SET_DEVICE_PERMISSIONS command
> + * @agent_id:  Identifier of agent
> + * @device_id: Identifier of device
> + * @flags:     A set of flags
> + */
> +struct scmi_base_set_device_permissions_in {
> +       u32 agent_id;
> +       u32 device_id;
> +       u32 flags;
> +};
> +
> +#define SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS BIT(0)
> +
> +/**
> + * struct scmi_base_set_protocol_permissions_in - Parameters for
> + *                             SCMI_BASE_SET_PROTOCOL_PERMISSIONS command
> + * @agent_id:          Identifier of agent
> + * @device_id:         Identifier of device
> + * @command_id:                Identifier of command
> + * @flags:             A set of flags
> + */
> +struct scmi_base_set_protocol_permissions_in {
> +       u32 agent_id;
> +       u32 device_id;
> +       u32 command_id;
> +       u32 flags;
> +};
> +
> +#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_COMMAND GENMASK(7, 0)
> +#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS BIT(0)
> +
> +/**
> + * struct scmi_base_reset_agent_configuration_in - Parameters for
> + *                             SCMI_BASE_RESET_AGENT_CONFIGURATION command
> + * @agent_id:  Identifier of agent
> + * @flags:     A set of flags
> + */
> +struct scmi_base_reset_agent_configuration_in {
> +       u32 agent_id;
> +       u32 flags;
> +};
> +
> +#define SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS BIT(0)
> +
> +/**
> + * struct scmi_base_ops - SCMI base protocol interfaces
> + * @protocol_version:                  Function pointer for
> + *                                             PROTOCOL_VERSION
> + * @protocol_attrs:                    Function pointer for
> + *                                             PROTOCOL_ATTRIBUTES
> + * @protocol_message_attrs:            Function pointer for
> + *                                             PROTOCOL_MESSAGE_ATTRIBUTES
> + * @base_discover_vendor:              Function pointer for
> + *                                             BASE_DISCOVER_VENDOR
> + * @base_discover_sub_vendor:          Function pointer for
> + *                                             BASE_DISCOVER_SUB_VENDOR
> + * @base_discover_impl_version:                Function pointer for
> + *                                             BASE_DISCOVER_IMPLEMENTATION_VERSION
> + * @base_discover_list_protocols:      Function pointer for
> + *                                             BASE_DISCOVER_LIST_PROTOCOLS
> + * @base_discover_agent:               Function pointer for
> + *                                             BASE_DISCOVER_AGENT
> + * @base_notify_errors:                        Function pointer for
> + *                                             BASE_NOTIFY_ERRORS
> + * @base_set_device_permissions:       Function pointer for
> + *                                             BASE_SET_DEVICE_PROTOCOLS
> + * @base_set_protocol_permissions:     Function pointer for
> + *                                             BASE_SET_PROTOCOL_PERMISSIONS
> + * @base_reset_agent_configuration:    Function pointer for
> + *                                             BASE_RESET_AGENT_CONFIGURATION
> + */
> +struct scmi_base_ops {
> +       int (*protocol_version)(struct udevice *dev, u32 *version);
> +       int (*protocol_attrs)(struct udevice *dev, u32 *num_agents,
> +                             u32 *num_protocols);
> +       int (*protocol_message_attrs)(struct udevice *dev, u32 message_id,
> +                                     u32 *attributes);
> +       int (*base_discover_vendor)(struct udevice *dev, u8 *vendor);
> +       int (*base_discover_sub_vendor)(struct udevice *dev, u8 *sub_vendor);
> +       int (*base_discover_impl_version)(struct udevice *dev, u32 *impl_version);
> +       int (*base_discover_list_protocols)(struct udevice *dev, u8 **protocols);
> +       int (*base_discover_agent)(struct udevice *dev, u32 agent_id,
> +                                  u32 *ret_agent_id, u8 *name);
> +       int (*base_notify_errors)(struct udevice *dev, u32 enable);
> +       int (*base_set_device_permissions)(struct udevice *dev, u32 agent_id,
> +                                          u32 device_id, u32 flags);
> +       int (*base_set_protocol_permissions)(struct udevice *dev, u32 agent_id,
> +                                            u32 device_id, u32 command_id,
> +                                            u32 flags);
> +       int (*base_reset_agent_configuration)(struct udevice *dev, u32 agent_id,
> +                                             u32 flags);

Please can you add full function comments for each of these?

>  };
>
>  /*
> --
> 2.41.0
>

Regards,
Simon

Re: [PATCH 05/10] firmware: scmi: fake base protocol commands on sandbox

[Simon Glass]

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This is a simple implementation of SCMI base protocol for sandbox.
> The main use is in SCMI unit test.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/firmware/scmi/sandbox-scmi_agent.c | 359 ++++++++++++++++++++-
>  1 file changed, 358 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 06/10] test: dm: simplify SCMI unit test on sandbox

[Simon Glass]

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Adding SCMI base protocol makes it inconvenient to hold the agent instance
> (udevice) locally since the agent device will be re-created per each test.
> Just remove it and simply the test flows.
> The test scenario is not changed at all.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  arch/sandbox/include/asm/scmi_test.h       |  7 ++-
>  drivers/firmware/scmi/sandbox-scmi_agent.c | 20 +------
>  drivers/firmware/scmi/scmi_agent-uclass.c  | 14 -----
>  test/dm/scmi.c                             | 64 +++++++---------------
>  4 files changed, 26 insertions(+), 79 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[Simon Glass]

Hi AKASHI,

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Added is a new unit test for SCMI base protocol, which will exercise all
> the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
>   $ ut dm scmi_base
> It is assumed that test.dtb is used as sandbox's device tree.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>
> diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> index 881be3171b7c..563017bb63e0 100644
> --- a/test/dm/scmi.c
> +++ b/test/dm/scmi.c
> @@ -16,6 +16,9 @@
>  #include <clk.h>
>  #include <dm.h>
>  #include <reset.h>
> +#include <scmi_agent.h>
> +#include <scmi_agent-uclass.h>
> +#include <scmi_protocols.h>
>  #include <asm/scmi_test.h>
>  #include <dm/device-internal.h>
>  #include <dm/test.h>
> @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
>  }
>  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
>
> +static int dm_test_scmi_base(struct unit_test_state *uts)
> +{
> +       struct udevice *agent_dev, *base;
> +       struct scmi_agent_priv *priv;
> +       const struct scmi_base_ops *ops;
> +       u32 version, num_agents, num_protocols, impl_version;
> +       u32 attributes, agent_id;
> +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> +       u8 *protocols;
> +       int ret;
> +
> +       /* preparation */
> +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> +                                             &agent_dev));
> +       ut_assertnonnull(agent_dev);
> +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> +                                                 SCMI_PROTOCOL_ID_BASE));
> +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> +
> +       /* version */
> +       ret = (*ops->protocol_version)(base, &version);

Can you add uclass helpers to call each of the methods? That is how it
is commonly done. You should not be calling ops->xxx directly here.

> +       ut_assertok(ret);
> +       ut_asserteq(priv->version, version);
> +
> +       /* protocol attributes */
> +       ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols);
> +       ut_assertok(ret);
> +       ut_asserteq(priv->num_agents, num_agents);
> +       ut_asserteq(priv->num_protocols, num_protocols);
> +
> +       /* discover vendor */
> +       ret = (*ops->base_discover_vendor)(base, vendor);
> +       ut_assertok(ret);
> +       ut_asserteq_str(priv->vendor, vendor);
> +
> +       /* message attributes */
> +       ret = (*ops->protocol_message_attrs)(base,
> +                                            SCMI_BASE_DISCOVER_SUB_VENDOR,
> +                                            &attributes);
> +       ut_assertok(ret);
> +       ut_assertok(attributes);
> +
> +       /* discover sub vendor */
> +       ret = (*ops->base_discover_sub_vendor)(base, vendor);
> +       ut_assertok(ret);
> +       ut_asserteq_str(priv->sub_vendor, vendor);
> +
> +       /* impl version */
> +       ret = (*ops->base_discover_impl_version)(base, &impl_version);
> +       ut_assertok(ret);
> +       ut_asserteq(priv->impl_version, impl_version);
> +
> +       /* discover agent (my self) */
> +       ret = (*ops->base_discover_agent)(base, 0xffffffff,
> +                                         &agent_id, agent_name);
> +       ut_assertok(ret);
> +       ut_asserteq(priv->agent_id, agent_id);
> +       ut_asserteq_str(priv->agent_name, agent_name);
> +
> +       /* discover protocols */
> +       ret = (*ops->base_discover_list_protocols)(base, &protocols);
> +       ut_asserteq(num_protocols, ret);
> +       ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols);
> +       free(protocols);
> +
> +       /*
> +        * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting
> +        * access and protocol permissions, but doesn't allow unsetting them nor
> +        * resetting the configurations.
> +        */
> +       /* set device permissions */
> +       ret = (*ops->base_set_device_permissions)(base, agent_id, 0,
> +                                                 SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> +       ut_assertok(ret); /* SCMI_SUCCESS */
> +       ret = (*ops->base_set_device_permissions)(base, agent_id, 1,
> +                                                 SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> +       ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> +       ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0);
> +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +
> +       /* set protocol permissions */
> +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
> +                                                   SCMI_PROTOCOL_ID_CLOCK,
> +                                                   SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
> +       ut_assertok(ret); /* SCMI_SUCCESS */
> +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1,
> +                                                   SCMI_PROTOCOL_ID_CLOCK,
> +                                                   SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
> +       ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
> +                                                   SCMI_PROTOCOL_ID_CLOCK, 0);
> +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +
> +       /* reset agent configuration */
> +       ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
> +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +       ret = (*ops->base_reset_agent_configuration)(base, agent_id,
> +                                                    SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS);
> +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +       ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
> +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT);
> +
>  static int dm_test_scmi_clocks(struct unit_test_state *uts)
>  {
>         struct sandbox_scmi_agent *agent;
> --
> 2.41.0
>

Regards,
Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[Simon Glass]

Hi AKASHI,

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This command, "scmi", provides a command line interface to various SCMI
> protocols. It supports at least initially SCMI base protocol with the sub
> command, "base", and is intended mainly for debug purpose.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/Kconfig  |   8 ++
>  cmd/Makefile |   1 +
>  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 382 insertions(+)
>  create mode 100644 cmd/scmi.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 02e54f1e50fe..065470a76295 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2504,6 +2504,14 @@ 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 protocols,
> +         including Base protocol.

Please mention what is SCMI

>  endmenu
>
>  menu "Filesystem commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 6c37521b4e2b..826e0b74b587 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -156,6 +156,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..c97f77e97d95
> --- /dev/null
> +++ b/cmd/scmi.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  SCMI utility command
> + *
> + *  Copyright (c) 2023 Linaro Limited
> + *             Author: AKASHI Takahiro
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dm/device.h>
> +#include <dm/uclass.h> /* uclass_get_device */

Goes before linux/bitfield.h

> +#include <exports.h>
> +#include <scmi_agent.h>
> +#include <scmi_agent-uclass.h>
> +#include <asm/types.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +
> +static struct udevice *agent;
> +static struct udevice *base_proto;
> +static const struct scmi_base_ops *ops;
> +
> +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"},
> +};
> +
> +/**
> + * scmi_convert_id_to_string() - get the name of SCMI protocol
> + *
> + * @id:                Protocol ID
> + *
> + * Get the printable name of the protocol, @id
> + *
> + * Return:     Name string on success, NULL on failure
> + */
> +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)

scmi_lookup_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_base_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_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
> +                            char * const argv[])
> +{
> +       u32 agent_id, num_protocols;
> +       u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
> +       int i, ret;
> +
> +       if (argc != 1)
> +               return CMD_RET_USAGE;
> +
> +       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 = (*ops->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);
> +       }
> +       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",
> +                              scmi_convert_id_to_string(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_base_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_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
> +                               char * const argv[])
> +{
> +       u32 agent_id, device_id, flags, attributes;
> +       char *end;
> +       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;
> +
> +       ret = (*ops->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 = (*ops->base_set_device_permissions)(base_proto, agent_id,
> +                                                 device_id, flags);

Please use helpers rather than using ops directly.

Regards,
Simon

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[Simon Glass]

Hi AKASHI,

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This is a help text for scmi command.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 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..20cdae4b877d
> --- /dev/null
> +++ b/doc/usage/cmd/scmi.rst
> @@ -0,0 +1,98 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +scmi command
> +============
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    scmi base info
> +    scmi base perm_dev <agent id> <device id> <flags>
> +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> +    scmi base reset <agent id> <flags>
> +
> +Description
> +-----------
> +
> +The scmi command is used to access and operate on SCMI server.
> +
> +scmi base info
> +~~~~~~~~~~~~~~
> +    Show base information about SCMI server and supported protocols
> +
> +scmi base perm_dev
> +~~~~~~~~~~~~~~~~~~
> +    Allow or deny access permission to the device
> +
> +scmi base perm_proto
> +~~~~~~~~~~~~~~~~~~~~
> +    Allow or deny access to the protocol on the device
> +
> +scmi base reset
> +~~~~~~~~~~~~~~~
> +    Reset the existing configurations
> +
> +Parameters are used as follows:
> +
> +<agent id>
> +    Agent ID

what is this?

> +
> +<device id>
> +    Device ID

what is this?

> +
> +<command id>
> +    Protocol ID, should not be 0x10 (base protocol)

what is this? Please add more detail

> +
> +<flags>
> +    Flags to control the action. See SCMI specification for
> +    defined values.

?

Please add the flags here, or at the very least provide a URL and page
number, etc.

> +
> +Example
> +-------
> +
> +Obtain basic information about SCMI server:
> +
> +::
> +
> +    => scmi base 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 base perm_dev 1 0 1
> +
> +Reset configurations with all access permission settings retained:
> +
> +::
> +
> +    => scmi base 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.
> --
> 2.41.0
>

Regards,
Simon

Re: [PATCH 10/10] test: dm: add scmi command test

[Simon Glass]

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> In this test, "scmi" with different sub-commands is tested.
> 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>
> ---
>  test/dm/scmi.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 04/10] sandbox: remove SCMI base node definition from test.dts

[Simon Glass]

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> SCMI base protocol is mandatory and doesn't need to be listed in a device
> tree.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  arch/sandbox/dts/test.dts | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 01/10] firmware: scmi: implement SCMI base protocol

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:09:45PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > SCMI base protocol is mandatory according to the SCMI specification.
> >
> > With this patch, SCMI base protocol can be accessed via SCMI transport
> > layers. All the commands, except SCMI_BASE_NOTIFY_ERRORS, are supported.
> > This is because U-Boot doesn't support interrupts and the current transport
> > layers are not able to handle asynchronous messages properly.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/firmware/scmi/Makefile |   1 +
> >  drivers/firmware/scmi/base.c   | 517 +++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h         |   1 +
> >  include/scmi_protocols.h       | 201 ++++++++++++-
> >  4 files changed, 718 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/firmware/scmi/base.c
> >
> > diff --git a/drivers/firmware/scmi/Makefile b/drivers/firmware/scmi/Makefile
> > index b2ff483c75a1..1a23d4981709 100644
> > --- a/drivers/firmware/scmi/Makefile
> > +++ b/drivers/firmware/scmi/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-y  += scmi_agent-uclass.o
> > +obj-y  += base.o
> >  obj-y  += smt.o
> >  obj-$(CONFIG_SCMI_AGENT_SMCCC)         += smccc_agent.o
> >  obj-$(CONFIG_SCMI_AGENT_MAILBOX)       += mailbox_agent.o
> 
> [..]
> 
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 307ad6931ca7..f7a110852321 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -116,6 +116,7 @@ enum uclass_id {
> >         UCLASS_RNG,             /* Random Number Generator */
> >         UCLASS_RTC,             /* Real time clock device */
> >         UCLASS_SCMI_AGENT,      /* Interface with an SCMI server */
> > +       UCLASS_SCMI_BASE,       /* Interface for SCMI Base protocol */
> >         UCLASS_SCSI,            /* SCSI device */
> >         UCLASS_SERIAL,          /* Serial UART */
> >         UCLASS_SIMPLE_BUS,      /* Bus with child devices */
> > diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> > index a220cb2a91ad..769041654534 100644
> > --- a/include/scmi_protocols.h
> > +++ b/include/scmi_protocols.h
> > @@ -15,6 +15,8 @@
> >   * https://developer.arm.com/docs/den0056/b
> >   */
> >
> > +#define SCMI_BASE_NAME_LENGTH_MAX 16
> > +
> >  enum scmi_std_protocol {
> >         SCMI_PROTOCOL_ID_BASE = 0x10,
> >         SCMI_PROTOCOL_ID_POWER_DOMAIN = 0x11,
> > @@ -41,12 +43,207 @@ enum scmi_status_code {
> >  };
> >
> >  /*
> > - * Generic message IDs
> > + * SCMI Base Protocol
> >   */
> > -enum scmi_discovery_id {
> > +#define SCMI_BASE_PROTOCOL_VERSION 0x20000
> > +
> > +enum scmi_base_message_id {
> >         SCMI_PROTOCOL_VERSION = 0x0,
> >         SCMI_PROTOCOL_ATTRIBUTES = 0x1,
> >         SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> > +       SCMI_BASE_DISCOVER_VENDOR = 0x3,
> > +       SCMI_BASE_DISCOVER_SUB_VENDOR = 0x4,
> > +       SCMI_BASE_DISCOVER_IMPL_VERSION = 0x5,
> > +       SCMI_BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
> > +       SCMI_BASE_DISCOVER_AGENT = 0x7,
> > +       SCMI_BASE_NOTIFY_ERRORS = 0x8,
> > +       SCMI_BASE_SET_DEVICE_PERMISSIONS = 0x9,
> > +       SCMI_BASE_SET_PROTOCOL_PERMISSIONS = 0xa,
> > +       SCMI_BASE_RESET_AGENT_CONFIGURATION = 0xb,
> > +};
> > +
> > +/**
> > + * struct scmi_protocol_version_out - Response for SCMI_PROTOCOL_VERSION
> > + *                                     command
> > + * @status:    SCMI command status
> > + * @version:   Protocol version
> > + */
> > +struct scmi_protocol_version_out {
> > +       s32 status;
> > +       u32 version;
> > +};
> > +
> > +/**
> > + * struct scmi_protocol_attrs_out - Response for SCMI_PROTOCOL_ATTRIBUTES
> > + *                                     command
> > + * @status:    SCMI command status
> > + * @attributes:        Protocol attributes or implementation details
> > + */
> > +struct scmi_protocol_attrs_out {
> > +       s32 status;
> > +       u32 attributes;
> > +};
> > +
> > +#define SCMI_PROTOCOL_ATTRS_NUM_AGENTS(attributes) \
> > +                               (((attributes) & GENMASK(15, 8)) >> 8)
> > +#define SCMI_PROTOCOL_ATTRS_NUM_PROTOCOLS(attributes) \
> > +                               ((attributes) & GENMASK(7, 0))
> > +
> > +/**
> > + * struct scmi_protocol_msg_attrs_out - Response for
> > + *                                     SCMI_PROTOCOL_MESSAGE_ATTRIBUTES command
> > + * @status:    SCMI command status
> > + * @attributes:        Message-specific attributes
> > + */
> > +struct scmi_protocol_msg_attrs_out {
> > +       s32 status;
> > +       u32 attributes;
> > +};
> > +
> > +/**
> > + * struct scmi_base_discover_vendor_out - Response for
> > + *                                       SCMI_BASE_DISCOVER_VENDOR or
> > + *                                       SCMI_BASE_DISCOVER_SUB_VENDOR command
> > + * @status:            SCMI command status
> > + * @vendor_identifier: Identifier of vendor or sub-vendor in string
> > + */
> > +struct scmi_base_discover_vendor_out {
> > +       s32 status;
> > +       u8 vendor_identifier[SCMI_BASE_NAME_LENGTH_MAX];
> > +};
> > +
> > +/**
> > + * struct scmi_base_discover_impl_version_out - Response for
> > + *                                     SCMI_BASE_DISCOVER_IMPL_VERSION command
> > + * @status:            SCMI command status
> > + * @impl_version:      Vendor-specific implementation version
> > + */
> > +struct scmi_base_discover_impl_version_out {
> > +       s32 status;
> > +       u32 impl_version;
> > +};
> > +
> > +/**
> > + * struct scmi_base_discover_list_protocols_out - Response for
> > + *                             SCMI_BASE_DISCOVER_LIST_PROTOCOLS command
> > + * @status:            SCMI command status
> > + * @num_protocols:     Number of protocols in @protocol
> > + * @protocols:         Array of packed protocol ID's
> > + */
> > +struct scmi_base_discover_list_protocols_out {
> > +       s32 status;
> > +       u32 num_protocols;
> > +       u32 protocols[3];
> > +};
> > +
> > +/**
> > + * struct scmi_base_discover_agent_out - Response for
> > + *                                      SCMI_BASE_DISCOVER_AGENT command
> > + * @status:    SCMI command status
> > + * @agent_id:  Identifier of agent
> > + * @name:      Name of agent in string
> > + */
> > +struct scmi_base_discover_agent_out {
> > +       s32 status;
> > +       u32 agent_id;
> > +       u8 name[SCMI_BASE_NAME_LENGTH_MAX];
> > +};
> > +
> > +#define SCMI_BASE_NOTIFY_ERRORS_ENABLE BIT(0)
> > +
> > +/**
> > + * struct scmi_base_set_device_permissions_in - Parameters for
> > + *                                     SCMI_BASE_SET_DEVICE_PERMISSIONS command
> > + * @agent_id:  Identifier of agent
> > + * @device_id: Identifier of device
> > + * @flags:     A set of flags
> > + */
> > +struct scmi_base_set_device_permissions_in {
> > +       u32 agent_id;
> > +       u32 device_id;
> > +       u32 flags;
> > +};
> > +
> > +#define SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS BIT(0)
> > +
> > +/**
> > + * struct scmi_base_set_protocol_permissions_in - Parameters for
> > + *                             SCMI_BASE_SET_PROTOCOL_PERMISSIONS command
> > + * @agent_id:          Identifier of agent
> > + * @device_id:         Identifier of device
> > + * @command_id:                Identifier of command
> > + * @flags:             A set of flags
> > + */
> > +struct scmi_base_set_protocol_permissions_in {
> > +       u32 agent_id;
> > +       u32 device_id;
> > +       u32 command_id;
> > +       u32 flags;
> > +};
> > +
> > +#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_COMMAND GENMASK(7, 0)
> > +#define SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS BIT(0)
> > +
> > +/**
> > + * struct scmi_base_reset_agent_configuration_in - Parameters for
> > + *                             SCMI_BASE_RESET_AGENT_CONFIGURATION command
> > + * @agent_id:  Identifier of agent
> > + * @flags:     A set of flags
> > + */
> > +struct scmi_base_reset_agent_configuration_in {
> > +       u32 agent_id;
> > +       u32 flags;
> > +};
> > +
> > +#define SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS BIT(0)
> > +
> > +/**
> > + * struct scmi_base_ops - SCMI base protocol interfaces
> > + * @protocol_version:                  Function pointer for
> > + *                                             PROTOCOL_VERSION
> > + * @protocol_attrs:                    Function pointer for
> > + *                                             PROTOCOL_ATTRIBUTES
> > + * @protocol_message_attrs:            Function pointer for
> > + *                                             PROTOCOL_MESSAGE_ATTRIBUTES
> > + * @base_discover_vendor:              Function pointer for
> > + *                                             BASE_DISCOVER_VENDOR
> > + * @base_discover_sub_vendor:          Function pointer for
> > + *                                             BASE_DISCOVER_SUB_VENDOR
> > + * @base_discover_impl_version:                Function pointer for
> > + *                                             BASE_DISCOVER_IMPLEMENTATION_VERSION
> > + * @base_discover_list_protocols:      Function pointer for
> > + *                                             BASE_DISCOVER_LIST_PROTOCOLS
> > + * @base_discover_agent:               Function pointer for
> > + *                                             BASE_DISCOVER_AGENT
> > + * @base_notify_errors:                        Function pointer for
> > + *                                             BASE_NOTIFY_ERRORS
> > + * @base_set_device_permissions:       Function pointer for
> > + *                                             BASE_SET_DEVICE_PROTOCOLS
> > + * @base_set_protocol_permissions:     Function pointer for
> > + *                                             BASE_SET_PROTOCOL_PERMISSIONS
> > + * @base_reset_agent_configuration:    Function pointer for
> > + *                                             BASE_RESET_AGENT_CONFIGURATION
> > + */
> > +struct scmi_base_ops {
> > +       int (*protocol_version)(struct udevice *dev, u32 *version);
> > +       int (*protocol_attrs)(struct udevice *dev, u32 *num_agents,
> > +                             u32 *num_protocols);
> > +       int (*protocol_message_attrs)(struct udevice *dev, u32 message_id,
> > +                                     u32 *attributes);
> > +       int (*base_discover_vendor)(struct udevice *dev, u8 *vendor);
> > +       int (*base_discover_sub_vendor)(struct udevice *dev, u8 *sub_vendor);
> > +       int (*base_discover_impl_version)(struct udevice *dev, u32 *impl_version);
> > +       int (*base_discover_list_protocols)(struct udevice *dev, u8 **protocols);
> > +       int (*base_discover_agent)(struct udevice *dev, u32 agent_id,
> > +                                  u32 *ret_agent_id, u8 *name);
> > +       int (*base_notify_errors)(struct udevice *dev, u32 enable);
> > +       int (*base_set_device_permissions)(struct udevice *dev, u32 agent_id,
> > +                                          u32 device_id, u32 flags);
> > +       int (*base_set_protocol_permissions)(struct udevice *dev, u32 agent_id,
> > +                                            u32 device_id, u32 command_id,
> > +                                            u32 flags);
> > +       int (*base_reset_agent_configuration)(struct udevice *dev, u32 agent_id,
> > +                                             u32 flags);
> 
> Please can you add full function comments for each of these?

Yes, I will.

Thanks,
-Takahiro Akashi


> >  };
> >
> >  /*
> > --
> > 2.41.0
> >
> 
> Regards,
> Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This command, "scmi", provides a command line interface to various SCMI
> > protocols. It supports at least initially SCMI base protocol with the sub
> > command, "base", and is intended mainly for debug purpose.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/Kconfig  |   8 ++
> >  cmd/Makefile |   1 +
> >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 382 insertions(+)
> >  create mode 100644 cmd/scmi.c
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 02e54f1e50fe..065470a76295 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2504,6 +2504,14 @@ 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 protocols,
> > +         including Base protocol.
> 
> Please mention what is SCMI

I will give a full spelling.

> >  endmenu
> >
> >  menu "Filesystem commands"
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 6c37521b4e2b..826e0b74b587 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -156,6 +156,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..c97f77e97d95
> > --- /dev/null
> > +++ b/cmd/scmi.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  SCMI utility command
> > + *
> > + *  Copyright (c) 2023 Linaro Limited
> > + *             Author: AKASHI Takahiro
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <dm/device.h>
> > +#include <dm/uclass.h> /* uclass_get_device */
> 
> Goes before linux/bitfield.h

Can you tell me why?

> > +#include <exports.h>
> > +#include <scmi_agent.h>
> > +#include <scmi_agent-uclass.h>
> > +#include <asm/types.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +
> > +static struct udevice *agent;
> > +static struct udevice *base_proto;
> > +static const struct scmi_base_ops *ops;
> > +
> > +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"},
> > +};
> > +
> > +/**
> > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > + *
> > + * @id:                Protocol ID
> > + *
> > + * Get the printable name of the protocol, @id
> > + *
> > + * Return:     Name string on success, NULL on failure
> > + */
> > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> 
> scmi_lookup_id?

Not sure your intention.
The name above gives us exactly what this function does for pretty printing
instead of a number.
I think that 'lookup' implies we try to determine if the id exists or not.

> > +{
> > +       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_base_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_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                            char * const argv[])
> > +{
> > +       u32 agent_id, num_protocols;
> > +       u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
> > +       int i, ret;
> > +
> > +       if (argc != 1)
> > +               return CMD_RET_USAGE;
> > +
> > +       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 = (*ops->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);
> > +       }
> > +       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",
> > +                              scmi_convert_id_to_string(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_base_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_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                               char * const argv[])
> > +{
> > +       u32 agent_id, device_id, flags, attributes;
> > +       char *end;
> > +       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;
> > +
> > +       ret = (*ops->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 = (*ops->base_set_device_permissions)(base_proto, agent_id,
> > +                                                 device_id, flags);
> 
> Please use helpers rather than using ops directly.

Okay.

-Takahiro Akashi

> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Added is a new unit test for SCMI base protocol, which will exercise all
> > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> >   $ ut dm scmi_base
> > It is assumed that test.dtb is used as sandbox's device tree.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > index 881be3171b7c..563017bb63e0 100644
> > --- a/test/dm/scmi.c
> > +++ b/test/dm/scmi.c
> > @@ -16,6 +16,9 @@
> >  #include <clk.h>
> >  #include <dm.h>
> >  #include <reset.h>
> > +#include <scmi_agent.h>
> > +#include <scmi_agent-uclass.h>
> > +#include <scmi_protocols.h>
> >  #include <asm/scmi_test.h>
> >  #include <dm/device-internal.h>
> >  #include <dm/test.h>
> > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> >  }
> >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> >
> > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > +{
> > +       struct udevice *agent_dev, *base;
> > +       struct scmi_agent_priv *priv;
> > +       const struct scmi_base_ops *ops;
> > +       u32 version, num_agents, num_protocols, impl_version;
> > +       u32 attributes, agent_id;
> > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > +       u8 *protocols;
> > +       int ret;
> > +
> > +       /* preparation */
> > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > +                                             &agent_dev));
> > +       ut_assertnonnull(agent_dev);
> > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > +                                                 SCMI_PROTOCOL_ID_BASE));
> > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > +
> > +       /* version */
> > +       ret = (*ops->protocol_version)(base, &version);
> 
> Can you add uclass helpers to call each of the methods? That is how it
> is commonly done. You should not be calling ops->xxx directly here.

Yes, I will add inline functions instead.

-Takahiro Akashi

> > +       ut_assertok(ret);
> > +       ut_asserteq(priv->version, version);
> > +
> > +       /* protocol attributes */
> > +       ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols);
> > +       ut_assertok(ret);
> > +       ut_asserteq(priv->num_agents, num_agents);
> > +       ut_asserteq(priv->num_protocols, num_protocols);
> > +
> > +       /* discover vendor */
> > +       ret = (*ops->base_discover_vendor)(base, vendor);
> > +       ut_assertok(ret);
> > +       ut_asserteq_str(priv->vendor, vendor);
> > +
> > +       /* message attributes */
> > +       ret = (*ops->protocol_message_attrs)(base,
> > +                                            SCMI_BASE_DISCOVER_SUB_VENDOR,
> > +                                            &attributes);
> > +       ut_assertok(ret);
> > +       ut_assertok(attributes);
> > +
> > +       /* discover sub vendor */
> > +       ret = (*ops->base_discover_sub_vendor)(base, vendor);
> > +       ut_assertok(ret);
> > +       ut_asserteq_str(priv->sub_vendor, vendor);
> > +
> > +       /* impl version */
> > +       ret = (*ops->base_discover_impl_version)(base, &impl_version);
> > +       ut_assertok(ret);
> > +       ut_asserteq(priv->impl_version, impl_version);
> > +
> > +       /* discover agent (my self) */
> > +       ret = (*ops->base_discover_agent)(base, 0xffffffff,
> > +                                         &agent_id, agent_name);
> > +       ut_assertok(ret);
> > +       ut_asserteq(priv->agent_id, agent_id);
> > +       ut_asserteq_str(priv->agent_name, agent_name);
> > +
> > +       /* discover protocols */
> > +       ret = (*ops->base_discover_list_protocols)(base, &protocols);
> > +       ut_asserteq(num_protocols, ret);
> > +       ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols);
> > +       free(protocols);
> > +
> > +       /*
> > +        * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting
> > +        * access and protocol permissions, but doesn't allow unsetting them nor
> > +        * resetting the configurations.
> > +        */
> > +       /* set device permissions */
> > +       ret = (*ops->base_set_device_permissions)(base, agent_id, 0,
> > +                                                 SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> > +       ut_assertok(ret); /* SCMI_SUCCESS */
> > +       ret = (*ops->base_set_device_permissions)(base, agent_id, 1,
> > +                                                 SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> > +       ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> > +       ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0);
> > +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +
> > +       /* set protocol permissions */
> > +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
> > +                                                   SCMI_PROTOCOL_ID_CLOCK,
> > +                                                   SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
> > +       ut_assertok(ret); /* SCMI_SUCCESS */
> > +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1,
> > +                                                   SCMI_PROTOCOL_ID_CLOCK,
> > +                                                   SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS);
> > +       ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> > +       ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0,
> > +                                                   SCMI_PROTOCOL_ID_CLOCK, 0);
> > +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +
> > +       /* reset agent configuration */
> > +       ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
> > +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +       ret = (*ops->base_reset_agent_configuration)(base, agent_id,
> > +                                                    SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS);
> > +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +       ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0);
> > +       ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +
> > +       return 0;
> > +}
> > +
> > +DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT);
> > +
> >  static int dm_test_scmi_clocks(struct unit_test_state *uts)
> >  {
> >         struct sandbox_scmi_agent *agent;
> > --
> > 2.41.0
> >
> 
> Regards,
> Simon

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This is a help text for scmi command.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 98 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..20cdae4b877d
> > --- /dev/null
> > +++ b/doc/usage/cmd/scmi.rst
> > @@ -0,0 +1,98 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +scmi command
> > +============
> > +
> > +Synopsis
> > +--------
> > +
> > +::
> > +
> > +    scmi base info
> > +    scmi base perm_dev <agent id> <device id> <flags>
> > +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> > +    scmi base reset <agent id> <flags>
> > +
> > +Description
> > +-----------
> > +
> > +The scmi command is used to access and operate on SCMI server.
> > +
> > +scmi base info
> > +~~~~~~~~~~~~~~
> > +    Show base information about SCMI server and supported protocols
> > +
> > +scmi base perm_dev
> > +~~~~~~~~~~~~~~~~~~
> > +    Allow or deny access permission to the device
> > +
> > +scmi base perm_proto
> > +~~~~~~~~~~~~~~~~~~~~
> > +    Allow or deny access to the protocol on the device
> > +
> > +scmi base reset
> > +~~~~~~~~~~~~~~~
> > +    Reset the existing configurations
> > +
> > +Parameters are used as follows:
> > +
> > +<agent id>
> > +    Agent ID
> 
> what is this?

I thought that the meaning was trivial in SCMI context.
Will change it to "SCMI Agent ID".


> > +
> > +<device id>
> > +    Device ID
> 
> what is this?

Again, will change it to "SCMI Device ID".

> > +
> > +<command id>
> > +    Protocol ID, should not be 0x10 (base protocol)
> 
> what is this? Please add more detail

Again, will change it to "SCMI Protocol ID".
I think that users should be familiar with those terms
if they want to use these interfaces.

> > +
> > +<flags>
> > +    Flags to control the action. See SCMI specification for
> > +    defined values.
> 
> ?
> 
> Please add the flags here, or at the very least provide a URL and page
> number, etc.

I intentionally avoid providing details here because a set of flags
acceptable to a specific SCMI server may depend on the server
and its implementation version.
The interface on U-Boot is just a wrapper to make a call to SCMI server
via a transport layer and doesn't care what the parameters means.
That said, I agree to referring to a URL to SCMI specification somewhere
in this document.

Thanks,
-Takahiro Akashi

> > +
> > +Example
> > +-------
> > +
> > +Obtain basic information about SCMI server:
> > +
> > +::
> > +
> > +    => scmi base 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 base perm_dev 1 0 1
> > +
> > +Reset configurations with all access permission settings retained:
> > +
> > +::
> > +
> > +    => scmi base 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.
> > --
> > 2.41.0
> >
> 
> Regards,
> Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[Simon Glass]

Hi,

On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > This command, "scmi", provides a command line interface to various SCMI
> > > protocols. It supports at least initially SCMI base protocol with the sub
> > > command, "base", and is intended mainly for debug purpose.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  cmd/Kconfig  |   8 ++
> > >  cmd/Makefile |   1 +
> > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 382 insertions(+)
> > >  create mode 100644 cmd/scmi.c
> > >
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 02e54f1e50fe..065470a76295 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -2504,6 +2504,14 @@ 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 protocols,
> > > +         including Base protocol.
> >
> > Please mention what is SCMI
>
> I will give a full spelling.
>
> > >  endmenu
> > >
> > >  menu "Filesystem commands"
> > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > index 6c37521b4e2b..826e0b74b587 100644
> > > --- a/cmd/Makefile
> > > +++ b/cmd/Makefile
> > > @@ -156,6 +156,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..c97f77e97d95
> > > --- /dev/null
> > > +++ b/cmd/scmi.c
> > > @@ -0,0 +1,373 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + *  SCMI utility command
> > > + *
> > > + *  Copyright (c) 2023 Linaro Limited
> > > + *             Author: AKASHI Takahiro
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <command.h>
> > > +#include <dm/device.h>
> > > +#include <dm/uclass.h> /* uclass_get_device */
> >
> > Goes before linux/bitfield.h
>
> Can you tell me why?

It is just a convention:
https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

>
> > > +#include <exports.h>
> > > +#include <scmi_agent.h>
> > > +#include <scmi_agent-uclass.h>
> > > +#include <asm/types.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +
> > > +static struct udevice *agent;
> > > +static struct udevice *base_proto;
> > > +static const struct scmi_base_ops *ops;
> > > +
> > > +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"},
> > > +};
> > > +
> > > +/**
> > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > + *
> > > + * @id:                Protocol ID
> > > + *
> > > + * Get the printable name of the protocol, @id
> > > + *
> > > + * Return:     Name string on success, NULL on failure
> > > + */
> > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> >
> > scmi_lookup_id?
>
> Not sure your intention.
> The name above gives us exactly what this function does for pretty printing
> instead of a number.
> I think that 'lookup' implies we try to determine if the id exists or not.

I am just trying to avoid the code turning into Java :-)

How about scmi_get_name() ?

Regards,
Simon

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[Simon Glass]

Hi ,

On Mon, 3 Jul 2023 at 02:19, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > This is a help text for scmi command.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 98 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..20cdae4b877d
> > > --- /dev/null
> > > +++ b/doc/usage/cmd/scmi.rst
> > > @@ -0,0 +1,98 @@
> > > +.. SPDX-License-Identifier: GPL-2.0+:
> > > +
> > > +scmi command
> > > +============
> > > +
> > > +Synopsis
> > > +--------
> > > +
> > > +::
> > > +
> > > +    scmi base info
> > > +    scmi base perm_dev <agent id> <device id> <flags>
> > > +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> > > +    scmi base reset <agent id> <flags>
> > > +
> > > +Description
> > > +-----------
> > > +
> > > +The scmi command is used to access and operate on SCMI server.
> > > +
> > > +scmi base info
> > > +~~~~~~~~~~~~~~
> > > +    Show base information about SCMI server and supported protocols
> > > +
> > > +scmi base perm_dev
> > > +~~~~~~~~~~~~~~~~~~
> > > +    Allow or deny access permission to the device
> > > +
> > > +scmi base perm_proto
> > > +~~~~~~~~~~~~~~~~~~~~
> > > +    Allow or deny access to the protocol on the device
> > > +
> > > +scmi base reset
> > > +~~~~~~~~~~~~~~~
> > > +    Reset the existing configurations
> > > +
> > > +Parameters are used as follows:
> > > +
> > > +<agent id>
> > > +    Agent ID
> >
> > what is this?
>
> I thought that the meaning was trivial in SCMI context.
> Will change it to "SCMI Agent ID".

What is SCMI? Really you should explain and link to information about
things you mention in documentation. How else is anyone supposed to
figure out what you are talking about?

"SCMI Agent ID" doesn't tell us much. What form does it take? Is it a
string? How do you find it out?

>
>
> > > +
> > > +<device id>
> > > +    Device ID
> >
> > what is this?
>
> Again, will change it to "SCMI Device ID".

That doesn't help much. The purpose of documentation is to explain
things for people who don't already know it. We need to try to be as
helpful as possible.

>
> > > +
> > > +<command id>
> > > +    Protocol ID, should not be 0x10 (base protocol)
> >
> > what is this? Please add more detail
>
> Again, will change it to "SCMI Protocol ID".
> I think that users should be familiar with those terms
> if they want to use these interfaces.

Maybe, but it still isn't clear what it is. A string? It is confusing
that the command ID is also a protocol ID.

>
> > > +
> > > +<flags>
> > > +    Flags to control the action. See SCMI specification for
> > > +    defined values.
> >
> > ?
> >
> > Please add the flags here, or at the very least provide a URL and page
> > number, etc.
>
> I intentionally avoid providing details here because a set of flags
> acceptable to a specific SCMI server may depend on the server
> and its implementation version.
> The interface on U-Boot is just a wrapper to make a call to SCMI server
> via a transport layer and doesn't care what the parameters means.
> That said, I agree to referring to a URL to SCMI specification somewhere
> in this document.

That will help. But also please add that this is a hex value (right?)


>
> Thanks,
> -Takahiro Akashi
>
> > > +
> > > +Example
> > > +-------
> > > +
> > > +Obtain basic information about SCMI server:
> > > +
> > > +::
> > > +
> > > +    => scmi base 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 base perm_dev 1 0 1
> > > +
> > > +Reset configurations with all access permission settings retained:
> > > +
> > > +::
> > > +
> > > +    => scmi base 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.
> > > --
> > > 2.41.0
> > >
Regards,
Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[Simon Glass]

Hi,

On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > >   $ ut dm scmi_base
> > > It is assumed that test.dtb is used as sandbox's device tree.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 112 insertions(+)
> > >
> > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > index 881be3171b7c..563017bb63e0 100644
> > > --- a/test/dm/scmi.c
> > > +++ b/test/dm/scmi.c
> > > @@ -16,6 +16,9 @@
> > >  #include <clk.h>
> > >  #include <dm.h>
> > >  #include <reset.h>
> > > +#include <scmi_agent.h>
> > > +#include <scmi_agent-uclass.h>
> > > +#include <scmi_protocols.h>
> > >  #include <asm/scmi_test.h>
> > >  #include <dm/device-internal.h>
> > >  #include <dm/test.h>
> > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > >  }
> > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > >
> > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > +{
> > > +       struct udevice *agent_dev, *base;
> > > +       struct scmi_agent_priv *priv;
> > > +       const struct scmi_base_ops *ops;
> > > +       u32 version, num_agents, num_protocols, impl_version;
> > > +       u32 attributes, agent_id;
> > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > +       u8 *protocols;
> > > +       int ret;
> > > +
> > > +       /* preparation */
> > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > +                                             &agent_dev));
> > > +       ut_assertnonnull(agent_dev);
> > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > +
> > > +       /* version */
> > > +       ret = (*ops->protocol_version)(base, &version);
> >
> > Can you add uclass helpers to call each of the methods? That is how it
> > is commonly done. You should not be calling ops->xxx directly here.
>
> Yes, I will add inline functions instead.

I don't mean inline...see all the other uclasses which define a
function which is implemented in the uclass. It is confusing when one
uclass does something different. People might copy this style and then
the code base diverges. Did you not notice this when looking around
the source tree?

Regards,
Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> Hi,
> 
> On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > This command, "scmi", provides a command line interface to various SCMI
> > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > command, "base", and is intended mainly for debug purpose.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  cmd/Kconfig  |   8 ++
> > > >  cmd/Makefile |   1 +
> > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 382 insertions(+)
> > > >  create mode 100644 cmd/scmi.c
> > > >
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 02e54f1e50fe..065470a76295 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -2504,6 +2504,14 @@ 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 protocols,
> > > > +         including Base protocol.
> > >
> > > Please mention what is SCMI
> >
> > I will give a full spelling.
> >
> > > >  endmenu
> > > >
> > > >  menu "Filesystem commands"
> > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > --- a/cmd/Makefile
> > > > +++ b/cmd/Makefile
> > > > @@ -156,6 +156,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..c97f77e97d95
> > > > --- /dev/null
> > > > +++ b/cmd/scmi.c
> > > > @@ -0,0 +1,373 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + *  SCMI utility command
> > > > + *
> > > > + *  Copyright (c) 2023 Linaro Limited
> > > > + *             Author: AKASHI Takahiro
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <command.h>
> > > > +#include <dm/device.h>
> > > > +#include <dm/uclass.h> /* uclass_get_device */
> > >
> > > Goes before linux/bitfield.h
> >
> > Can you tell me why?
> 
> It is just a convention:
> https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

Okay.

> >
> > > > +#include <exports.h>
> > > > +#include <scmi_agent.h>
> > > > +#include <scmi_agent-uclass.h>
> > > > +#include <asm/types.h>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/bitops.h>
> > > > +
> > > > +static struct udevice *agent;
> > > > +static struct udevice *base_proto;
> > > > +static const struct scmi_base_ops *ops;
> > > > +
> > > > +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"},
> > > > +};
> > > > +
> > > > +/**
> > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > + *
> > > > + * @id:                Protocol ID
> > > > + *
> > > > + * Get the printable name of the protocol, @id
> > > > + *
> > > > + * Return:     Name string on success, NULL on failure
> > > > + */
> > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > >
> > > scmi_lookup_id?
> >
> > Not sure your intention.
> > The name above gives us exactly what this function does for pretty printing
> > instead of a number.
> > I think that 'lookup' implies we try to determine if the id exists or not.
> 
> I am just trying to avoid the code turning into Java :-)

Java?

> How about scmi_get_name() ?

Well, more specifically scmi_get_protocol_name()?

-Takahiro Akashi

> Regards,
> Simon

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[AKASHI Takahiro]

Hi Simon,

On Mon, Jul 03, 2023 at 02:30:55PM +0100, Simon Glass wrote:
> Hi ,
> 
> On Mon, 3 Jul 2023 at 02:19, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > This is a help text for scmi command.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 98 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..20cdae4b877d
> > > > --- /dev/null
> > > > +++ b/doc/usage/cmd/scmi.rst
> > > > @@ -0,0 +1,98 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0+:
> > > > +
> > > > +scmi command
> > > > +============
> > > > +
> > > > +Synopsis
> > > > +--------
> > > > +
> > > > +::
> > > > +
> > > > +    scmi base info
> > > > +    scmi base perm_dev <agent id> <device id> <flags>
> > > > +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> > > > +    scmi base reset <agent id> <flags>
> > > > +
> > > > +Description
> > > > +-----------
> > > > +
> > > > +The scmi command is used to access and operate on SCMI server.
> > > > +
> > > > +scmi base info
> > > > +~~~~~~~~~~~~~~
> > > > +    Show base information about SCMI server and supported protocols
> > > > +
> > > > +scmi base perm_dev
> > > > +~~~~~~~~~~~~~~~~~~
> > > > +    Allow or deny access permission to the device
> > > > +
> > > > +scmi base perm_proto
> > > > +~~~~~~~~~~~~~~~~~~~~
> > > > +    Allow or deny access to the protocol on the device
> > > > +
> > > > +scmi base reset
> > > > +~~~~~~~~~~~~~~~
> > > > +    Reset the existing configurations
> > > > +
> > > > +Parameters are used as follows:
> > > > +
> > > > +<agent id>
> > > > +    Agent ID
> > >
> > > what is this?
> >
> > I thought that the meaning was trivial in SCMI context.
> > Will change it to "SCMI Agent ID".
> 
> What is SCMI? Really you should explain and link to information about
> things you mention in documentation. How else is anyone supposed to

Generally yes, but please note that SCMI and related drivers are already
there in U-Boot. For instance, see drivers/firmware/scmi/Kconfig.
I don't think we need any more explanation about what is SCMI everywhere
the word, "SCMI", appears.

> figure out what you are talking about?

Again, those who don't know about SCMI and if SCMI server is installed
on their systems will never use this command.

> 
> "SCMI Agent ID" doesn't tell us much. What form does it take? Is it a
> string? How do you find it out?

While I still believe that What SCMI agent ID means is trivial for
those who read the SCMI specification, I will give a short description
about possible values.

> >
> >
> > > > +
> > > > +<device id>
> > > > +    Device ID
> > >
> > > what is this?
> >
> > Again, will change it to "SCMI Device ID".
> 
> That doesn't help much. The purpose of documentation is to explain
> things for people who don't already know it. We need to try to be as
> helpful as possible.

The same above.
Please note that the definition of "device (ID)", except its data type,
is out of scope of SCMI specification.
It doesn't describe any means by which values be identified/retrieved.

> >
> > > > +
> > > > +<command id>
> > > > +    Protocol ID, should not be 0x10 (base protocol)
> > >
> > > what is this? Please add more detail
> >
> > Again, will change it to "SCMI Protocol ID".
> > I think that users should be familiar with those terms
> > if they want to use these interfaces.
> 
> Maybe, but it still isn't clear what it is. A string?

Will add a short description about its data type/format.

> It is confusing
> that the command ID is also a protocol ID.

Yes, I know, but the confusion exists in SCMI specification.
It sometimes seems to use both words almost interchangeably, even
in a single context. For instance, the section 4.2.2.11 of [1]
says,

4.2.2.11 BASE_SET_PROTOCOL_PERMISSIONS
 ...
 This command BASE_SET_PROTOCOL_PERMISSIONS is used to ...
 (This "command" has a yet another meaning.)

Parameters
uint32_t command_id     Bits[31:8] Reserved, must be zero
                        Bits[7:0] Protocol ID

[1]  https://developer.arm.com/documentation/den0056/e/?lang=en

So using "command_id" for a parameter name and "Protocol ID"
as a (part of) value is very much aligned with the specification.

That said, I will change "command_id" to "protocol_id" for now.

> >
> > > > +
> > > > +<flags>
> > > > +    Flags to control the action. See SCMI specification for
> > > > +    defined values.
> > >
> > > ?
> > >
> > > Please add the flags here, or at the very least provide a URL and page
> > > number, etc.
> >
> > I intentionally avoid providing details here because a set of flags
> > acceptable to a specific SCMI server may depend on the server
> > and its implementation version.
> > The interface on U-Boot is just a wrapper to make a call to SCMI server
> > via a transport layer and doesn't care what the parameters means.
> > That said, I agree to referring to a URL to SCMI specification somewhere
> > in this document.
> 
> That will help. But also please add that this is a hex value (right?)

Will do.

Thanks,
-Takahiro Akashi

> 
> >
> > Thanks,
> > -Takahiro Akashi
> >
> > > > +
> > > > +Example
> > > > +-------
> > > > +
> > > > +Obtain basic information about SCMI server:
> > > > +
> > > > +::
> > > > +
> > > > +    => scmi base 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 base perm_dev 1 0 1
> > > > +
> > > > +Reset configurations with all access permission settings retained:
> > > > +
> > > > +::
> > > > +
> > > > +    => scmi base 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.
> > > > --
> > > > 2.41.0
> > > >
> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

Hi Simon,

On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> Hi,
> 
> On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > >   $ ut dm scmi_base
> > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 112 insertions(+)
> > > >
> > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > index 881be3171b7c..563017bb63e0 100644
> > > > --- a/test/dm/scmi.c
> > > > +++ b/test/dm/scmi.c
> > > > @@ -16,6 +16,9 @@
> > > >  #include <clk.h>
> > > >  #include <dm.h>
> > > >  #include <reset.h>
> > > > +#include <scmi_agent.h>
> > > > +#include <scmi_agent-uclass.h>
> > > > +#include <scmi_protocols.h>
> > > >  #include <asm/scmi_test.h>
> > > >  #include <dm/device-internal.h>
> > > >  #include <dm/test.h>
> > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > >  }
> > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > >
> > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > +{
> > > > +       struct udevice *agent_dev, *base;
> > > > +       struct scmi_agent_priv *priv;
> > > > +       const struct scmi_base_ops *ops;
> > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > +       u32 attributes, agent_id;
> > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > +       u8 *protocols;
> > > > +       int ret;
> > > > +
> > > > +       /* preparation */
> > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > +                                             &agent_dev));
> > > > +       ut_assertnonnull(agent_dev);
> > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > +
> > > > +       /* version */
> > > > +       ret = (*ops->protocol_version)(base, &version);
> > >
> > > Can you add uclass helpers to call each of the methods? That is how it
> > > is commonly done. You should not be calling ops->xxx directly here.
> >
> > Yes, I will add inline functions instead.
> 
> I don't mean inline...see all the other uclasses which define a

Okay, I will *real* functions.

> function which is implemented in the uclass. It is confusing when one
> uclass does something different. People might copy this style and then
> the code base diverges. Did you not notice this when looking around
> the source tree?

But one concern came up in my mind.
Contrary to ordinary "device controllers", there exists only a single
implementation of driver for each of "udevice"'s associated with SCMI
protocols including the base protocol.

So if I follow your suggestion, the code (base.c) might look like:
===
static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
{
  ...
}

struct scmi_base_ops scmi_base_ops = {

  .base_discover_vendor = __scmi_base_discover_vendor,

}

int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
{
  struct scmi_base_ops *ops;

  ops = scmi_base_dev_ops(dev);

  return ops->base_discover_vendor(dev, vendor);
}
===

We will have to have similar definitions for every operation in ops.
It looks quite weird to me as there are always pairs of functions,
like __scmi_base_discover_vendor() and scmi_base_discover_vendor().

We can avoid this redundant code easily by eliminating "ops" abstraction.
But as far as I remember, you insist that every driver that complies
to U-Boot driver model should have a "ops".

What do you make of this?

Thanks
-Takahiro Akashi

> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[Simon Glass]

Hi,

On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > Hi,
> >
> > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > >   $ ut dm scmi_base
> > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 112 insertions(+)
> > > > >
> > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > --- a/test/dm/scmi.c
> > > > > +++ b/test/dm/scmi.c
> > > > > @@ -16,6 +16,9 @@
> > > > >  #include <clk.h>
> > > > >  #include <dm.h>
> > > > >  #include <reset.h>
> > > > > +#include <scmi_agent.h>
> > > > > +#include <scmi_agent-uclass.h>
> > > > > +#include <scmi_protocols.h>
> > > > >  #include <asm/scmi_test.h>
> > > > >  #include <dm/device-internal.h>
> > > > >  #include <dm/test.h>
> > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > >  }
> > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > >
> > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > +{
> > > > > +       struct udevice *agent_dev, *base;
> > > > > +       struct scmi_agent_priv *priv;
> > > > > +       const struct scmi_base_ops *ops;
> > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > +       u32 attributes, agent_id;
> > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > +       u8 *protocols;
> > > > > +       int ret;
> > > > > +
> > > > > +       /* preparation */
> > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > +                                             &agent_dev));
> > > > > +       ut_assertnonnull(agent_dev);
> > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > +
> > > > > +       /* version */
> > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > >
> > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > is commonly done. You should not be calling ops->xxx directly here.
> > >
> > > Yes, I will add inline functions instead.
> >
> > I don't mean inline...see all the other uclasses which define a
>
> Okay, I will *real* functions.
>
> > function which is implemented in the uclass. It is confusing when one
> > uclass does something different. People might copy this style and then
> > the code base diverges. Did you not notice this when looking around
> > the source tree?
>
> But one concern came up in my mind.
> Contrary to ordinary "device controllers", there exists only a single
> implementation of driver for each of "udevice"'s associated with SCMI
> protocols including the base protocol.
>
> So if I follow your suggestion, the code (base.c) might look like:
> ===
> static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> {
>   ...
> }
>
> struct scmi_base_ops scmi_base_ops = {
>
>   .base_discover_vendor = __scmi_base_discover_vendor,
>
> }
>
> int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> {
>   struct scmi_base_ops *ops;
>
>   ops = scmi_base_dev_ops(dev);
>
>   return ops->base_discover_vendor(dev, vendor);
> }
> ===
>
> We will have to have similar definitions for every operation in ops.
> It looks quite weird to me as there are always pairs of functions,
> like __scmi_base_discover_vendor() and scmi_base_discover_vendor().

Yes I understand that you only have one driver at present. Is there
not a sandbox driver?

>
> We can avoid this redundant code easily by eliminating "ops" abstraction.
> But as far as I remember, you insist that every driver that complies
> to U-Boot driver model should have a "ops".
>
> What do you make of this?

Well there are some exceptions, but yes that is the idea. Operations
should be in a 'ops' struct and documented and implemented in a
consistent way.

Regards,
Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[Simon Glass]

Hi,

On Tue, 4 Jul 2023 at 02:26, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> > Hi,
> >
> > On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > This command, "scmi", provides a command line interface to various SCMI
> > > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > > command, "base", and is intended mainly for debug purpose.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  cmd/Kconfig  |   8 ++
> > > > >  cmd/Makefile |   1 +
> > > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 382 insertions(+)
> > > > >  create mode 100644 cmd/scmi.c
> > > > >
> > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > index 02e54f1e50fe..065470a76295 100644
> > > > > --- a/cmd/Kconfig
> > > > > +++ b/cmd/Kconfig
> > > > > @@ -2504,6 +2504,14 @@ 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 protocols,
> > > > > +         including Base protocol.
> > > >
> > > > Please mention what is SCMI
> > >
> > > I will give a full spelling.
> > >
> > > > >  endmenu
> > > > >
> > > > >  menu "Filesystem commands"
> > > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > > --- a/cmd/Makefile
> > > > > +++ b/cmd/Makefile
> > > > > @@ -156,6 +156,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..c97f77e97d95
> > > > > --- /dev/null
> > > > > +++ b/cmd/scmi.c
> > > > > @@ -0,0 +1,373 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + *  SCMI utility command
> > > > > + *
> > > > > + *  Copyright (c) 2023 Linaro Limited
> > > > > + *             Author: AKASHI Takahiro
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <command.h>
> > > > > +#include <dm/device.h>
> > > > > +#include <dm/uclass.h> /* uclass_get_device */
> > > >
> > > > Goes before linux/bitfield.h
> > >
> > > Can you tell me why?
> >
> > It is just a convention:
> > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
>
> Okay.
>
> > >
> > > > > +#include <exports.h>
> > > > > +#include <scmi_agent.h>
> > > > > +#include <scmi_agent-uclass.h>
> > > > > +#include <asm/types.h>
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/bitops.h>
> > > > > +
> > > > > +static struct udevice *agent;
> > > > > +static struct udevice *base_proto;
> > > > > +static const struct scmi_base_ops *ops;
> > > > > +
> > > > > +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"},
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > > + *
> > > > > + * @id:                Protocol ID
> > > > > + *
> > > > > + * Get the printable name of the protocol, @id
> > > > > + *
> > > > > + * Return:     Name string on success, NULL on failure
> > > > > + */
> > > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > > >
> > > > scmi_lookup_id?
> > >
> > > Not sure your intention.
> > > The name above gives us exactly what this function does for pretty printing
> > > instead of a number.
> > > I think that 'lookup' implies we try to determine if the id exists or not.
> >
> > I am just trying to avoid the code turning into Java :-)
>
> Java?

Long names like stories

>
> > How about scmi_get_name() ?
>
> Well, more specifically scmi_get_protocol_name()?

Perhaps get_prot_name() since it is static? Anyway, please make it short-ish.

We really need some limits on identifier names.

Regards,
Simon

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[Simon Glass]

Hi,

On Tue, 4 Jul 2023 at 03:05, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, Jul 03, 2023 at 02:30:55PM +0100, Simon Glass wrote:
> > Hi ,
> >
> > On Mon, 3 Jul 2023 at 02:19, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > This is a help text for scmi command.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 98 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..20cdae4b877d
> > > > > --- /dev/null
> > > > > +++ b/doc/usage/cmd/scmi.rst
> > > > > @@ -0,0 +1,98 @@
> > > > > +.. SPDX-License-Identifier: GPL-2.0+:
> > > > > +
> > > > > +scmi command
> > > > > +============
> > > > > +
> > > > > +Synopsis
> > > > > +--------
> > > > > +
> > > > > +::
> > > > > +
> > > > > +    scmi base info
> > > > > +    scmi base perm_dev <agent id> <device id> <flags>
> > > > > +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> > > > > +    scmi base reset <agent id> <flags>
> > > > > +
> > > > > +Description
> > > > > +-----------
> > > > > +
> > > > > +The scmi command is used to access and operate on SCMI server.
> > > > > +
> > > > > +scmi base info
> > > > > +~~~~~~~~~~~~~~
> > > > > +    Show base information about SCMI server and supported protocols
> > > > > +
> > > > > +scmi base perm_dev
> > > > > +~~~~~~~~~~~~~~~~~~
> > > > > +    Allow or deny access permission to the device
> > > > > +
> > > > > +scmi base perm_proto
> > > > > +~~~~~~~~~~~~~~~~~~~~
> > > > > +    Allow or deny access to the protocol on the device
> > > > > +
> > > > > +scmi base reset
> > > > > +~~~~~~~~~~~~~~~
> > > > > +    Reset the existing configurations
> > > > > +
> > > > > +Parameters are used as follows:
> > > > > +
> > > > > +<agent id>
> > > > > +    Agent ID
> > > >
> > > > what is this?
> > >
> > > I thought that the meaning was trivial in SCMI context.
> > > Will change it to "SCMI Agent ID".
> >
> > What is SCMI? Really you should explain and link to information about
> > things you mention in documentation. How else is anyone supposed to
>
> Generally yes, but please note that SCMI and related drivers are already
> there in U-Boot. For instance, see drivers/firmware/scmi/Kconfig.
> I don't think we need any more explanation about what is SCMI everywhere
> the word, "SCMI", appears.
>
> > figure out what you are talking about?
>
> Again, those who don't know about SCMI and if SCMI server is installed
> on their systems will never use this command.
>
> >
> > "SCMI Agent ID" doesn't tell us much. What form does it take? Is it a
> > string? How do you find it out?
>
> While I still believe that What SCMI agent ID means is trivial for
> those who read the SCMI specification, I will give a short description
> about possible values.
>
> > >
> > >
> > > > > +
> > > > > +<device id>
> > > > > +    Device ID
> > > >
> > > > what is this?
> > >
> > > Again, will change it to "SCMI Device ID".
> >
> > That doesn't help much. The purpose of documentation is to explain
> > things for people who don't already know it. We need to try to be as
> > helpful as possible.
>
> The same above.
> Please note that the definition of "device (ID)", except its data type,
> is out of scope of SCMI specification.
> It doesn't describe any means by which values be identified/retrieved.
>
> > >
> > > > > +
> > > > > +<command id>
> > > > > +    Protocol ID, should not be 0x10 (base protocol)
> > > >
> > > > what is this? Please add more detail
> > >
> > > Again, will change it to "SCMI Protocol ID".
> > > I think that users should be familiar with those terms
> > > if they want to use these interfaces.
> >
> > Maybe, but it still isn't clear what it is. A string?
>
> Will add a short description about its data type/format.
>
> > It is confusing
> > that the command ID is also a protocol ID.
>
> Yes, I know, but the confusion exists in SCMI specification.
> It sometimes seems to use both words almost interchangeably, even
> in a single context. For instance, the section 4.2.2.11 of [1]
> says,
>
> 4.2.2.11 BASE_SET_PROTOCOL_PERMISSIONS
>  ...
>  This command BASE_SET_PROTOCOL_PERMISSIONS is used to ...
>  (This "command" has a yet another meaning.)
>
> Parameters
> uint32_t command_id     Bits[31:8] Reserved, must be zero
>                         Bits[7:0] Protocol ID
>
> [1]  https://developer.arm.com/documentation/den0056/e/?lang=en
>
> So using "command_id" for a parameter name and "Protocol ID"
> as a (part of) value is very much aligned with the specification.
>
> That said, I will change "command_id" to "protocol_id" for now.
>
> > >
> > > > > +
> > > > > +<flags>
> > > > > +    Flags to control the action. See SCMI specification for
> > > > > +    defined values.
> > > >
> > > > ?
> > > >
> > > > Please add the flags here, or at the very least provide a URL and page
> > > > number, etc.
> > >
> > > I intentionally avoid providing details here because a set of flags
> > > acceptable to a specific SCMI server may depend on the server
> > > and its implementation version.
> > > The interface on U-Boot is just a wrapper to make a call to SCMI server
> > > via a transport layer and doesn't care what the parameters means.
> > > That said, I agree to referring to a URL to SCMI specification somewhere
> > > in this document.
> >
> > That will help. But also please add that this is a hex value (right?)
>
> Will do.

OK, well do what you can. Documentation which assumes a lot of new
knowledge is not very useful. Linking to docs is good but it still
helps to spell out acronyms at least once in each place, and add a
little summary about what SCMI is for. Linking to docs can be a pain
when the links disappear, also.

Regards,
Simon

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

On Fri, Jul 07, 2023 at 11:35:52AM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 4 Jul 2023 at 02:26, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> > > Hi,
> > >
> > > On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > > > Hi AKASHI,
> > > > >
> > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > This command, "scmi", provides a command line interface to various SCMI
> > > > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > > > command, "base", and is intended mainly for debug purpose.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > ---
> > > > > >  cmd/Kconfig  |   8 ++
> > > > > >  cmd/Makefile |   1 +
> > > > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 382 insertions(+)
> > > > > >  create mode 100644 cmd/scmi.c
> > > > > >
> > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > > index 02e54f1e50fe..065470a76295 100644
> > > > > > --- a/cmd/Kconfig
> > > > > > +++ b/cmd/Kconfig
> > > > > > @@ -2504,6 +2504,14 @@ 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 protocols,
> > > > > > +         including Base protocol.
> > > > >
> > > > > Please mention what is SCMI
> > > >
> > > > I will give a full spelling.
> > > >
> > > > > >  endmenu
> > > > > >
> > > > > >  menu "Filesystem commands"
> > > > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > > > --- a/cmd/Makefile
> > > > > > +++ b/cmd/Makefile
> > > > > > @@ -156,6 +156,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..c97f77e97d95
> > > > > > --- /dev/null
> > > > > > +++ b/cmd/scmi.c
> > > > > > @@ -0,0 +1,373 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + *  SCMI utility command
> > > > > > + *
> > > > > > + *  Copyright (c) 2023 Linaro Limited
> > > > > > + *             Author: AKASHI Takahiro
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <command.h>
> > > > > > +#include <dm/device.h>
> > > > > > +#include <dm/uclass.h> /* uclass_get_device */
> > > > >
> > > > > Goes before linux/bitfield.h
> > > >
> > > > Can you tell me why?
> > >
> > > It is just a convention:
> > > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
> >
> > Okay.
> >
> > > >
> > > > > > +#include <exports.h>
> > > > > > +#include <scmi_agent.h>
> > > > > > +#include <scmi_agent-uclass.h>
> > > > > > +#include <asm/types.h>
> > > > > > +#include <linux/bitfield.h>
> > > > > > +#include <linux/bitops.h>
> > > > > > +
> > > > > > +static struct udevice *agent;
> > > > > > +static struct udevice *base_proto;
> > > > > > +static const struct scmi_base_ops *ops;
> > > > > > +
> > > > > > +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"},
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > > > + *
> > > > > > + * @id:                Protocol ID
> > > > > > + *
> > > > > > + * Get the printable name of the protocol, @id
> > > > > > + *
> > > > > > + * Return:     Name string on success, NULL on failure
> > > > > > + */
> > > > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > > > >
> > > > > scmi_lookup_id?
> > > >
> > > > Not sure your intention.
> > > > The name above gives us exactly what this function does for pretty printing
> > > > instead of a number.
> > > > I think that 'lookup' implies we try to determine if the id exists or not.
> > >
> > > I am just trying to avoid the code turning into Java :-)
> >
> > Java?
> 
> Long names like stories

Well, recent software specification likes long names anyway.
(i.e. UEFI)

> >
> > > How about scmi_get_name() ?
> >
> > Well, more specifically scmi_get_protocol_name()?
> 
> Perhaps get_prot_name() since it is static? Anyway, please make it short-ish.

Okay as far as this function is contained in this file.

-Takahiro Akashi


> We really need some limits on identifier names.
> 
> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > > Hi,
> > >
> > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > > Hi AKASHI,
> > > > >
> > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > > >   $ ut dm scmi_base
> > > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > ---
> > > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 112 insertions(+)
> > > > > >
> > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > > --- a/test/dm/scmi.c
> > > > > > +++ b/test/dm/scmi.c
> > > > > > @@ -16,6 +16,9 @@
> > > > > >  #include <clk.h>
> > > > > >  #include <dm.h>
> > > > > >  #include <reset.h>
> > > > > > +#include <scmi_agent.h>
> > > > > > +#include <scmi_agent-uclass.h>
> > > > > > +#include <scmi_protocols.h>
> > > > > >  #include <asm/scmi_test.h>
> > > > > >  #include <dm/device-internal.h>
> > > > > >  #include <dm/test.h>
> > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > > >  }
> > > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > > >
> > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > > +{
> > > > > > +       struct udevice *agent_dev, *base;
> > > > > > +       struct scmi_agent_priv *priv;
> > > > > > +       const struct scmi_base_ops *ops;
> > > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > > +       u32 attributes, agent_id;
> > > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > > +       u8 *protocols;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       /* preparation */
> > > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > > +                                             &agent_dev));
> > > > > > +       ut_assertnonnull(agent_dev);
> > > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > > +
> > > > > > +       /* version */
> > > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > > >
> > > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > > is commonly done. You should not be calling ops->xxx directly here.
> > > >
> > > > Yes, I will add inline functions instead.
> > >
> > > I don't mean inline...see all the other uclasses which define a
> >
> > Okay, I will *real* functions.
> >
> > > function which is implemented in the uclass. It is confusing when one
> > > uclass does something different. People might copy this style and then
> > > the code base diverges. Did you not notice this when looking around
> > > the source tree?
> >
> > But one concern came up in my mind.
> > Contrary to ordinary "device controllers", there exists only a single
> > implementation of driver for each of "udevice"'s associated with SCMI
> > protocols including the base protocol.
> >
> > So if I follow your suggestion, the code (base.c) might look like:
> > ===
> > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > {
> >   ...
> > }
> >
> > struct scmi_base_ops scmi_base_ops = {
> >
> >   .base_discover_vendor = __scmi_base_discover_vendor,
> >
> > }
> >
> > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > {
> >   struct scmi_base_ops *ops;
> >
> >   ops = scmi_base_dev_ops(dev);
> >
> >   return ops->base_discover_vendor(dev, vendor);
> > }
> > ===
> >
> > We will have to have similar definitions for every operation in ops.
> > It looks quite weird to me as there are always pairs of functions,
> > like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
> 
> Yes I understand that you only have one driver at present. Is there
> not a sandbox driver?

No.
Please remember that SCMI protocol drivers on U-Boot are nothing but
stubs that makes a call to SCMI servers, supporting common communication
channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).

Sandbox driver, if is properly named, is also implemented as a sort of
transport layer, where a invocation is replaced with a function call which
mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.

In this sense, there will exist only a single driver under the current
form of framework forever.

> 
> >
> > We can avoid this redundant code easily by eliminating "ops" abstraction.
> > But as far as I remember, you insist that every driver that complies
> > to U-Boot driver model should have a "ops".
> >
> > What do you make of this?
> 
> Well there are some exceptions, but yes that is the idea. Operations
> should be in a 'ops' struct and documented and implemented in a
> consistent way.

Is it your choice that I should keep "ops" structure in this specific
implementation?

-Takahiro Akashi

> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[Simon Glass]

Hi,

On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > > > Hi AKASHI,
> > > > > >
> > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > >
> > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > > > >   $ ut dm scmi_base
> > > > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > > > >
> > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > > ---
> > > > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 112 insertions(+)
> > > > > > >
> > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > > > --- a/test/dm/scmi.c
> > > > > > > +++ b/test/dm/scmi.c
> > > > > > > @@ -16,6 +16,9 @@
> > > > > > >  #include <clk.h>
> > > > > > >  #include <dm.h>
> > > > > > >  #include <reset.h>
> > > > > > > +#include <scmi_agent.h>
> > > > > > > +#include <scmi_agent-uclass.h>
> > > > > > > +#include <scmi_protocols.h>
> > > > > > >  #include <asm/scmi_test.h>
> > > > > > >  #include <dm/device-internal.h>
> > > > > > >  #include <dm/test.h>
> > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > > > >  }
> > > > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > > > >
> > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > > > +{
> > > > > > > +       struct udevice *agent_dev, *base;
> > > > > > > +       struct scmi_agent_priv *priv;
> > > > > > > +       const struct scmi_base_ops *ops;
> > > > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > > > +       u32 attributes, agent_id;
> > > > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > > > +       u8 *protocols;
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       /* preparation */
> > > > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > > > +                                             &agent_dev));
> > > > > > > +       ut_assertnonnull(agent_dev);
> > > > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > > > +
> > > > > > > +       /* version */
> > > > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > > > >
> > > > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > > > is commonly done. You should not be calling ops->xxx directly here.
> > > > >
> > > > > Yes, I will add inline functions instead.
> > > >
> > > > I don't mean inline...see all the other uclasses which define a
> > >
> > > Okay, I will *real* functions.
> > >
> > > > function which is implemented in the uclass. It is confusing when one
> > > > uclass does something different. People might copy this style and then
> > > > the code base diverges. Did you not notice this when looking around
> > > > the source tree?
> > >
> > > But one concern came up in my mind.
> > > Contrary to ordinary "device controllers", there exists only a single
> > > implementation of driver for each of "udevice"'s associated with SCMI
> > > protocols including the base protocol.
> > >
> > > So if I follow your suggestion, the code (base.c) might look like:
> > > ===
> > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > {
> > >   ...
> > > }
> > >
> > > struct scmi_base_ops scmi_base_ops = {
> > >
> > >   .base_discover_vendor = __scmi_base_discover_vendor,
> > >
> > > }
> > >
> > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > {
> > >   struct scmi_base_ops *ops;
> > >
> > >   ops = scmi_base_dev_ops(dev);
> > >
> > >   return ops->base_discover_vendor(dev, vendor);
> > > }
> > > ===
> > >
> > > We will have to have similar definitions for every operation in ops.
> > > It looks quite weird to me as there are always pairs of functions,
> > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
> >
> > Yes I understand that you only have one driver at present. Is there
> > not a sandbox driver?
>
> No.
> Please remember that SCMI protocol drivers on U-Boot are nothing but
> stubs that makes a call to SCMI servers, supporting common communication
> channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).
>
> Sandbox driver, if is properly named, is also implemented as a sort of
> transport layer, where a invocation is replaced with a function call which
> mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.
>
> In this sense, there will exist only a single driver under the current
> form of framework forever.

OK, so driver model is used for the transport but not the top-level
driver? I see.

>
> >
> > >
> > > We can avoid this redundant code easily by eliminating "ops" abstraction.
> > > But as far as I remember, you insist that every driver that complies
> > > to U-Boot driver model should have a "ops".
> > >
> > > What do you make of this?
> >
> > Well there are some exceptions, but yes that is the idea. Operations
> > should be in a 'ops' struct and documented and implemented in a
> > consistent way.
>
> Is it your choice that I should keep "ops" structure in this specific
> implementation?

I can't actually find this patch on patchwork.

But yes, you do need a function for each ops call. They should be used
in the tests, which should not directly call functions using
ops->xxx()

Regards,
Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

Hi Simon,

On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote:
> Hi,
> 
> On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > > > > Hi AKASHI,
> > > > > > >
> > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > > > > >   $ ut dm scmi_base
> > > > > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > > > > >
> > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > > > ---
> > > > > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 112 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > > > > --- a/test/dm/scmi.c
> > > > > > > > +++ b/test/dm/scmi.c
> > > > > > > > @@ -16,6 +16,9 @@
> > > > > > > >  #include <clk.h>
> > > > > > > >  #include <dm.h>
> > > > > > > >  #include <reset.h>
> > > > > > > > +#include <scmi_agent.h>
> > > > > > > > +#include <scmi_agent-uclass.h>
> > > > > > > > +#include <scmi_protocols.h>
> > > > > > > >  #include <asm/scmi_test.h>
> > > > > > > >  #include <dm/device-internal.h>
> > > > > > > >  #include <dm/test.h>
> > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > > > > >  }
> > > > > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > > > > >
> > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > > > > +{
> > > > > > > > +       struct udevice *agent_dev, *base;
> > > > > > > > +       struct scmi_agent_priv *priv;
> > > > > > > > +       const struct scmi_base_ops *ops;
> > > > > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > > > > +       u32 attributes, agent_id;
> > > > > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > > > > +       u8 *protocols;
> > > > > > > > +       int ret;
> > > > > > > > +
> > > > > > > > +       /* preparation */
> > > > > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > > > > +                                             &agent_dev));
> > > > > > > > +       ut_assertnonnull(agent_dev);
> > > > > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > > > > +
> > > > > > > > +       /* version */
> > > > > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > > > > >
> > > > > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > > > > is commonly done. You should not be calling ops->xxx directly here.
> > > > > >
> > > > > > Yes, I will add inline functions instead.
> > > > >
> > > > > I don't mean inline...see all the other uclasses which define a
> > > >
> > > > Okay, I will *real* functions.
> > > >
> > > > > function which is implemented in the uclass. It is confusing when one
> > > > > uclass does something different. People might copy this style and then
> > > > > the code base diverges. Did you not notice this when looking around
> > > > > the source tree?
> > > >
> > > > But one concern came up in my mind.
> > > > Contrary to ordinary "device controllers", there exists only a single
> > > > implementation of driver for each of "udevice"'s associated with SCMI
> > > > protocols including the base protocol.
> > > >
> > > > So if I follow your suggestion, the code (base.c) might look like:
> > > > ===
> > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > {
> > > >   ...
> > > > }
> > > >
> > > > struct scmi_base_ops scmi_base_ops = {
> > > >
> > > >   .base_discover_vendor = __scmi_base_discover_vendor,
> > > >
> > > > }
> > > >
> > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > {
> > > >   struct scmi_base_ops *ops;
> > > >
> > > >   ops = scmi_base_dev_ops(dev);
> > > >
> > > >   return ops->base_discover_vendor(dev, vendor);
> > > > }
> > > > ===
> > > >
> > > > We will have to have similar definitions for every operation in ops.
> > > > It looks quite weird to me as there are always pairs of functions,
> > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
> > >
> > > Yes I understand that you only have one driver at present. Is there
> > > not a sandbox driver?
> >
> > No.
> > Please remember that SCMI protocol drivers on U-Boot are nothing but
> > stubs that makes a call to SCMI servers, supporting common communication
> > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).
> >
> > Sandbox driver, if is properly named, is also implemented as a sort of
> > transport layer, where a invocation is replaced with a function call which
> > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.
> >
> > In this sense, there will exist only a single driver under the current
> > form of framework forever.
> 
> OK, so driver model is used for the transport but not the top-level
> driver? I see.

I'm not sure if you fully understand.
Yes, transports, or interchangeably named as a channel, are modeled
as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c
(Their names, *_agent, are misleading in my opinion as their functionality
is purely a transport method to SCMI server. An agent means, in SCMI jargon,
a user or client of SCMI server.)

On top of that, each SCMI protocol, the base protocol in my patch set,
is also modeled as a U-Boot device.
You can see another example, say, in drivers/firmware/clk/clk_scmi.c.

Since there is no corresponding uclass for the base protocol, I create
a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete
device object.

> >
> > >
> > > >
> > > > We can avoid this redundant code easily by eliminating "ops" abstraction.
> > > > But as far as I remember, you insist that every driver that complies
> > > > to U-Boot driver model should have a "ops".
> > > >
> > > > What do you make of this?
> > >
> > > Well there are some exceptions, but yes that is the idea. Operations
> > > should be in a 'ops' struct and documented and implemented in a
> > > consistent way.
> >
> > Is it your choice that I should keep "ops" structure in this specific
> > implementation?
> 
> I can't actually find this patch on patchwork.

Indeed (why?), but you have already seen it.
https://lists.denx.de/pipermail/u-boot/2023-June/521288.html

> But yes, you do need a function for each ops call. They should be used
> in the tests, which should not directly call functions using
> ops->xxx()

Even though there is no practical benefit to do so. Right?

-Takahiro Akashi

> 
> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

Hi Simon,

On Tue, Jul 11, 2023 at 12:41:58PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Mon, 10 Jul 2023 at 19:02, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > > > > > > Hi AKASHI,
> > > > > > > > >
> > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > > > > > > >   $ ut dm scmi_base
> > > > > > > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  1 file changed, 112 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > > > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > > > > > > --- a/test/dm/scmi.c
> > > > > > > > > > +++ b/test/dm/scmi.c
> > > > > > > > > > @@ -16,6 +16,9 @@
> > > > > > > > > >  #include <clk.h>
> > > > > > > > > >  #include <dm.h>
> > > > > > > > > >  #include <reset.h>
> > > > > > > > > > +#include <scmi_agent.h>
> > > > > > > > > > +#include <scmi_agent-uclass.h>
> > > > > > > > > > +#include <scmi_protocols.h>
> > > > > > > > > >  #include <asm/scmi_test.h>
> > > > > > > > > >  #include <dm/device-internal.h>
> > > > > > > > > >  #include <dm/test.h>
> > > > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > > > > > > >  }
> > > > > > > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > > > > > > >
> > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > > > > > > +{
> > > > > > > > > > +       struct udevice *agent_dev, *base;
> > > > > > > > > > +       struct scmi_agent_priv *priv;
> > > > > > > > > > +       const struct scmi_base_ops *ops;
> > > > > > > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > > > > > > +       u32 attributes, agent_id;
> > > > > > > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > > > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > > > > > > +       u8 *protocols;
> > > > > > > > > > +       int ret;
> > > > > > > > > > +
> > > > > > > > > > +       /* preparation */
> > > > > > > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > > > > > > +                                             &agent_dev));
> > > > > > > > > > +       ut_assertnonnull(agent_dev);
> > > > > > > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > > > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > > > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > > > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > > > > > > +
> > > > > > > > > > +       /* version */
> > > > > > > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > > > > > > >
> > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > > > > > > is commonly done. You should not be calling ops->xxx directly here.
> > > > > > > >
> > > > > > > > Yes, I will add inline functions instead.
> > > > > > >
> > > > > > > I don't mean inline...see all the other uclasses which define a
> > > > > >
> > > > > > Okay, I will *real* functions.
> > > > > >
> > > > > > > function which is implemented in the uclass. It is confusing when one
> > > > > > > uclass does something different. People might copy this style and then
> > > > > > > the code base diverges. Did you not notice this when looking around
> > > > > > > the source tree?
> > > > > >
> > > > > > But one concern came up in my mind.
> > > > > > Contrary to ordinary "device controllers", there exists only a single
> > > > > > implementation of driver for each of "udevice"'s associated with SCMI
> > > > > > protocols including the base protocol.
> > > > > >
> > > > > > So if I follow your suggestion, the code (base.c) might look like:
> > > > > > ===
> > > > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > > > {
> > > > > >   ...
> > > > > > }
> > > > > >
> > > > > > struct scmi_base_ops scmi_base_ops = {
> > > > > >
> > > > > >   .base_discover_vendor = __scmi_base_discover_vendor,
> > > > > >
> > > > > > }
> > > > > >
> > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > > > {
> > > > > >   struct scmi_base_ops *ops;
> > > > > >
> > > > > >   ops = scmi_base_dev_ops(dev);
> > > > > >
> > > > > >   return ops->base_discover_vendor(dev, vendor);
> > > > > > }
> > > > > > ===
> > > > > >
> > > > > > We will have to have similar definitions for every operation in ops.
> > > > > > It looks quite weird to me as there are always pairs of functions,
> > > > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
> > > > >
> > > > > Yes I understand that you only have one driver at present. Is there
> > > > > not a sandbox driver?
> > > >
> > > > No.
> > > > Please remember that SCMI protocol drivers on U-Boot are nothing but
> > > > stubs that makes a call to SCMI servers, supporting common communication
> > > > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).
> > > >
> > > > Sandbox driver, if is properly named, is also implemented as a sort of
> > > > transport layer, where a invocation is replaced with a function call which
> > > > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.
> > > >
> > > > In this sense, there will exist only a single driver under the current
> > > > form of framework forever.
> > >
> > > OK, so driver model is used for the transport but not the top-level
> > > driver? I see.
> >
> > I'm not sure if you fully understand.
> > Yes, transports, or interchangeably named as a channel, are modeled
> > as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c
> > (Their names, *_agent, are misleading in my opinion as their functionality
> > is purely a transport method to SCMI server. An agent means, in SCMI jargon,
> > a user or client of SCMI server.)
> >
> > On top of that, each SCMI protocol, the base protocol in my patch set,
> > is also modeled as a U-Boot device.
> > You can see another example, say, in drivers/firmware/clk/clk_scmi.c.
> >
> > Since there is no corresponding uclass for the base protocol, I create
> > a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete
> > device object.
> 
> It doesn't need to be 'concrete', whatever that means. A uclass is a
> model of hardware or something else. We have lots of drivers that
> don't deal with a hardware device.

Yes, I know.

> >
> > > >
> > > > >
> > > > > >
> > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction.
> > > > > > But as far as I remember, you insist that every driver that complies
> > > > > > to U-Boot driver model should have a "ops".
> > > > > >
> > > > > > What do you make of this?
> > > > >
> > > > > Well there are some exceptions, but yes that is the idea. Operations
> > > > > should be in a 'ops' struct and documented and implemented in a
> > > > > consistent way.
> > > >
> > > > Is it your choice that I should keep "ops" structure in this specific
> > > > implementation?
> > >
> > > I can't actually find this patch on patchwork.
> >
> > Indeed (why?), but you have already seen it.
> > https://lists.denx.de/pipermail/u-boot/2023-June/521288.html
> 
> I mean patchwork.

Yes, I know.
But even a bunch of patches listed in patchwork are left "new" state forever.

> >
> > > But yes, you do need a function for each ops call. They should be used
> > > in the tests, which should not directly call functions using
> > > ops->xxx()
> >
> > Even though there is no practical benefit to do so. Right?
> 
> I don't have a useful answer to that question. If you want to
> contribute to U-Boot, please follow its conventions. It is just
> frustrating for people doing code review, with limited time.

I simply wanted to double-check the rule since you mention,
in another thread, that there are some exceptions.

> While xxx may be unique in U-Boot, I very much doubt it always is, so
> using 'ops' is not helping people looking through the code, nor is it
> obvious what is happening without looking up 'ops'. The helper
> functions should be used in all cases.
> 
> The thing is, this convention has been in place since the start of
> driver model, so I wonder how you have missed it?

Well, probably I was confused with many uses of function pointers
for boot and runtime services in UEFI code.

That said, after rethinking, I decided to remove udevice-related
code from my patch here, i.e. there will be no U-Boot device
for the base protocol as it is a protocol not a concrete device.
Its sole purpose is to get *meta* information about SCMI server
and the services, then manipulate them.

The analogy is that HTTP or FTP is a protocol over an ethernet
device to access a remote application (server), but is not a
device object in any sense.

> Please also fix the other uclasses, as I see for example that
> devm_scmi_process_msg() calls ops->process_msg() when it should have
> an exported function in that file.

I'm not sure that this is the case. devm_scmi_process_mesg()
is just a wrapper, as the name suggests, to ops->process_msg()
like other uclass driver operations, say blk_read/blk_write().
The only difference, if any, "ops" doesn't derive from the device
itself, but it parent (or grand^* parent).

If you really want, I will fix it (but in a separate patch).

-Takahiro Akashi


> Every uclass should have its
> operations in a struct, clearly documented, as well as helper
> functions to call each operation.
> 
> Regards,
> Simon

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[Simon Glass]

Hi,

On Thu, 13 Jul 2023 at 18:42, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Jul 11, 2023 at 12:41:58PM -0600, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Mon, 10 Jul 2023 at 19:02, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > > > > > > > > > Hi AKASHI,
> > > > > > > > > >
> > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > > > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > > > > > > > > >   $ ut dm scmi_base
> > > > > > > > > > > It is assumed that test.dtb is used as sandbox's device tree.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  1 file changed, 112 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > > > > > > > > > index 881be3171b7c..563017bb63e0 100644
> > > > > > > > > > > --- a/test/dm/scmi.c
> > > > > > > > > > > +++ b/test/dm/scmi.c
> > > > > > > > > > > @@ -16,6 +16,9 @@
> > > > > > > > > > >  #include <clk.h>
> > > > > > > > > > >  #include <dm.h>
> > > > > > > > > > >  #include <reset.h>
> > > > > > > > > > > +#include <scmi_agent.h>
> > > > > > > > > > > +#include <scmi_agent-uclass.h>
> > > > > > > > > > > +#include <scmi_protocols.h>
> > > > > > > > > > >  #include <asm/scmi_test.h>
> > > > > > > > > > >  #include <dm/device-internal.h>
> > > > > > > > > > >  #include <dm/test.h>
> > > > > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts)
> > > > > > > > > > >  }
> > > > > > > > > > >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > > > > > > > > > >
> > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > > > > > > > > > +{
> > > > > > > > > > > +       struct udevice *agent_dev, *base;
> > > > > > > > > > > +       struct scmi_agent_priv *priv;
> > > > > > > > > > > +       const struct scmi_base_ops *ops;
> > > > > > > > > > > +       u32 version, num_agents, num_protocols, impl_version;
> > > > > > > > > > > +       u32 attributes, agent_id;
> > > > > > > > > > > +       char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > > > > > > > > > +            agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > > > > > > > > > +       u8 *protocols;
> > > > > > > > > > > +       int ret;
> > > > > > > > > > > +
> > > > > > > > > > > +       /* preparation */
> > > > > > > > > > > +       ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > > > > > > > > > +                                             &agent_dev));
> > > > > > > > > > > +       ut_assertnonnull(agent_dev);
> > > > > > > > > > > +       ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > > > > > > > > > +       ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > > > > > > > > > +                                                 SCMI_PROTOCOL_ID_BASE));
> > > > > > > > > > > +       ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > > > > > > > > > +
> > > > > > > > > > > +       /* version */
> > > > > > > > > > > +       ret = (*ops->protocol_version)(base, &version);
> > > > > > > > > >
> > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it
> > > > > > > > > > is commonly done. You should not be calling ops->xxx directly here.
> > > > > > > > >
> > > > > > > > > Yes, I will add inline functions instead.
> > > > > > > >
> > > > > > > > I don't mean inline...see all the other uclasses which define a
> > > > > > >
> > > > > > > Okay, I will *real* functions.
> > > > > > >
> > > > > > > > function which is implemented in the uclass. It is confusing when one
> > > > > > > > uclass does something different. People might copy this style and then
> > > > > > > > the code base diverges. Did you not notice this when looking around
> > > > > > > > the source tree?
> > > > > > >
> > > > > > > But one concern came up in my mind.
> > > > > > > Contrary to ordinary "device controllers", there exists only a single
> > > > > > > implementation of driver for each of "udevice"'s associated with SCMI
> > > > > > > protocols including the base protocol.
> > > > > > >
> > > > > > > So if I follow your suggestion, the code (base.c) might look like:
> > > > > > > ===
> > > > > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > > > > {
> > > > > > >   ...
> > > > > > > }
> > > > > > >
> > > > > > > struct scmi_base_ops scmi_base_ops = {
> > > > > > >
> > > > > > >   .base_discover_vendor = __scmi_base_discover_vendor,
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor)
> > > > > > > {
> > > > > > >   struct scmi_base_ops *ops;
> > > > > > >
> > > > > > >   ops = scmi_base_dev_ops(dev);
> > > > > > >
> > > > > > >   return ops->base_discover_vendor(dev, vendor);
> > > > > > > }
> > > > > > > ===
> > > > > > >
> > > > > > > We will have to have similar definitions for every operation in ops.
> > > > > > > It looks quite weird to me as there are always pairs of functions,
> > > > > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
> > > > > >
> > > > > > Yes I understand that you only have one driver at present. Is there
> > > > > > not a sandbox driver?
> > > > >
> > > > > No.
> > > > > Please remember that SCMI protocol drivers on U-Boot are nothing but
> > > > > stubs that makes a call to SCMI servers, supporting common communication
> > > > > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).
> > > > >
> > > > > Sandbox driver, if is properly named, is also implemented as a sort of
> > > > > transport layer, where a invocation is replaced with a function call which
> > > > > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.
> > > > >
> > > > > In this sense, there will exist only a single driver under the current
> > > > > form of framework forever.
> > > >
> > > > OK, so driver model is used for the transport but not the top-level
> > > > driver? I see.
> > >
> > > I'm not sure if you fully understand.
> > > Yes, transports, or interchangeably named as a channel, are modeled
> > > as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c
> > > (Their names, *_agent, are misleading in my opinion as their functionality
> > > is purely a transport method to SCMI server. An agent means, in SCMI jargon,
> > > a user or client of SCMI server.)
> > >
> > > On top of that, each SCMI protocol, the base protocol in my patch set,
> > > is also modeled as a U-Boot device.
> > > You can see another example, say, in drivers/firmware/clk/clk_scmi.c.
> > >
> > > Since there is no corresponding uclass for the base protocol, I create
> > > a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete
> > > device object.
> >
> > It doesn't need to be 'concrete', whatever that means. A uclass is a
> > model of hardware or something else. We have lots of drivers that
> > don't deal with a hardware device.
>
> Yes, I know.
>
> > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction.
> > > > > > > But as far as I remember, you insist that every driver that complies
> > > > > > > to U-Boot driver model should have a "ops".
> > > > > > >
> > > > > > > What do you make of this?
> > > > > >
> > > > > > Well there are some exceptions, but yes that is the idea. Operations
> > > > > > should be in a 'ops' struct and documented and implemented in a
> > > > > > consistent way.
> > > > >
> > > > > Is it your choice that I should keep "ops" structure in this specific
> > > > > implementation?
> > > >
> > > > I can't actually find this patch on patchwork.
> > >
> > > Indeed (why?), but you have already seen it.
> > > https://lists.denx.de/pipermail/u-boot/2023-June/521288.html
> >
> > I mean patchwork.
>
> Yes, I know.
> But even a bunch of patches listed in patchwork are left "new" state forever.
>
> > >
> > > > But yes, you do need a function for each ops call. They should be used
> > > > in the tests, which should not directly call functions using
> > > > ops->xxx()
> > >
> > > Even though there is no practical benefit to do so. Right?
> >
> > I don't have a useful answer to that question. If you want to
> > contribute to U-Boot, please follow its conventions. It is just
> > frustrating for people doing code review, with limited time.
>
> I simply wanted to double-check the rule since you mention,
> in another thread, that there are some exceptions.
>
> > While xxx may be unique in U-Boot, I very much doubt it always is, so
> > using 'ops' is not helping people looking through the code, nor is it
> > obvious what is happening without looking up 'ops'. The helper
> > functions should be used in all cases.
> >
> > The thing is, this convention has been in place since the start of
> > driver model, so I wonder how you have missed it?
>
> Well, probably I was confused with many uses of function pointers
> for boot and runtime services in UEFI code.

Perhaps, but you mustn't add to the confusion with more counter-examples.

>
> That said, after rethinking, I decided to remove udevice-related
> code from my patch here, i.e. there will be no U-Boot device
> for the base protocol as it is a protocol not a concrete device.
> Its sole purpose is to get *meta* information about SCMI server
> and the services, then manipulate them.
>
> The analogy is that HTTP or FTP is a protocol over an ethernet
> device to access a remote application (server), but is not a
> device object in any sense.

Please don't. I don't know what you are referring to with a concrete
device, but it isn't relevant to driver model. It has lots of virtual
things in it.

>
> > Please also fix the other uclasses, as I see for example that
> > devm_scmi_process_msg() calls ops->process_msg() when it should have
> > an exported function in that file.
>
> I'm not sure that this is the case. devm_scmi_process_mesg()
> is just a wrapper, as the name suggests, to ops->process_msg()
> like other uclass driver operations, say blk_read/blk_write().
> The only difference, if any, "ops" doesn't derive from the device
> itself, but it parent (or grand^* parent).
>
> If you really want, I will fix it (but in a separate patch).

Then call the function on the parent device if you like. See how this
works in the pmic subsystem, for example.

>
> -Takahiro Akashi
>
>
> > Every uclass should have its
> > operations in a struct, clearly documented, as well as helper
> > functions to call each operation.
> >
> > Regards,
> > Simon

Regards,
Simon