[PATCH 00/10] firmware: scmi: add SCMI base protocol support
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
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); };
/*
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 @@
*/
+#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
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 @@
*/
+#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
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 *
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 */
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>;
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
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:
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
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];
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
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;
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
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
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
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
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
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
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
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
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);
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_FIRMWAREdefault nhelpThis 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
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_FIRMWAREdefault nhelpThis 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
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_FIRMWAREdefault nhelpThis 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-fil...
+#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
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_FIRMWAREdefault nhelpThis 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-fil...
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
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_FIRMWAREdefault nhelpThis 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-fil...
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
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_FIRMWAREdefault nhelpThis 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-fil...
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
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.
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: 30: platform> 1: OSPM2: PSCI# of protocols: 4Power domain managementPerformance domain managementClock managementSensor managementvendor: Linarosub vendor: PMWGimpl 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
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: 30: platform> 1: OSPM2: PSCI# of protocols: 4Power domain managementPerformance domain managementClock managementSensor managementvendor: Linarosub vendor: PMWGimpl 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
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: 30: platform> 1: OSPM2: PSCI# of protocols: 4Power domain managementPerformance domain managementClock managementSensor managementvendor: Linarosub vendor: PMWGimpl 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
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: 30: platform> 1: OSPM2: PSCI# of protocols: 4Power domain managementPerformance domain managementClock managementSensor managementvendor: Linarosub vendor: PMWGimpl 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
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
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;
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
participants (2)
-
AKASHI Takahiro -
Simon Glass