On Tue, 21 Aug 2018 14:57:25 +0200
Boris Brezillon [off-list ref] wrote:

On Tue, 21 Aug 2018 14:27:16 +0200
Alban [off-list ref] wrote:

quoted

On Tue, 21 Aug 2018 07:44:04 +0200
Boris Brezillon [off-list ref] wrote:
  

quoted

On Tue, 21 Aug 2018 00:53:27 +0200
Alban [off-list ref] wrote:
    

quoted

On Sun, 19 Aug 2018 18:46:09 +0200
Boris Brezillon [off-list ref] wrote:
      

quoted

On Sun, 19 Aug 2018 13:31:06 +0200
Alban [off-list ref] wrote:
        

quoted

On Fri, 17 Aug 2018 18:27:20 +0200
Boris Brezillon [off-list ref] wrote:
          

quoted

Hi Bartosz,

On Fri, 10 Aug 2018 10:05:03 +0200
Bartosz Golaszewski [off-list ref] wrote:
            

quoted

From: Alban Bedel <albeu@free.fr>

Allow drivers that use the nvmem API to read data
stored on MTD devices. For this the mtd devices are
registered as read-only NVMEM providers.

Signed-off-by: Alban Bedel <albeu@free.fr>
[Bartosz:
  - use the managed variant of nvmem_register(),
  - set the nvmem name]
Signed-off-by: Bartosz Golaszewski
[off-list ref]              

What happened to the 2 other patches of Alban's series?
I'd really like the DT case to be handled/agreed on in
the same patchset, but IIRC, Alban and Srinivas disagreed
on how this should be represented. I hope this time we'll
come to an agreement, because the MTD <-> NVMEM glue has
been floating around for quite some time...            

These other patches were to fix what I consider a
fundamental flaw in the generic NVMEM bindings, however we
couldn't agree on this point. Bartosz later contacted me to
take over this series and I suggested to just change the
MTD NVMEM binding to use a compatible string on the NVMEM
cells as an alternative solution to fix the clash with the
old style MTD partition.

However all this has no impact on the code needed to add
NVMEM support to MTD, so the above patch didn't change at
all.          

It does have an impact on the supported binding though.
nvmem->dev.of_node is automatically assigned to
mtd->dev.of_node, which means people will be able to define
their NVMEM cells directly under the MTD device and reference
them from other nodes (even if it's not documented), and as
you said, it conflict with the old MTD partition bindings. So
we'd better agree on this binding before merging this
patch.        

Unless the nvmem cell node has a compatible string, then it
won't be considered as a partition by the MTD code. That is
were the clash is, both bindings allow free named child nodes
without a compatible string.      

Except the current nvmem cells parsing code does not enforce
that, and existing DTs rely on this behavior, so we're screwed.
Or are you suggesting to add a new "bool check_cells_compat;"
field to nvmem_config?    

There is no nvmem cell parsing at the moment. The DT lookup just
resolve the phandle to the cell node, take the parent node and
search for the nvmem provider that has this OF node. So extending
it in case the node has a *new* compatible string would not break
users of the old binding, none of them has a compatible string.  

But we want to enforce the compat check on MTD devices, otherwise old
MTD partitions (those defined under the MTD node) will be considered
as NVMEM cells by the NVMEM framework. Hence the bool
check_cells_compat field.

That would only be needed if the NVMEM framework would do "forward"
parsing, creating data structure for each NVMEM cell found under an
NVMEM provider. However currently it doesn't do that and only goes
"backward", starting by resolving a phandle pointing to a cell, then
finding the provider that the cell belongs to.

This also has the side effect that nvmem cells defined in DT don't
appear in sysfs, unlike those defined from board code.

quoted

quoted

quoted

quoted

I see several options:

1/ provide a way to tell the NVMEM framework not to use
parent->of_node even if it's != NULL. This way we really don't
support defining NVMEM cells in the DT, and also don't support
referencing the nvmem device using a phandle.        

I really don't get what the point of this would be. Make the
whole API useless?      

No, just allow Bartosz to get his changes merged without waiting
for you and Srinivas to agree on how to handle the new binding.
As I said earlier, this mtd <-> nvmem stuff has been around for
quite some time, and instead of trying to find an approach that
makes everyone happy, you decided to let the patchset die.    

As long as that wouldn't prevent using DT in the future I'm fine
with it.
  

quoted

quoted

quoted

2/ define a new binding where all nvmem-cells are placed in an
   "nvmem" subnode (just like we have this "partitions"
subnode for partitions), and then add a config->of_node field
so that the nvmem provider can explicitly specify the DT node
representing the nvmem device. We'll also need to set this
field to ERR_PTR(-ENOENT) in case this node does not exist so
that the nvmem framework knows that it should not assign
nvmem->dev.of_node to parent->of_node        

This is not good. First the NVMEM device is only a virtual
concept of the Linux kernel, it has no place in the DT.      

nvmem-cells is a virtual concept too, still, you define them in
the DT.    

To be honest I also think that naming this concept "nvmem" in the
DT was a bad idea. Perhaps something like "driver-data" or
"data-cell" would have been better as that would make it clear what
this is about, nvmem is just the Linux implementation of this
concept.  

I'm fine using a different name.

quoted

quoted

quoted

Secondly the NVMEM
provider (here the MTD device) then has to manually parse its DT
node to find this subnode, pass it to the NVMEM framework to
later again resolve it back to the MTD device.      

We don't resolve it back to the MTD device, because the MTD
device is just the parent of the nvmem device.
   

quoted

Not very complex but still a lot of
useless code, just registering the MTD device is a lot simpler
and much more inline with most other kernel API that register a
"service" available from a device.      

I'm not a big fan of this option either, but I thought I had to
propose it.
    

quoted

quoted

3/ only declare partitions as nvmem providers. This would
solve the problem we have with partitions defined in the DT
since defining sub-partitions in the DT is not (yet?)
supported and partition nodes are supposed to be leaf nodes.
Still, I'm not a big fan of this solution because it will
prevent us from supporting sub-partitions if we ever
want/need to.        

That sound like a poor workaround.      

Yes, that's a workaround. And the reason I propose it, is, again,
because I don't want to block Bartosz.
    

quoted

Remember that this problem could
appear with any device that has a binding that use child
nodes.      

I'm talking about partitions, and you're talking about mtd
devices. Right now partitions don't have subnodes, and if we
define that partition subnodes should describe nvmem-cells, then
it becomes part of the official binding. So, no, the problem you
mention does not (yet) exist.    

That would add another binding that allow free named child nodes
without compatible string although experience has repeatedly shown
that this was a bad idea.  

Yes, I agree. Just thought it was important to have this solution in
the list, even if it's just to reject it.

quoted

quoted

quoted

quoted

4/ Add a ->of_xlate() hook that would be called if present by
the framework instead of using the default parsing we have
right now.        

That is a bit cleaner, but I don't think it would be worse the
complexity.      

But it's way more flexible than putting everything in the nvmem
framework. BTW, did you notice that nvmem-cells parsing does not
work with flashes bigger than 4GB, because the framework assumes
#address-cells and #size-cells are always 1. That's probably
something we'll have to fix for the MTD case.    

Yes, however that's just an implementation limitation which is
trivial to solve.  

Agree. I was just pointing it in case you hadn't noticed.

quoted

quoted

quoted

Furthermore xlate functions are more about converting
from hardware parameters to internal kernel representation than
to hide extra DT parsing.      

Hm, how is that different? ->of_xlate() is just a way for drivers
to have their own DT representation, which is exactly what we
want here.    

There is a big difference. DT represent the hardware and the
relationship between the devices in an OS independent format. We
don't add extra stuff in there just to map back internal Linux API
details.  

And I'm not talking about adding SW information in the DT, I'm talking
about HW specific description. We have the same solution for pinctrl
configs (it's HW/driver specific).

For pinctrl I do understand, these beast can be very different from SoC
to SoC, having a single biding for all doesn't make much sense.

However here we are talking about a simple linear storage, nothing
special at all. I could see the need for an xlate to for example
support a device with several partitions, but not to just allow each
driver to have slightly incompatible bindings.

quoted

quoted

quoted

quoted

5/ Tell the nvmem framework the name of the subnode containing
nvmem cell definitions (if NULL that means cells are directly
defined under the nvmem provider node). We would set it to
"nvmem-cells" (or whatever you like) for the MTD case.        

If so please match on compatible and not on the node name.      

If you like.
    

quoted

6/ Extend the current NVMEM cell lookup to check if the parent
node of the cell has a compatible string set to "nvmem-cells".
If it doesn't it mean we have the current binding and this node
is the NVMEM device. If it does the device node is just the
next parent. This is trivial to implement (literally 2 lines of
code) and cover all the cases currently known.      

Except Srinivas was not happy with this solution, and this
stalled the discussion. I'm trying to find other options and you
keep rejecting all of them to come back to this one.    

Well, I think this is the best solution :/
  

quoted

quoted

7/ Just add a compatible string to the nvmem cell. No code
change is needed,      

That's not true!!!    

What is not true in this statement? The current nvmem lookup don't
care about compatible strings, so the cell lookup would just works
fine. The MTD partition parser won't consider them as a partition
because of the compatible string. Problem solved!  

No because partitions defined the old way (as direct subnodes of the
MTD node) will be considered as NVMEM cells by the NVMEM framework,
and I don't want that.

As I explained above that is not currently the case. If the NVMEM,
framework is ever changed to explicitly parse NVMEM cells in advance
we can first update the few existing users to add the compatible string.

Plus, I don't want people to start defining their NVMEM cells and
forget the compat string (which would work just fine because the
NVMEM framework doesn't care).

A review of a new DTS should check that it use each binding correctly,
AFAIK the DT people do that. We could also add a warning when there is
no compatible string, that would also help pushing people to update
their DTS.

quoted

quoted

What forces people to add this compatible in their
DT? Nothing. I'll tell you what will happen: people will start
defining their nvmem cells directly under the MTD node because
that *works*, and even if the binding is not documented and we
consider it invalid, we'll be stuck supporting it forever.    

Do note that undocumented bindings are not allowed. DTS that use
undocumented bindings (normally) just get rejected.  

Except that's just in theory. In practice, if people can do something
wrong, they'll complain if you later fix the bug and break their
setup. So no, if we go for the "nvmem cells have an 'nvmem-cell'
compat", then I'd like the NVMEM framework to enforce that somehow.

That should be trivial to implement.

Alban