[PATCH 1/4] firmware: add new driver for SCMI firmwares
Simon Glass
sjg at chromium.org
Sun Aug 16 05:39:30 CEST 2020
Hi Etienne,
On Tue, 4 Aug 2020 at 03:33, Etienne Carriere
<etienne.carriere at linaro.org> wrote:
>
> Hello Simon,
>
> Thanks for the feedback, I'll fix the changes in my v2.
> and sorry for these delayed answers.
>
> On Sun, 26 Jul 2020 at 16:55, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Etienne,
> >
> > On Fri, 17 Jul 2020 at 09:38, Etienne Carriere
> > <etienne.carriere at linaro.org> wrote:
> > >
> > > 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://developer.arm.com/architectures/system-architectures/software-standards/scmi
> > > Signed-off-by: Etienne Carriere <etienne.carriere at 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
> >
> > Please write out SCMI in full somewhere and add a link to the spec.
> >
> > > + 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 {
> >
> > Function comment
> >
> > > + 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)
> > > + 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);
> >
> > Looks like this should be a new uclass and child driver?
>
> I think a uclass is overkilling here.
> A scmi agent device is a firmware driver that binds to devices of
> other uclasses (clock, reset, ...).
> When scmi agent device is probed to get the related clock, reset, ...
> devices be probed.
> So it does not need any child.
> (see also related comment below)
I'm not sure I agree, but let's see how it turns out.
>
>
> >
> > Also we should have a sandbox implementation so this code can be tested.
>
> Ok, I understand the sandbox test stuff.
> Should it be part of the v2 this series, can it be done in parallel,
> or a later v<N>?
I think it should be part of the series. New code should have tests.
[..]
> > > +
> > > +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;
> >
> > ENOTSUP?
>
> ENOTSUP stands for protocols.
> Here, it is the communication link that is bugged. Maybe ENXIO?
OK...just want to avoid -EINVAL as it is so widely used for
device-tree reading; it is too generic.
> > > +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);
> >
> > method_ops should be the uclass operations. I think you need a new
> > uclass or add something to UCLASS_FIRMWARE.
>
> Hmmm, I see you point. I'll consider.
> Yet, i think it would be a uclass with a unique type of child: a scmi
> agent driver.
> Your thoughts?
Yes that's right.
>
>
>
> >
> > > +
> > > + 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);
> >
> > Can we instead add a compatible string to the device tree so driver
> > model creates these children automatically?
>
> Here the bound device already belongs to an existing uclass driver, at
> least we expect them to.
> These are clock, reset, power_domain uclass.
Right, but if the child nodes have compatible strings, driver model
will automatically bind the children and you don't need to write this
code.
[..]
Regards,
Simon
More information about the U-Boot
mailing list