[PATCH V3 0/2] nvmem: add and use generic MMIO NVMEM

[PATCH V3 0/2] nvmem: add and use generic MMIO NVMEM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

MMIO accessible NVMEM devices should be simple enough to allow using a
single binding & driver for them.

In V3 I didn't decide to modify existing Mediatek & UniPhier drivers as
there are some concerns about adding writing support to them. If needed
that can be done later.

I decided to support Broadcom's NVRAM with generic driver though. That
is my main goal right now. To let generic code handle NVRAM access and
let NVRAM specific driver handle its content. That way it can be reused
for other NVMEM devices later (once we get layouts support).

Rafał Miłecki (2):
  dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM
    devices
  nvmem: add generic driver for devices with MMIO access

 .../devicetree/bindings/nvmem/mmio.yaml       |  50 +++++++
 drivers/nvmem/Kconfig                         |  10 ++
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/mmio.c                          | 125 ++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mmio.yaml
 create mode 100644 drivers/nvmem/mmio.c

-- 
2.34.1

[PATCH V3 1/2] dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM devices

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Content of some NVMEM devices can be read using MMIO. Some of them
(probably very few though) may be also programmable that way. Add
generic binding to allow describing such hardware.

This *doesn't* apply to any more complicated devices that need more
complex interface e.g. for writing. While such devices could be
supported for reading purposes by the same driver - they should get
their own binding.

This binding will gain even more usability once we fully support NVMEM
layouts (describing content of NVMEM devices in an independent way).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: Make it clear this binding should NOT be used for more complex devices
---
 .../devicetree/bindings/nvmem/mmio.yaml       | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mmio.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/mmio.yaml b/Documentation/devicetree/bindings/nvmem/mmio.yaml
new file mode 100644
index 000000000000..9ca96b7a4856
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/mmio.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/mmio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MMIO access based NVMEM
+
+description: |
+  This binding describes simple NVMEM devices with content that can be accessed
+  using MMIO (memory-mapped I/O access).
+
+  More complex devices that support any other access than a simple memory
+  mapping should use a custom binding. In such cases this binding's compatible
+  should NOT be used even as a fallback.
+
+  This binding is designed to describe just an NVMEM content access method. The
+  way of handling actual content should be described independently (on top of
+  this binding).
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    const: mmio-nvmem
+
+  reg:
+    maxItems: 1
+
+  reg-io-width:
+    description: |
+      The size (in bytes) of the IO accesses that should be performed
+      on the device.
+    enum: [1, 2, 4, 8]
+
+required:
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    nvmem@10000 {
+        compatible = "mmio-nvmem";
+        reg = <0x10000000 0x10000>;
+    };
-- 
2.34.1

Re: [PATCH V3 1/2] dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM devices

[Rob Herring]

On Mon, 20 Feb 2023 18:49:29 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Content of some NVMEM devices can be read using MMIO. Some of them
> (probably very few though) may be also programmable that way. Add
> generic binding to allow describing such hardware.
> 
> This *doesn't* apply to any more complicated devices that need more
> complex interface e.g. for writing. While such devices could be
> supported for reading purposes by the same driver - they should get
> their own binding.
> 
> This binding will gain even more usability once we fully support NVMEM
> layouts (describing content of NVMEM devices in an independent way).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V3: Make it clear this binding should NOT be used for more complex devices
> ---
>  .../devicetree/bindings/nvmem/mmio.yaml       | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/mmio.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V3 2/2] nvmem: add generic driver for devices with MMIO access

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Some NVMEM devices can be accessed by simply mapping memory and reading
from / writing to it. This driver adds support for a generic
"mmio-nvmem" DT binding used by such devices.

One of such devices is Broadcom's NVRAM. It's already supported (see
NVMEM_BRCM_NVRAM) but existing driver covers both:
1. NVMEM device access
2. NVMEM content parsing

Once we get support for NVMEM layouts then existing NVRAM driver will
get converted into a layout and generic driver will take over
responsibility for data access. That's why it claims "brcm,nvram"
(conditionally for now).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: Support "reg-io-width", basic writing & "brcm,nvram" string
---
 drivers/nvmem/Kconfig  |  10 ++++
 drivers/nvmem/Makefile |   2 +
 drivers/nvmem/mmio.c   | 125 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 drivers/nvmem/mmio.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 6dec38805041..189ea85bd67d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -166,6 +166,16 @@ config NVMEM_MICROCHIP_OTPC
 	  This driver enable the OTP controller available on Microchip SAMA7G5
 	  SoCs. It controls the access to the OTP memory connected to it.
 
+config NVMEM_MMIO
+	tristate "MMIO access based NVMEM support"
+	depends on HAS_IOMEM
+	help
+	  This driver provides support for NVMEM devices that can be accessed
+	  using MMIO.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-mmio.
+
 config NVMEM_MTK_EFUSE
 	tristate "Mediatek SoCs EFUSE support"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 6a1efffa88f0..767a9db2bfc1 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -35,6 +35,8 @@ obj-$(CONFIG_NVMEM_MESON_MX_EFUSE)	+= nvmem_meson_mx_efuse.o
 nvmem_meson_mx_efuse-y			:= meson-mx-efuse.o
 obj-$(CONFIG_NVMEM_MICROCHIP_OTPC)	+= nvmem-microchip-otpc.o
 nvmem-microchip-otpc-y			:= microchip-otpc.o
+obj-$(CONFIG_NVMEM_MMIO)		+= nvmem-mmio.o
+nvmem-mmio-y				:= mmio.o
 obj-$(CONFIG_NVMEM_MTK_EFUSE)		+= nvmem_mtk-efuse.o
 nvmem_mtk-efuse-y			:= mtk-efuse.o
 obj-$(CONFIG_NVMEM_MXS_OCOTP)		+= nvmem-mxs-ocotp.o
diff --git a/drivers/nvmem/mmio.c b/drivers/nvmem/mmio.c
new file mode 100644
index 000000000000..e247c943eea2
--- /dev/null
+++ b/drivers/nvmem/mmio.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Rafał Miłecki <rafal@milecki.pl>
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+struct mmio_nvmem {
+	void __iomem *base;
+	u32 io_width;
+};
+
+static int mmio_nvmem_read(void *context, unsigned int offset, void *val, size_t bytes)
+{
+	struct mmio_nvmem *priv = context;
+	u32 *dst32;
+	u16 *dst16;
+	u8 *dst8;
+
+	if (priv->io_width && WARN_ON(bytes % priv->io_width))
+		return -EINVAL;
+
+	switch (priv->io_width) {
+	case 1:
+		for (dst8 = val; bytes; bytes -= 1, offset += 1)
+			*dst8++ = readb(priv->base + offset);
+		break;
+	case 2:
+		for (dst16 = val; bytes; bytes -= 2, offset += 2)
+			*dst16++ = readw(priv->base + offset);
+		break;
+	case 4:
+		for (dst32 = val; bytes; bytes -= 4, offset += 4)
+			*dst32++ = readl(priv->base + offset);
+		break;
+	default:
+		memcpy_fromio(val, priv->base + offset, bytes);
+	}
+
+	return 0;
+}
+
+static int mmio_nvmem_write(void *context, unsigned int offset, void *val, size_t bytes)
+{
+	struct mmio_nvmem *priv = context;
+
+	switch (priv->io_width) {
+	case 1:
+	case 2:
+	case 4:
+		return -EOPNOTSUPP;
+	default:
+		memcpy_toio(priv->base + offset, val, bytes);
+	}
+
+	return 0;
+}
+
+static int mmio_nvmem_probe(struct platform_device *pdev)
+{
+	struct nvmem_config config = {
+		.name = "mmio-nvmem",
+		.id = NVMEM_DEVID_AUTO,
+		.read_only = true,
+		.reg_read = mmio_nvmem_read,
+	};
+	struct device *dev = &pdev->dev;
+	struct mmio_nvmem *priv;
+	struct resource *res;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	of_property_read_u32(dev->of_node, "reg-io-width", &priv->io_width);
+
+	config.dev = dev;
+	config.size = resource_size(res);
+	config.word_size = priv->io_width;
+	config.stride = priv->io_width;
+	config.priv = priv;
+	if (!device_property_present(dev, "read-only"))
+		config.reg_write = mmio_nvmem_write;
+
+	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
+}
+
+static const struct of_device_id mmio_nvmem_of_match_table[] = {
+	{ .compatible = "mmio-nvmem", },
+	/* Custom bindings */
+#if !IS_ENABLED(CONFIG_NVMEM_BRCM_NVRAM)
+	{ .compatible = "brcm,nvram", },
+#endif
+	{},
+};
+
+static struct platform_driver mmio_nvmem_driver = {
+	.probe = mmio_nvmem_probe,
+	.driver = {
+		.name = "mmio_nvmem",
+		.of_match_table = mmio_nvmem_of_match_table,
+	},
+};
+
+static int __init mmio_nvmem_init(void)
+{
+	return platform_driver_register(&mmio_nvmem_driver);
+}
+
+subsys_initcall_sync(mmio_nvmem_init);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, mmio_nvmem_of_match_table);
-- 
2.34.1

Re: [PATCH V3 2/2] nvmem: add generic driver for devices with MMIO access

[Randy Dunlap]

Hi,

On 2/20/23 09:49, Rafał Miłecki wrote:
> +static int mmio_nvmem_write(void *context, unsigned int offset, void *val, size_t bytes)
> +{
> +	struct mmio_nvmem *priv = context;
> +
> +	switch (priv->io_width) {
> +	case 1:
> +	case 2:
> +	case 4:
> +		return -EOPNOTSUPP;

I'm just curious: (since read supports those cases)

what size writes are typically used here?
And what value for priv->io_width?



Thanks.

> +	default:
> +		memcpy_toio(priv->base + offset, val, bytes);
> +	}
> +
> +	return 0;
> +}

-- 
~Randy

Re: [PATCH V3 2/2] nvmem: add generic driver for devices with MMIO access

[Rafał Miłecki]

On 2023-02-20 20:55, Randy Dunlap wrote:
> On 2/20/23 09:49, Rafał Miłecki wrote:
>> +static int mmio_nvmem_write(void *context, unsigned int offset, void 
>> *val, size_t bytes)
>> +{
>> +	struct mmio_nvmem *priv = context;
>> +
>> +	switch (priv->io_width) {
>> +	case 1:
>> +	case 2:
>> +	case 4:
>> +		return -EOPNOTSUPP;
> 
> I'm just curious: (since read supports those cases)
> 
> what size writes are typically used here?
> And what value for priv->io_width?

Sorry, I really don't have any statistics on this or any real overview
of hardware that supports MMIO writes. It's probably a minority of
devices that allows it.

Re: [PATCH V3 2/2] nvmem: add generic driver for devices with MMIO access

[Srinivas Kandagatla]

Thanks Rafal for the patch,

On 20/02/2023 17:49, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Some NVMEM devices can be accessed by simply mapping memory and reading
> from / writing to it. This driver adds support for a generic
> "mmio-nvmem" DT binding used by such devices.
> 
> One of such devices is Broadcom's NVRAM. It's already supported (see
> NVMEM_BRCM_NVRAM) but existing driver covers both:
> 1. NVMEM device access
> 2. NVMEM content parsing

> 
> Once we get support for NVMEM layouts then existing NVRAM driver will
> get converted into a layout and generic driver will take over
> responsibility for data access. That's why it claims "brcm,nvram"
> (conditionally for now).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V3: Support "reg-io-width", basic writing & "brcm,nvram" string
> ---
>   drivers/nvmem/Kconfig  |  10 ++++
>   drivers/nvmem/Makefile |   2 +
>   drivers/nvmem/mmio.c   | 125 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 137 insertions(+)
>   create mode 100644 drivers/nvmem/mmio.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 6dec38805041..189ea85bd67d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -166,6 +166,16 @@ config NVMEM_MICROCHIP_OTPC
>   	  This driver enable the OTP controller available on Microchip SAMA7G5
>   	  SoCs. It controls the access to the OTP memory connected to it.
>   
> +config NVMEM_MMIO
> +	tristate "MMIO access based NVMEM support"
> +	depends on HAS_IOMEM
> +	help
> +	  This driver provides support for NVMEM devices that can be accessed
> +	  using MMIO.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nvmem-mmio.
> +
>   config NVMEM_MTK_EFUSE
>   	tristate "Mediatek SoCs EFUSE support"
>   	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 6a1efffa88f0..767a9db2bfc1 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -35,6 +35,8 @@ obj-$(CONFIG_NVMEM_MESON_MX_EFUSE)	+= nvmem_meson_mx_efuse.o
>   nvmem_meson_mx_efuse-y			:= meson-mx-efuse.o
>   obj-$(CONFIG_NVMEM_MICROCHIP_OTPC)	+= nvmem-microchip-otpc.o
>   nvmem-microchip-otpc-y			:= microchip-otpc.o
> +obj-$(CONFIG_NVMEM_MMIO)		+= nvmem-mmio.o
> +nvmem-mmio-y				:= mmio.o
>   obj-$(CONFIG_NVMEM_MTK_EFUSE)		+= nvmem_mtk-efuse.o
>   nvmem_mtk-efuse-y			:= mtk-efuse.o
>   obj-$(CONFIG_NVMEM_MXS_OCOTP)		+= nvmem-mxs-ocotp.o
> diff --git a/drivers/nvmem/mmio.c b/drivers/nvmem/mmio.c
> new file mode 100644
> index 000000000000..e247c943eea2
> --- /dev/null
> +++ b/drivers/nvmem/mmio.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Rafał Miłecki <rafal@milecki.pl>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +struct mmio_nvmem {
> +	void __iomem *base;
> +	u32 io_width;
> +};
> +
> +static int mmio_nvmem_read(void *context, unsigned int offset, void *val, size_t bytes)
> +{
> +	struct mmio_nvmem *priv = context;
> +	u32 *dst32;
> +	u16 *dst16;
> +	u8 *dst8;
> +
> +	if (priv->io_width && WARN_ON(bytes % priv->io_width))
> +		return -EINVAL;
> +
If there is a code path in core that would allow this to happen then it 
needs to be fixed at core level rather than handling it in provider.

> +	switch (priv->io_width) {
> +	case 1:
> +		for (dst8 = val; bytes; bytes -= 1, offset += 1)
> +			*dst8++ = readb(priv->base + offset);
> +		break;
> +	case 2:
> +		for (dst16 = val; bytes; bytes -= 2, offset += 2)
> +			*dst16++ = readw(priv->base + offset);
> +		break;
> +	case 4:
> +		for (dst32 = val; bytes; bytes -= 4, offset += 4)
> +			*dst32++ = readl(priv->base + offset);
> +		break;
> +	default:
> +		memcpy_fromio(val, priv->base + offset, bytes);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mmio_nvmem_write(void *context, unsigned int offset, void *val, size_t bytes)
> +{
> +	struct mmio_nvmem *priv = context;
> +
> +	switch (priv->io_width) {
If io_width is 0 core considers defaults it to 1. But in this patch its 
possible to still have priv->io_width 0. This is the behavior that you want?

> +	case 1:
> +	case 2:
> +	case 4:
> +		return -EOPNOTSUPP;
> +	default:
> +		memcpy_toio(priv->base + offset, val, bytes);
so, are we saying that we only support writes to 8 bytes io_width? Any 
reason for doing this way?

> +	}
> +
> +	return 0;
> +}
> +
> +static int mmio_nvmem_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config config = {
> +		.name = "mmio-nvmem",
> +		.id = NVMEM_DEVID_AUTO,
> +		.read_only = true,
> +		.reg_read = mmio_nvmem_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct mmio_nvmem *priv;
> +	struct resource *res;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	of_property_read_u32(dev->of_node, "reg-io-width", &priv->io_width);
> +
> +	config.dev = dev;
> +	config.size = resource_size(res);
> +	config.word_size = priv->io_width;
> +	config.stride = priv->io_width;
> +	config.priv = priv;
> +	if (!device_property_present(dev, "read-only"))
> +		config.reg_write = mmio_nvmem_write;
core should have already marked it readonly based on this property.

> +
> +	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id mmio_nvmem_of_match_table[] = {
> +	{ .compatible = "mmio-nvmem", },
> +	/* Custom bindings */
> +#if !IS_ENABLED(CONFIG_NVMEM_BRCM_NVRAM)
> +	{ .compatible = "brcm,nvram", },
> +#endif

If CONFIG_NVMEM_BRCM_NVRAM is not enabled we want to add "brcm,nvram" 
to this driver compatibles, that is something confusing to be honest 
atleast at this transition stage.


How about making nvmem-mmio export functions for drivers like brcm to 
call directly.


> +	{},
> +};
> +
> +static struct platform_driver mmio_nvmem_driver = {
> +	.probe = mmio_nvmem_probe,
> +	.driver = {
> +		.name = "mmio_nvmem",
> +		.of_match_table = mmio_nvmem_of_match_table,
> +	},
> +};
> +
> +static int __init mmio_nvmem_init(void)
> +{
> +	return platform_driver_register(&mmio_nvmem_driver);
> +}
> +
> +subsys_initcall_sync(mmio_nvmem_init);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, mmio_nvmem_of_match_table);