2023-02-01 06:52:31

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 0/4] nvmem: add and use generic MMIO NVMEM

From: Rafał Miłecki <[email protected]>

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

I already replaced two existing drivers. With NVMEM layouts introduced I
can also replace Broadcom's NVRAM driver but that work depends on my
pending U-Boot patchset (it provides required helpers).

Rafał Miłecki (4):
dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM
devices
nvmem: add generic driver for devices with MMIO access
nvmem: mtk-efuse: replace driver with a generic MMIO one
nvmem: uniphier-efuse: replace driver with a generic MMIO one

.../devicetree/bindings/nvmem/mmio.yaml | 46 +++++++++
drivers/nvmem/Kconfig | 24 +++--
drivers/nvmem/Makefile | 6 +-
drivers/nvmem/mmio.c | 84 ++++++++++++++++
drivers/nvmem/mtk-efuse.c | 97 -------------------
drivers/nvmem/uniphier-efuse.c | 78 ---------------
6 files changed, 148 insertions(+), 187 deletions(-)
create mode 100644 Documentation/devicetree/bindings/nvmem/mmio.yaml
create mode 100644 drivers/nvmem/mmio.c
delete mode 100644 drivers/nvmem/mtk-efuse.c
delete mode 100644 drivers/nvmem/uniphier-efuse.c

--
2.34.1



2023-02-01 06:52:44

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 1/4] dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM devices

From: Rafał Miłecki <[email protected]>

With the NVMEM layouts binding in place we should now use:
1. NVMEM device access bindings
2. NVMEM content description bindings

This binding allows describing NVMEM devices that can be MMIO accessed.

Signed-off-by: Rafał Miłecki <[email protected]>
---
.../devicetree/bindings/nvmem/mmio.yaml | 46 +++++++++++++++++++
1 file changed, 46 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..27e3f6142769
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/mmio.yaml
@@ -0,0 +1,46 @@
+# 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 that can be accessed by using MMIO
+ (memory-mapped I/O access).
+
+ It's a generic solution for providing NVMEM content access. The way of
+ handling actual content may be device specific and can be described using a
+ proper layout.
+
+maintainers:
+ - Rafał Miłecki <[email protected]>
+
+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


2023-02-01 06:52:49

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 2/4] nvmem: add generic driver for devices with MMIO access

From: Rafał Miłecki <[email protected]>

With nvmem layouts in place we should now work on plain content access
NVMEM drivers (e.g. MMIO one). Actual NVMEM content handling should go
to layout drivers.

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/nvmem/Kconfig | 10 ++++++
drivers/nvmem/Makefile | 2 ++
drivers/nvmem/mmio.c | 80 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 92 insertions(+)
create mode 100644 drivers/nvmem/mmio.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 789729ff7e50..9eb5e93f0455 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -170,6 +170,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 442f9a4876a5..2f2bed7cdf24 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -36,6 +36,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..19c8880dc675
--- /dev/null
+++ b/drivers/nvmem/mmio.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Rafał Miłecki <[email protected]>
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+struct mmio_nvmem {
+ void __iomem *base;
+};
+
+static int mmio_nvmem_read(void *context, unsigned int offset, void *val, size_t bytes)
+{
+ struct mmio_nvmem *priv = context;
+
+ memcpy_fromio(val, priv->base, bytes);
+
+ return 0;
+}
+
+static int mmio_nvmem_probe(struct platform_device *pdev)
+{
+ struct nvmem_config config = {
+ .name = "mmio-nvmem",
+ .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);
+
+ config.dev = dev;
+ config.size = resource_size(res);
+ config.word_size = sizeof(u8);
+ config.stride = sizeof(u8);
+ config.priv = priv;
+
+ if (!device_property_present(dev, "read-only"))
+ dev_warn(dev, "Writing is not supported yet");
+
+ return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
+}
+
+static const struct of_device_id mmio_nvmem_of_match_table[] = {
+ { .compatible = "mmio-nvmem", },
+ {},
+};
+
+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


2023-02-01 06:52:52

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

From: Rafał Miłecki <[email protected]>

Mediatek EFUSE uses a simple MMIO that can be handled with a generic
driver. Replace this driver to avoid code duplication.

Keep Kconfig symbol for a release or two to help with "make oldconfig".

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/nvmem/Kconfig | 7 ++-
drivers/nvmem/Makefile | 2 -
drivers/nvmem/mmio.c | 3 ++
drivers/nvmem/mtk-efuse.c | 97 ---------------------------------------
4 files changed, 6 insertions(+), 103 deletions(-)
delete mode 100644 drivers/nvmem/mtk-efuse.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 9eb5e93f0455..4d652c7382a6 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -184,12 +184,11 @@ config NVMEM_MTK_EFUSE
tristate "Mediatek SoCs EFUSE support"
depends on ARCH_MEDIATEK || COMPILE_TEST
depends on HAS_IOMEM
+ select NVMEM_MMIO
help
- This is a driver to access hardware related data like sensor
- calibration, HDMI impedance etc.
+ This driver has been replaced by a generic MMIO implementation.

- This driver can also be built as a module. If so, the module
- will be called efuse-mtk.
+ Update your config as this symbol will be dropped in the next release.

config NVMEM_MXS_OCOTP
tristate "Freescale MXS On-Chip OTP Memory Support"
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 2f2bed7cdf24..7a8e29ea408e 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -38,8 +38,6 @@ 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
nvmem-mxs-ocotp-y := mxs-ocotp.o
obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
diff --git a/drivers/nvmem/mmio.c b/drivers/nvmem/mmio.c
index 19c8880dc675..253ade72e0c3 100644
--- a/drivers/nvmem/mmio.c
+++ b/drivers/nvmem/mmio.c
@@ -57,6 +57,9 @@ static int mmio_nvmem_probe(struct platform_device *pdev)

static const struct of_device_id mmio_nvmem_of_match_table[] = {
{ .compatible = "mmio-nvmem", },
+ /* Custom bindings that were introduced before the mmio one */
+ { .compatible = "mediatek,mt8173-efuse", },
+ { .compatible = "mediatek,efuse", },
{},
};

diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
deleted file mode 100644
index a08e0aedd21c..000000000000
--- a/drivers/nvmem/mtk-efuse.c
+++ /dev/null
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2015 MediaTek Inc.
- * Author: Andrew-CT Chen <[email protected]>
- */
-
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/io.h>
-#include <linux/nvmem-provider.h>
-#include <linux/platform_device.h>
-
-struct mtk_efuse_priv {
- void __iomem *base;
-};
-
-static int mtk_reg_read(void *context,
- unsigned int reg, void *_val, size_t bytes)
-{
- struct mtk_efuse_priv *priv = context;
- void __iomem *addr = priv->base + reg;
- u8 *val = _val;
- int i;
-
- for (i = 0; i < bytes; i++, val++)
- *val = readb(addr + i);
-
- return 0;
-}
-
-static int mtk_efuse_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct nvmem_device *nvmem;
- struct nvmem_config econfig = {};
- struct mtk_efuse_priv *priv;
-
- 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);
-
- econfig.stride = 1;
- econfig.word_size = 1;
- econfig.reg_read = mtk_reg_read;
- econfig.size = resource_size(res);
- econfig.priv = priv;
- econfig.dev = dev;
- nvmem = devm_nvmem_register(dev, &econfig);
-
- return PTR_ERR_OR_ZERO(nvmem);
-}
-
-static const struct of_device_id mtk_efuse_of_match[] = {
- { .compatible = "mediatek,mt8173-efuse",},
- { .compatible = "mediatek,efuse",},
- {/* sentinel */},
-};
-MODULE_DEVICE_TABLE(of, mtk_efuse_of_match);
-
-static struct platform_driver mtk_efuse_driver = {
- .probe = mtk_efuse_probe,
- .driver = {
- .name = "mediatek,efuse",
- .of_match_table = mtk_efuse_of_match,
- },
-};
-
-static int __init mtk_efuse_init(void)
-{
- int ret;
-
- ret = platform_driver_register(&mtk_efuse_driver);
- if (ret) {
- pr_err("Failed to register efuse driver\n");
- return ret;
- }
-
- return 0;
-}
-
-static void __exit mtk_efuse_exit(void)
-{
- return platform_driver_unregister(&mtk_efuse_driver);
-}
-
-subsys_initcall(mtk_efuse_init);
-module_exit(mtk_efuse_exit);
-
-MODULE_AUTHOR("Andrew-CT Chen <[email protected]>");
-MODULE_DESCRIPTION("Mediatek EFUSE driver");
-MODULE_LICENSE("GPL v2");
--
2.34.1


2023-02-01 06:52:55

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 4/4] nvmem: uniphier-efuse: replace driver with a generic MMIO one

From: Rafał Miłecki <[email protected]>

UniPhier eFuse uses a simple MMIO that can be handled with a generic
driver. Replace this driver to avoid code duplication.

Keep Kconfig symbol for a release or two to help with "make oldconfig".

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/nvmem/Kconfig | 7 ++-
drivers/nvmem/Makefile | 2 -
drivers/nvmem/mmio.c | 1 +
drivers/nvmem/uniphier-efuse.c | 78 ----------------------------------
4 files changed, 4 insertions(+), 84 deletions(-)
delete mode 100644 drivers/nvmem/uniphier-efuse.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 4d652c7382a6..d65950da39b0 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -359,12 +359,11 @@ config NVMEM_UNIPHIER_EFUSE
tristate "UniPhier SoCs eFuse support"
depends on ARCH_UNIPHIER || COMPILE_TEST
depends on HAS_IOMEM
+ select NVMEM_MMIO
help
- This is a simple driver to dump specified values of UniPhier SoC
- from eFuse.
+ This driver has been replaced by a generic MMIO implementation.

- This driver can also be built as a module. If so, the module
- will be called nvmem-uniphier-efuse.
+ Update your config as this symbol will be dropped in the next release.

config NVMEM_VF610_OCOTP
tristate "VF610 SoC OCOTP support"
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 7a8e29ea408e..70dd1154c832 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -67,8 +67,6 @@ obj-$(CONFIG_NVMEM_SUNPLUS_OCOTP) += nvmem_sunplus_ocotp.o
nvmem_sunplus_ocotp-y := sunplus-ocotp.o
obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem_sunxi_sid.o
nvmem_sunxi_sid-y := sunxi_sid.o
-obj-$(CONFIG_NVMEM_UNIPHIER_EFUSE) += nvmem-uniphier-efuse.o
-nvmem-uniphier-efuse-y := uniphier-efuse.o
obj-$(CONFIG_NVMEM_VF610_OCOTP) += nvmem-vf610-ocotp.o
nvmem-vf610-ocotp-y := vf610-ocotp.o
obj-$(CONFIG_NVMEM_ZYNQMP) += nvmem_zynqmp_nvmem.o
diff --git a/drivers/nvmem/mmio.c b/drivers/nvmem/mmio.c
index 253ade72e0c3..f44ff2a56c56 100644
--- a/drivers/nvmem/mmio.c
+++ b/drivers/nvmem/mmio.c
@@ -60,6 +60,7 @@ static const struct of_device_id mmio_nvmem_of_match_table[] = {
/* Custom bindings that were introduced before the mmio one */
{ .compatible = "mediatek,mt8173-efuse", },
{ .compatible = "mediatek,efuse", },
+ { .compatible = "socionext,uniphier-efuse", },
{},
};

diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c
deleted file mode 100644
index aca910b3b6f8..000000000000
--- a/drivers/nvmem/uniphier-efuse.c
+++ /dev/null
@@ -1,78 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * UniPhier eFuse driver
- *
- * Copyright (C) 2017 Socionext Inc.
- */
-
-#include <linux/device.h>
-#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/nvmem-provider.h>
-#include <linux/platform_device.h>
-
-struct uniphier_efuse_priv {
- void __iomem *base;
-};
-
-static int uniphier_reg_read(void *context,
- unsigned int reg, void *_val, size_t bytes)
-{
- struct uniphier_efuse_priv *priv = context;
- u8 *val = _val;
- int offs;
-
- for (offs = 0; offs < bytes; offs += sizeof(u8))
- *val++ = readb(priv->base + reg + offs);
-
- return 0;
-}
-
-static int uniphier_efuse_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct nvmem_device *nvmem;
- struct nvmem_config econfig = {};
- struct uniphier_efuse_priv *priv;
-
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- priv->base = devm_ioremap_resource(dev, res);
- if (IS_ERR(priv->base))
- return PTR_ERR(priv->base);
-
- econfig.stride = 1;
- econfig.word_size = 1;
- econfig.read_only = true;
- econfig.reg_read = uniphier_reg_read;
- econfig.size = resource_size(res);
- econfig.priv = priv;
- econfig.dev = dev;
- nvmem = devm_nvmem_register(dev, &econfig);
-
- return PTR_ERR_OR_ZERO(nvmem);
-}
-
-static const struct of_device_id uniphier_efuse_of_match[] = {
- { .compatible = "socionext,uniphier-efuse",},
- {/* sentinel */},
-};
-MODULE_DEVICE_TABLE(of, uniphier_efuse_of_match);
-
-static struct platform_driver uniphier_efuse_driver = {
- .probe = uniphier_efuse_probe,
- .driver = {
- .name = "uniphier-efuse",
- .of_match_table = uniphier_efuse_of_match,
- },
-};
-module_platform_driver(uniphier_efuse_driver);
-
-MODULE_AUTHOR("Keiji Hayashibara <[email protected]>");
-MODULE_DESCRIPTION("UniPhier eFuse driver");
-MODULE_LICENSE("GPL v2");
--
2.34.1


2023-02-01 07:50:53

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 0/4] nvmem: add and use generic MMIO NVMEM

On 1.02.2023 07:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> MMIO accessible NVMEM devices should be simple enough to allow using a
> single binding & driver for them.
>
> I already replaced two existing drivers. With NVMEM layouts introduced I
> can also replace Broadcom's NVRAM driver but that work depends on my
> pending U-Boot patchset (it provides required helpers).

This was meant to be marked as V2.

Changes:
1. Replaced io with mmio to be clear
2. Replaced 2 existing drivers with generic implementation

2023-02-01 08:48:50

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

> From: Rafał Miłecki <[email protected]>
>
> Mediatek EFUSE uses a simple MMIO that can be handled with a generic
> driver. Replace this driver to avoid code duplication.

I don't think this is the correct approach. You'll restrict that driver
to being read-only. I admit that right now, it's read only, but it can
be extended to also support efuse writing. With this changes, it's not
possible.

> static const struct of_device_id mmio_nvmem_of_match_table[] = {
> { .compatible = "mmio-nvmem", },
>+ /* Custom bindings that were introduced before the mmio one */
>+ { .compatible = "mediatek,mt8173-efuse", },
>+ { .compatible = "mediatek,efuse", },

Why do you assume that all mediatek efuses will be the same? This should
rather be something like (in the dts/binding):

compatible = "mediatek,mt8173-efuse", "mmio-nvmem";

So if there is no driver for the particular efuse, it will fall back to the
generic one.

-michael

2023-02-01 09:31:10

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

On 1.02.2023 09:48, Michael Walle wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> Mediatek EFUSE uses a simple MMIO that can be handled with a generic
>> driver. Replace this driver to avoid code duplication.
>
> I don't think this is the correct approach. You'll restrict that driver
> to being read-only. I admit that right now, it's read only, but it can
> be extended to also support efuse writing. With this changes, it's not
> possible.
>
>> static const struct of_device_id mmio_nvmem_of_match_table[] = {
>> { .compatible = "mmio-nvmem", },
>> + /* Custom bindings that were introduced before the mmio one */
>> + { .compatible = "mediatek,mt8173-efuse", },
>> + { .compatible = "mediatek,efuse", },
>
> Why do you assume that all mediatek efuses will be the same? This should
> rather be something like (in the dts/binding):
>
> compatible = "mediatek,mt8173-efuse", "mmio-nvmem";
>
> So if there is no driver for the particular efuse, it will fall back to the
> generic one.

Oh great, I'm making circles now.

Rob suggested I should convert existing drivers, see:
[PATCH 2/2] nvmem: add generic driver for devices with I/O based access
and I thought efuse ones should be good.

Please tell me how I should handle brcm,nvram without wasting more time.
I thought I had it sorted out but I just wasted 2 days.


I believe I need to make brcm,nvram NVMEM layout. Without converting it
I'm afraid you'll refuse my changes adding cell post processing (that
happened to my U-Boot attempts).

Before I convert brcm,nvram to NVMEM layout I need some binding & driver
providing MMIO device access. How to handle that?



2023-02-01 09:41:49

by Kunihiko Hayashi

[permalink] [raw]
Subject: Re: [PATCH 2/4] nvmem: add generic driver for devices with MMIO access

Hi,

On 2023/02/01 15:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> With nvmem layouts in place we should now work on plain content access
> NVMEM drivers (e.g. MMIO one). Actual NVMEM content handling should go
> to layout drivers.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> drivers/nvmem/Kconfig | 10 ++++++
> drivers/nvmem/Makefile | 2 ++
> drivers/nvmem/mmio.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 92 insertions(+)
> create mode 100644 drivers/nvmem/mmio.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 789729ff7e50..9eb5e93f0455 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -170,6 +170,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 442f9a4876a5..2f2bed7cdf24 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -36,6 +36,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..19c8880dc675
> --- /dev/null
> +++ b/drivers/nvmem/mmio.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Rafał Miłecki <[email protected]>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +struct mmio_nvmem {
> + void __iomem *base;
> +};
> +
> +static int mmio_nvmem_read(void *context, unsigned int offset, void *val,
> size_t bytes)
> +{
> + struct mmio_nvmem *priv = context;
> +
> + memcpy_fromio(val, priv->base, bytes);
> +
> + return 0;
> +}
> +
> +static int mmio_nvmem_probe(struct platform_device *pdev)
> +{
> + struct nvmem_config config = {
> + .name = "mmio-nvmem",

The fixed name breaks sysfs for multiple nvmem instances.

sysfs: cannot create duplicate filename '/bus/nvmem/devices/mmio-nvmem0'

> + .read_only = true,

As Michael said in the mediatek patch,
I also think it's hard to make read-only fixed in the generic driver.

> + .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);
> +
> + config.dev = dev;
> + config.size = resource_size(res);
> + config.word_size = sizeof(u8);
> + config.stride = sizeof(u8);
> + config.priv = priv;
> +
> + if (!device_property_present(dev, "read-only"))
> + dev_warn(dev, "Writing is not supported yet");

Isn't the logic opposite?
Anyway, the driver doesn't use "read-only" properties for selection.

> +
> + return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id mmio_nvmem_of_match_table[] = {
> + { .compatible = "mmio-nvmem", },
> + {},
> +};
> +
> +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);

Thank you,

---
Best Regards
Kunihiko Hayashi

2023-02-01 10:46:11

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

> Before I convert brcm,nvram to NVMEM layout I need some binding &
> driver
> providing MMIO device access. How to handle that?

I'm not arguing against having the mmio nvmem driver. But I don't
think we should sacrifice possible write access with other drivers. And
I presume write access won't be possible with your generic driver as it
probably isn't just a memcpy_toio().

It is a great fallback for some nvmem peripherals which just maps a
memory region, but doesn't replace a proper driver for an nvmem device.

What bothers me the most isn't the driver change. The driver can be
resurrected once someone will do proper write access, but the generic
"mediatek,efuse" compatible together with the comment above the older
compatible string. These imply that you should use "mediatek,efuse",
but we don't know if all mediatek efuse peripherals will be the
same - esp. for writing which is usually more complex than the reading.

nitpick btw: why not "nvmem-mmio"?

So it's either:
(1) compatible = "mediatek,mt8173-efuse"
(2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"

(1) will be supported any anyway for older dts and you need to add
the specific compatibles to the nvmem-mmio driver - or keep the
driver as is.

With (2) you wouldn't need to do that and the kernel can load the
proper driver if available or fall back to the nvmem-mmio one. I'd
even make that one "default y" so it will be available on future
kernels and boards can already make use of the nvmem device even
if there is no proper driver for them.

I'd prefer (2). Dunno what the dt maintainers agree.

-michael

2023-02-01 11:01:16

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

On 1.02.2023 11:46, Michael Walle wrote:
>> Before I convert brcm,nvram to NVMEM layout I need some binding & driver
>> providing MMIO device access. How to handle that?
>
> I'm not arguing against having the mmio nvmem driver. But I don't
> think we should sacrifice possible write access with other drivers. And
> I presume write access won't be possible with your generic driver as it
> probably isn't just a memcpy_toio().
>
> It is a great fallback for some nvmem peripherals which just maps a
> memory region, but doesn't replace a proper driver for an nvmem device.

OK, then maybe I'll retry again with generic MMIO and without converting
existing specific drivers. That is what (AFAIU) Rob asked though.


> What bothers me the most isn't the driver change. The driver can be
> resurrected once someone will do proper write access, but the generic
> "mediatek,efuse" compatible together with the comment above the older
> compatible string. These imply that you should use "mediatek,efuse",
> but we don't know if all mediatek efuse peripherals will be the
> same - esp. for writing which is usually more complex than the reading.

mediatek,efuse was already there, don't blame me for it ;)


> nitpick btw: why not "nvmem-mmio"?

Because I read from left to right ;)
It's MMIO based NVMEM. Not MMIO on top of NVMEM.

Sure, we have "nvmem-cells" but that is because those are cells of NVMEM
device.

Unless my English knowledge fails me.


> So it's either:
>  (1) compatible = "mediatek,mt8173-efuse"
>  (2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"
>
> (1) will be supported any anyway for older dts and you need to add
> the specific compatibles to the nvmem-mmio driver - or keep the
> driver as is.
>
> With (2) you wouldn't need to do that and the kernel can load the
> proper driver if available or fall back to the nvmem-mmio one. I'd
> even make that one "default y" so it will be available on future
> kernels and boards can already make use of the nvmem device even
> if there is no proper driver for them.
>
> I'd prefer (2). Dunno what the dt maintainers agree.

(2) looks OK, Rob, Krzysztof?

2023-02-01 11:11:20

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

>> nitpick btw: why not "nvmem-mmio"?
>
> Because I read from left to right ;)

if you have more compatibles do you prefer

a-grp
bcdef-grp
gh-grp

or

grp-a
grp-bcdef
grp-gh

To my eyes the latter looks nicer, but it's just a matter of taste ;)

-michael

2023-02-01 11:53:18

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH 2/4] nvmem: add generic driver for devices with MMIO access



On 01/02/2023 06:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> With nvmem layouts in place we should now work on plain content access
> NVMEM drivers (e.g. MMIO one). Actual NVMEM content handling should go
> to layout drivers.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> drivers/nvmem/Kconfig | 10 ++++++
> drivers/nvmem/Makefile | 2 ++
> drivers/nvmem/mmio.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 92 insertions(+)
> create mode 100644 drivers/nvmem/mmio.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 789729ff7e50..9eb5e93f0455 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -170,6 +170,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 442f9a4876a5..2f2bed7cdf24 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -36,6 +36,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..19c8880dc675
> --- /dev/null
> +++ b/drivers/nvmem/mmio.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Rafał Miłecki <[email protected]>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +struct mmio_nvmem {
> + void __iomem *base;
> +};
> +
> +static int mmio_nvmem_read(void *context, unsigned int offset, void *val, size_t bytes)
> +{
> + struct mmio_nvmem *priv = context;
> +
> + memcpy_fromio(val, priv->base, bytes);

How does this work with different register strides?

> +
> + return 0;
> +}
> +
> +static int mmio_nvmem_probe(struct platform_device *pdev)
> +{
> + struct nvmem_config config = {
> + .name = "mmio-nvmem",


you could use NVMEM_DEVID_AUTO.


But this is not great, this is going to break the existing abi for
converted drivers. Either we find a way to pass compatible specific data
and override the default values.

Also there are going to be cases where access to registers will require
additional resources like clks or pd.


--srini

> + .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);
> +
> + config.dev = dev;
> + config.size = resource_size(res);
> + config.word_size = sizeof(u8);
> + config.stride = sizeof(u8);
> + config.priv = priv;
> +
> + if (!device_property_present(dev, "read-only"))
> + dev_warn(dev, "Writing is not supported yet");
> +
> + return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id mmio_nvmem_of_match_table[] = {
> + { .compatible = "mmio-nvmem", },
> + {},
> +};
> +
> +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);

2023-02-01 18:54:08

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

On Wed, Feb 01, 2023 at 11:46:01AM +0100, Michael Walle wrote:
> > Before I convert brcm,nvram to NVMEM layout I need some binding & driver
> > providing MMIO device access. How to handle that?
>
> I'm not arguing against having the mmio nvmem driver. But I don't
> think we should sacrifice possible write access with other drivers. And
> I presume write access won't be possible with your generic driver as it
> probably isn't just a memcpy_toio().
>
> It is a great fallback for some nvmem peripherals which just maps a
> memory region, but doesn't replace a proper driver for an nvmem device.
>
> What bothers me the most isn't the driver change. The driver can be
> resurrected once someone will do proper write access, but the generic
> "mediatek,efuse" compatible together with the comment above the older
> compatible string. These imply that you should use "mediatek,efuse",
> but we don't know if all mediatek efuse peripherals will be the
> same - esp. for writing which is usually more complex than the reading.

Because the kernel can't pick the "best" driver when there are multiple
matches, it's all Mediatek platforms use the generic driver or all use
the Mediatek driver.

Personally, I think it is easy enough to revive the driver if needed
unless writing is a soon and likely feature.

The other way to share is providing library functions for drivers to
use. Then the Mediatek driver can use the generic read functions and
custom write functions.

> nitpick btw: why not "nvmem-mmio"?
>
> So it's either:
> (1) compatible = "mediatek,mt8173-efuse"
> (2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"
>
> (1) will be supported any anyway for older dts and you need to add
> the specific compatibles to the nvmem-mmio driver - or keep the
> driver as is.
>
> With (2) you wouldn't need to do that and the kernel can load the
> proper driver if available or fall back to the nvmem-mmio one. I'd
> even make that one "default y" so it will be available on future
> kernels and boards can already make use of the nvmem device even
> if there is no proper driver for them.
>
> I'd prefer (2). Dunno what the dt maintainers agree.

No because you are changing the DT. The DT can't change when you want to
change drivers. This thinking is one reason why 'generic' bindings are
rejected.

Rob

2023-02-01 20:15:58

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

Am 2023-02-01 19:54, schrieb Rob Herring:
> On Wed, Feb 01, 2023 at 11:46:01AM +0100, Michael Walle wrote:
>> > Before I convert brcm,nvram to NVMEM layout I need some binding & driver
>> > providing MMIO device access. How to handle that?
>>
>> I'm not arguing against having the mmio nvmem driver. But I don't
>> think we should sacrifice possible write access with other drivers.
>> And
>> I presume write access won't be possible with your generic driver as
>> it
>> probably isn't just a memcpy_toio().
>>
>> It is a great fallback for some nvmem peripherals which just maps a
>> memory region, but doesn't replace a proper driver for an nvmem
>> device.
>>
>> What bothers me the most isn't the driver change. The driver can be
>> resurrected once someone will do proper write access, but the generic
>> "mediatek,efuse" compatible together with the comment above the older
>> compatible string. These imply that you should use "mediatek,efuse",
>> but we don't know if all mediatek efuse peripherals will be the
>> same - esp. for writing which is usually more complex than the
>> reading.
>
> Because the kernel can't pick the "best" driver when there are multiple
> matches, it's all Mediatek platforms use the generic driver or all use
> the Mediatek driver.

Isn't that the whole point of having multiple compatible strings?
compatible = "fsl,imx27-mmc", "fsl,imx21-mmc";
The OS might either load the driver for "fsl,imx21-mmc" or one for
"fsl,imx27-mmc", with the latter considered to be the preferred one.

> Personally, I think it is easy enough to revive the driver if needed
> unless writing is a soon and likely feature.

That what was actually triggered my initial reply. We are planning a
new board with a mediatek SoC and we'll likely need the write support.

But I thought the "mediatek,efuse" was a new compatible with this patch
and the (new!) comment make it looks like these compatible are
deprecated
in favor of "mmio-nvmem". Which would make it impossible to distinguish
between the different efuse peripherals and thus make it impossible to
add write support.

> The other way to share is providing library functions for drivers to
> use. Then the Mediatek driver can use the generic read functions and
> custom write functions.
>
>> nitpick btw: why not "nvmem-mmio"?
>>
>> So it's either:
>> (1) compatible = "mediatek,mt8173-efuse"
>> (2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"
>>
>> (1) will be supported any anyway for older dts and you need to add
>> the specific compatibles to the nvmem-mmio driver - or keep the
>> driver as is.
>>
>> With (2) you wouldn't need to do that and the kernel can load the
>> proper driver if available or fall back to the nvmem-mmio one. I'd
>> even make that one "default y" so it will be available on future
>> kernels and boards can already make use of the nvmem device even
>> if there is no proper driver for them.
>>
>> I'd prefer (2). Dunno what the dt maintainers agree.
>
> No because you are changing the DT. The DT can't change when you want
> to
> change drivers. This thinking is one reason why 'generic' bindings are
> rejected.

There is no change in the DT. Newer bindings will have

compatible = "vendor,ip-block", "mmio-nvmem"

when the ip block is compatible with mmio-nvmem. Otherwise I don't get
why there is a mmio-nvmem compatible at all. Just having

compatible = "mmio-nvmem"

looks wrong as it would just work correctly in some minor cases, i.e.
when write support is just a memcpy_toio() - or we deliberately ignore
any write support. But even then, you always tell people to add specific
compatibles for the case when quirks are needed..

-michael

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

Il 01/02/23 07:47, Rafał Miłecki ha scritto:
> From: Rafał Miłecki <[email protected]>
>
> With nvmem layouts in place we should now work on plain content access
> NVMEM drivers (e.g. MMIO one). Actual NVMEM content handling should go
> to layout drivers.
>
> Signed-off-by: Rafał Miłecki <[email protected]>

I agree but if you want to really bring on this change, you should add
some kind of write support... and clocks... and regulators with different
voltage levels (write/fuse-blow voltage, read voltage if any)...

Describing this entire thing in device-tree should be possible, but then
some SoCs may need a specific register sequence in order to enter writing
mode, which is something that you can't just put in devicetree, so this
driver should be a framework on its own - hence not as simple as proposed.

Regards,
Angelo

2023-02-02 23:44:45

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

On Wed, Feb 01, 2023 at 09:15:50PM +0100, Michael Walle wrote:
> Am 2023-02-01 19:54, schrieb Rob Herring:
> > On Wed, Feb 01, 2023 at 11:46:01AM +0100, Michael Walle wrote:
> > > > Before I convert brcm,nvram to NVMEM layout I need some binding & driver
> > > > providing MMIO device access. How to handle that?
> > >
> > > I'm not arguing against having the mmio nvmem driver. But I don't
> > > think we should sacrifice possible write access with other drivers.
> > > And
> > > I presume write access won't be possible with your generic driver as
> > > it
> > > probably isn't just a memcpy_toio().
> > >
> > > It is a great fallback for some nvmem peripherals which just maps a
> > > memory region, but doesn't replace a proper driver for an nvmem
> > > device.
> > >
> > > What bothers me the most isn't the driver change. The driver can be
> > > resurrected once someone will do proper write access, but the generic
> > > "mediatek,efuse" compatible together with the comment above the older
> > > compatible string. These imply that you should use "mediatek,efuse",
> > > but we don't know if all mediatek efuse peripherals will be the
> > > same - esp. for writing which is usually more complex than the
> > > reading.
> >
> > Because the kernel can't pick the "best" driver when there are multiple
> > matches, it's all Mediatek platforms use the generic driver or all use
> > the Mediatek driver.
>
> Isn't that the whole point of having multiple compatible strings?
> compatible = "fsl,imx27-mmc", "fsl,imx21-mmc";
> The OS might either load the driver for "fsl,imx21-mmc" or one for
> "fsl,imx27-mmc", with the latter considered to be the preferred one.

No, there's some assumption they would be similar enough to be served by
the same driver.

While it seems like we'd want to fix this, it's been an issue I've been
aware of as long as I've been involved with DT and yet I don't recall
anyone really having an issue. Could just be everyone couples their
kernel and dtb versions...

> > Personally, I think it is easy enough to revive the driver if needed
> > unless writing is a soon and likely feature.
>
> That what was actually triggered my initial reply. We are planning a
> new board with a mediatek SoC and we'll likely need the write support.

If write support is on the horizon, then I'd say keep the Mediatek
driver.

> But I thought the "mediatek,efuse" was a new compatible with this patch
> and the (new!) comment make it looks like these compatible are deprecated
> in favor of "mmio-nvmem". Which would make it impossible to distinguish
> between the different efuse peripherals and thus make it impossible to
> add write support.

No, it's in the schema and dts files.

>
> > The other way to share is providing library functions for drivers to
> > use. Then the Mediatek driver can use the generic read functions and
> > custom write functions.
> >
> > > nitpick btw: why not "nvmem-mmio"?
> > >
> > > So it's either:
> > > (1) compatible = "mediatek,mt8173-efuse"
> > > (2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"
> > >
> > > (1) will be supported any anyway for older dts and you need to add
> > > the specific compatibles to the nvmem-mmio driver - or keep the
> > > driver as is.
> > >
> > > With (2) you wouldn't need to do that and the kernel can load the
> > > proper driver if available or fall back to the nvmem-mmio one. I'd
> > > even make that one "default y" so it will be available on future
> > > kernels and boards can already make use of the nvmem device even
> > > if there is no proper driver for them.
> > >
> > > I'd prefer (2). Dunno what the dt maintainers agree.
> >
> > No because you are changing the DT. The DT can't change when you want to
> > change drivers. This thinking is one reason why 'generic' bindings are
> > rejected.
>
> There is no change in the DT. Newer bindings will have
>
> compatible = "vendor,ip-block", "mmio-nvmem"
>
> when the ip block is compatible with mmio-nvmem. Otherwise I don't get
> why there is a mmio-nvmem compatible at all. Just having
>
> compatible = "mmio-nvmem"
>
> looks wrong as it would just work correctly in some minor cases, i.e.
> when write support is just a memcpy_toio() - or we deliberately ignore
> any write support. But even then, you always tell people to add specific
> compatibles for the case when quirks are needed..

Yes, I do. I was assuming these are simple cases unlikely to need
platform specific support. We're just reading static bits from
registers...

If you may need write support or have other complications, then
"mmio-nvmem" should not be used. You can use the driver, but it should
match with a platform specific compatible, not the generic one.

Rob

2023-02-03 21:08:35

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: nvmem: mmio: new binding for MMIO accessible NVMEM devices

On Wed, Feb 01, 2023 at 07:47:14AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> With the NVMEM layouts binding in place we should now use:
> 1. NVMEM device access bindings
> 2. NVMEM content description bindings
>
> This binding allows describing NVMEM devices that can be MMIO accessed.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../devicetree/bindings/nvmem/mmio.yaml | 46 +++++++++++++++++++
> 1 file changed, 46 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..27e3f6142769
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mmio.yaml
> @@ -0,0 +1,46 @@
> +# 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 that can be accessed by using MMIO
> + (memory-mapped I/O access).
> +
> + It's a generic solution for providing NVMEM content access. The way of
> + handling actual content may be device specific and can be described using a
> + proper layout.

Please add some guidance based on the discussion about when this should
and shouldn't be used. Specifically, anything with potential write
accesses should use a device specific compatible and not the generic
one.

> +
> +maintainers:
> + - Rafał Miłecki <[email protected]>
> +
> +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
>