Re: [PATCH 1/4] firmware: add new driver for SCMI firmwares

Hell Peng,

Thanks for the comments. Please find below few questions for some of your comments.

On Mon, 20 Jul 2020 at 04:01, Peng Fan peng.fan@nxp.com wrote:

Hi Etienne,

+Sudeep

...

Subject: [PATCH 1/4] firmware: add new driver for SCMI firmwares

Thanks for posting this out.

...

This change introduces SCMI agent driver in U-Boot in the firmware U-class.

SCMI agent driver is designed for platforms that embed a SCMI server in a firmware hosted for example by a companion co-processor or the secure world of the executing processor.

SCMI protocols allow an SCMI agent to discover and access external resources as clock, reset controllers and many more. SCMI agent and server communicate following the SCMI specification [1]. SCMI agent complies with the DT bindings defined in the Linux kernel source tree regarding SCMI agent description since v5.8-rc1.

These bindings describe 2 supported message transport layer: using mailbox uclass devices or using Arm SMC invocation instruction. Both use a piece or shared memory for message data exchange.

In the current state, the SCMI agent driver does not bind to any SCMI protocol to a U-Boot device driver. Former changes will implement dedicated driver (i.e. an SCMI clock driver or an SCMI reset controller driver) and add bind supported SCMI protocols in scmi_agent_bind().

Links: [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelo per.arm.com%2Farchitectures%2Fsystem-architectures%2Fsoftware-standar ds%2Fscmi&data=02%7C01%7Cpeng.fan%40nxp.com%7C39c55064be5 04bf248a708d82a6775cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 C0%7C637305971142916174&sdata=5kCgz3kzzk4qHy598u79zz3hV17yV zdPxM531sOAnUs%3D&reserved=0 Signed-off-by: Etienne Carriere etienne.carriere@linaro.org

---

drivers/firmware/Kconfig | 15 ++ drivers/firmware/Makefile | 1 + drivers/firmware/scmi.c | 439 ++++++++++++++++++++++++++++++++++++++ include/scmi.h | 82 +++++++ 4 files changed, 537 insertions(+) create mode 100644 drivers/firmware/scmi.c create mode 100644 include/scmi.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index b70a2063551..f7c7ee7a5aa 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -1,6 +1,21 @@ config FIRMWARE bool "Enable Firmware driver support"

+config SCMI_FIRMWARE

• bool "Enable SCMI support"
  

• select FIRMWARE
  

• select OF_TRANSLATE
  

• depends on DM_MAILBOX || ARM_SMCCC
  

• help
  

•   An SCMI agent communicates with a related SCMI server firmware
  

•   located in another sub-system, as a companion micro controller
  

•   or a companion host in the CPU system.
  

•   Communications between agent (client) and the SCMI server are
  

•   based on message exchange. Messages can be exchange over tranport
  

•   channels as a mailbox device or an Arm SMCCC service with some
  

•   piece of identified shared memory.
  

config SPL_FIRMWARE bool "Enable Firmware driver support in SPL" depends on FIRMWARE diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index a0c250a473e..3965838179f 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -2,4 +2,5 @@ obj-$(CONFIG_FIRMWARE) += firmware-uclass.o obj-$(CONFIG_$(SPL_)ARM_PSCI_FW) += psci.o obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o obj-$(CONFIG_SANDBOX) += firmware-sandbox.o +obj-$(CONFIG_SCMI_FIRMWARE) += scmi.o obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware-zynqmp.o diff --git a/drivers/firmware/scmi.c b/drivers/firmware/scmi.c new file mode 100644 index 00000000000..fa8a91c3f3d --- /dev/null +++ b/drivers/firmware/scmi.c @@ -0,0 +1,439 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2015-2019, Arm Limited and Contributors. All rights

reserved.

• • Copyright (C) 2019-2020 Linaro Limited.
• */

+#include <common.h> +#include <cpu_func.h> +#include <dm.h> +#include <errno.h> +#include <mailbox.h> +#include <memalign.h> +#include <scmi.h> +#include <asm/system.h> +#include <asm/types.h> +#include <dm/device-internal.h> +#include <dm/devres.h> +#include <dm/lists.h> +#include <dm/ofnode.h> +#include <linux/arm-smccc.h> +#include <linux/compat.h> +#include <linux/errno.h> +#include <linux/io.h> +#include <linux/ioport.h>

+#define TIMEOUT_US_10MS 10000

+struct error_code {

• int scmi;
  

• int errno;
  

+};

+static const struct error_code scmi_linux_errmap[] = {

• { .scmi = SCMI_NOT_SUPPORTED, .errno = -EOPNOTSUPP, },
  

• { .scmi = SCMI_INVALID_PARAMETERS, .errno = -EINVAL, },
  

• { .scmi = SCMI_DENIED, .errno = -EACCES, },
  

• { .scmi = SCMI_NOT_FOUND, .errno = -ENOENT, },
  

• { .scmi = SCMI_OUT_OF_RANGE, .errno = -ERANGE, },
  

• { .scmi = SCMI_BUSY, .errno = -EBUSY, },
  

• { .scmi = SCMI_COMMS_ERROR, .errno = -ECOMM, },
  

• { .scmi = SCMI_GENERIC_ERROR, .errno = -EIO, },
  

• { .scmi = SCMI_HARDWARE_ERROR, .errno = -EREMOTEIO, },
  

• { .scmi = SCMI_PROTOCOL_ERROR, .errno = -EPROTO, }, };
  

+int scmi_to_linux_errno(s32 scmi_code) +{

• int n;
  

• if (scmi_code == 0)
  

!scmi_code

...

•         return 0;
  

• for (n = 0; n < ARRAY_SIZE(scmi_linux_errmap); n++)
  

•         if (scmi_code == scmi_linux_errmap[n].scmi)
  

•                 return scmi_linux_errmap[1].errno;
  

• return -EPROTO;
  

+}

+struct method_ops {

• int (*process_msg)(struct udevice *dev, struct scmi_msg *msg);
  

• int (*remove_agent)(struct udevice *dev); };
  

+struct scmi_agent {

• struct method_ops *method_ops;
  

• void *method_priv;
  

+};

+/*

• • Shared Memory based Transport (SMT) message buffer management
• • SMT uses 28 byte header prior message payload to handle the state of
• • the communication channel realized by the shared memory area and
• • to define SCMI protocol information the payload relates to.
• */

+struct scmi_smt_header {

• __le32 reserved;
  

• __le32 channel_status;
  

+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1) +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)

• __le32 reserved1[2];
  

• __le32 flags;
  

+#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)

• __le32 length;
  

• __le32 msg_header;
  

• u8 msg_payload[0];
  

+};

+#define SMT_HEADER_TOKEN(token) (((token) << 18) & GENMASK(31, 18)) +#define SMT_HEADER_PROTOCOL_ID(proto) (((proto) << 10) & GENMASK(17, 10)) +#define SMT_HEADER_MESSAGE_TYPE(type) (((type) << 18) & GENMASK(9, 8)) +#define SMT_HEADER_MESSAGE_ID(id) ((id) & GENMASK(7, 0))

+struct scmi_shm_buf {

• u8 *buf;
  

• size_t size;
  

+};

+static int get_shm_buffer(struct udevice *dev, struct scmi_shm_buf +*shm) {

• int rc;
  

• struct ofnode_phandle_args args;
  

• struct resource resource;
  

• fdt32_t faddr;
  

• phys_addr_t paddr;
  

• rc = dev_read_phandle_with_args(dev, "shmem", NULL, 0, 0, &args);
  

• if (rc)
  

•         return rc;
  

• rc = ofnode_read_resource(args.node, 0, &resource);
  

• if (rc)
  

•         return rc;
  

• faddr = cpu_to_fdt32(resource.start);
  

• paddr = ofnode_translate_address(args.node, &faddr);
  

• shm->size = resource_size(&resource);
  

• if (shm->size < sizeof(struct scmi_smt_header)) {
  

•         dev_err(dev, "Shared memory buffer too small\n");
  

•         return -EINVAL;
  

• }
  

• shm->buf = devm_ioremap(dev, paddr, shm->size);
  

• if (!shm->buf)
  

•         return -ENOMEM;
  

• if (dcache_status())
  

•         mmu_set_region_dcache_behaviour((uintptr_t)shm->buf,
  

•                                         shm->size, DCACHE_OFF);
  

Could dev_read_addr be used to replace the upper code block?

I'll check that but i'm not sure it will simplify.

...

• return 0;
  

+}

+static int write_msg_to_smt(struct udevice *dev, struct scmi_shm_buf *shm_buf,

•                     struct scmi_msg *msg)
  

+{

• struct scmi_smt_header *hdr = (void *)shm_buf->buf;
  

• if ((!msg->in_msg && msg->in_msg_sz) ||
  

•     (!msg->out_msg && msg->out_msg_sz))
  

•         return -EINVAL;
  

• if (!(hdr->channel_status &
  

SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE)) {

•         dev_dbg(dev, "Channel busy\n");
  

•         return -EBUSY;
  

• }
  

• if (shm_buf->size < (sizeof(*hdr) + msg->in_msg_sz) ||
  

•     shm_buf->size < (sizeof(*hdr) + msg->out_msg_sz)) {
  

•         dev_dbg(dev, "Buffer too small\n");
  

•         return -ETOOSMALL;
  

• }
  

• /* Load message in shared memory */
  

• hdr->channel_status &= ~SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE;
  

• hdr->length = msg->in_msg_sz + sizeof(hdr->msg_header);
  

• hdr->msg_header = SMT_HEADER_TOKEN(0) |
  

•                   SMT_HEADER_MESSAGE_TYPE(0) |
  

•                   SMT_HEADER_PROTOCOL_ID(msg->protocol_id) |
  

•                   SMT_HEADER_MESSAGE_ID(msg->message_id);
  

• memcpy(hdr->msg_payload, msg->in_msg, msg->in_msg_sz);
  

memcpy_toio?

...

• return 0;
  

+}

+static int read_resp_from_smt(struct udevice *dev, struct scmi_shm_buf *shm_buf,

•                       struct scmi_msg *msg)
  

+{

• struct scmi_smt_header *hdr = (void *)shm_buf->buf;
  

• if (!(hdr->channel_status &
  

SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE)) {

•         dev_err(dev, "Channel unexpectedly busy\n");
  

•         return -EBUSY;
  

• }
  

• if (hdr->channel_status & SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR)
  

{

•         dev_err(dev, "Channel error reported, reset channel\n");
  

•         return -ECOMM;
  

• }
  

• if (hdr->length > msg->out_msg_sz + sizeof(hdr->msg_header)) {
  

•         dev_err(dev, "Buffer to small\n");
  

•         return -ETOOSMALL;
  

• }
  

• /* Get the data */
  

• msg->out_msg_sz = hdr->length - sizeof(hdr->msg_header);
  

• memcpy(msg->out_msg, hdr->msg_payload, msg->out_msg_sz);
  

• return 0;
  

+}

+static void clear_smt_channel(struct scmi_shm_buf *shm_buf) {

• struct scmi_smt_header *hdr = (void *)shm_buf->buf;
  

• hdr->channel_status &=
  

~SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR; +}

+struct scmi_mbox_channel {

• struct scmi_shm_buf shm_buf;
  

• struct mbox_chan mbox;
  

• ulong timeout_us;
  

+};

+static int mbox_process_msg(struct udevice *dev, struct scmi_msg *msg) +{

• struct scmi_agent *agent = dev_get_priv(dev);
  

• struct scmi_mbox_channel *chan = agent->method_priv;
  

• int rc;
  

• rc = write_msg_to_smt(dev, &chan->shm_buf, msg);
  

• if (rc)
  

•         return rc;
  

• /* Give shm addr to mbox in case it is meaningful */
  

• rc = mbox_send(&chan->mbox, chan->shm_buf.buf);
  

• if (rc) {
  

•         dev_err(dev, "Message send failed: %d\n", rc);
  

•         goto out;
  

• }
  

• /* Receive the response */
  

• rc = mbox_recv(&chan->mbox, chan->shm_buf.buf, chan->timeout_us);
  

• if (rc) {
  

•         dev_err(dev, "Response failed: %d, abort\n", rc);
  

•         goto out;
  

• }
  

• rc = read_resp_from_smt(dev, &chan->shm_buf, msg);
  

+out:

• clear_smt_channel(&chan->shm_buf);
  

• return rc;
  

+}

+struct method_ops mbox_channel_ops = {

• .process_msg = mbox_process_msg,
  

+};

+static int probe_mailbox_channel(struct udevice *dev) {

• struct scmi_agent *agent = dev_get_priv(dev);
  

• struct scmi_mbox_channel *chan;
  

• int rc;
  

• chan = devm_kzalloc(dev, sizeof(*chan), GFP_KERNEL);
  

• if (!chan)
  

•         return -ENOMEM;
  

• chan->timeout_us = TIMEOUT_US_10MS;
  

• rc = mbox_get_by_index(dev, 0, &chan->mbox);
  

• if (rc) {
  

•         dev_err(dev, "Failed to find mailbox: %d\n", rc);
  

•         goto out;
  

• }
  

• rc = get_shm_buffer(dev, &chan->shm_buf);
  

• if (rc)
  

•         dev_err(dev, "Failed to get shm resources: %d\n", rc);
  

+out:

• if (rc) {
  

•         devm_kfree(dev, chan);
  

• } else {
  

•         agent->method_ops = &mbox_channel_ops;
  

•         agent->method_priv = (void *)chan;
  

• }
  

• return rc;
  

+}

+struct scmi_arm_smc_channel {

• ulong func_id;
  

• struct scmi_shm_buf shm_buf;
  

+};

+#define SMCCC_RET_NOT_SUPPORTED ((unsigned long)-1)

+static int arm_smc_process_msg(struct udevice *dev, struct scmi_msg +*msg) {

• struct scmi_agent *agent = dev_get_priv(dev);
  

• struct scmi_arm_smc_channel *chan = agent->method_priv;
  

• struct arm_smccc_res res;
  

• int rc;
  

• rc = write_msg_to_smt(dev, &chan->shm_buf, msg);
  

• if (rc)
  

•         return rc;
  

• arm_smccc_smc(chan->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
  

• if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
  

•         rc = -EINVAL;
  

• else
  

•         rc = read_resp_from_smt(dev, &chan->shm_buf, msg);
  

• clear_smt_channel(&chan->shm_buf);
  

Should the clear be done in firmware side?

Clear is done by the client once it has read out the status set by the firmware.

...

• return rc;
  

+}

+struct method_ops arm_smc_channel_ops = {

• .process_msg = arm_smc_process_msg,
  

+};

+static int probe_arm_smc_channel(struct udevice *dev) {

• struct scmi_agent *agent = dev_get_priv(dev);
  

• struct scmi_arm_smc_channel *chan;
  

• ofnode node = dev_ofnode(dev);
  

• u32 func_id;
  

• int rc;
  

• chan = devm_kzalloc(dev, sizeof(*chan), GFP_KERNEL);
  

• if (!chan)
  

•         return -ENOMEM;
  

• if (ofnode_read_u32(node, "arm,smc-id", &func_id)) {
  

•         dev_err(dev, "Missing property func-id\n");
  

•         return -EINVAL;
  

• }
  

• chan->func_id = func_id;
  

• rc = get_shm_buffer(dev, &chan->shm_buf);
  

• if (rc) {
  

•         dev_err(dev, "Failed to get shm resources: %d\n", rc);
  

•         return rc;
  

• }
  

• agent->method_ops = &arm_smc_channel_ops;
  

• agent->method_priv = (void *)chan;
  

• return rc;
  

+}

+/*

• • Exported functions by the SCMI agent */

+int scmi_send_and_process_msg(struct udevice *dev, struct scmi_msg +*msg) {

• struct scmi_agent *agent = dev_get_priv(dev);
  

• return agent->method_ops->process_msg(dev, msg); }
  

+static int scmi_remove(struct udevice *dev) {

• struct scmi_agent *agent = dev_get_priv(dev);
  

• if (agent->method_ops->remove_agent)
  

•         return agent->method_ops->remove_agent(dev);
  

• return 0;
  

+}

+enum scmi_transport_channel {

• SCMI_MAILBOX_TRANSPORT,
  

• SCMI_ARM_SMCCC_TRANSPORT,
  

+};

+static int scmi_probe(struct udevice *dev) {

• switch (dev_get_driver_data(dev)) {
  

• case SCMI_MAILBOX_TRANSPORT:
  

•         if (IS_ENABLED(CONFIG_DM_MAILBOX))
  

•                 return probe_mailbox_channel(dev);
  

•         break;
  

• case SCMI_ARM_SMCCC_TRANSPORT:
  

•         if (IS_ENABLED(CONFIG_ARM_SMCCC))
  

•                 return probe_arm_smc_channel(dev);
  

•         break;
  

• default:
  

•         break;
  

• }
  

• return -EINVAL;
  

+}

+static int scmi_bind(struct udevice *dev) {

• int rc = 0;
  

• ofnode node;
  

• struct driver *drv;
  

• dev_for_each_subnode(node, dev) {
  

•         u32 protocol_id;
  

•         if (!ofnode_is_available(node))
  

•                 continue;
  

•         if (ofnode_read_u32(node, "reg", &protocol_id))
  

•                 continue;
  

•         switch (protocol_id) {
  

•         default:
  

•                 dev_info(dev, "Ignore unsupported SCMI protocol %u\n",
  

•                          protocol_id);
  

•                 continue;
  

•         }
  

•         rc = device_bind_ofnode(dev, drv, ofnode_get_name(node),
  

•                                 NULL, node, NULL);
  

•         if (rc)
  

•                 break;
  

• }
  

• if (rc)
  

•         device_unbind(dev);
  

• return rc;
  

+}

+static const struct udevice_id scmi_ids[] = { #ifdef CONFIG_DM_MAILBOX

• { .compatible = "arm,scmi", .data = SCMI_MAILBOX_TRANSPORT },
  

#endif +#ifdef CONFIG_ARM_SMCCC

• { .compatible = "arm,scmi-smc", .data =
  

SCMI_ARM_SMCCC_TRANSPORT }, +#endif

To minimize the code base, how about following Linux kernel to split mailbox and smc to different files?

Is it preferred instead of using several #ifdef in this single source file? If splitting into several source files, it also means each source file will need to export API function that in the end, are local to SCMI agent drivers. Since the implementation is quite short , i though a single source for the whole SCMI agent base + transport layers was simpler.

Regards, Etienne

Regards, Peng.

...

• { }
  

+};

+U_BOOT_DRIVER(scmi) = {

• .name           = "scmi",
  

• .id             = UCLASS_FIRMWARE,
  

• .of_match       = scmi_ids,
  

• .priv_auto_alloc_size = sizeof(struct scmi_agent),
  

• .bind           = scmi_bind,
  

• .probe          = scmi_probe,
  

• .remove         = scmi_remove,
  

• .flags          = DM_FLAG_OS_PREPARE,
  

+}; diff --git a/include/scmi.h b/include/scmi.h new file mode 100644 index 00000000000..e12e322991d --- /dev/null +++ b/include/scmi.h @@ -0,0 +1,82 @@ +/* SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */ +/*

• • Copyright (c) 2015-2019, Arm Limited and Contributors. All rights

reserved.

• • Copyright (C) 2019, Linaro Limited
• */

+#ifndef SCMI_H +#define SCMI_H

+#include <asm/types.h>

+/**

• • An SCMI agent represent on communication path from a device driver

+to

• • the remote SCMI server which driver sends messages to and receives
• • response messages from.
• */

+struct scmi_agent;

+enum scmi_std_protocol {

• SCMI_PROTOCOL_ID_BASE = 0x10,
  

• SCMI_PROTOCOL_ID_POWER_DOMAIN = 0x11,
  

• SCMI_PROTOCOL_ID_SYSTEM = 0x12,
  

• SCMI_PROTOCOL_ID_PERF = 0x13,
  

• SCMI_PROTOCOL_ID_CLOCK = 0x14,
  

• SCMI_PROTOCOL_ID_SENSOR = 0x15,
  

• SCMI_PROTOCOL_ID_RESET_DOMAIN = 0x16,
  

+};

+enum scmi_status_code {

• SCMI_SUCCESS =  0,
  

• SCMI_NOT_SUPPORTED = -1,
  

• SCMI_INVALID_PARAMETERS = -2,
  

• SCMI_DENIED = -3,
  

• SCMI_NOT_FOUND = -4,
  

• SCMI_OUT_OF_RANGE = -5,
  

• SCMI_BUSY = -6,
  

• SCMI_COMMS_ERROR = -7,
  

• SCMI_GENERIC_ERROR = -8,
  

• SCMI_HARDWARE_ERROR = -9,
  

• SCMI_PROTOCOL_ERROR = -10,
  

+};

+/*

• • struct scmi_msg - Context of a SCMI message sent and the response

+received

• • @protocol_id: SCMI protocol ID
• • @message_id: SCMI message ID for a defined protocol ID
• • @in_msg: pointer to the message payload sent by the driver
• • @in_msg_sz: byte size of the message payload sent
• • @out_msg: pointer to buffer to store response message payload
• • @out_msg_size: Byte size of the response buffer or payload */

+struct scmi_msg {

• unsigned int protocol_id;
  

• unsigned int message_id;
  

• u8 *in_msg;
  

• size_t in_msg_sz;
  

• u8 *out_msg;
  

• size_t out_msg_sz;
  

+};

+/**

• • scmi_send_and_process_msg() - send and process a SCMI message
• • Send a message to a SCMI server through a target SCMI agent device.
• • Caller sets scmi_msg::out_msg_sz to the output message buffer size.
• • On return, scmi_msg::out_msg_sz stores the response payload size.
• • @dev: SCMI agent device
• • @msg: Message structure reference
• • @return 0 on success, a negative errno otherwise */ int

+scmi_send_and_process_msg(struct udevice *dev, struct scmi_msg *msg);

+/**

• • scmi_to_linux_errno() - Convert an SCMI error code into a Linux

+errno code

• • @scmi_errno: SCMI error code value
• • @return 0 for successful status and a negative errno otherwise */

+int scmi_to_linux_errno(s32 scmi_errno);

+#endif /* SCMI_H */

2.17.1