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

[Rafał Miłecki]

On 10.03.2023 10:22, Srinivas Kandagatla wrote:
> On 09/03/2023 11:20, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@xxxxxxxxxx>
> >
> > NVMEM subsystem looks for fixed NVMEM cells (specified in DT) by
> > default. This behaviour was totally safe in early days before adding
> > support for dynamic cells and with simple DT syntax.
> >
> > With every new supported NVMEM device with dynamic cells the current
> > behaviour becomes non-optimal:
> > 1. It results in unneeded iterating over DT nodes
> > 2. It may result in false discovery of cells (in case DT subnodes
> >     contain "reg" property)
>
> Am really not sure what is going on here,
> I did raise some issues with this overall approch to start with at [1] none of which are discussed and now I see v3 :-)
>
> [1] https://lore.kernel.org/lkml/20230309094010.1051573-1-michael@xxxxxxxx/T/#m7706b640979aabf251436e017b8189413661a53a

I updated commit message to address your concerns. I thought I made it
clear. I don't know how to emphasize it better.

I'll try to answer nevertheless, please see below.


On 9.03.2023 10:37, Srinivas Kandagatla wrote:
> On 24/02/2023 07:29, Rafał Miłecki wrote:
>> 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.
>>
>> Also with upcoming support for NVMEM layouts no new binding or driver
>> should support fixed cells defined in device node.
>
> This is not very clear, are you saying that we should not support fixed cells? If that is the case then you are proabably taking this in wrong direction. nvmem was built based on the fact that drivers can read from a fixed offsets. Dynamic cells is something very new, that does not mean that we should ditch fixed cells support in dt.

I DON'T deprecate or drop support for fixed layouts (fixed NVMEM cells).
Period.
I WON'T drop support for old binding. We stay backward compatible.
Period.

In this patch's body I wrote:
"with the support for NVMEM layouts we may & should have *new* bindings allow fixed NVMEM cells only in the "nvmem-layout" subnode"
that clearly means I still want to ALLOW fixed NVMEM cells - just in the *nvmem-layout* node.

I want to KEEP support for fixed NVMEM cells.
I just want them to be preferably defined in the "nvmem-layout" node.


>> 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.
>
> Shouldn't this be opposite, let the new providers tell that cells are created at runtime?
>
> or even better if there is a way to detect if we can set this flag dynamically based on layout/post-processing configuration.
>
> that should be much cleaner approch.

I tried to address this concert in the following part of commit body:

> The best approach seems to be making NVMEM core looking for fixed DT
> cells in **device** node an opt-in feature. It's a feature that over
> time should get deprecated in a favor of using "nvmem-layouts" also for
> fixed NVMEM cells.

New NVMEM provider bindings and drivers will get developed. I would
want all new bindings to use "nvmem-layout" for describing NVMEM cells
(no matter if fixed or dynamic).

That means all new drivers WILL NOT need to set "use_fixed_of_cells".

So over time "use_fixed_of_cells" will become a minority. It'w would be
pitty to have every new driver to request NVMEM code to skip looking for
NVMEM cells in **device** node.

So my answer is: no. I don't believe it should be opposite. Looking for
fixed NVMEM cells in **device** DT node should be an opt-in.

If by some miracle I manage to get my patches through then you'll forget
about "use_fixed_of_cells" next month. Noone will need it for any new
stuff. It'll stay for backward compatibility only.