[PATCH v5 05/16] firmware: scmi: implement SCMI base protocol
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Oct 5 11:58:09 CEST 2023
Hi Etienne,
On Thu, Oct 05, 2023 at 07:06:47AM +0000, Etienne CARRIERE - foss wrote:
> Hello Akashi-san,
>
>
> > From: U-Boot <u-boot-bounces at lists.denx.de> on behalf of AKASHI Takahiro <takahiro.akashi at 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 at linaro.org>
> > Reviewed-by: Simon Glass <sjg at 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 at foss.st.com> with comments addressed or not.
> I have successfully tested the whole PATCH v5 series on my platform.
Thank you for your review and testing.
>
> > 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 at 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.
Yes, I will add comments.
>
> > +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.
Yes.
> 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.
Right, but all DM devices declare its own operations arrays in headers.
Although I don't have a strong opinion, I'd like to follow this tradition.
-Takahiro Akashi
> 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)
More information about the U-Boot
mailing list