From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 15 Aug 2020 21:39:30 -0600 Subject: [PATCH 1/4] firmware: add new driver for SCMI firmwares In-Reply-To: References: <20200717153731.10643-1-etienne.carriere@linaro.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Etienne, On Tue, 4 Aug 2020 at 03:33, Etienne Carriere 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 wrote: > > > > Hi Etienne, > > > > On Fri, 17 Jul 2020 at 09:38, Etienne Carriere > > 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 > > > --- > > > > > > 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 > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#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? 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