Re: [PATCH v5 05/16] firmware: scmi: implement SCMI base protocol

[Etienne CARRIERE - foss <etienne.carriere@foss.st.com>]

Hello Akashi-san,


> From: U-Boot <u-boot-bounces@lists.denx.de> on behalf of AKASHI Takahiro <takahiro.akashi@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.akashi@linaro.org>
> Reviewed-by: Simon Glass <sjg@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.carriere@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.akashi@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)