[PATCH v1 0/2] Introduce Meson SM driver
Hello!
This patchset adds Meson Secure Monitor driver, which export following generic API:
- sm_call_read() - sm_call_write() - sm_call()
Also, now use this new API in arch/arm/mach-meson/sm.c helper functions.
Alexey Romanov (2): drivers: firmware: introduce Meson Secure Monitor driver asm/arch: mach-meson: use secure monitor driver
arch/arm/mach-meson/Kconfig | 1 + arch/arm/mach-meson/sm.c | 161 +++++++++++++--------- drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 8 files changed, 373 insertions(+), 64 deletions(-) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesn't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call() - sm_call_write() - sm_call_read()
A typical driver usage example is shown here:
1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); 2. handle = meson_sm_get_handle(dev); 3. handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru --- arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig index 669ca09a00a..b8746d27f63 100644 --- a/arch/arm/mach-meson/Kconfig +++ b/arch/arm/mach-meson/Kconfig @@ -11,6 +11,7 @@ config MESON64_COMMON select PWRSEQ select MMC_PWRSEQ select BOARD_LATE_INIT + select MESON_FIRMWARE imply CMD_DM
config MESON_GX diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index eae1c8ddc9f..17b70fdea6d 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -37,6 +37,15 @@ config ZYNQMP_FIRMWARE Say yes to enable ZynqMP firmware interface driver. If in doubt, say N.
+config MESON_FIRMWARE + bool "Meson Firmware interface" + select FIRMWARE + help + This option enables Meson firmware interface, + which is used by different drivers to communicate + with the firmware for various platform management + services. + config ARM_SMCCC_FEATURES bool "Arm SMCCC features discovery" depends on ARM_PSCI_FW @@ -45,4 +54,5 @@ config ARM_SMCCC_FEATURES the PSCI driver is always probed and binds dirvers registered to the Arm SMCCC services if any and reported as supported by the SMCCC firmware.
+source "drivers/firmware/meson/Kconfig" source "drivers/firmware/scmi/Kconfig" diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 7ce83d72bd3..a6300be27ad 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o obj-$(CONFIG_SANDBOX) += firmware-sandbox.o obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware-zynqmp.o obj-$(CONFIG_SCMI_FIRMWARE) += scmi/ +obj-$(CONFIG_MESON_FIRMWARE) += meson/ diff --git a/drivers/firmware/meson/Kconfig b/drivers/firmware/meson/Kconfig new file mode 100644 index 00000000000..0fd4f3251e1 --- /dev/null +++ b/drivers/firmware/meson/Kconfig @@ -0,0 +1,6 @@ +config MESON_SM + bool "Amlogic Secure Monitor driver" + depends on ARCH_MESON + default y + help + Say y here to enable the Amlogic secure monitor driver. diff --git a/drivers/firmware/meson/Makefile b/drivers/firmware/meson/Makefile new file mode 100644 index 00000000000..b5d26f150b0 --- /dev/null +++ b/drivers/firmware/meson/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only + +obj-$(CONFIG_MESON_SM) += meson_sm.o diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c new file mode 100644 index 00000000000..28eacb89810 --- /dev/null +++ b/drivers/firmware/meson/meson_sm.c @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2023 SberDevices, Inc. + * + * Author: Alexey Romanov avromanov@sberdevices.ru + */ + +#include <dm.h> +#include <common.h> +#include <stdlib.h> +#include <regmap.h> +#include <syscon.h> +#include <asm/ptrace.h> +#include <asm/system.h> +#include <linux/err.h> +#include <linux/sizes.h> +#include <linux/bitfield.h> +#include <meson/sm_handle.h> + +struct meson_sm_cmd { + u32 smc_id; +}; + +#define SET_CMD(index, id) \ + [index] = { \ + .smc_id = id, \ + } + +struct meson_sm_data { + u32 cmd_get_shmem_in; + u32 cmd_get_shmem_out; + unsigned int shmem_size; + struct meson_sm_cmd cmd[]; +}; + +struct meson_sm_priv { + void *sm_shmem_in; + void *sm_shmem_out; + struct meson_sm_handle handle; + const struct meson_sm_data *data; +}; + +static unsigned long __meson_sm_call(u32 cmd, u32 arg0, u32 arg1, u32 arg2, + u32 arg3, u32 arg4) +{ + struct pt_regs r; + + r.regs[0] = cmd; + r.regs[1] = arg0; + r.regs[2] = arg1; + r.regs[3] = arg2; + r.regs[4] = arg3; + r.regs[5] = arg4; + + smc_call(&r); + + return r.regs[0]; +}; + +static u32 meson_sm_get_cmd(const struct meson_sm_data *data, + u32 cmd_index) +{ + struct meson_sm_cmd cmd; + + if (cmd_index >= MESON_SMC_CMD_COUNT) + return 0; + + cmd = data->cmd[cmd_index]; + return cmd.smc_id; +} + +static int meson_sm_call(struct udevice *dev, u32 cmd_index, u32 *retval, + u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 cmd, ret; + + cmd = meson_sm_get_cmd(priv->data, cmd_index); + if (!cmd) + return -ENOENT; + + ret = __meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4); + if (retval) + *retval = ret; + + return 0; +} + +static int meson_sm_call_read(struct udevice *dev, void *buffer, size_t size, + u32 cmd_index, u32 offset, u32 cnt) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 nbytes; + int ret; + + if (!buffer || size > priv->data->shmem_size) + return -EINVAL; + + ret = meson_sm_call(dev, cmd_index, &nbytes, offset, cnt, 0, 0, 0); + if (ret) + return ret; + + if (nbytes > size) + return -ENOBUFS; + + /* In some cases (for example GET_CHIP_ID command), + * SMC doesn't return the number of bytes read, even + * though the bytes were actually read into sm_shmem_out. + * So this check is needed. + */ + ret = nbytes; + if (!nbytes) + nbytes = size; + + memcpy(buffer, priv->sm_shmem_out, nbytes); + + return ret; +} + +static int meson_sm_call_write(struct udevice *dev, void *buffer, size_t size, + u32 cmd_index, u32 offset, u32 cnt) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 nbytes; + int ret; + + if (!buffer || size > priv->data->shmem_size) + return -EINVAL; + + memcpy(priv->sm_shmem_in, buffer, size); + + ret = meson_sm_call(dev, cmd_index, &nbytes, offset, cnt, 0, 0, 0); + if (ret) + return ret; + + if (!nbytes) + return -EIO; + + return nbytes; +} + +struct meson_sm_handle *meson_sm_get_handle(struct udevice *dev) +{ + struct meson_sm_priv *priv; + struct meson_sm_handle *handle; + + priv = dev_get_priv(dev); + if (!priv) + return ERR_PTR(-EINVAL); + + handle = &priv->handle; + + return handle; +} + +static const struct meson_sm_ops sm_ops = { + .sm_call = meson_sm_call, + .sm_call_read = meson_sm_call_read, + .sm_call_write = meson_sm_call_write, +}; + +static int meson_sm_probe(struct udevice *dev) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + + priv->handle.ops = sm_ops; + priv->data = (struct meson_sm_data *)dev_get_driver_data(dev); + if (!priv->data) + return -EINVAL; + + priv->sm_shmem_in = + (void *)__meson_sm_call(priv->data->cmd_get_shmem_in, 0, 0, 0, 0, 0); + + if (!priv->sm_shmem_in) + return -ENOMEM; + + priv->sm_shmem_out = + (void *)__meson_sm_call(priv->data->cmd_get_shmem_out, 0, 0, 0, 0, 0); + + if (!priv->sm_shmem_out) + return -ENOMEM; + + pr_debug("meson sm driver probed\n" + "shmem_in addr: 0x%p, shmem_out addr: 0x%p\n", + priv->sm_shmem_in, + priv->sm_shmem_out); + + return 0; +} + +static const struct meson_sm_data meson_sm_gxbb_data = { + .cmd_get_shmem_in = 0x82000020, + .cmd_get_shmem_out = 0x82000021, + .shmem_size = SZ_4K, + .cmd = { + SET_CMD(MESON_SMC_CMD_EFUSE_READ, 0x82000030), + SET_CMD(MESON_SMC_CMD_EFUSE_WRITE, 0x82000031), + SET_CMD(MESON_SMC_CMD_CHIP_ID_GET, 0x82000044), + SET_CMD(MESON_SMC_CMD_PWRDM_SET, 0x82000093), + }, +}; + +static const struct udevice_id meson_sm_ids[] = { + { + .compatible = "amlogic,meson-gxbb-sm", + .data = (ulong)&meson_sm_gxbb_data, + }, + { } +}; + +U_BOOT_DRIVER(meson_sm) = { + .name = "meson_sm", + .id = UCLASS_FIRMWARE, + .of_match = meson_sm_ids, + .probe = meson_sm_probe, + .priv_auto = sizeof(struct meson_sm_priv), +}; diff --git a/include/meson/sm_handle.h b/include/meson/sm_handle.h new file mode 100644 index 00000000000..a3c69b77bd6 --- /dev/null +++ b/include/meson/sm_handle.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2023 SberDevices, Inc. + * + * Author: Alexey Romanov avromanov@sberdevices.ru + */ + +#ifndef _MESON_SM_H_ +#define _MESON_SM_H_ + +#include <dm/device.h> + +enum meson_smc_cmd { + MESON_SMC_CMD_EFUSE_READ, + MESON_SMC_CMD_EFUSE_WRITE, + MESON_SMC_CMD_CHIP_ID_GET, + MESON_SMC_CMD_PWRDM_SET, + MESON_SMC_CMD_COUNT, +}; + +struct meson_sm_ops { + int (*sm_call)(struct udevice *dev, u32 cmd, u32 *ret, + u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4); + + int (*sm_call_read)(struct udevice *dev, void *buffer, + size_t size, u32 cmd, u32 offset, u32 cnt); + + int (*sm_call_write)(struct udevice *dev, void *buffer, + size_t size, u32 cmd, u32 offset, u32 cnt); +}; + +struct meson_sm_handle { + struct meson_sm_ops ops; +}; + +struct meson_sm_handle *meson_sm_get_handle(struct udevice *dev); + +#endif /* _MESON_SM_H_ */
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesn't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
Regards, SImon
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesni't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Regards, SImon
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesni't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Regards, Simon
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesni't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
Regards, Simon
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesni't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
Also instead of:
1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote:
At the moment, only smc API is a set of functions in arch/arm/mach-meson/sm.c. This approach is hard to configure and also doesni't contain any generic API for calling smc.
This patch add Meson SM driver with generic API (struct meson_sm_ops):
- sm_call()
- sm_call_write()
- sm_call_read()
A typical driver usage example is shown here:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
- handle = meson_sm_get_handle(dev);
- handle->ops.sm_call(dev, cmd, ...);
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru
arch/arm/mach-meson/Kconfig | 1 + drivers/firmware/Kconfig | 10 ++ drivers/firmware/Makefile | 1 + drivers/firmware/meson/Kconfig | 6 + drivers/firmware/meson/Makefile | 3 + drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ include/meson/sm_handle.h | 38 ++++++ 7 files changed, 276 insertions(+) create mode 100644 drivers/firmware/meson/Kconfig create mode 100644 drivers/firmware/meson/Makefile create mode 100644 drivers/firmware/meson/meson_sm.c create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
+AKASHI Takahiro
Hi Alexey,
On Tue, 11 Jul 2023 at 04:25, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
Hi Alexey,
On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote: > > At the moment, only smc API is a set of functions in > arch/arm/mach-meson/sm.c. This approach is hard to configure > and also doesni't contain any generic API for calling smc. > > This patch add Meson SM driver with generic API (struct meson_sm_ops): > > - sm_call() > - sm_call_write() > - sm_call_read() > > A typical driver usage example is shown here: > > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); > 2. handle = meson_sm_get_handle(dev); > 3. handle->ops.sm_call(dev, cmd, ...); > > Signed-off-by: Alexey Romanov avromanov@sberdevices.ru > --- > arch/arm/mach-meson/Kconfig | 1 + > drivers/firmware/Kconfig | 10 ++ > drivers/firmware/Makefile | 1 + > drivers/firmware/meson/Kconfig | 6 + > drivers/firmware/meson/Makefile | 3 + > drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ > include/meson/sm_handle.h | 38 ++++++ > 7 files changed, 276 insertions(+) > create mode 100644 drivers/firmware/meson/Kconfig > create mode 100644 drivers/firmware/meson/Makefile > create mode 100644 drivers/firmware/meson/meson_sm.c > create mode 100644 include/meson/sm_handle.h
Please can you use the remoteproc uclass for this and add a proper driver?
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
Honestly I'm not sure, but I question why you are putting all this on the reviewer. Can't you look around the code base and figure out a sensible approach and defend it in your patch?
I've just reviewed the SCMI stuff which use UCLASS_MISC in one case. I'm not sure if this is related to firmware or not, or whether what you are doing relates.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
OK, well please take a look at all this and see what you think is best. If UCLASS_FIRMWARE is correct, then please create a proper firmware.h file and define what the uclass operations are. You can't just randomly create your own private operations and ignore the rest of the code base.
Driver model is intended to bring order to the chaos that used to exist with drivers in U-Boot. Please see [1] for an introduction.
Please try to dig in and understand how things should be done. It is important for the health of the project.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
-- Thank you, Alexey
[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_A...
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Hi Alexey,
On Tue, 11 Jul 2023 at 04:25, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello, Simon!
On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote: > Hi Alexey, > > On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote: > > > > At the moment, only smc API is a set of functions in > > arch/arm/mach-meson/sm.c. This approach is hard to configure > > and also doesni't contain any generic API for calling smc. > > > > This patch add Meson SM driver with generic API (struct meson_sm_ops): > > > > - sm_call() > > - sm_call_write() > > - sm_call_read() > > > > A typical driver usage example is shown here: > > > > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); > > 2. handle = meson_sm_get_handle(dev); > > 3. handle->ops.sm_call(dev, cmd, ...); > > > > Signed-off-by: Alexey Romanov avromanov@sberdevices.ru > > --- > > arch/arm/mach-meson/Kconfig | 1 + > > drivers/firmware/Kconfig | 10 ++ > > drivers/firmware/Makefile | 1 + > > drivers/firmware/meson/Kconfig | 6 + > > drivers/firmware/meson/Makefile | 3 + > > drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ > > include/meson/sm_handle.h | 38 ++++++ > > 7 files changed, 276 insertions(+) > > create mode 100644 drivers/firmware/meson/Kconfig > > create mode 100644 drivers/firmware/meson/Makefile > > create mode 100644 drivers/firmware/meson/meson_sm.c > > create mode 100644 include/meson/sm_handle.h > > Please can you use the remoteproc uclass for this and add a proper driver? >
I don't see it architecturally well. Can you explain please?
This driver is just ARM SMC fw interface. There seems to be nothing to do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
Honestly I'm not sure, but I question why you are putting all this on the reviewer. Can't you look around the code base and figure out a sensible approach and defend it in your patch?
I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c? If so, it has nothing to do with the discussion here.
It is a kind of pseudo device for dm ut, which is subject to manage with the faked scmi server in sandbox (sandbox-scmi_agent.c). It is connected, for instance, to clock lines or vol regulators. (see sandbox's test.dts.)
I believe that similar usages can be seen across drivers/, for instance, drivers/clk/clk_sandbox_test.c.
Do you see anything wrong with it?
I'm not sure if this is related to firmware or not, or whether what you are doing relates.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call) instruction escalates the cpu's EL (Execution Level) to EL3 (secure). This interface is clearly defined by Arm, and many high level services are defined and implemented on top of that. PSCI and FF-A are ones among those users, and even vendor specific functions may be allowed.
From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is
just a C helper function around one SMC assembly. I don't think we need to have UCLASS_SMC as it is basically a communication method.
Nevertheless, I agree with Simon in the point that the SMC related code can be a bit reworked since there are duplicated code. Especially, the followings are essentially the same: -- arch/arm/cpu/armv8/fwcall.c with smc_call() psci_system_reset() psci_system_off() -- drivers/firmware/psci.c with arm_smccc_smc() psci_sys_reset() psci_sys_poweroff()
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to this class do not have any common operations. (And UCLASS_MISC as well.)
-Takahiro Akashi
OK, well please take a look at all this and see what you think is best. If UCLASS_FIRMWARE is correct, then please create a proper firmware.h file and define what the uclass operations are. You can't just randomly create your own private operations and ignore the rest of the code base.
Driver model is intended to bring order to the chaos that used to exist with drivers in U-Boot. Please see [1] for an introduction.
Please try to dig in and understand how things should be done. It is important for the health of the project.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
-- Thank you, Alexey
[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_A...
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
Hi Alexey,
On Tue, 11 Jul 2023 at 04:25, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
Hi Alexey,
On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote: > > Hello, Simon! > > On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote: > > Hi Alexey, > > > > On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote: > > > > > > At the moment, only smc API is a set of functions in > > > arch/arm/mach-meson/sm.c. This approach is hard to configure > > > and also doesni't contain any generic API for calling smc. > > > > > > This patch add Meson SM driver with generic API (struct meson_sm_ops): > > > > > > - sm_call() > > > - sm_call_write() > > > - sm_call_read() > > > > > > A typical driver usage example is shown here: > > > > > > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); > > > 2. handle = meson_sm_get_handle(dev); > > > 3. handle->ops.sm_call(dev, cmd, ...); > > > > > > Signed-off-by: Alexey Romanov avromanov@sberdevices.ru > > > --- > > > arch/arm/mach-meson/Kconfig | 1 + > > > drivers/firmware/Kconfig | 10 ++ > > > drivers/firmware/Makefile | 1 + > > > drivers/firmware/meson/Kconfig | 6 + > > > drivers/firmware/meson/Makefile | 3 + > > > drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ > > > include/meson/sm_handle.h | 38 ++++++ > > > 7 files changed, 276 insertions(+) > > > create mode 100644 drivers/firmware/meson/Kconfig > > > create mode 100644 drivers/firmware/meson/Makefile > > > create mode 100644 drivers/firmware/meson/meson_sm.c > > > create mode 100644 include/meson/sm_handle.h > > > > Please can you use the remoteproc uclass for this and add a proper driver? > > > > I don't see it architecturally well. Can you explain please? > > This driver is just ARM SMC fw interface. There seems to be nothing to > do here for remoteproc uclass.
Well you seem to be implementing a remote CPU interface, which is what remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
Also there is a pending series on FFA - is that related? It uses smc from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
Honestly I'm not sure, but I question why you are putting all this on the reviewer. Can't you look around the code base and figure out a sensible approach and defend it in your patch?
I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c? If so, it has nothing to do with the discussion here.
It is a kind of pseudo device for dm ut, which is subject to manage with the faked scmi server in sandbox (sandbox-scmi_agent.c). It is connected, for instance, to clock lines or vol regulators. (see sandbox's test.dts.)
I believe that similar usages can be seen across drivers/, for instance, drivers/clk/clk_sandbox_test.c.
Do you see anything wrong with it?
At this point I'm just confused. Perhaps MISC and FIRMWARE should not be used as much? It seems that FIRMWARE doesn't really mean anything at present?
I'm not sure if this is related to firmware or not, or whether what you are doing relates.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call) instruction escalates the cpu's EL (Execution Level) to EL3 (secure). This interface is clearly defined by Arm, and many high level services are defined and implemented on top of that. PSCI and FF-A are ones among those users, and even vendor specific functions may be allowed.
From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is just a C helper function around one SMC assembly. I don't think we need to have UCLASS_SMC as it is basically a communication method.
Nevertheless, I agree with Simon in the point that the SMC related code can be a bit reworked since there are duplicated code. Especially, the followings are essentially the same: -- arch/arm/cpu/armv8/fwcall.c with smc_call() psci_system_reset() psci_system_off() -- drivers/firmware/psci.c with arm_smccc_smc() psci_sys_reset() psci_sys_poweroff()
OK, what that is a start.
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to this class do not have any common operations.
Well misc.h does actually have operations, but yes, people misuse it. Perhaps we should invent something else for these people?
For FIRMWARE, what should it mean? It needs to have a firmware.h file with some information.
(And UCLASS_MISC as well.)
-Takahiro Akashi
OK, well please take a look at all this and see what you think is best. If UCLASS_FIRMWARE is correct, then please create a proper firmware.h file and define what the uclass operations are. You can't just randomly create your own private operations and ignore the rest of the code base.
Driver model is intended to bring order to the chaos that used to exist with drivers in U-Boot. Please see [1] for an introduction.
Please try to dig in and understand how things should be done. It is important for the health of the project.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
-- Thank you, Alexey
[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_A...
Regards, Simon
Hi Neil,
I would like to know your opinion before I change the patches.
On Sat, Jul 15, 2023 at 05:40:53PM -0600, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
Hi Alexey,
On Tue, 11 Jul 2023 at 04:25, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote: > Hi Alexey, > > On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote: > > > > Hello, Simon! > > > > On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote: > > > Hi Alexey, > > > > > > On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote: > > > > > > > > At the moment, only smc API is a set of functions in > > > > arch/arm/mach-meson/sm.c. This approach is hard to configure > > > > and also doesni't contain any generic API for calling smc. > > > > > > > > This patch add Meson SM driver with generic API (struct meson_sm_ops): > > > > > > > > - sm_call() > > > > - sm_call_write() > > > > - sm_call_read() > > > > > > > > A typical driver usage example is shown here: > > > > > > > > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); > > > > 2. handle = meson_sm_get_handle(dev); > > > > 3. handle->ops.sm_call(dev, cmd, ...); > > > > > > > > Signed-off-by: Alexey Romanov avromanov@sberdevices.ru > > > > --- > > > > arch/arm/mach-meson/Kconfig | 1 + > > > > drivers/firmware/Kconfig | 10 ++ > > > > drivers/firmware/Makefile | 1 + > > > > drivers/firmware/meson/Kconfig | 6 + > > > > drivers/firmware/meson/Makefile | 3 + > > > > drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ > > > > include/meson/sm_handle.h | 38 ++++++ > > > > 7 files changed, 276 insertions(+) > > > > create mode 100644 drivers/firmware/meson/Kconfig > > > > create mode 100644 drivers/firmware/meson/Makefile > > > > create mode 100644 drivers/firmware/meson/meson_sm.c > > > > create mode 100644 include/meson/sm_handle.h > > > > > > Please can you use the remoteproc uclass for this and add a proper driver? > > > > > > > I don't see it architecturally well. Can you explain please? > > > > This driver is just ARM SMC fw interface. There seems to be nothing to > > do here for remoteproc uclass. > > Well you seem to be implementing a remote CPU interface, which is what > remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
> > Also there is a pending series on FFA - is that related? It uses smc > from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
Honestly I'm not sure, but I question why you are putting all this on the reviewer. Can't you look around the code base and figure out a sensible approach and defend it in your patch?
I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c? If so, it has nothing to do with the discussion here.
It is a kind of pseudo device for dm ut, which is subject to manage with the faked scmi server in sandbox (sandbox-scmi_agent.c). It is connected, for instance, to clock lines or vol regulators. (see sandbox's test.dts.)
I believe that similar usages can be seen across drivers/, for instance, drivers/clk/clk_sandbox_test.c.
Do you see anything wrong with it?
At this point I'm just confused. Perhaps MISC and FIRMWARE should not be used as much? It seems that FIRMWARE doesn't really mean anything at present?
I'm not sure if this is related to firmware or not, or whether what you are doing relates.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call) instruction escalates the cpu's EL (Execution Level) to EL3 (secure). This interface is clearly defined by Arm, and many high level services are defined and implemented on top of that. PSCI and FF-A are ones among those users, and even vendor specific functions may be allowed.
From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is just a C helper function around one SMC assembly. I don't think we need to have UCLASS_SMC as it is basically a communication method.
Nevertheless, I agree with Simon in the point that the SMC related code can be a bit reworked since there are duplicated code. Especially, the followings are essentially the same: -- arch/arm/cpu/armv8/fwcall.c with smc_call() psci_system_reset() psci_system_off() -- drivers/firmware/psci.c with arm_smccc_smc() psci_sys_reset() psci_sys_poweroff()
OK, what that is a start.
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to this class do not have any common operations.
Well misc.h does actually have operations, but yes, people misuse it. Perhaps we should invent something else for these people?
For FIRMWARE, what should it mean? It needs to have a firmware.h file with some information.
(And UCLASS_MISC as well.)
-Takahiro Akashi
OK, well please take a look at all this and see what you think is best. If UCLASS_FIRMWARE is correct, then please create a proper firmware.h file and define what the uclass operations are. You can't just randomly create your own private operations and ignore the rest of the code base.
Driver model is intended to bring order to the chaos that used to exist with drivers in U-Boot. Please see [1] for an introduction.
Please try to dig in and understand how things should be done. It is important for the health of the project.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
-- Thank you, Alexey
[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_A...
Regards, Simon
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Neil
Hi Alexey,
On Tue, 11 Jul 2023 at 04:25, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hi Simon,
On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
Hi Alexey,
On Mon, 10 Jul 2023 at 02:34, Alexey Romanov AVRomanov@sberdevices.ru wrote:
Hello!
On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote: > Hi Alexey, > > On Fri, 7 Jul 2023 at 09:43, Alexey Romanov AVRomanov@sberdevices.ru wrote: >> >> Hello, Simon! >> >> On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote: >>> Hi Alexey, >>> >>> On Thu, 6 Jul 2023 at 14:16, Alexey Romanov avromanov@sberdevices.ru wrote: >>>> >>>> At the moment, only smc API is a set of functions in >>>> arch/arm/mach-meson/sm.c. This approach is hard to configure >>>> and also doesni't contain any generic API for calling smc. >>>> >>>> This patch add Meson SM driver with generic API (struct meson_sm_ops): >>>> >>>> - sm_call() >>>> - sm_call_write() >>>> - sm_call_read() >>>> >>>> A typical driver usage example is shown here: >>>> >>>> 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev); >>>> 2. handle = meson_sm_get_handle(dev); >>>> 3. handle->ops.sm_call(dev, cmd, ...); >>>> >>>> Signed-off-by: Alexey Romanov avromanov@sberdevices.ru >>>> --- >>>> arch/arm/mach-meson/Kconfig | 1 + >>>> drivers/firmware/Kconfig | 10 ++ >>>> drivers/firmware/Makefile | 1 + >>>> drivers/firmware/meson/Kconfig | 6 + >>>> drivers/firmware/meson/Makefile | 3 + >>>> drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++ >>>> include/meson/sm_handle.h | 38 ++++++ >>>> 7 files changed, 276 insertions(+) >>>> create mode 100644 drivers/firmware/meson/Kconfig >>>> create mode 100644 drivers/firmware/meson/Makefile >>>> create mode 100644 drivers/firmware/meson/meson_sm.c >>>> create mode 100644 include/meson/sm_handle.h >>> >>> Please can you use the remoteproc uclass for this and add a proper driver? >>> >> >> I don't see it architecturally well. Can you explain please? >> >> This driver is just ARM SMC fw interface. There seems to be nothing to >> do here for remoteproc uclass. > > Well you seem to be implementing a remote CPU interface, which is what > remoteproc is for. How does Linux do this?
The main idea of the patchset is to abstract the smc calls (which run on the same CPU) and make a request to the firmware that runs on a higher EL. UCLASS_REMOTEPROC may give the impression that we are interacting with another CPU, although this is not the case.
Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and start(), and I don't even know how to apply my current changes to them.
You can return -ENOSYS if not implemented.
My implementation is very close to the Linux implementation, they also use the firmware driver without remoteproc:
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_...
Yes that seems like it doesn't use any common infra. I don't think it is a great model for U-Boot though.
I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is OK.
> > Also there is a pending series on FFA - is that related? It uses smc > from what I can tell.
Not entirely clear, my changes don't seem to be related to this patchset.
So perhaps this needs a new UCLASS_SMC? I see various other SMC calls in U-Boot but no one has taken the initiative to think about this in terms of driver model.
What will be the feature of this uclass? If it's just us adding a new uclass with a different name... Don't know if that makes amy sense? Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only in the name.
Honestly I'm not sure, but I question why you are putting all this on the reviewer. Can't you look around the code base and figure out a sensible approach and defend it in your patch?
I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c? If so, it has nothing to do with the discussion here.
It is a kind of pseudo device for dm ut, which is subject to manage with the faked scmi server in sandbox (sandbox-scmi_agent.c). It is connected, for instance, to clock lines or vol regulators. (see sandbox's test.dts.)
I believe that similar usages can be seen across drivers/, for instance, drivers/clk/clk_sandbox_test.c.
Do you see anything wrong with it?
At this point I'm just confused. Perhaps MISC and FIRMWARE should not be used as much? It seems that FIRMWARE doesn't really mean anything at present?
I'm not sure if this is related to firmware or not, or whether what you are doing relates.
It might just need one operation, to make a call, passing a regs struct, perhaps? The uclass would presumably be ARM-specific.
I really don't think this is UCLASS_FIRMWARE. That seems to be for loading firmware. It doesn't even have a firmware.h header.
I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use smc_call() function. Or firmware-zynqmo.c. In this case, we then have to convert them to UCLASS_SMC in the same way?
SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call) instruction escalates the cpu's EL (Execution Level) to EL3 (secure). This interface is clearly defined by Arm, and many high level services are defined and implemented on top of that. PSCI and FF-A are ones among those users, and even vendor specific functions may be allowed.
From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is just a C helper function around one SMC assembly. I don't think we need to have UCLASS_SMC as it is basically a communication method.
Nevertheless, I agree with Simon in the point that the SMC related code can be a bit reworked since there are duplicated code. Especially, the followings are essentially the same: -- arch/arm/cpu/armv8/fwcall.c with smc_call() psci_system_reset() psci_system_off() -- drivers/firmware/psci.c with arm_smccc_smc() psci_sys_reset() psci_sys_poweroff()
OK, what that is a start.
It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver that can work with any interface.
IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to this class do not have any common operations.
Well misc.h does actually have operations, but yes, people misuse it. Perhaps we should invent something else for these people?
For FIRMWARE, what should it mean? It needs to have a firmware.h file with some information.
(And UCLASS_MISC as well.)
-Takahiro Akashi
OK, well please take a look at all this and see what you think is best. If UCLASS_FIRMWARE is correct, then please create a proper firmware.h file and define what the uclass operations are. You can't just randomly create your own private operations and ignore the rest of the code base.
Driver model is intended to bring order to the chaos that used to exist with drivers in U-Boot. Please see [1] for an introduction.
Please try to dig in and understand how things should be done. It is important for the health of the project.
Also instead of:
- uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
use:
ret = uclass_first_device_err(UCLASS_SMC, &dev)
using device tree to find the device.
Regards, Simon
-- Thank you, Alexey
[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_A...
Regards, Simon
Hi Neil,
On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org neil.armstrong@linaro.org wrote:
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else?
Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware?
[..]
Regards, Simon
On 21/08/2023 21:11, Simon Glass wrote:
Hi Neil,
On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org neil.armstrong@linaro.org wrote:
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else?
Nop, it's based on the same "firmware" naming as Linux, which is an interface with a system control firmware like PSCI, SCPI... not to interact with loadable co-processors.
Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other vendor specific ones like Alexey is changing, all via the same instruction call.
Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware?
I have no opinion on that, I don't think the call type is significant here.
Neil
[..]
Regards, Simon
Hi,
On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote:
On 21/08/2023 21:11, Simon Glass wrote:
Hi Neil,
On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org neil.armstrong@linaro.org wrote:
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
+AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else?
Nop, it's based on the same "firmware" naming as Linux, which is an interface with a system control firmware like PSCI, SCPI... not to interact with loadable co-processors.
Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other vendor specific ones like Alexey is changing, all via the same instruction call.
Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware?
You propose to create UCLASS with an interface consisting of functions of something like: ->smc_call(), ->hvc_call()? In this case, it seems ARM specific.
Or UCLASS with only one callback, different for different archs, which will call the hypervisor or something like that. In my understanding, this add-on are redundant.
I still think UCLASS firmware is the best fit for my Secure Monitor implementation at the moment.
I have no opinion on that, I don't think the call type is significant here.
Neil
[..]
Regards, Simon
Hi Alexey,
On Tue, 22 Aug 2023 at 06:59, Alexey Romanov avromanov@salutedevices.com wrote:
Hi,
On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote:
On 21/08/2023 21:11, Simon Glass wrote:
Hi Neil,
On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org neil.armstrong@linaro.org wrote:
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote: > +AKASHI Takahiro
Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else?
Nop, it's based on the same "firmware" naming as Linux, which is an interface with a system control firmware like PSCI, SCPI... not to interact with loadable co-processors.
Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other vendor specific ones like Alexey is changing, all via the same instruction call.
Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware?
You propose to create UCLASS with an interface consisting of functions of something like: ->smc_call(), ->hvc_call()? In this case, it seems ARM specific.
Yes, but we have a lot of arch-specific interfaces.
Or UCLASS with only one callback, different for different archs, which will call the hypervisor or something like that. In my understanding, this add-on are redundant.
OK.
I still think UCLASS firmware is the best fit for my Secure Monitor implementation at the moment.
How about you create a UCLASS_MESON_SM then?
I don't really like the idea of a uclass with no standard API. One goal is to help people understand things and can't see that helping.
I have no opinion on that, I don't think the call type is significant here.
Neil
[..]
Regards, Simon
-- Thank you, Alexey
Regards, Simon
Hi Simon,
On Tue, Aug 22, 2023 at 12:56:32PM -0600, Simon Glass wrote:
Hi Alexey,
On Tue, 22 Aug 2023 at 06:59, Alexey Romanov avromanov@salutedevices.com wrote:
Hi,
On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote:
On 21/08/2023 21:11, Simon Glass wrote:
Hi Neil,
On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org neil.armstrong@linaro.org wrote:
Hi,
On 16/07/2023 01:40, Simon Glass wrote:
Hi,
On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote: > > +AKASHI Takahiro > > Me?
Yes, I'm asking for your help to try to clean this stuff up.
The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU.
And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions.
So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course.
So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined.
Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else?
Nop, it's based on the same "firmware" naming as Linux, which is an interface with a system control firmware like PSCI, SCPI... not to interact with loadable co-processors.
Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other vendor specific ones like Alexey is changing, all via the same instruction call.
Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware?
You propose to create UCLASS with an interface consisting of functions of something like: ->smc_call(), ->hvc_call()? In this case, it seems ARM specific.
Yes, but we have a lot of arch-specific interfaces.
Or UCLASS with only one callback, different for different archs, which will call the hypervisor or something like that. In my understanding, this add-on are redundant.
OK.
I still think UCLASS firmware is the best fit for my Secure Monitor implementation at the moment.
How about you create a UCLASS_MESON_SM then?
I don't really like the idea of a uclass with no standard API. One goal is to help people understand things and can't see that helping.
OK. Will do it in v2.
I have no opinion on that, I don't think the call type is significant here.
Neil
[..]
Regards, Simon
-- Thank you, Alexey
Regards, Simon
Now we have to use UCLASS_FIRMWARE meson secure monitor driver instead of raw smc_call() function call.
Signed-off-by: Alexey Romanov avromanov@sberdevices.ru --- arch/arm/mach-meson/sm.c | 161 +++++++++++++++++++++++---------------- 1 file changed, 97 insertions(+), 64 deletions(-)
diff --git a/arch/arm/mach-meson/sm.c b/arch/arm/mach-meson/sm.c index d600c64d0be..347ff448f79 100644 --- a/arch/arm/mach-meson/sm.c +++ b/arch/arm/mach-meson/sm.c @@ -16,72 +16,74 @@ #include <linux/kernel.h> #include <dm.h> #include <linux/bitfield.h> +#include <meson/sm_handle.h> #include <regmap.h> #include <syscon.h>
-#define FN_GET_SHARE_MEM_INPUT_BASE 0x82000020 -#define FN_GET_SHARE_MEM_OUTPUT_BASE 0x82000021 -#define FN_EFUSE_READ 0x82000030 -#define FN_EFUSE_WRITE 0x82000031 -#define FN_CHIP_ID 0x82000044 -#define FN_PWRDM_SET 0x82000093 - -static void *shmem_input; -static void *shmem_output; - -static void meson_init_shmem(void) +static inline struct udevice *meson_get_sm_device(void) { - struct pt_regs regs; - - if (shmem_input && shmem_output) - return; + struct udevice *dev; + int err;
- regs.regs[0] = FN_GET_SHARE_MEM_INPUT_BASE; - smc_call(®s); - shmem_input = (void *)regs.regs[0]; - - regs.regs[0] = FN_GET_SHARE_MEM_OUTPUT_BASE; - smc_call(®s); - shmem_output = (void *)regs.regs[0]; + err = uclass_get_device_by_name(UCLASS_FIRMWARE, "secure-monitor", &dev); + if (err) { + pr_err("Mesom SM device not found\n"); + return ERR_PTR(err); + }
- debug("Secure Monitor shmem: 0x%p 0x%p\n", shmem_input, shmem_output); + return dev; }
ssize_t meson_sm_read_efuse(uintptr_t offset, void *buffer, size_t size) { - struct pt_regs regs; - - meson_init_shmem(); - - regs.regs[0] = FN_EFUSE_READ; - regs.regs[1] = offset; - regs.regs[2] = size; - - smc_call(®s); - - if (regs.regs[0] == 0) - return -1; + struct meson_sm_handle *handle; + struct udevice *dev; + int err; + + dev = meson_get_sm_device(); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + handle = meson_sm_get_handle(dev); + if (IS_ERR(handle)) { + pr_err("Failed to get Meson SM handle\n"); + return PTR_ERR(handle); + }
- memcpy(buffer, shmem_output, min(size, regs.regs[0])); + err = handle->ops.sm_call_read(dev, buffer, size, + MESON_SMC_CMD_EFUSE_READ, offset, size); + if (err < 0) { + pr_err("Failed to read efuse memory (%d)\n", err); + return err; + }
- return regs.regs[0]; + return err; }
ssize_t meson_sm_write_efuse(uintptr_t offset, void *buffer, size_t size) { - struct pt_regs regs; - - meson_init_shmem(); - - memcpy(shmem_input, buffer, size); - - regs.regs[0] = FN_EFUSE_WRITE; - regs.regs[1] = offset; - regs.regs[2] = size; + struct meson_sm_handle *handle; + struct udevice *dev; + int err; + + dev = meson_get_sm_device(); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + handle = meson_sm_get_handle(dev); + if (IS_ERR(handle)) { + pr_err("Failed to get Meson SM handle\n"); + return PTR_ERR(handle); + }
- smc_call(®s); + err = handle->ops.sm_call_write(dev, buffer, size, + MESON_SMC_CMD_EFUSE_WRITE, offset, size); + if (err < 0) { + pr_err("Failed to write efuse memory (%d)\n", err); + return err; + }
- return regs.regs[0]; + return err; }
#define SM_CHIP_ID_LENGTH 119 @@ -90,18 +92,35 @@ ssize_t meson_sm_write_efuse(uintptr_t offset, void *buffer, size_t size)
int meson_sm_get_serial(void *buffer, size_t size) { - struct pt_regs regs; - - meson_init_shmem(); + struct meson_sm_handle *handle; + struct udevice *dev; + u8 *id_buffer; + int err; + + dev = meson_get_sm_device(); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + handle = meson_sm_get_handle(dev); + if (IS_ERR(handle)) { + pr_err("Failed to get Meson SM handle\n"); + return PTR_ERR(handle); + }
- regs.regs[0] = FN_CHIP_ID; - regs.regs[1] = 0; - regs.regs[2] = 0; + id_buffer = malloc(sizeof(u8) * SM_CHIP_ID_LENGTH); + if (!id_buffer) + return -ENOMEM;
- smc_call(®s); + err = handle->ops.sm_call_read(dev, id_buffer, SM_CHIP_ID_LENGTH, + MESON_SMC_CMD_CHIP_ID_GET, 0, 0); + if (err < 0) { + pr_err("Failed to read serial number (%d)\n", err); + free(id_buffer); + return err; + }
- memcpy(buffer, shmem_output + SM_CHIP_ID_OFFSET, - min_t(size_t, size, SM_CHIP_ID_SIZE)); + memcpy(buffer, id_buffer + SM_CHIP_ID_OFFSET, size); + free(id_buffer);
return 0; } @@ -141,13 +160,27 @@ int meson_sm_get_reboot_reason(void)
int meson_sm_pwrdm_set(size_t index, int cmd) { - struct pt_regs regs; - - regs.regs[0] = FN_PWRDM_SET; - regs.regs[1] = index; - regs.regs[2] = cmd; + struct meson_sm_handle *handle; + struct udevice *dev; + int err; + + dev = meson_get_sm_device(); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + handle = meson_sm_get_handle(dev); + if (IS_ERR(handle)) { + pr_err("Failed to get Meson SM handle\n"); + return PTR_ERR(handle); + }
- smc_call(®s); + err = handle->ops.sm_call(dev, MESON_SMC_CMD_PWRDM_SET, NULL, + index, cmd, 0, 0, 0); + if (err) { + pr_err("Failed to %s power domain ind=%zu (%d)\n", cmd == PWRDM_ON ? + "enable" : "disable", index, err); + return err; + }
- return regs.regs[0]; + return 0; }
participants (6)
-
AKASHI Takahiro -
Alexey Romanov -
Alexey Romanov -
Alexey Romanov
-
neil.armstrong＠linaro.org -
Simon Glass