Re: [PATCH V2] nvmem: add explicit config option to read OF fixed cells

From: Rafał Miłecki
Date: Wed Mar 08 2023 - 15:33:26 EST

On 2023-03-08 17:34, Miquel Raynal wrote:
Hi Rafał,

zajec5@xxxxxxxxx wrote on Fri, 24 Feb 2023 08:29:03 +0100:

From: Rafał Miłecki <rafal@xxxxxxxxxx>

NVMEM subsystem looks for fixed NVMEM cells (specified in DT) by
default. This behaviour made sense in early days before adding support
for dynamic cells.

With every new supported NVMEM device with dynamic cells current
behaviour becomes non-optimal. It results in unneeded iterating over DT
nodes and may result in false discovery of cells (depending on used DT
properties).

This behaviour has actually caused a problem already with the MTD
subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.

That's true, but I expect this to be really MTD specific.

A concrete proposal below.

Also with upcoming support for NVMEM layouts no new binding or driver
should support fixed cells defined in device node.

I'm not sure I agree with this statement. We are not preventing new
binding/driver to use fixed cells, or...? We offer a new way to expose
nvmem cells with another way than "fixed-offset" and "fixed-size" OF
nodes.

From what I understood all new NVMEM bindings should have cells defined
in the nvmem-layout { } node. That's what I mean by saying they should
not be defined in device node (but its "nvmem-layout" instead).


Solve this by modifying drivers for bindings that support specifying
fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
read cells from DT.

It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
enabled them to don't risk any breakage.

Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
[for drivers/nvmem/meson-{efuse,mx-efuse}.c]
Acked-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
---
V2: Fix stm32-romem.c typo breaking its compilation
Pick Martin's Acked-by
Add paragraph about layouts deprecating use_fixed_of_cells
---
drivers/mtd/mtdcore.c | 2 ++
drivers/nvmem/apple-efuses.c | 1 +
drivers/nvmem/core.c | 8 +++++---
drivers/nvmem/imx-ocotp-scu.c | 1 +
drivers/nvmem/imx-ocotp.c | 1 +
drivers/nvmem/meson-efuse.c | 1 +
drivers/nvmem/meson-mx-efuse.c | 1 +
drivers/nvmem/microchip-otpc.c | 1 +
drivers/nvmem/mtk-efuse.c | 1 +
drivers/nvmem/qcom-spmi-sdam.c | 1 +
drivers/nvmem/qfprom.c | 1 +
drivers/nvmem/rave-sp-eeprom.c | 1 +
drivers/nvmem/rockchip-efuse.c | 1 +
drivers/nvmem/sc27xx-efuse.c | 1 +
drivers/nvmem/sprd-efuse.c | 1 +
drivers/nvmem/stm32-romem.c | 1 +
drivers/nvmem/sunplus-ocotp.c | 1 +
drivers/nvmem/sunxi_sid.c | 1 +
drivers/nvmem/uniphier-efuse.c | 1 +
drivers/nvmem/zynqmp_nvmem.c | 1 +
drivers/rtc/nvmem.c | 1 +
drivers/w1/slaves/w1_ds250x.c | 1 +
include/linux/nvmem-provider.h | 2 ++
23 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0feacb9fbdac..1bb479c0f758 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -523,6 +523,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
config.dev = &mtd->dev;
config.name = dev_name(&mtd->dev);
config.owner = THIS_MODULE;
+ config.use_fixed_of_cells = of_device_is_compatible(node, "nvmem-cells");

I am wondering how mtd specific this is? For me all OF nodes containing
the nvmem-cells compatible should be treated as cells providers and
populate nvmem cells as for each children.

Why don't we just check for this compatible to be present? in
nvmem_add_cells_from_of() ? And if not we just skip the operation.

This way we still follow the bindings (even though using nvmem-cells in
the compatible property to require cells population was a mistake in
the first place, as discussed in the devlink thread recently) but there
is no need for a per-driver config option?

This isn't mtd specific. Please check this patch for all occurrences of
the:
use_fixed_of_cells = true

The very first one: drivers/nvmem/apple-efuses.c driver for the
"apple,efuses" binding. That binding supports fixed OF cells, see:
Documentation/devicetree/bindings/nvmem/apple,efuses.yaml


config.reg_read = mtd_nvmem_reg_read;
config.size = mtd->size;
config.word_size = 1;
@@ -891,6 +892,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
config.name = kasprintf(GFP_KERNEL, "%s-%s", dev_name(&mtd->dev), compatible);
config.id = NVMEM_DEVID_NONE;
config.owner = THIS_MODULE;
+ config.use_fixed_of_cells = true;
config.type = NVMEM_TYPE_OTP;
config.root_only = true;
config.ignore_wp = true;
diff --git a/drivers/nvmem/apple-efuses.c b/drivers/nvmem/apple-efuses.c
index 9b7c87102104..0119bac43b2c 100644
--- a/drivers/nvmem/apple-efuses.c
+++ b/drivers/nvmem/apple-efuses.c
@@ -36,6 +36,7 @@ static int apple_efuses_probe(struct platform_device *pdev)
struct resource *res;
struct nvmem_config config = {
.dev = &pdev->dev,
+ .use_fixed_of_cells = true,
.read_only = true,
.reg_read = apple_efuses_read,
.stride = sizeof(u32),
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 174ef3574e07..6783cd8478d7 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -844,9 +844,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
if (rval)
goto err_remove_cells;

- rval = nvmem_add_cells_from_of(nvmem);
- if (rval)
- goto err_remove_cells;
+ if (config->use_fixed_of_cells) {
+ rval = nvmem_add_cells_from_of(nvmem);
+ if (rval)
+ goto err_remove_cells;
+ }

dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);


Thanks,
Miquèl