Re: [PATCH V2] nvmem: add explicit config option to read OF fixed cells
From: Rafał Miłecki
Date: Thu Mar 09 2023 - 01:56:22 EST
On 8.03.2023 19:31, Miquel Raynal wrote:
Hi Rafał,
rafal@xxxxxxxxxx wrote on Wed, 08 Mar 2023 19:12:32 +0100:
On 2023-03-08 19:06, Miquel Raynal wrote:
Hi Rafał,
rafal@xxxxxxxxxx wrote on Wed, 08 Mar 2023 17:55:46 +0100:
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).
Layouts are just another possibility, either you user the nvmem-cells
compatible and produce nvmem cells with fixed OF nodes, or you use the
nvmem-layout container. I don't think all new bindings should have
cells in layouts. It depends if the content is static or not.
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
I'm saying: based on what has been enforced so far, I would expect all
fixed cell providers to come with nvmem-cells as compatible, no?
If that's the case we could use that as a common denominator?
Sorry, I don't get it. Have you checked
Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
?
It's a NVMEM provied binding with fixed cells that doesn't use
nvmem-cells as compatible. There are many more.
Oh yeah you're right, I'm mixing things. Well I guess you're right
then, it's such a mess, we have to tell the core the parsing method.
So maybe another question: do we have other situations than mtd which
sometimes expect the nvmem core to parse the OF nodes to populate cells,
and sometimes not?
I'm not aware of that. Please also check my patch. The only case I set
"use_fixed_of_cells" conditionally is mtd code. In other cases it's
hardcoded to "true".
Also, what about "of_children_are_cells" ? Because actually in most
cases it's a "fixed of cell", so I don't find the current naming
descriptive enough for something so touchy.
That would be just incorrect because this new config property
("use_fixed_of_cells") is only about FIXED cells.
There are cases of OF children being cells but NOT being fixed cells.
They should NOT be parsed by the nvmem_add_cells_from_of().
Example:
a607a850ba1f ("dt-bindings: nvmem: u-boot,env: add basic NVMEM cells")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df
So that would result in U-Boot env:
1. Having OF children nodes being cells
2. Setting "of_children_are_cells" to false (counter-intuitive) to avoid nvmem_add_cells_from_of()