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

[Miquel Raynal]

Hi Rafał,

zajec5@xxxxxxxxx wrote on Thu, 9 Mar 2023 07:56:05 +0100:

> 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

This is backwards. That's why layouts have been proposed: having
a clear framework were the nvmem core should or should not play with
the OF children nodes. If each binding is different, you end up with
the mess we have today, where nobody knows how to properly
populate the cells.

Anyway, it's not a big deal either, if the cells are empty we can
easily check that and have *yet another* specific case in the core.

> 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()

Agreed. So what about config.ignore_of_children?
- mtd sets this to !is_compatible(nvmem-cells)
- nobody else touches it (the core don't try to parse the cell if it's
  empty so U-Boot env driver works)

Thanks,
Miquèl