[PATCH v1 0/2] Introduce Meson SM driver

[PATCH v1 0/2] Introduce Meson SM driver

[Alexey Romanov]

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

-- 
2.38.1

[PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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_ */
-- 
2.38.1

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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:
>
> 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?

Regards,
SImon

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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. 

> Regards,
> SImon

-- 
Thank you,
Alexey

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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?

Also there is a pending series on FFA - is that related? It uses smc
from what I can tell.

Regards,
Simon

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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.

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_sm.c

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

-- 
Thank you,
Alexey

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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_sm.c

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

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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_sm.c
> 
> 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:
> 
> 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

-- 
Thank you,
Alexey

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

+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_sm.c
> >
> > 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:
> >
> > 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
>
> --
> Thank you,
> Alexey

[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[AKASHI Takahiro]

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_sm.c
> > >
> > > 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:
> > >
> > > 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
> >
> > --
> > Thank you,
> > Alexey
> 
> [1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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_sm.c
> > > >
> > > > 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:
> > > >
> > > > 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
> > >
> > > --
> > > Thank you,
> > > Alexey
> >
> > [1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D

Regards,
Simon

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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_sm.c
> > > > >
> > > > > 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:
> > > > >
> > > > > 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
> > > >
> > > > --
> > > > Thank you,
> > > > Alexey
> > >
> > > [1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D
> 
> Regards,
> Simon

-- 
Thank you,
Alexey

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[neil.armstrong]

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_sm.c
>>>>>
>>>>> 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:
>>>>>
>>>>> 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
>>>>
>>>> --
>>>> Thank you,
>>>> Alexey
>>>
>>> [1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D
> 
> Regards,
> Simon

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[neil.armstrong]

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

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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
> 

-- 
Thank you,
Alexey

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

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

Re: [PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Alexey Romanov]

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

-- 
Thank you,
Alexey

[PATCH v1 2/2] asm/arch: mach-meson: use secure monitor driver

[Alexey Romanov]

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(&regs);
-	shmem_input = (void *)regs.regs[0];
-
-	regs.regs[0] = FN_GET_SHARE_MEM_OUTPUT_BASE;
-	smc_call(&regs);
-	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(&regs);
-
-	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(&regs);
+	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(&regs);
+	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(&regs);
+	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;
 }
-- 
2.38.1