Hi Etienne,
On Tue, 4 Aug 2020 at 03:33, Etienne Carriere etienne.carriere@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@chromium.org wrote:
Hi Etienne,
On Fri, 17 Jul 2020 at 09:38, Etienne Carriere etienne.carriere@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-standa... 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 FIRMWAREselect OF_TRANSLATEdepends on DM_MAILBOX || ARM_SMCCChelpAn SCMI agent communicates with a related SCMI server firmwarePlease write out SCMI in full somewhere and add a link to the spec.
located in another sub-system, as a companion micro controlleror a companion host in the CPU system.Communications between agent (client) and the SCMI server arebased on message exchange. Messages can be exchange over tranportchannels as a mailbox device or an Arm SMCCC service with somepiece 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