[PATCH 0/3] add SMC based regmap driver for secure syscon access

When running under a secure monitor, some peripherals are setup as accessible
by secure world only. When those peripherals are system controllers, they might
need to be accessed by the normal world for some peripheral configuration.

In order to keep the existing code working for such devices (which usually uses
the regmap from a syscon), this series adds support for a regmap that uses SMCs
(Secure Monitor Call) to request access to registers. The secure monitor will
then catch these accesses and decide whether or not the normal world is allowed
to access the requested register.

As said, most drivers that needs access to registers that are shared in a system
controller often uses syscon. Currently, syscon uses a regmap_mmio which allows
to read and write registers using MMIO accesses. Support is added in this series
to also support "syscon-smc" compatible which will use a SMC regmap instead of a 
MMIO one.

Clément Léger (3):
  regmap: add regmap using ARM SMCCC
  syscon: add support for "syscon-smc" compatible
  dt-bindings: mfd: add "syscon-smc" YAML description

 .../devicetree/bindings/mfd/syscon-smc.yaml   |  57 ++++++
 drivers/base/regmap/Kconfig                   |   7 +-
 drivers/base/regmap/Makefile                  |   1 +
 drivers/base/regmap/regmap-smccc.c            | 131 ++++++++++++++
 drivers/mfd/syscon.c                          | 170 +++++++++++++++---
 include/linux/regmap.h                        |  38 ++++
 6 files changed, 378 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/syscon-smc.yaml
 create mode 100644 drivers/base/regmap/regmap-smccc.c

-- 
2.32.0

When running under secure monitor control, some controllers can be placed in
secure world and their access is thus not possible from normal world. However,
these controllers frequently contain registers than are needed by the normal
world for a few specific operations.

This patch adds a regmap where registers are accessed using SMCs. The secure
monitor is then responsible to allow or deny access to the requested registers.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/regmap/Kconfig        |   7 +-
 drivers/base/regmap/Makefile       |   1 +
 drivers/base/regmap/regmap-smccc.c | 131 +++++++++++++++++++++++++++++
 include/linux/regmap.h             |  38 +++++++++
 4 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-smccc.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 159bac6c5046..6957a6b21ad9 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig

@@ -4,7 +4,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM || REGMAP_MDIO)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM || REGMAP_MDIO || REGMAP_SMCCC)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	select MDIO_BUS if REGMAP_MDIO
 	bool

@@ -65,3 +65,8 @@ config REGMAP_I3C
 config REGMAP_SPI_AVMM
 	tristate
 	depends on SPI
+
+config REGMAP_SMCCC
+	default y if HAVE_ARM_SMCCC_DISCOVERY
+	tristate
+	depends on HAVE_ARM_SMCCC_DISCOVERY

diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 11facb32a027..3d92503a3b4e 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile

@@ -20,3 +20,4 @@ obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
 obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
 obj-$(CONFIG_REGMAP_MDIO) += regmap-mdio.o
+obj-$(CONFIG_REGMAP_SMCCC) += regmap-smccc.o

diff --git a/drivers/base/regmap/regmap-smccc.c b/drivers/base/regmap/regmap-smccc.c
new file mode 100644
index 000000000000..a64d58f97d21
--- /dev/null
+++ b/drivers/base/regmap/regmap-smccc.c

@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2021 Bootlin
+
+#include <linux/arm-smccc.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define REGMAP_SMC_READ		0
+#define REGMAP_SMC_WRITE	1
+
+struct regmap_smccc_ctx {
+	u32 regmap_smc_id;
+};
+
+static int regmap_smccc_reg_write(void *context, unsigned int reg,
+				  unsigned int val)
+{
+	struct regmap_smccc_ctx *ctx = context;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ctx->regmap_smc_id, REGMAP_SMC_WRITE, reg, val,
+			     &res);
+
+	if (res.a0)
+		return -EACCES;
+
+	return 0;
+}
+
+static int regmap_smccc_reg_read(void *context, unsigned int reg,
+				 unsigned int *val)
+{
+	struct regmap_smccc_ctx *ctx = context;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ctx->regmap_smc_id, REGMAP_SMC_READ, reg, &res);
+
+	if (res.a0)
+		return -EACCES;
+
+	*val = res.a1;
+
+	return 0;
+}
+
+static struct regmap_bus regmap_smccc = {
+	.reg_write = regmap_smccc_reg_write,
+	.reg_read = regmap_smccc_reg_read,
+};
+
+static int regmap_smccc_bits_is_supported(int val_bits)
+{
+	switch (val_bits) {
+	case 8:
+	case 16:
+	case 32:
+		return 0;
+	case 64:
+	/*
+	 * SMCs are using registers to pass information so if architecture is
+	 * not using 64 bits registers, we won't be able to pass information
+	 * transparently.
+	 */
+#if !defined(CONFIG_64BIT)
+		return -EINVAL;
+#else
+		return 0;
+#endif
+	default:
+		return -EINVAL;
+	}
+}
+
+static struct regmap_smccc_ctx *smccc_regmap_init_ctx(
+					const struct regmap_config *config,
+					u32 regmap_smc_id)
+{
+	int ret;
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+
+	ret = regmap_smccc_bits_is_supported(config->val_bits);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->regmap_smc_id = regmap_smc_id;
+
+	return ctx;
+}
+
+struct regmap *__regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+				   const struct regmap_config *config,
+				   struct lock_class_key *lock_key,
+				   const char *lock_name)
+{
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = smccc_regmap_init_ctx(config, regmap_smc_id);
+	if (IS_ERR(ctx))
+		return ERR_CAST(ctx);
+
+	return __regmap_init(dev, &regmap_smccc, ctx, config, lock_key,
+			     lock_name);
+}
+EXPORT_SYMBOL_GPL(__regmap_init_smccc);
+
+struct regmap *__devm_regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+					const struct regmap_config *config,
+					struct lock_class_key *lock_key,
+					const char *lock_name)
+{
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = smccc_regmap_init_ctx(config, regmap_smc_id);
+	if (IS_ERR(ctx))
+		return ERR_CAST(ctx);
+
+	return __devm_regmap_init(dev, &regmap_smccc, ctx, config, lock_key,
+				  lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_smccc);
+
+MODULE_AUTHOR("Clément Léger <clement.leger@bootlin.com>");
+MODULE_DESCRIPTION("Regmap SMCCC Module");
+MODULE_LICENSE("GPL v2");

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f5f08dd0a116..08aa523403e8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h

@@ -591,6 +591,11 @@ struct regmap *__regmap_init_spi_avmm(struct spi_device *spi,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name);
 
+struct regmap *__regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+				   const struct regmap_config *config,
+				   struct lock_class_key *lock_key,
+				   const char *lock_name);
+
 struct regmap *__devm_regmap_init(struct device *dev,
 				  const struct regmap_bus *bus,
 				  void *bus_context,

@@ -655,6 +660,10 @@ struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
 					   const struct regmap_config *config,
 					   struct lock_class_key *lock_key,
 					   const char *lock_name);
+struct regmap *__devm_regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+					const struct regmap_config *config,
+					struct lock_class_key *lock_key,
+					const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.

@@ -881,6 +890,20 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__regmap_init_spi_avmm, #config,		\
 				 spi, config)
 
+/**
+ * regmap_init_smccc() - Initialize register map for ARM SMCCC
+ *
+ * @dev: Device that will be interacted with
+ * @smc_id: SMC id to used for calls
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.
+ */
+#define regmap_init_smccc(dev, smc_id, config)			\
+	__regmap_lockdep_wrapper(__regmap_init_smccc, #config,	\
+				 dev, smc_id, config)
+
 /**
  * devm_regmap_init() - Initialise managed register map
  *

@@ -1110,6 +1133,21 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_spi_avmm, #config,	\
 				 spi, config)
 
+/**
+ * devm_regmap_init_smccc() - Initialize register map for ARM SMCCC
+ *
+ * @dev: Device that will be interacted with
+ * @smc_id: SMC id to used for calls
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The map will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_smccc(dev, smc_id, config)			\
+	__regmap_lockdep_wrapper(__devm_regmap_init_smccc, #config,	\
+				 dev, smc_id, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);

-- 
2.32.0

On Fri, Jul 23, 2021 at 03:52:37PM +0200, Clément Léger wrote:

When running under secure monitor control, some controllers can be placed in
secure world and their access is thus not possible from normal world. However,
these controllers frequently contain registers than are needed by the normal
world for a few specific operations.

This patch adds a regmap where registers are accessed using SMCs. The secure
monitor is then responsible to allow or deny access to the requested registers.

I can't see any SMC specification for this interface?  Frankly I have
some very substantial concerns about the use case for this over exposing
the functionality of whatever device the SMC is gating access to through
SMC interfaces specific to that functionality.  Exposing raw access to a
(presumed?) subset of whatever device functionality feels like the wrong
abstraction level to be working at and like an invitation to system
integrators to do things that are going to get them into trouble down
the line.

If the end user really is just twiddling a few bits here and there I'd
expect those functionality specific services to be pretty simple to do,
slightly more effort on the secure monitor side but a lot safer.  If
there is a use case for passing through an entire device for some reason
(ran out of controllers or something?) then I think we probably want an
abstraction at the bus level so we don't need to add custom support to
every device that we want to pass through and it's clear what's going on.

Hi Mark,

Le Fri, 23 Jul 2021 15:43:18 +0100,
Mark Brown [off-list ref] a écrit :

On Fri, Jul 23, 2021 at 03:52:37PM +0200, Clément Léger wrote:

quoted

When running under secure monitor control, some controllers can be
placed in secure world and their access is thus not possible from
normal world. However, these controllers frequently contain
registers than are needed by the normal world for a few specific
operations.  

quoted

This patch adds a regmap where registers are accessed using SMCs.
The secure monitor is then responsible to allow or deny access to
the requested registers.  

I can't see any SMC specification for this interface?  Frankly I have
some very substantial concerns about the use case for this over
exposing the functionality of whatever device the SMC is gating
access to through SMC interfaces specific to that functionality.

This would require to modify drivers to check if the access should be
done using SMCs, parse the device tree to find appropriate SMC ids for
each functionality, add dependencies in KConfig on
HAVE_ARM_SMCCC_DISCOVERY, and do SMC calls instead of regmap access.
I'm not saying this is not the way to go but this is clearly more
intrusive than keeping the existing syscon support.

Exposing raw access to a (presumed?) subset of whatever device
functionality feels like the wrong abstraction level to be working at
and like an invitation to system integrators to do things that are
going to get them into trouble down the line.

Indeed, access is reduced to a subset of registers offset which are
checked by the TEE.

If the end user really is just twiddling a few bits here and there I'd
expect those functionality specific services to be pretty simple to
do, slightly more effort on the secure monitor side but a lot safer.

The SMC id is supposed to be unique for a given device. The TEE check is
merely a register offset check and a value check. But I agree that the
attack surface is larger than with a SMC targeted for a single
functionality though.

If there is a use case for passing through an entire device for some
reason (ran out of controllers or something?) then I think we
probably want an abstraction at the bus level so we don't need to add
custom support to every device that we want to pass through and it's
clear what's going on.

In our use case, only a few registers located in a secure controller
is needed to be done. We don't have a use case for an entire device
access.

Clément

On Fri, Jul 23, 2021 at 05:53:15PM +0200, Clément Léger wrote:

Mark Brown [off-list ref] a écrit :

quoted

I can't see any SMC specification for this interface?  Frankly I have
some very substantial concerns about the use case for this over
exposing the functionality of whatever device the SMC is gating
access to through SMC interfaces specific to that functionality.

This would require to modify drivers to check if the access should be
done using SMCs, parse the device tree to find appropriate SMC ids for
each functionality, add dependencies in KConfig on
HAVE_ARM_SMCCC_DISCOVERY, and do SMC calls instead of regmap access.
I'm not saying this is not the way to go but this is clearly more
intrusive than keeping the existing syscon support.

You're not doing this at the syscon level, you're doing this at the
regmap level.  Any user of this code is going to have to be modified to
use the SMCCC regmap and discover the relevant SMCCC interfaces no
matter what, but by having it we're saying that that's a sensible and
reasonable thing to do and encouraging implementations as a result.

Device specific regmap interfaces do not require adding anything to the
core, there's the reg_read() and reg_write() callbacks for this, if
there is a sensible use case for this at the syscon level and only the
syscon level (but I really do strongly question if it's a good idea at
all) then you can use those without adding a generic interface for
defining SMCCC conduits as regmaps.  TBH what's being added to the
regmap core is so trival that I don't see what we'd be gaining anyway
even if this was widely used, it's not helping with the SMCCC
enumeration side at all.

quoted

Exposing raw access to a (presumed?) subset of whatever device
functionality feels like the wrong abstraction level to be working at
and like an invitation to system integrators to do things that are
going to get them into trouble down the line.

Indeed, access is reduced to a subset of registers offset which are
checked by the TEE.

I really think it would be clearer and safer to have the TEE expose
specific operations that encode the intent of whatever it is trying to
accomplish rather than just expose the register map and then audit the
operations that are going on in the register map after the fact.  It
seems like it's going to be more error prone to do things this way,
especially as this starts getting used as a generic pipe for exposing
things and things get built up - as well as auditing concerns if any
problems are identified it's going to be harder to track the intent of
what the non-secure world is doing.

System controllers can be placed under secure monitor control when running
under them. In order to keep existing code which accesses such system
controllers using a syscon, add support for "syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look for an
"arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
created to forward register access to the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 145 insertions(+), 25 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 765c0210cb52..eb727b146315 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c

@@ -40,7 +40,15 @@ static const struct regmap_config syscon_regmap_config = {
 	.reg_stride = 4,
 };
 
-static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
+static void syscon_add(struct syscon *syscon)
+{
+	spin_lock(&syscon_list_slock);
+	list_add_tail(&syscon->list, &syscon_list);
+	spin_unlock(&syscon_list_slock);
+}
+
+static struct syscon *of_syscon_register_mmio(struct device_node *np,
+					      bool check_clk)
 {
 	struct clk *clk;
 	struct syscon *syscon;

@@ -132,10 +140,6 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	syscon->regmap = regmap;
 	syscon->np = np;
 
-	spin_lock(&syscon_list_slock);
-	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
-
 	return syscon;
 
 err_attach:

@@ -150,8 +154,49 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	return ERR_PTR(ret);
 }
 
+#ifdef CONFIG_REGMAP_SMCCC
+static struct syscon *of_syscon_register_smccc(struct device_node *np)
+{
+	struct syscon *syscon;
+	struct regmap *regmap;
+	u32 reg_io_width = 4, smc_id;
+	int ret;
+	struct regmap_config syscon_config = syscon_regmap_config;
+
+	ret = of_property_read_u32(np, "arm,smc-id", &smc_id);
+	if (ret)
+		return ERR_PTR(-ENODEV);
+
+	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return ERR_PTR(-ENOMEM);
+
+	of_property_read_u32(np, "reg-io-width", &reg_io_width);
+
+	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x", np, smc_id);
+	syscon_config.val_bits = reg_io_width * 8;
+
+	regmap = regmap_init_smccc(NULL, smc_id, &syscon_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_regmap;
+	}
+
+	syscon->regmap = regmap;
+	syscon->np = np;
+
+	return syscon;
+
+err_regmap:
+	kfree(syscon_config.name);
+	kfree(syscon);
+
+	return ERR_PTR(ret);
+}
+#endif
+
 static struct regmap *device_node_get_regmap(struct device_node *np,
-					     bool check_clk)
+					     bool check_clk, bool use_smccc)
 {
 	struct syscon *entry, *syscon = NULL;
 

@@ -165,8 +210,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 
 	spin_unlock(&syscon_list_slock);
 
-	if (!syscon)
-		syscon = of_syscon_register(np, check_clk);
+	if (!syscon) {
+		if (use_smccc)
+#ifdef CONFIG_REGMAP_SMCCC
+			syscon = of_syscon_register_smccc(np);
+#else
+			syscon = NULL;
+#endif
+		else
+			syscon = of_syscon_register_mmio(np, check_clk);
+
+		if (!IS_ERR(syscon))
+			syscon_add(syscon);
+	}
 
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);

@@ -176,16 +232,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 
 struct regmap *device_node_to_regmap(struct device_node *np)
 {
-	return device_node_get_regmap(np, false);
+	return device_node_get_regmap(np, false, false);
 }
 EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	if (!of_device_is_compatible(np, "syscon"))
-		return ERR_PTR(-EINVAL);
+	if (of_device_is_compatible(np, "syscon"))
+		return device_node_get_regmap(np, true, false);
+
+	if (of_device_is_compatible(np, "syscon-smc"))
+		return device_node_get_regmap(np, true, true);
 
-	return device_node_get_regmap(np, true);
+	return ERR_PTR(-EINVAL);
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 

@@ -273,19 +332,19 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
 
-static int syscon_probe(struct platform_device *pdev)
+struct syscon_driver_data {
+	int (*probe_func)(struct platform_device *pdev, struct device *dev,
+			  struct syscon *syscon);
+};
+
+static int syscon_probe_mmio(struct platform_device *pdev,
+			     struct device *dev,
+			     struct syscon *syscon)
 {
-	struct device *dev = &pdev->dev;
-	struct syscon_platform_data *pdata = dev_get_platdata(dev);
-	struct syscon *syscon;
 	struct regmap_config syscon_config = syscon_regmap_config;
 	struct resource *res;
 	void __iomem *base;
 
-	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
-	if (!syscon)
-		return -ENOMEM;
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENOENT;

@@ -295,23 +354,84 @@ static int syscon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	syscon_config.max_register = resource_size(res) - 4;
-	if (pdata)
-		syscon_config.name = pdata->label;
+
 	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);
 	if (IS_ERR(syscon->regmap)) {
 		dev_err(dev, "regmap init failed\n");
 		return PTR_ERR(syscon->regmap);
 	}
 
-	platform_set_drvdata(pdev, syscon);
+	dev_dbg(dev, "regmap_mmio %pR registered\n", res);
+
+	return 0;
+}
+
+static const struct syscon_driver_data syscon_mmio_data = {
+	.probe_func = &syscon_probe_mmio,
+};
+
+#ifdef CONFIG_REGMAP_SMCCC
+
+static int syscon_probe_smc(struct platform_device *pdev,
+			    struct device *dev,
+			    struct syscon *syscon)
+{
+	struct regmap_config syscon_config = syscon_regmap_config;
+	int smc_id, ret;
+
+	ret = of_property_read_u32(dev->of_node, "arm,smc-id", &smc_id);
+	if (!ret)
+		return -ENODEV;
+
+	syscon->regmap = devm_regmap_init_smccc(dev, smc_id, &syscon_config);
+	if (IS_ERR(syscon->regmap)) {
+		dev_err(dev, "regmap init failed\n");
+		return PTR_ERR(syscon->regmap);
+	}
 
-	dev_dbg(dev, "regmap %pR registered\n", res);
+	dev_dbg(dev, "regmap_smccc %x registered\n", smc_id);
+
+	return 0;
+}
+
+static const struct syscon_driver_data syscon_smc_data = {
+	.probe_func = &syscon_probe_smc,
+};
+#endif
+
+static int syscon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct syscon_platform_data *pdata = dev_get_platdata(dev);
+	struct regmap_config syscon_config = syscon_regmap_config;
+	struct syscon *syscon;
+	const struct syscon_driver_data *driver_data;
+
+	if (pdata)
+		syscon_config.name = pdata->label;
+
+	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return -ENOMEM;
+
+	driver_data = (const struct syscon_driver_data *)
+				platform_get_device_id(pdev)->driver_data;
+
+	ret = driver_data->probe_func(pdev, dev, syscon);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, syscon);
 
 	return 0;
 }
 
 static const struct platform_device_id syscon_ids[] = {
-	{ "syscon", },
+	{ .name = "syscon",	.driver_data = (kernel_ulong_t)&syscon_mmio_data},
+#ifdef CONFIG_REGMAP_SMCCC
+	{ .name = "syscon-smc",	.driver_data = (kernel_ulong_t)&syscon_smc_data},
+#endif
 	{ }
 };
 

-- 
2.32.0

On Fri, 23 Jul 2021, Clément Léger wrote:

System controllers can be placed under secure monitor control when running
under them. In order to keep existing code which accesses such system
controllers using a syscon, add support for "syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look for an
"arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
created to forward register access to the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 145 insertions(+), 25 deletions(-)

I'm going to let Arnd have at this, but just a couple of points.

So this is Arm specific.

Not sure we want to be creating bespoke support for specific
architectures/platforms in the generic syscon implementation.

... and we certainly don't want to be doing so using #ifdefery.

Please find a better way to support this feature.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Le Fri, 23 Jul 2021 16:27:59 +0100,
Lee Jones [off-list ref] a écrit :

On Fri, 23 Jul 2021, Clément Léger wrote:

quoted

System controllers can be placed under secure monitor control when
running under them. In order to keep existing code which accesses
such system controllers using a syscon, add support for
"syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look
for an "arm,smc-id" property to execute the appropriate SMC. A SMC
regmap is then created to forward register access to the secure
monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/mfd/syscon.c | 170
++++++++++++++++++++++++++++++++++++------- 1 file changed, 145
insertions(+), 25 deletions(-)  

I'm going to let Arnd have at this, but just a couple of points.

quoted

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 765c0210cb52..eb727b146315 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c

@@ -40,7 +40,15 @@ static const struct regmap_config

syscon_regmap_config = { .reg_stride = 4,
 };
 
-static struct syscon *of_syscon_register(struct device_node *np,
bool check_clk) +static void syscon_add(struct syscon *syscon)
+{
+	spin_lock(&syscon_list_slock);
+	list_add_tail(&syscon->list, &syscon_list);
+	spin_unlock(&syscon_list_slock);
+}
+
+static struct syscon *of_syscon_register_mmio(struct device_node
*np,
+					      bool check_clk)
 {
 	struct clk *clk;
 	struct syscon *syscon;

@@ -132,10 +140,6 @@ static struct syscon

*of_syscon_register(struct device_node *np, bool check_clk)
syscon->regmap = regmap; syscon->np = np;
 
-	spin_lock(&syscon_list_slock);
-	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
-
 	return syscon;
 
 err_attach:

@@ -150,8 +154,49 @@ static struct syscon

*of_syscon_register(struct device_node *np, bool check_clk) return
ERR_PTR(ret); }
 
+#ifdef CONFIG_REGMAP_SMCCC
+static struct syscon *of_syscon_register_smccc(struct device_node
*np) +{
+	struct syscon *syscon;
+	struct regmap *regmap;
+	u32 reg_io_width = 4, smc_id;
+	int ret;
+	struct regmap_config syscon_config = syscon_regmap_config;
+
+	ret = of_property_read_u32(np, "arm,smc-id", &smc_id);  

So this is Arm specific.

Not sure we want to be creating bespoke support for specific
architectures/platforms in the generic syscon implementation.

Agreed, I wanted to keep an existing property name but having such
thing in the syscon is indeed a bad idea.

quoted

+	if (ret)
+		return ERR_PTR(-ENODEV);
+
+	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return ERR_PTR(-ENOMEM);
+
+	of_property_read_u32(np, "reg-io-width", &reg_io_width);
+
+	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x",
np, smc_id);
+	syscon_config.val_bits = reg_io_width * 8;
+
+	regmap = regmap_init_smccc(NULL, smc_id, &syscon_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_regmap;
+	}
+
+	syscon->regmap = regmap;
+	syscon->np = np;
+
+	return syscon;
+
+err_regmap:
+	kfree(syscon_config.name);
+	kfree(syscon);
+
+	return ERR_PTR(ret);
+}
+#endif
+
 static struct regmap *device_node_get_regmap(struct device_node
*np,
-					     bool check_clk)
+					     bool check_clk, bool
use_smccc) {
 	struct syscon *entry, *syscon = NULL;
 

@@ -165,8 +210,19 @@ static struct regmap

*device_node_get_regmap(struct device_node *np, 
 	spin_unlock(&syscon_list_slock);
 
-	if (!syscon)
-		syscon = of_syscon_register(np, check_clk);
+	if (!syscon) {
+		if (use_smccc)
+#ifdef CONFIG_REGMAP_SMCCC
+			syscon = of_syscon_register_smccc(np);
+#else
+			syscon = NULL;
+#endif  

... and we certainly don't want to be doing so using #ifdefery.

Please find a better way to support this feature.

Agreed too, best solution would probably be to allow having multiple
syscon "backends" split in multiple files which would create the
correct regmap according to the devicetree.

Clément

quoted

+		else
+			syscon = of_syscon_register_mmio(np,
check_clk); +
+		if (!IS_ERR(syscon))
+			syscon_add(syscon);
+	}
 
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);

@@ -176,16 +232,19 @@ static struct regmap

*device_node_get_regmap(struct device_node *np, 
 struct regmap *device_node_to_regmap(struct device_node *np)
 {
-	return device_node_get_regmap(np, false);
+	return device_node_get_regmap(np, false, false);
 }
 EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	if (!of_device_is_compatible(np, "syscon"))
-		return ERR_PTR(-EINVAL);
+	if (of_device_is_compatible(np, "syscon"))
+		return device_node_get_regmap(np, true, false);
+
+	if (of_device_is_compatible(np, "syscon-smc"))
+		return device_node_get_regmap(np, true, true);
 
-	return device_node_get_regmap(np, true);
+	return ERR_PTR(-EINVAL);
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 

@@ -273,19 +332,19 @@ struct regmap

*syscon_regmap_lookup_by_phandle_optional(struct device_node *np, }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
 
-static int syscon_probe(struct platform_device *pdev)
+struct syscon_driver_data {
+	int (*probe_func)(struct platform_device *pdev, struct
device *dev,
+			  struct syscon *syscon);
+};
+
+static int syscon_probe_mmio(struct platform_device *pdev,
+			     struct device *dev,
+			     struct syscon *syscon)
 {
-	struct device *dev = &pdev->dev;
-	struct syscon_platform_data *pdata = dev_get_platdata(dev);
-	struct syscon *syscon;
 	struct regmap_config syscon_config = syscon_regmap_config;
 	struct resource *res;
 	void __iomem *base;
 
-	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
-	if (!syscon)
-		return -ENOMEM;
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENOENT;

@@ -295,23 +354,84 @@ static int syscon_probe(struct

platform_device *pdev) return -ENOMEM;
 
 	syscon_config.max_register = resource_size(res) - 4;
-	if (pdata)
-		syscon_config.name = pdata->label;
+
 	syscon->regmap = devm_regmap_init_mmio(dev, base,
&syscon_config); if (IS_ERR(syscon->regmap)) {
 		dev_err(dev, "regmap init failed\n");
 		return PTR_ERR(syscon->regmap);
 	}
 
-	platform_set_drvdata(pdev, syscon);
+	dev_dbg(dev, "regmap_mmio %pR registered\n", res);
+
+	return 0;
+}
+
+static const struct syscon_driver_data syscon_mmio_data = {
+	.probe_func = &syscon_probe_mmio,
+};
+
+#ifdef CONFIG_REGMAP_SMCCC
+
+static int syscon_probe_smc(struct platform_device *pdev,
+			    struct device *dev,
+			    struct syscon *syscon)
+{
+	struct regmap_config syscon_config = syscon_regmap_config;
+	int smc_id, ret;
+
+	ret = of_property_read_u32(dev->of_node, "arm,smc-id",
&smc_id);
+	if (!ret)
+		return -ENODEV;
+
+	syscon->regmap = devm_regmap_init_smccc(dev, smc_id,
&syscon_config);
+	if (IS_ERR(syscon->regmap)) {
+		dev_err(dev, "regmap init failed\n");
+		return PTR_ERR(syscon->regmap);
+	}
 
-	dev_dbg(dev, "regmap %pR registered\n", res);
+	dev_dbg(dev, "regmap_smccc %x registered\n", smc_id);
+
+	return 0;
+}
+
+static const struct syscon_driver_data syscon_smc_data = {
+	.probe_func = &syscon_probe_smc,
+};
+#endif
+
+static int syscon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct syscon_platform_data *pdata = dev_get_platdata(dev);
+	struct regmap_config syscon_config = syscon_regmap_config;
+	struct syscon *syscon;
+	const struct syscon_driver_data *driver_data;
+
+	if (pdata)
+		syscon_config.name = pdata->label;
+
+	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return -ENOMEM;
+
+	driver_data = (const struct syscon_driver_data *)
+
platform_get_device_id(pdev)->driver_data; +
+	ret = driver_data->probe_func(pdev, dev, syscon);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, syscon);
 
 	return 0;
 }
 
 static const struct platform_device_id syscon_ids[] = {
-	{ "syscon", },
+	{ .name = "syscon",	.driver_data =
(kernel_ulong_t)&syscon_mmio_data}, +#ifdef CONFIG_REGMAP_SMCCC
+	{ .name = "syscon-smc",	.driver_data =
(kernel_ulong_t)&syscon_smc_data}, +#endif
 	{ }
 };
   

On Fri, Jul 23, 2021 at 3:52 PM Clément Léger [off-list ref] wrote:

System controllers can be placed under secure monitor control when running
under them. In order to keep existing code which accesses such system
controllers using a syscon, add support for "syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look for an
"arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
created to forward register access to the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I don't see anything wrong with the implementation, but this worries
me conceptually, because of the ways this might get abused:

- this creates one more way to keep device drivers hidden away
  behind firmware when they should be in the kernel. You can already
  do that with separate SMC calls, but adding an indirection makes it
  sneakier. If the 'registers' in here are purely

- This may be seen as an easy way out for firmware writers that just
   expose a bare register-level interface when the correct solution would
   be to create a high-level interface.

There is also a problem with locking: In the case that both firmware and
kernel have to access registers within a syscon area, you may need to
have a semaphore to protect an atomic sequence of accesses, but since
the interface only provides a single register load/store, there is no way for
a kernel driver to serialize against a firmware-internal driver.

        Arnd

On Fri, Jul 23, 2021 at 06:07:44PM +0200, Arnd Bergmann wrote:

There is also a problem with locking: In the case that both firmware and
kernel have to access registers within a syscon area, you may need to
have a semaphore to protect an atomic sequence of accesses, but since
the interface only provides a single register load/store, there is no way for
a kernel driver to serialize against a firmware-internal driver.

The standard solution to this for the read/modify/write case would be to
expose an explicit update_bits() operation (some hardware does this for
concurrency and/or bus bandwidth/latency reasons), though that doesn't
help with larger or multi-register sequences (and to be clear as I've
been saying I don't think we should do this at all).

Subject: Re: [PATCH 2/3] syscon: add support for "syscon-smc" compatible

On Fri, Jul 23, 2021 at 3:52 PM Clément Léger [off-list ref]
wrote:

quoted

System controllers can be placed under secure monitor control when
running under them. In order to keep existing code which accesses such
system controllers using a syscon, add support for "syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look for
an "arm,smc-id" property to execute the appropriate SMC. A SMC regmap
is then created to forward register access to the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I don't see anything wrong with the implementation,

I also vote for such an implementation. Such as we have a chip has a misc
register space, part as below:

44h USB Wake-up Control Register (DGO 10) (USB_WAKEUP) 
48h PTD Pads Compensation Cell Configuration Register
4Ch Lower CA35 TS Timer First Compare Value (TSTMR_CMP0_VAL_L)
50h Upper CA35 TS Timer First Compare Value
54h Lower CA35 TS Timer Second Compare value
58h Upper CA35 TS Timer Second Compare Value
5Ch CA35 Core0 Reset Vector Base Address (DGO 8) (RVBARADDR0) 
60h CA35 Core1 Reset Vector Base Address (DGO 9) (RVBARADDR1) 
64h Medium Quality Sound Configuration Register (MQS1_CF) 32 RW 0100_0000h

It contains several functions, we need protect 5Ch, 60h to avoid
Non-secure world modify it. Others could be directly used by Linux kernel.
But we could only hide the whole register space in secure world to make
5C/60h register not touch by linux.

We not find a good way to provide high-level interface for such
a misc register space, provide register level interface would make
it easy for various drivers to use.

Thanks,
Peng.


but this worries me

conceptually, because of the ways this might get abused:

- this creates one more way to keep device drivers hidden away
  behind firmware when they should be in the kernel. You can already
  do that with separate SMC calls, but adding an indirection makes it
  sneakier. If the 'registers' in here are purely

- This may be seen as an easy way out for firmware writers that just
   expose a bare register-level interface when the correct solution would
   be to create a high-level interface.

There is also a problem with locking: In the case that both firmware and
kernel have to access registers within a syscon area, you may need to have a
semaphore to protect an atomic sequence of accesses, but since the interface
only provides a single register load/store, there is no way for a kernel driver to
serialize against a firmware-internal driver.

        Arnd

Hi "Clément,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on regmap/for-next]
[also build test WARNING on robh/for-next v5.14-rc2 next-20210723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
config: riscv-nommu_k210_defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5b8123662c54263f6fc86b6ef9e296739fe78353
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708
        git checkout 5b8123662c54263f6fc86b6ef9e296739fe78353
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All warnings (new ones prefixed by >>):

   drivers/mfd/syscon.c: In function 'syscon_probe':

quoted

drivers/mfd/syscon.c:407:23: warning: variable 'syscon_config' set but not used [-Wunused-but-set-variable]

     407 |  struct regmap_config syscon_config = syscon_regmap_config;
         |                       ^~~~~~~~~~~~~


vim +/syscon_config +407 drivers/mfd/syscon.c

   401	
   402	static int syscon_probe(struct platform_device *pdev)
   403	{
   404		int ret;
   405		struct device *dev = &pdev->dev;
   406		struct syscon_platform_data *pdata = dev_get_platdata(dev);
 > 407		struct regmap_config syscon_config = syscon_regmap_config;
   408		struct syscon *syscon;
   409		const struct syscon_driver_data *driver_data;
   410	
   411		if (pdata)
   412			syscon_config.name = pdata->label;
   413	
   414		syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
   415		if (!syscon)
   416			return -ENOMEM;
   417	
   418		driver_data = (const struct syscon_driver_data *)
   419					platform_get_device_id(pdev)->driver_data;
   420	
   421		ret = driver_data->probe_func(pdev, dev, syscon);
   422		if (ret)
   423			return ret;
   424	
   425		platform_set_drvdata(pdev, syscon);
   426	
   427		return 0;
   428	}
   429	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

This patch adds documentation for the "syscon-smc" compatible which describes
a syscon using a SMC regmap instead of a MMIO one. This allows accessing system
controllers that are set as secure by using SMC handled by the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 .../devicetree/bindings/mfd/syscon-smc.yaml   | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/syscon-smc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/syscon-smc.yaml b/Documentation/devicetree/bindings/mfd/syscon-smc.yaml
new file mode 100644
index 000000000000..6ce1392c5e7f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/syscon-smc.yaml

@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/syscon-smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: System Controller Registers R/W via SMC Device Tree Bindings
+
+description: |
+  System controller SMC node represents a register region containing a set
+  of miscellaneous registers accessed through a secure monitor.
+  The typical use-case is the same as the syscon one but when running with a
+  secure monitor.
+
+maintainers:
+  - Lee Jones <lee.jones@linaro.org>
+
+properties:
+  compatible:
+    anyOf:
+      - items:
+          - enum:
+              - atmel,sama5d2-sfr
+
+          - const: syscon-smc
+
+      - contains:
+          const: syscon-smc
+        minItems: 2
+        maxItems: 4  # Should be enough
+
+  arm,smc-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      The ATF smc function id used by the firmware.
+
+  reg-io-width:
+    description: |
+      The size (in bytes) of the IO accesses that should be performed
+      on the device.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8]
+
+required:
+  - compatible
+  - arm,smc-id
+
+additionalProperties: false
+
+examples:
+  - |
+    sfr {
+        compatible = "atmel,sama5d2-sfr", "syscon-smc";
+        arm,smc-id = <0x02000300>;
+    };
+
+...

-- 
2.32.0

On Fri, Jul 23, 2021 at 03:52:39PM +0200, Clément Léger wrote:

I regret having 'syscon' as a compatible, so nak on a 2nd flavor of it. 
It's only purpose is a hint to Linux to automagically create a regmap for 
you.

All you need is the specific compatible, atmel,sama5d2-sfr, and you can 
imply the rest of this from it. That's assuming the conclusion is a 
register read/write interface on SMC is a good idea, but I don't think 
it is.

Rob

Le Thu, 29 Jul 2021 15:19:19 -0600,
Rob Herring [off-list ref] a écrit :

On Fri, Jul 23, 2021 at 03:52:39PM +0200, Clément Léger wrote:

quoted

This patch adds documentation for the "syscon-smc" compatible which
describes a syscon using a SMC regmap instead of a MMIO one. This
allows accessing system controllers that are set as secure by using
SMC handled by the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 .../devicetree/bindings/mfd/syscon-smc.yaml   | 57
+++++++++++++++++++ 1 file changed, 57 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/mfd/syscon-smc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/syscon-smc.yaml

b/Documentation/devicetree/bindings/mfd/syscon-smc.yaml new file
mode 100644 index 000000000000..6ce1392c5e7f

--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/syscon-smc.yaml

@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/syscon-smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: System Controller Registers R/W via SMC Device Tree Bindings
+
+description: |
+  System controller SMC node represents a register region

containing a set
+  of miscellaneous registers accessed through a secure monitor.
+  The typical use-case is the same as the syscon one but when
running with a
+  secure monitor.
+
+maintainers:
+  - Lee Jones [off-list ref]
+
+properties:
+  compatible:
+    anyOf:
+      - items:
+          - enum:
+              - atmel,sama5d2-sfr
+
+          - const: syscon-smc  

I regret having 'syscon' as a compatible, so nak on a 2nd flavor of
it. It's only purpose is a hint to Linux to automagically create a
regmap for you.

Indeed.

All you need is the specific compatible, atmel,sama5d2-sfr, and you
can imply the rest of this from it. That's assuming the conclusion is
a register read/write interface on SMC is a good idea, but I don't
think it is.

Ok noted, I'll try to find something else to implement that.

Clément

Rob