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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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@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.

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@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)