Hello Akashi-san,
> From: U-Boot <u-boot-boun...@lists.denx.de> on behalf of AKASHI Takahiro
> <takahiro.aka...@linaro.org>
> 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 <takahiro.aka...@linaro.org>
> Reviewed-by: Simon Glass <s...@chromium.org>
> ---
> 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 <etienne.carri...@foss.st.com> 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 000000000000..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 <takahiro.aka...@linaro.org>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <scmi_agent.h>
> +#include <scmi_protocols.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <asm/types.h>
> +#include <dm/device_compat.h>
> +#include <linux/kernel.h>
> +
> +/**
> + * 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,
> + .out_msg_sz = sizeof(out),
> + };
> + int ret;
> +
> + ret = devm_scmi_process_msg(dev, &msg);
> + 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 success, error code on failure
> + */
> +static int scmi_protocol_attrs_int(struct udevice *dev, u32 *num_agents,
> + u32 *num_protocols)
> +{
> + 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, &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;
> +}
> +
(snip)
> +
> +/**
> + * scmi_base_probe - probe base protocol device
> + * @dev: SCMI device
> + *
> + * Probe the device for SCMI base protocol and initialize the private data.
> + *
> + * Return: 0 on success, error code on failure
> + */
> +static int scmi_base_probe(struct udevice *dev)
> +{
> + int ret;
> +
> + ret = devm_scmi_of_get_channel(dev);
> + if (ret) {
> + dev_err(dev, "get_channel failed\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +struct scmi_base_ops scmi_base_ops = {
Could be static.
By the way, struct scmi_base_ops is defined in the header file but I don't
think it needs to be known from other drivers/env, even in the case of the
sandbox tests.
Maybe struct scmi_base_ops could be defined in this source file.
Regards,
Etienne
> + /* Commands */
> + .protocol_version = scmi_base_protocol_version_int,
> + .protocol_attrs = scmi_protocol_attrs_int,
> + .protocol_message_attrs = scmi_protocol_message_attrs_int,
> + .base_discover_vendor = scmi_base_discover_vendor_int,
> + .base_discover_sub_vendor = scmi_base_discover_sub_vendor_int,
> + .base_discover_impl_version = scmi_base_discover_impl_version_int,
> + .base_discover_list_protocols = scmi_base_discover_list_protocols_int,
> + .base_discover_agent = scmi_base_discover_agent_int,
> + .base_notify_errors = NULL,
> + .base_set_device_permissions = scmi_base_set_device_permissions_int,
> + .base_set_protocol_permissions =
> scmi_base_set_protocol_permissions_int,
> + .base_reset_agent_configuration =
> + scmi_base_reset_agent_configuration_int,
> +};
> +
> +int scmi_base_protocol_version(struct udevice *dev, u32 *version)
> +{
> + const struct scmi_base_ops *ops = device_get_ops(dev);
> +
> + if (ops->protocol_version)
> + return (*ops->protocol_version)(dev, version);
> +
> + return -EOPNOTSUPP;
> +}
> +
> +int scmi_base_protocol_attrs(struct udevice *dev, u32 *num_agents,
> + u32 *num_protocols)
> +{
> + const struct scmi_base_ops *ops = device_get_ops(dev);
> +
> + if (ops->protocol_attrs)
> + return (*ops->protocol_attrs)(dev, num_agents, num_protocols);
> +
> + return -EOPNOTSUPP;
> +}
> +
(snip)
