RE: [PATCH v2 03/12] firmware: scmi: move scmi_bind_protocols() backward
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:37
>
> Move the location of scmi_bind_protocols() backward for changes
> in later patches.
> There is no change in functionality.
>
> Signed-off-by: AKASHI Takahiro
> ---
> drivers/firmware/scmi/scmi_agent-uclass.c | 118 +++---
> 1 file changed, 59 insertions(+), 59 deletions(-)
>
Reviewed-by: Etienne Carriere
> diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c
> b/drivers/firmware/scmi/scmi_agent-uclass.c
> index 39cf15c88f75..8693e4c447b7 100644
> --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> @@ -52,65 +52,6 @@ int scmi_to_linux_errno(s32 scmi_code)
> return -EPROTO;
> }
>
> -/*
> - * SCMI agent devices binds devices of various uclasses depeding on
> - * the FDT description. scmi_bind_protocol() is a generic bind sequence
> - * called by the uclass at bind stage, that is uclass post_bind.
> - */
> -static int scmi_bind_protocols(struct udevice *dev)
> -{
> - int ret = 0;
> - ofnode node;
> - const char *name;
> -
> - dev_for_each_subnode(node, dev) {
> - struct driver *drv = NULL;
> - u32 protocol_id;
> -
> - if (!ofnode_is_enabled(node))
> - continue;
> -
> - if (ofnode_read_u32(node, "reg", _id))
> - continue;
> -
> - name = ofnode_get_name(node);
> - switch (protocol_id) {
> - case SCMI_PROTOCOL_ID_CLOCK:
> - if (CONFIG_IS_ENABLED(CLK_SCMI))
> - drv = DM_DRIVER_GET(scmi_clock);
> - break;
> - case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> - if (IS_ENABLED(CONFIG_RESET_SCMI))
> - drv = DM_DRIVER_GET(scmi_reset_domain);
> - break;
> - case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> - if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
> - node = ofnode_find_subnode(node,
> "regulators");
> - if (!ofnode_valid(node)) {
> - dev_err(dev, "no regulators node\n");
> - return -ENXIO;
> - }
> - drv = DM_DRIVER_GET(scmi_voltage_domain);
> - }
> - break;
> - default:
> - break;
> - }
> -
> - if (!drv) {
> - dev_dbg(dev, "Ignore unsupported SCMI protocol %#x\n",
> - protocol_id);
> - continue;
> - }
> -
> - ret = device_bind(dev, drv, name, NULL, node, NULL);
> - if (ret)
> - break;
> - }
> -
> - return ret;
> -}
> -
> static struct udevice *find_scmi_transport_device(struct udevice *dev)
> {
> struct udevice *parent = dev;
> @@ -208,6 +149,65 @@ int devm_scmi_process_msg(struct udevice *dev, struct
> scmi_msg *msg)
> return scmi_process_msg(parent, priv->channel, msg);
> }
>
> +/*
> + * SCMI agent devices binds devices of various uclasses depeding on
> + * the FDT description. scmi_bind_protocol() is a generic bind sequence
> + * called by the uclass at bind stage, that is uclass post_bind.
> + */
> +static int scmi_bind_protocols(struct udevice *dev)
> +{
> + int ret = 0;
> + ofnode node;
> + const char *name;
> +
> + dev_for_each_subnode(node, dev) {
> + struct driver *drv = NULL;
> + u32 protocol_id;
> +
> + if (!ofnode_is_enabled(node))
> + continue;
> +
> + if (ofnode_read_u32(node, "reg", _id))
> + continue;
> +
> + name = ofnode_get_name(node);
> + switch (protocol_id) {
> + case SCMI_PROTOCOL_ID_CLOCK:
> + if (CONFIG_IS_ENABLED(CLK_SCMI))
> + drv = DM_DRIVER_GET(scmi_clock);
> + break;
> + case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> + if (IS_ENABLED(CONFIG_RESET_SCMI))
> + drv = DM_DRIVER_GET(scmi_reset_domain);
> + break;
> + case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> +
RE: [PATCH v2 06/12] sandbox: remove SCMI base node definition from test.dts
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> SCMI base protocol is mandatory and doesn't need to be listed in a device
> tree.
>
> Signed-off-by: AKASHI Takahiro
> Reviewed-by: Simon Glass
Reviewed-by: Etienne Carriere
> ---
> arch/sandbox/dts/test.dts | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index ff9f9222e6f9..2aad94681148 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -682,10 +682,6 @@
> #address-cells = <1>;
> #size-cells = <0>;
>
> - protocol@10 {
> - reg = <0x10>;
> - };
> -
> clk_scmi: protocol@14 {
> reg = <0x14>;
> #clock-cells = <1>;
> --
> 2.41.0
RE: [PATCH v2 08/12] test: dm: simplify SCMI unit test on sandbox
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> 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 simplify the test flows.
> The test scenario is not changed at all.
>
> Signed-off-by: AKASHI Takahiro
> Reviewed-by: Simon Glass
> ---
Reviewed-by: Etienne Carriere
> arch/sandbox/include/asm/scmi_test.h | 7 ++-
> drivers/firmware/scmi/sandbox-scmi_agent.c | 20 +--
> drivers/firmware/scmi/scmi_agent-uclass.c | 3 -
> test/dm/scmi.c | 64 +++---
> 4 files changed, 26 insertions(+), 68 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 = 180 },
> };
>
> -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 _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 279c2c218913..91cb172f3005 100644
> --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> @@ -412,9 +412,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
> #include
>
> -static int ut_assert_scmi_state_preprobe(struct unit_test_state *uts)
> -{
> - struct sandbox_scmi_service
RE: [PATCH v2 07/12] firmware: scmi: fake base protocol commands on sandbox
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> This is a simple implementation of SCMI base protocol for sandbox.
> The main use is in SCMI unit test.
>
> Signed-off-by: AKASHI Takahiro
> Reviewed-by: Simon Glass
Reviewed-by: Etienne Carriere
with reported typo addressed.
> ---
> 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
> #include
> #include
> +#include
> +#include
>
> /*
> * 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
typo: s/udevice/dev/g here and in the other function inline descripion
comments below.
BR,
etienne
> + * @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
RE: [PATCH v2 10/12] cmd: add scmi command for SCMI firmware
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> This command, "scmi", may provide a command line interface to various SCMI
> protocols. It supports at least initially SCMI base protocol and is
> intended mainly for debug purpose.
>
> Signed-off-by: AKASHI Takahiro
> ---
> v2
> * remove sub command category, 'scmi base', for simplicity
Reviewed-by: Etienne Carriere
> ---
> cmd/Kconfig | 9 ++
> cmd/Makefile | 1 +
> cmd/scmi.c | 334 +++
> 3 files changed, 344 insertions(+)
> create mode 100644 cmd/scmi.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 02e54f1e50fe..15d1d7b9863f 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2504,6 +2504,15 @@ config CMD_CROS_EC
> a number of sub-commands for performing EC tasks such as
> updating its flash, accessing a small saved context area
> and talking to the I2C bus behind the EC (if there is one).
> +
> +config CMD_SCMI
> + bool "Enable scmi command"
> + depends on SCMI_FIRMWARE
> + default n
> + help
> + This command provides user interfaces to several SCMI (System
> + Control and Management Interface) protocols available on Arm
> + platforms to manage system resources.
> endmenu
>
> menu "Filesystem commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 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 ..5ecd9051b48d
> --- /dev/null
> +++ b/cmd/scmi.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * SCMI (System Control and Management Interface) utility command
> + *
> + * Copyright (c) 2023 Linaro Limited
> + * Author: AKASHI Takahiro
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include /* uclass_get_device */
> +#include
> +#include
> +
> +static struct udevice *agent;
> +static struct udevice *base_proto;
> +
> +struct {
> + enum scmi_std_protocol id;
> + const char *name;
> +} protocol_name[] = {
> + {SCMI_PROTOCOL_ID_BASE, "Base"},
> + {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> + {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> + {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> + {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> + {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> + {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> + {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> +};
> +
> +/**
> + * get_proto_name() - get the name of SCMI protocol
> + *
> + * @id: SCMI Protocol ID
> + *
> + * Get the printable name of the protocol, @id
> + *
> + * Return: Name string on success, NULL on failure
> + */
> +static const char *get_proto_name(enum scmi_std_protocol id)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
> + if (id == protocol_name[i].id)
> + return protocol_name[i].name;
> +
> + return NULL;
> +}
> +
> +/**
> + * do_scmi_info() - get the information of SCMI services
> + *
> + * @cmdtp: Command table
> + * @flag: Command flag
> + * @argc: Number of arguments
> + * @argv: Argument array
> + *
> + * Get the information of SCMI services using various interfaces
> + * provided by the Base protocol.
> + *
> + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> + */
> +static int do_scmi_info(struct cmd_tbl *cmdtp, int flag, int argc,
> + char * const argv[])
> +{
> + 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"
RE: [PATCH v2 11/12] doc: cmd: add documentation for scmi
> From: AKASHI Takahiro > Sent: Wednesday, July 26, 2023 10:38 > > This is a help text for scmi command. > > Signed-off-by: AKASHI Takahiro > --- > v2 > * add more descriptions about SCMI Reviewed-by: Etienne Carriere > --- > doc/usage/cmd/scmi.rst | 126 + > 1 file changed, 126 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 ..aebcfe95101d > --- /dev/null > +++ b/doc/usage/cmd/scmi.rst > @@ -0,0 +1,126 @@ > +.. SPDX-License-Identifier: GPL-2.0+: > + > +scmi command > + > + > +Synopsis > + > + > +:: > + > + scmi info > + scmi perm_dev > + scmi perm_proto > + scmi reset > + > +Description > +--- > + > +Arm System Control and Management Interface (SCMI hereafter) is a set of > +standardised interfaces to manage system resources, like clocks, power > +domains, pin controls, reset and so on, in a system-wide manner. > + > +An entity which provides those services is called a SCMI firmware (or > +SCMI server if you like) may be placed/implemented by EL3 software or > +by a dedicated system control processor (SCP) or else. > + > +A user of SCMI interfaces, including U-Boot, is called a SCMI agent and > +may issues commands, which are defined in each protocol for specific system > +resources, to SCMI server via a communication channel, called a transport. > +Those interfaces are independent from the server's implementation thanks to > +a tranport layer. > + > +For more details, see the `SCMI specification`_. > + > +While most of system resources managed under SCMI protocols are implemented > +and handled as standard U-Boot devices, for example clk_scmi, scmi command > +provides additional management functionality against SCMI server. > + > +scmi info > +~ > + Show base information about SCMI server and supported protocols > + > +scmi perm_dev > +~ > + Allow or deny access permission to the device > + > +scmi perm_proto > +~~~ > + Allow or deny access to the protocol on the device > + > +scmi reset > +~~ > + Reset the already-configured permissions against the device > + > +Parameters are used as follows: > + > + > + SCMI Agent ID, hex value By the way, this documentation states hexadecimal value are awaited but not the embedded doc (patch v2 10/12). Would it be worth it? > + > + > + SCMI Device ID, hex value > + > + Please note that what a device means is not defined > + in the specification. > + > + > + SCMI Protocol ID, hex value > + > + It must not be 0x10 (base protocol) > + > + > + Flags to control the action, hex value > + > + 0 to deny, 1 to allow. The other values are reserved and allowed > + values may depend on the implemented version of SCMI server in > + the future. See SCMI specification for more details. > + > +Example > +--- > + > +Obtain basic information about SCMI server: > + > +:: > + > + => scmi info > + SCMI device: scmi > + protocol version: 0x2 > + # 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: 0x20b > + > +Ask for access permission to device#0: > + > +:: > + > + => scmi perm_dev 1 0 1 > + > +Reset configurations with all access permission settings retained: > + > +:: > + > + => scmi reset 1 0 > + > +Configuration > +- > + > +The scmi command is only available if CONFIG_CMD_SCMI=y. > +Default n because this command is mainly for debug purpose. > + > +Return value > + > + > +The return value ($?) is set to 0 if the operation succeeded, > +1 if the operation failed or -1 if the operation failed due to > +a syntax error. > + > +.. _`SCMI specification`: > https://developer.arm.com/documentation/den0056/e/?lang=en > -- > 2.41.0 >
RE: [PATCH v2 09/12] test: dm: add SCMI base protocol test
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> 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
> ---
> v2
> * use helper functions, removing direct uses of ops
> ---
Reviewed-by: Etienne Carriere
with reported issue fixed.
> test/dm/scmi.c | 109 +
> 1 file changed, 109 insertions(+)
>
> diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> index 881be3171b7c..41d46548f7fd 100644
> --- a/test/dm/scmi.c
> +++ b/test/dm/scmi.c
> @@ -16,6 +16,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -95,6 +98,112 @@ 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;
> + 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",
> + _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));
> +
> + /* version */
> + ret = scmi_base_protocol_version(base, );
> + ut_assertok(ret);
> + ut_asserteq(priv->version, version);
> +
> + /* protocol attributes */
> + ret = scmi_base_protocol_attrs(base, _agents, _protocols);
Typo: s/scmi_base_protocol_attrs/scmi_base_protocol_message_attrs/
This issue is fixed in patch 12/12 but should be squashed here to
prevent a build error.
BR,
etienne
> + ut_assertok(ret);
> + ut_asserteq(priv->num_agents, num_agents);
> + ut_asserteq(priv->num_protocols, num_protocols);
> +
> + /* discover vendor */
> + ret = scmi_base_discover_vendor(base, vendor);
> + ut_assertok(ret);
> + ut_asserteq_str(priv->vendor, vendor);
> +
> + /* message attributes */
> + ret = scmi_protocol_message_attrs(base,
> + SCMI_BASE_DISCOVER_SUB_VENDOR,
> + );
> + ut_assertok(ret);
> + ut_assertok(attributes);
> +
> + /* discover sub vendor */
> + ret = scmi_base_discover_sub_vendor(base, vendor);
> + ut_assertok(ret);
> + ut_asserteq_str(priv->sub_vendor, vendor);
> +
> + /* impl version */
> + ret = scmi_base_discover_impl_version(base, _version);
> + ut_assertok(ret);
> + ut_asserteq(priv->impl_version, impl_version);
> +
> + /* discover agent (my self) */
> + ret = scmi_base_discover_agent(base, 0x, _id,
> agent_name);
> + ut_assertok(ret);
> + ut_asserteq(priv->agent_id, agent_id);
> + ut_asserteq_str(priv->agent_name, agent_name);
> +
> + /* discover protocols */
> + ret = scmi_base_discover_list_protocols(base, );
> + 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 = scmi_base_set_device_permissions(base, agent_id, 0,
> +
> SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> + ut_assertok(ret); /* SCMI_SUCCESS */
> + ret = scmi_base_set_device_permissions(base, agent_id, 1,
> +
> SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> + ret = scmi_base_set_device_permissions(base, agent_id, 0, 0);
> + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> +
&g
Re: [RFC 2/3] firmware: scmi: add PROTOCOL_VERSION support for existing protocols on sandbox
> From: AKASHI Takahiro
> Sent: Friday, July 28, 2023 02:33
> Subject: [RFC 2/3] firmware: scmi: add PROTOCOL_VERSION support for existing
> protocols on sandbox
>
> In the next patch, SCMI_PROTOCOL_VERSION support is added on the existing
> SCMI protocols and the version check will be introduced.
> To finish "ut dm scmi_[clocks|resets|voltage_domains]" tests, sandbox SCMI
> agent should also implement/mimic this command.
>
> Signed-off-by: AKASHI Takahiro
Aside the typos, inherited from previous patch series, the patch looks all good
to me.
Reviewed-by: Etienne Carriere preferrably with
typos fixed.
> ---
> drivers/firmware/scmi/sandbox-scmi_agent.c | 80 ++
> 1 file changed, 80 insertions(+)
>
> diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c
> b/drivers/firmware/scmi/sandbox-scmi_agent.c
> index ab8afb01de40..a99fcb0ad4a9 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> @@ -43,6 +43,10 @@
> #define SANDBOX_SCMI_AGENT_NAME "OSPM"
> #define SANDBOX_SCMI_PLATFORM_NAME "platform"
>
> +#define SANDBOX_SCMI_CLOCK_PROTOCOL_VERSION SCMI_CLOCK_PROTOCOL_VERSION
> +#define SANDBOX_SCMI_RD_PROTOCOL_VERSION SCMI_RESETDOM_PROTOCOL_VERSION
> +#define SANDBOX_SCMI_VOLTD_PROTOCOL_VERSION SCMI_VOLTDOM_PROTOCOL_VERSION
> +
> static u8 protocols[] = {
> SCMI_PROTOCOL_ID_CLOCK,
> SCMI_PROTOCOL_ID_RESET_DOMAIN,
> @@ -440,6 +444,28 @@ static int
> sandbox_scmi_base_reset_agent_configuration(struct udevice *dev,
>
> /* Clock Protocol */
>
> +/**
> + * sandbox_scmi_clock_protocol_version - implement SCMI_PROTOCOL_VERSION
> + * @udevice: SCMI device
Typos: s/udevice/udev/g here and for the 2 other inline description comments
below.
> + * @msg: SCMI message
> + *
> + * Implement SCMI_PROTOCOL_VERSION command.
> + */
> +static int sandbox_scmi_clock_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_CLOCK_PROTOCOL_VERSION;
> + out->status = SCMI_SUCCESS;
> +
> + return 0;
> +}
> +
> static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev,
>struct scmi_msg *msg)
> {
> @@ -577,6 +603,30 @@ static int sandbox_scmi_clock_gate(struct udevice *dev,
> struct scmi_msg *msg)
> return 0;
> }
>
> +/* Reset Domain Protocol */
> +
> +/**
> + * sandbox_scmi_rd_protocol_version - implement SCMI_PROTOCOL_VERSION
> + * @udevice: SCMI device
> + * @msg: SCMI message
> + *
> + * Implement SCMI_PROTOCOL_VERSION command.
> + */
> +static int sandbox_scmi_rd_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_RD_PROTOCOL_VERSION;
> + out->status = SCMI_SUCCESS;
> +
> + return 0;
> +}
> +
> static int sandbox_scmi_rd_attribs(struct udevice *dev, struct scmi_msg *msg)
> {
> struct scmi_rd_attr_in *in = NULL;
> @@ -647,6 +697,30 @@ static int sandbox_scmi_rd_reset(struct udevice *dev,
> struct scmi_msg *msg)
> return 0;
> }
>
> +/* Voltage Domain Protocol */
> +
> +/**
> + * sandbox_scmi_voltd_protocol_version - implement SCMI_PROTOCOL_VERSION
> + * @udevice: SCMI device
> + * @msg: SCMI message
> + *
> + * Implement SCMI_PROTOCOL_VERSION command.
> + */
> +static int sandbox_scmi_voltd_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_VOLTD_PROTOCOL_VERSION;
> + out->status = SCMI_SUCCESS;
> +
> + return 0;
> +}
> +
> static int sandbox_scmi_voltd_attribs(struct udevice *dev, struct scmi_msg
> *msg)
> {
> struct scmi_voltd_attr_i
RE: [PATCH v2 12/12] test: dm: add scmi command test
> From: AKASHI Takahiro
> Sent: Wednesday, July 26, 2023 10:38
>
> In this test, "scmi" command is tested against different sub-commands.
> Please note that scmi command is for debug purpose and is not intended
> in production system.
>
> Signed-off-by: AKASHI Takahiro
> Reviewed-by: Simon Glass
> ---
> v2
> * use helper functions, removing direct uses of ops
Reviewed-by: Etienne Carriere
with patch on dm_test_scmi_base() squashed in patch 9/12.
> ---
> test/dm/scmi.c | 59 +++---
> 1 file changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> index 41d46548f7fd..a3e61355088a 100644
> --- a/test/dm/scmi.c
> +++ b/test/dm/scmi.c
> @@ -134,9 +134,9 @@ static int dm_test_scmi_base(struct unit_test_state *uts)
> ut_asserteq_str(priv->vendor, vendor);
>
> /* message attributes */
> - ret = scmi_protocol_message_attrs(base,
> - SCMI_BASE_DISCOVER_SUB_VENDOR,
> - );
> + ret = scmi_base_protocol_message_attrs(base,
> + SCMI_BASE_DISCOVER_SUB_VENDOR,
> + );
> ut_assertok(ret);
> ut_assertok(attributes);
>
> @@ -204,6 +204,59 @@ 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;
> +
> + /* preparation */
> + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> + _dev));
> + ut_assertnonnull(agent_dev);
> +
> + /* scmi info */
> + ut_assertok(run_command("scmi info", 0));
> +
> + ut_assert_nextline("SCMI device: scmi");
> + ut_assert_nextline(" protocol version: 0x2");
> + 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 perm_dev */
> + ut_assertok(run_command("scmi perm_dev 1 0 1", 0));
> + ut_assert_console_end();
> +
> + ut_assert(run_command("scmi perm_dev 1 0 0", 0));
> + ut_assert_nextline("Denying access to device:0 failed (-13)");
> + ut_assert_console_end();
> +
> + /* scmi perm_proto */
> + ut_assertok(run_command("scmi perm_proto 1 0 14 1", 0));
> + ut_assert_console_end();
> +
> + ut_assert(run_command("scmi perm_proto 1 0 14 0", 0));
> + ut_assert_nextline("Denying access to protocol:0x14 on device:0
> failed (-13)");
> + ut_assert_console_end();
> +
> + /* scmi reset */
> + ut_assert(run_command("scmi reset 1 1", 0));
> + ut_assert_nextline("Reset failed (-13)");
> + ut_assert_console_end();
> +
> + return 0;
> +}
> +
> +DM_TEST(dm_test_scmi_cmd, UT_TESTF_SCAN_FDT);
> +
> static int dm_test_scmi_clocks(struct unit_test_state *uts)
> {
> struct sandbox_scmi_agent *agent;
> --
> 2.41.0
>
Re: [RFC 1/3] firmware: scmi: add a check against availability of protocols
Hello Akashi-san,
> From: AKASHI Takahiro
> Sent: Friday, July 28, 2023 02:33
> Subject: [RFC 1/3] firmware: scmi: add a check against availability of
> protocols
>
> Now that we have Base protocol support, we will be able to check if a given
> protocol is really supported by the SCMI server (firmware).
>
> Signed-off-by: AKASHI Takahiro
> ---
Reviewed-by: Etienne Carriere
> drivers/firmware/scmi/scmi_agent-uclass.c | 41 +--
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c
> b/drivers/firmware/scmi/scmi_agent-uclass.c
> index 91cb172f3005..9494dca95bca 100644
> --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> @@ -44,6 +44,38 @@ static const struct error_code scmi_linux_errmap[] = {
> */
> struct udevice *scmi_agent;
>
> +/**
> + * scmi_protocol_is_supported - check availability of protocol
> + * @dev: SCMI agent device
> + * @proto_id: Identifier of protocol
> + *
> + * check if the protocol, @proto_id, is provided by the SCMI agent,
> + * @dev.
> + *
> + * Return: 0 on success, error code otherwise
> + */
> +static bool scmi_protocol_is_supported(struct udevice *dev,
> + enum scmi_std_protocol proto_id)
> +{
> + struct scmi_agent_priv *priv;
> + int i;
> +
> + if (proto_id == SCMI_PROTOCOL_ID_BASE)
> + return true;
> +
> + priv = dev_get_uclass_plat(dev);
> + if (!priv) {
> + dev_err(dev, "No priv data found\n");
> + return false;
> + }
> +
> + for (i = 0; i < priv->num_protocols; i++)
> + if (priv->protocols[i] == proto_id)
> + return true;
> +
> + return false;
> +}
> +
> struct udevice *scmi_get_protocol(struct udevice *dev,
> enum scmi_std_protocol id)
> {
> @@ -372,15 +404,18 @@ static int scmi_bind_protocols(struct udevice *dev)
> name = ofnode_get_name(node);
> switch (protocol_id) {
> case SCMI_PROTOCOL_ID_CLOCK:
> - if (CONFIG_IS_ENABLED(CLK_SCMI))
> + if (CONFIG_IS_ENABLED(CLK_SCMI) &&
> + scmi_protocol_is_supported(dev, protocol_id))
> drv = DM_DRIVER_GET(scmi_clock);
> break;
> case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> - if (IS_ENABLED(CONFIG_RESET_SCMI))
> + if (IS_ENABLED(CONFIG_RESET_SCMI) &&
> + scmi_protocol_is_supported(dev, protocol_id))
> drv = DM_DRIVER_GET(scmi_reset_domain);
> break;
> case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> - if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
> + if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI) &&
> + scmi_protocol_is_supported(dev, protocol_id)) {
> node = ofnode_find_subnode(node,
> "regulators");
> if (!ofnode_valid(node)) {
> dev_err(dev, "no regulators node\n");
> --
> 2.41.0
>
>
Re: [RFC 3/3] firmware: scmi: add a sanity check against protocol version
> From: AKASHI Takahiro
> Sent: Friday, July 28, 2023 02:33
> Subject: [RFC 3/3] firmware: scmi: add a sanity check against protocol version
>
> SCMI_PROTOCOL_VERSION is a mandatory command for all the SCMI protocols.
> With this patch, this command is implemented on each protocol.
> Then, we can assure that the feature set implemented by SCMI Server
> (firmware) is compatible with U-Boot, that is, each protocol's version
> must be equal to or higher than the one that U-Boot's corresponding driver
> expects.
>
> Signed-off-by: AKASHI Takahiro
> ---
This change enforces some protocol versions that may not be reported
by known open source scmi server implementations: scp-firmware, tf-a
and op-tee.
Latest scp-firwmare/op-tee/tf-a report the following verisons numbers:
* base protocol : 0x2 all
* clock protocol: 0x1 for scp-firwmare / 0x2 for op-tee and tf-a.
* reset-dom: 0x1000 for all
* voltage-dom: 0x1 for scp-firmware / 0x3 for op-tee / not used in tf-a.
* power-dom: 0x2 for scp-firmware / 0x21000 for tf-a / not used in op-tee.
Regarding the SCMI specification, these protocol versions evolve with the
specification version:
-- ---base -powerD --clock -resetD --voltD
SCMI v1.0: 0x1 0x1 0x1 --- ---
SCMI v2.0: 0x2 0x2 0x1 0x1 ---
SCMI v3.0: 0x2 0x21000 0x1 0x2 0x1
SCMI v3.1: 0x2 0x3 0x3 0x3 0x2
SCMI v3.2: 0x2 0x3 0x3 0x3 0x2
This patch enforces the versions defined in SCMI v3.2.
Being strict in protocol version is not ideal I think.
BR,
etienne
> drivers/clk/clk_scmi.c | 6 ++
> drivers/power/regulator/scmi_regulator.c | 6 ++
> drivers/reset/reset-scmi.c | 14 +-
> include/scmi_protocols.h | 6 ++
> 4 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> index 34a49363a51a..3591acb6d3a9 100644
> --- a/drivers/clk/clk_scmi.c
> +++ b/drivers/clk/clk_scmi.c
> @@ -138,12 +138,18 @@ static int scmi_clk_probe(struct udevice *dev)
> {
> struct clk *clk;
> size_t num_clocks, i;
> + u32 version;
> int ret;
>
> ret = devm_scmi_of_get_channel(dev);
> if (ret)
> return ret;
>
> + ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_CLOCK,
> + );
> + if (ret || version < SCMI_CLOCK_PROTOCOL_VERSION)
> + return -EINVAL;
> +
> if (!CONFIG_IS_ENABLED(CLK_CCF))
> return 0;
>
> diff --git a/drivers/power/regulator/scmi_regulator.c
> b/drivers/power/regulator/scmi_regulator.c
> index 08918b20872c..936768d0eeeb 100644
> --- a/drivers/power/regulator/scmi_regulator.c
> +++ b/drivers/power/regulator/scmi_regulator.c
> @@ -138,12 +138,18 @@ static int scmi_regulator_probe(struct udevice *dev)
> .out_msg = (u8 *),
> .out_msg_sz = sizeof(out),
> };
> + u32 version;
> int ret;
>
> ret = devm_scmi_of_get_channel(dev);
> if (ret)
> return ret;
>
> + ret = scmi_generic_protocol_version(dev,
> SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
> + );
> + if (ret || version < SCMI_VOLTDOM_PROTOCOL_VERSION)
> + return -EINVAL;
> +
> /* Check voltage domain is known from SCMI server */
> in.domain_id = pdata->domain_id;
>
> diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
> index b76711f0a8fb..dbd98dbdbc4f 100644
> --- a/drivers/reset/reset-scmi.c
> +++ b/drivers/reset/reset-scmi.c
> @@ -73,7 +73,19 @@ static const struct reset_ops scmi_reset_domain_ops = {
>
> static int scmi_reset_probe(struct udevice *dev)
> {
> - return devm_scmi_of_get_channel(dev);
> + u32 version;
> + int ret;
> +
> + ret = devm_scmi_of_get_channel(dev);
> + if (ret)
> + return ret;
> +
> + ret = scmi_generic_protocol_version(dev,
> SCMI_PROTOCOL_ID_RESET_DOMAIN,
> + );
> + if (ret || version < SCMI_RESETDOM_PROTOCOL_VERSION)
> + return -EINVAL;
> +
> + return 0;
> }
>
> U_BOOT_DRIVER(scmi_reset_domain) = {
> diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> index 64fd740472b5..6ab16efd49cc 100644
> --- a/include/scmi_protocols.h
> +++ b/include/scmi_protocols.h
> @@ -398,6 +398,8 @@ int scmi_generic_protocol_version(struct udevice *dev,
> * SCMI Clock Protocol
> */
>
> +#define SCMI_CLOCK_PROTOCOL_VERSION 0x2
TF-A and OP-TEE scmi servers report version 0x2 for clock protocol but
SCP-firmware reports version 0x1:
https://github.com/ARM-software/SCP-firmware/blob/master/module/scmi_clock/include/internal/scmi_clock.h#L16
> +
> enum scmi_clock_message_id {
> SCMI_CLOCK_ATTRIBUTES = 0x3,
>
[PATCH 2/2] tee: optee: don't enumerate services if there ain't any
Change optee driver service enumeration to not enumerate (and
allocate a zero sized shared memory buffer) when OP-TEE
reports that there is no service to enumerate.
This change fixes an existing issue that occurs when the such zero
sized shared memory buffer allocated from malloc() has a physical
address of offset 0 of a physical 4kB page. In such case, OP-TEE
secure world refuses to register the zero-sized shared memory
area and makes U-Boot optee service enumeration to fail.
Fixes: 94ccfb78a4d6 ("drivers: tee: optee: discover OP-TEE services")
Signed-off-by: Etienne Carriere
---
drivers/tee/optee/core.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 5308dd58ce..47f845cffe 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -139,6 +139,11 @@ static int enum_services(struct udevice *dev, struct
tee_shm **shm, size_t *coun
if (ret)
return ret;
+ if (!shm_size) {
+ *count = 0;
+ return 0;
+ }
+
ret = tee_shm_alloc(dev, shm_size, 0, shm);
if (ret) {
dev_err(dev, "Failed to allocated shared memory: %d\n", ret);
@@ -185,14 +190,15 @@ static int bind_service_drivers(struct udevice *dev)
ret = enum_services(dev, _list, _count, tee_sess,
PTA_CMD_GET_DEVICES);
- if (!ret)
+ if (!ret && service_count)
ret = bind_service_list(dev, service_list, service_count);
tee_shm_free(service_list);
+ service_list = NULL;
ret2 = enum_services(dev, _list, _count, tee_sess,
PTA_CMD_GET_DEVICES_SUPP);
- if (!ret2)
+ if (!ret2 && service_count)
ret2 = bind_service_list(dev, service_list, service_count);
tee_shm_free(service_list);
--
2.25.1
[PATCH 1/2] tee: optee: don't fail on services enumeration failure
Change optee probe function to only warn when service enumeration
sequence fails instead of reporting an optee driver probe failure.
Indeed U-Boot can still use OP-TEE even if some OP-TEE services are
not discovered.
Fixes: 94ccfb78a4d6 ("drivers: tee: optee: discover OP-TEE services")
Signed-off-by: Etienne Carriere
---
drivers/tee/optee/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 9a9b697e91..5308dd58ce 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -841,7 +841,7 @@ static int optee_probe(struct udevice *dev)
if (IS_ENABLED(CONFIG_OPTEE_SERVICE_DISCOVERY)) {
ret = bind_service_drivers(dev);
if (ret)
- return ret;
+ dev_warn(dev, "optee service enumeration failed: %d\n",
ret);
} else if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
/*
* Discovery of TAs on the TEE bus is not supported in U-Boot:
--
2.25.1
Re: [PATCH v6 08/14] firmware: scmi: add a version check against base protocol
> From: U-Boot on behalf of AKASHI Takahiro
>
> Sent: Wednesday, October 11, 2023 12:07 PM
>
> In SCMI base protocol version 2 (0x2), new interfaces,
> BASE_SET_DEVICE_PERMISSIONS/BASE_SET_PROTOCOL_PERMISSIONS/
> BASE_RESET_AGENT_CONFIGURATION, were added. Moreover, the api of
> BASE_DISCOVER_AGENT was changed to support self-agent discovery.
>
> So the driver expects SCMI firmware support version 2 of base protocol.
>
> Signed-off-by: AKASHI Takahiro
> ---
> v6
> * new commit
> ---
> drivers/firmware/scmi/base.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/firmware/scmi/base.c b/drivers/firmware/scmi/base.c
> index ee84e261945a..1d41a8a98fc6 100644
> --- a/drivers/firmware/scmi/base.c
> +++ b/drivers/firmware/scmi/base.c
> @@ -481,6 +481,7 @@ static int scmi_base_reset_agent_configuration_int(struct
> udevice *dev,
> */
> static int scmi_base_probe(struct udevice *dev)
> {
> + u32 version;
> int ret;
>
> ret = devm_scmi_of_get_channel(dev);
> @@ -488,6 +489,13 @@ static int scmi_base_probe(struct udevice *dev)
> dev_err(dev, "get_channel failed\n");
> return ret;
> }
> + ret = scmi_base_protocol_version_int(dev, );
> + if (ret) {
> + dev_err(dev, "getting protocol version failed\n");
> + return ret;
> + }
> + if (version < SCMI_BASE_PROTOCOL_VERSION)
> + return -EINVAL;
LGTM. The open source SCMI server implementations I'm aware of (scp-firmware,
tf-a and optee-os) all report SCMI Base protocol version v2.0.
Reviewed-by: Etienne Carriere
That said, maybe a more flexible implementation would support both version v1.0
(0x1) and v2.0
but disable permission commands for v1.0 compliant servers. Maybe in a later
change, if there is a need for.
I fear using such strict minima protocol version values for other SCMI
procotols make U-Boot client too restrictive.
BR,
Etienne
>
> return ret;
> }
> --
> 2.34.1
>
>
Re: [PATCH v5 05/16] firmware: scmi: implement SCMI base protocol
Hello Akashi-san,
> From: U-Boot on behalf of AKASHI Takahiro
>
> Sent: Tuesday, September 26, 2023 8:57 AM
>
> 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
> Reviewed-by: Simon Glass
> ---
> v3
> * strncpy (TODO)
> * remove a duplicated function prototype
> * use newly-allocated memory when return vendor name or agent name
> * revise function descriptions in a header
> v2
> * add helper functions, removing direct uses of ops
> * add function descriptions for each of functions in ops
> ---
This patch v5 looks good to me. 2 suggestions.
Reviewed-by: Etienne Carriere with comments
addressed or not.
I have successfully tested the whole PATCH v5 series on my platform.
> drivers/firmware/scmi/Makefile | 1 +
> drivers/firmware/scmi/base.c | 657 +
> include/dm/uclass-id.h | 1 +
> include/scmi_protocols.h | 351 ++
> 4 files changed, 1010 insertions(+)
> 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 ..dba143e1ff5d
> --- /dev/null
> +++ b/drivers/firmware/scmi/base.c
> @@ -0,0 +1,657 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * SCMI Base protocol as U-Boot device
> + *
> + * Copyright (C) 2023 Linaro Limited
> + * author: AKASHI Takahiro
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * scmi_generic_protocol_version - get protocol version
> + * @dev: SCMI device
> + * @id:SCMI protocol ID
> + * @version: Pointer to SCMI protocol version
> + *
> + * Obtain the protocol version number in @version.
> + *
> + * Return: 0 on success, error code on failure
> + */
> +int scmi_generic_protocol_version(struct udevice *dev,
> + enum scmi_std_protocol id, u32 *version)
> +{
> + struct scmi_protocol_version_out out;
> + struct scmi_msg msg = {
> + .protocol_id = id,
> + .message_id = SCMI_PROTOCOL_VERSION,
> + .out_msg = (u8 *),
> + .out_msg_sz = sizeof(out),
> + };
> + int ret;
> +
> + ret = devm_scmi_process_msg(dev, );
> + if (ret)
> + return ret;
> + if (out.status)
> + return scmi_to_linux_errno(out.status);
> +
> + *version = out.version;
> +
> + return 0;
> +}
> +
> +/**
> + * scmi_base_protocol_version_int - get Base protocol version
> + * @dev: SCMI device
> + * @version: Pointer to SCMI protocol version
> + *
> + * Obtain the protocol version number in @version for Base protocol.
> + *
> + * Return: 0 on success, error code on failure
> + */
I think these inline description comment for scmi_base_protocol_xxx_int()
would better be placed as description for the exported functions
scmi_base_protocol_xxx() and scmi_base_discover_xxx(). Either in the .c file or
in the header file.
Especially regarding the function
scmi_base_discover_vendor()/_discover_sub_vendor()/_discover_agent() where
caller is responsible for freeing the output string.
> +static int scmi_base_protocol_version_int(struct udevice *dev, u32 *version)
> +{
> + return scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_BASE,
> +version);
> +}
> +
> +/**
> + * scmi_protocol_attrs_int - get protocol attributes
> + * @dev: SCMI device
> + * @num_agents:Number of SCMI agents
> + * @num_protocols: Number of SCMI 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
Re: [PATCH v5 01/16] scmi: refactor the code to hide a channel from devices
> From: U-Boot on behalf of AKASHI Takahiro
>
> Sent: Tuesday, September 26, 2023 8:57 AM
>
> The commit 85dc58289238 ("firmware: scmi: prepare uclass to pass channel
> reference") added an explicit parameter, channel, but it seems to make
> the code complex.
>
> Hiding this parameter will allow for adding a generic (protocol-agnostic)
> helper function, i.e. for PROTOCOL_VERSION, in a later patch.
>
> Signed-off-by: AKASHI Takahiro
> Reviewed-by: Simon Glass
> ---
> v5
> * correct the size for per_child_auto
> v4
> * revive scmi_bind_protocols which was accidentally removed
> * remove .per_child_auto from the driver declaration as it is not needed
> v3
> * fix an issue on ST board (reported by Etienne)
> by taking care of cases where probed devices are children of
> SCMI protocol device (i.e. clock devices under CCF)
> See find_scmi_protocol_device().
> * move "per_device_plato_auto" to a succeeding right patch
> v2
> * new patch
> ---
> drivers/clk/clk_scmi.c| 27 ++
> drivers/firmware/scmi/scmi_agent-uclass.c | 105 --
> drivers/power/regulator/scmi_regulator.c | 26 ++
> drivers/reset/reset-scmi.c| 19 +---
> include/scmi_agent.h | 15 ++--
> 5 files changed, 104 insertions(+), 88 deletions(-)
>
Reviewed-by: Etienne Carriere
Re: [PATCH v5 02/16] firmware: scmi: use a protocol's own channel if assigned
> From: U-Boot on behalf of AKASHI Takahiro > > Sent: Tuesday, September 26, 2023 8:57 AM > > SCMI specification allows any protocol to have its own channel for > the transport. While the current SCMI driver may assign its channel > from a device tree, the core function, devm_scmi_process_msg(), doesn't > use a protocol's channel, but always use an agent's channel. > > With this commit, devm_scmi_process_msg() tries to find and use > a protocol's channel. If it doesn't exist, use an agent's. > > Signed-off-by: AKASHI Takahiro > --- > v5 > * new commit (fixing a potential bug) > --- > drivers/firmware/scmi/mailbox_agent.c | 5 +++-- > drivers/firmware/scmi/optee_agent.c | 5 +++-- > drivers/firmware/scmi/scmi_agent-uclass.c | 7 --- > drivers/firmware/scmi/smccc_agent.c | 5 +++-- > include/scmi_agent-uclass.h | 8 +--- > 5 files changed, 18 insertions(+), 12 deletions(-) > Reviewed-by: Etienne Carriere Thanks for addressing this protocol channel issue. BR, Etienne
Re: [PATCH v5 03/16] firmware: scmi: support dummy channels for sandbox agent
> > From: U-Boot on behalf of Simon Glass > > Sent: Monday, October 2, 2023 3:17 AM > > On Tue, 26 Sept 2023 at 00:58, AKASHI Takahiro > wrote: > > > > In sandbox scmi agent, channels are not used at all. But in this patch, > > dummy channels are supported in order to test protocol-specific channels. > > > > Signed-off-by: AKASHI Takahiro > > --- > > v5 > > * new commit > > --- > > arch/sandbox/include/asm/scmi_test.h | 13 > > drivers/firmware/scmi/sandbox-scmi_agent.c | 90 ++ > > 2 files changed, 103 insertions(+) > > > > Reviewed-by: Simon Glass > Reviewed-by: Etienne Carriere
Re: [PATCH v5 04/16] test: dm: add protocol-specific channel test
> From: U-Boot on behalf of Simon Glass > > Sent: Monday, October 2, 2023 3:17 AM > > On Tue, 26 Sept 2023 at 00:58, AKASHI Takahiro > wrote: > > > > Any SCMI protocol may have its own channel. > > Test this feature on sandbox as the necessary framework was added > > in a prior commit. > > > > Signed-off-by: AKASHI Takahiro > > --- > > v5 > > * new commit > > --- > > arch/sandbox/dts/test.dts | 1 + > > test/dm/scmi.c| 20 ++-- > > 2 files changed, 19 insertions(+), 2 deletions(-) > > > > Reviewed-by: Simon Glass > Reviewed-by: Etienne Carriere
Re: [PATCH v2 01/12] scmi: refactor the code to hide a channel from devices
Hello Takahiro-san,
> From: U-Boot on behalf of Simon Glass
>
> Sent: Thursday, July 27, 2023 2:50 AM
>
> On Wed, 26 Jul 2023 at 02:38, AKASHI Takahiro
> wrote:
> >
> > The commit 85dc58289238 ("firmware: scmi: prepare uclass to pass channel
> > reference") added an explicit parameter, channel, but it seems to make
> > the code complex.
> >
> > Hiding this parameter will allow for adding a generic (protocol-agnostic)
> > helper function, i.e. for PROTOCOL_VERSION, in a later patch.
> >
> > Signed-off-by: AKASHI Takahiro
> > ---
> > v2
> > * new patch
> > ---
> > drivers/clk/clk_scmi.c| 27 ++---
> > drivers/firmware/scmi/scmi_agent-uclass.c | 74 ++-
> > drivers/power/regulator/scmi_regulator.c | 27 +++--
> > drivers/reset/reset-scmi.c| 19 +-
> > include/scmi_agent.h | 15 +++--
> > 5 files changed, 86 insertions(+), 76 deletions(-)
> >
>
> Reviewed-by: Simon Glass
Sorry for this late feedback. I initially saw no issues with this patch series
but testing the series against stm32mp135f-dk board which is using SCMI
resources, I saw the board failed to boot. The issue is related to udevice
parent tree and common clock framework. I'll post comments on patch 01/12.
Best regards,
Etienne
Re: [PATCH v2 01/12] scmi: refactor the code to hide a channel from devices
Hello Akashi-san,
> From: AKASHI Takahiro
> Sent: Monday, August 7, 2023 12:03 PM
>
> Hi Etienne,
>
> On Thu, Aug 03, 2023 at 09:24:51AM +, Etienne CARRIERE wrote:
> > Hello Takahiro-san,
> >
> > Sorry again for this late feedback.
> > Testing the series against stm32mp135f-dk board which is using SCMI
> > resources, I see the board fails to boot.
> >
> > > From: AKASHI Takahiro
> > > To: tr...@konsulko.com, s...@chromium.org
> > > Cc: etienne.carri...@st.com, u-boot@lists.denx.de,
> > > AKASHI Takahiro
> > > Subject: [PATCH v2 01/12] scmi: refactor the code to hide a channel from
> > > devices
> > > Date: Wed, 26 Jul 2023 17:37:57 +0900 [thread overview]
> > > Message-ID: <20230726083808.140780-2-takahiro.aka...@linaro.org> (raw)
> > > In-Reply-To: <20230726083808.140780-1-takahiro.aka...@linaro.org>
> > >
> > > The commit 85dc58289238 ("firmware: scmi: prepare uclass to pass channel
> > > reference") added an explicit parameter, channel, but it seems to make
> > > the code complex.
> > >
> > > Hiding this parameter will allow for adding a generic (protocol-agnostic)
> > > helper function, i.e. for PROTOCOL_VERSION, in a later patch.
> > >
> > > Signed-off-by: AKASHI Takahiro
> > >
> > > ---
> > > v2
> > > * new patch
> > > ---
> > > drivers/clk/clk_scmi.c| 27 ++---
> > > drivers/firmware/scmi/scmi_agent-uclass.c | 74 ++-
> > > drivers/power/regulator/scmi_regulator.c | 27 +++--
> > > drivers/reset/reset-scmi.c| 19 +-
> > > include/scmi_agent.h | 15 +++--
> > > 5 files changed, 86 insertions(+), 76 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> > > index d172fed24c9d..34a49363a51a 100644
> > > --- a/drivers/clk/clk_scmi.c
> > > +++ b/drivers/clk/clk_scmi.c
> > > @@ -13,17 +13,8 @@
> > > #include
> > > #include
> > >
> > > -/**
> > > - * struct scmi_clk_priv - Private data for SCMI clocks
> > > - * @channel: Reference to the SCMI channel to use
> > > - */
> > > -struct scmi_clk_priv {
> > > - struct scmi_channel *channel;
> > > -};
> > > -
> > > static int scmi_clk_get_num_clock(struct udevice *dev, size_t
> > > *num_clocks)
> > > {
> > > - struct scmi_clk_priv *priv = dev_get_priv(dev);
> > > struct scmi_clk_protocol_attr_out out;
> > > struct scmi_msg msg = {
> > > .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> > > @@ -33,7 +24,7 @@ static int scmi_clk_get_num_clock(struct udevice *dev,
> > > size_t *num_clocks)
> > > };
> > > int ret;
> > >
> > > - ret = devm_scmi_process_msg(dev, priv->channel, );
> > > + ret = devm_scmi_process_msg(dev, );
> > > if (ret)
> > > return ret;
> > >
> > > @@ -44,7 +35,6 @@ static int scmi_clk_get_num_clock(struct udevice *dev,
> > > size_t *num_clocks)
> > >
> > > static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char
> > > **name)
> > > {
> > > - struct scmi_clk_priv *priv = dev_get_priv(dev);
> > > struct scmi_clk_attribute_in in = {
> > > .clock_id = clkid,
> > > };
> > > @@ -59,7 +49,7 @@ static int scmi_clk_get_attibute(struct udevice *dev,
> > > int clkid, char **name)
> > > };
> > > int ret;
> > >
> > > - ret = devm_scmi_process_msg(dev, priv->channel, );
> > > + ret = devm_scmi_process_msg(dev, );
> > > if (ret)
> > > return ret;
> > >
> > > @@ -70,7 +60,6 @@ static int scmi_clk_get_attibute(struct udevice *dev,
> > > int clkid, char **name)
> > >
> > > static int scmi_clk_gate(struct clk *clk, int enable)
> > > {
> > > - struct scmi_clk_priv *priv = dev_get_priv(clk->dev);
> > > struct scmi_clk_state_in in = {
> > > .clock_id = clk->id,
> > > .attributes = enable,
> > > @@ -81,7 +70,7 @@ static int scmi_clk_gate(struct clk *clk, int enable)
> > > in, out);
> > > int ret;
> > >
> > > - ret = devm_scmi_process_msg(clk->dev, priv->channel, );
&g
Re: [PATCH v2 00/21] FWU: Migrate FWU metadata to version 2
Hello Sughosh, Sorry for this very late reply especially since I have a quite negative feedback on the proposed changes. I don't think FWU metadata version 1 should be removed from U-Boot support. There are existing immutable boot agent relying on format v1, starting from the Synquacer boards based on SCP-firmware v2.11 [1] onward (at least up to latest v2.13 tag) and the stm32mp1 platforms based on TF-A v2.7 [2] onward (at least up to latest tag v2.10). These platforms should be able to update there EFI firmware hence needing the update agent (U-Boot) to support format v1. With the proposed series, the new format v2 contains the same information the previous mdata v1 based implementation did (apart that some info where built-in whereas v2 describe them from the mdata storage area). Could it be possible the implementation support both, using for example a internal structure fed from either format v1 or v2 content read from the storage, and used to update the mdata v1 or v2 format storage layout? [1] https://gitlab.arm.com/firmware/SCP-firmware/-/blob/v2.11.0/product/synquacer/include/fwu_mdata.h [2] https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/tags/v2.7/include/drivers/fwu/fwu_metadata.h Best regards, Etienne > From: Sughosh Ganu > > The following patches migrate the FWU metadata access code to version > 2 of the structure. This is based on the structure definition as > defined in the latest rev of the FWU Multi Bank Update specification > [1]. > > Since the version 1 of the structure has currently been adopted on a > couple of platforms, it was decided to have a clean migration of the > metadata to version 2 only, instead of supporting both the versions of > the structure. Also, based on consultations with the main author of > the specification, it is expected that any further changes in the > structure would be minor tweaks, and not be significant. Hence a > migration to version 2. > > Similar migration is also being done in TF-A, including migrating the > ST platform port to support version 2 of the metadata structure [2]. > > @Michal, I tested the metadata for the two image per bank case, and it > works fine on the ST board. Kindly test this on your board as well. > > @Kojima-san, Please help in testing the version 2 on your > board. Thanks. > > > [1] - https://developer.arm.com/documentation/den0118/latest/ > [2] - > https://review.trustedfirmware.org/q/topic:%22topics/fwu_metadata_v2_migration%22 > > Changes since V1: > > * Do not define flexible array members inside the structures. > * Access the image information related fields in the metadata using > the helper functions defined in an earlier patch. > * Access fwu_fw_store_desc structure using pointer arithmetic. > > (snip)
