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)