by Alban

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list

On Mon, 16 Apr 2018 16:04:29 -0500
Rob Herring <[email protected]> wrote:

> On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> >
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.
> >
> > Signed-off-by: Alban Bedel <[email protected]>
> > ---
> > Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++-------
> > drivers/nvmem/core.c | 10 +++++
> > 2 files changed, 48 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > index fd06c09..6b723e7 100644
> > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device.
> > This document is here to document this.
> >
> > = Data providers =
> > -Contains bindings specific to provider drivers and data cells as children
> > -of this node.
> > +A data provider should have a subnode named 'nvmem-cells' that contains
> > +a subnodes for each data cells.
> > +
> > +For backward compatibility the nvmem data cells can be direct children
> > +of the data provider. This use is discouraged as it can conflict with
> > +other bindings.
>
> I don't think we need to go this far. Whether this is necessary depends
> on the provider.

It depend more on the drivers implementation. Sure as long as the
driver only support the nvmem API it doesn't matter, both binding are
fine. But if it ever need to support another API the bindings might
clash and the whole device binding will need to be updated. So all in
all I see very few value in still allowing the old binding for new
devices, or do you seen any problem with the new binding?

However if the consensus is to keep both styles I will rewrite this
paragraph as needed.

Alban

Attachments:

(No filename) (836.00 B)
OpenPGP digital signature

2018-04-17 12:47:10

On Tue, 17 Apr 2018 13:54:07 +0100
Srinivas Kandagatla <[email protected]> wrote:

> On 24/03/18 23:24, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> >
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.
>
> Am not sure if this a really good idea to change nvmem bindings based on
> provider requirements. This can be a beginning of other problems!!

I think you misunderstood something here, this proposed new binding
would be for all new nvmem bindings, not just mtd backed nvmem.

> Did you know that we can pass nvmem cells info via nvmem config ?
>
> Why can't mtd-nvmem provider populate the nvmem_config->cells from
> its dt "nvmem-cells" subnode before it registers the provider?

The DT based lookup of nvmem-cells doesn't use nvmem_config->cells, so
that's not an option. In fact here the problem come from the MTD side
because it also had a similar binding using subnodes without compatible
string. Just to make things clear, here is an example of the clash
using the current nvmem binding on an unpartitioned MTD device:

flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;

calibration: calib@404 {
reg = <0x404 0x10>;
};
};

This will not only allow reading the calibration data from nvmem, but
will also create a partition on the MTD device, which is not acceptable.
With my proposed binding this would become:

flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;

nvmem-cells {
compatible = "nvmem-cells";
#address-cells = <1>;
#address-cells = <1>;

calibration: calib@404 {
reg = <0x404 0x10>;
};
};
};

Which would work fine as the MTD code will ignore the nvmem-cells
subnode thanks to its compatible string.

IMHO subnodes without any compatible properties should never be used in
such generic bindings, as it is very likely that it will at some point
clash with another generic binding or with a device specific binding.

Alban

Attachments:

(No filename) (836.00 B)
OpenPGP digital signature

2018-04-17 15:45:41

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list

Thanks for explaining,

On 17/04/18 15:54, Alban wrote:
> This will not only allow reading the calibration data from nvmem, but
> will also create a partition on the MTD device, which is not acceptable.
> With my proposed binding this would become:
>
> flash@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "s25sl064a";
> reg = <0>;
>
> nvmem-cells {
> compatible = "nvmem-cells";
> #address-cells = <1>;
> #address-cells = <1>;
>
> calibration: calib@404 {
> reg = <0x404 0x10>;
> };
> };

Why can't we make nvmem-cells node a nvmem provider in this case?
Which should work!

--srini

> };
>
> Which would work fine as the MTD code will ignore the nvmem-cells
> subnode thanks to its compatible string.

2018-04-17 17:08:12

(No filename) (836.00 B)
OpenPGP digital signature

2018-06-10 13:29:22

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list

On 10/06/18 12:36, Alban wrote:
> On Sun, 10 Jun 2018 11:32:36 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
>> On 08/06/18 18:07, Alban wrote:
>>> On Fri, 8 Jun 2018 12:34:12 +0100
>>> Srinivas Kandagatla <[email protected]> wrote:
>>>
>> ...
>>>
>>> I looked into this. It would work fine for the cells but not so nicely
>>> for the nvmem device API. The phandle for the nvmem device would have
>>> to reference the node passed here and not the real device. We would end
>>> up with a DT like this:
>>>
>>> flash@0 {
>>> compatible = "mtd";
>>> ...
>>> nvmem_dev: nvmem-cells {
>>> compatible = "nvmem-cells";
>>> ...
>>> };
>>> };
>>>
>>> other-device@10 {
>>> ...
>>> nvmem = <&nvmem_dev>;
>>> };
>>>
>>> Now if there is no cell defined we have this empty child node that make
>>> very little sense, it is just there to accommodate the nvmem API.
>>>
>> NO. This just looks fine!
>> nvmem-cells is the nvmem provider node without which you would not have
>> any provider instance.
>> All this looks as expected!
>> Am not sure what is the problem here!
>
> The problem is that DT should represent the hardware, not the OS API.
Exactly!! flash/mtd has nvmem provider which should be represented in
the DT.

There is no change in DT side from your original patch vs the new
approach. You still are going to have the same subnode.

Isn't it?

AFAIU, the new approach will make it explicit that there is a nvmem
provider in the DT.
...
> What should be represented is that other drivers can access data stored
> on this device. It is my understanding that this wouldn't be an
> acceptable binding as the nvmem provider node would only exists because
> of how the NVMEM API currently works, a correct binding would just
> directly reference the storage device without this extra node.
>
...

>> Having a subnode still sounds very fragile to me,
>> and this could be much specific case of MTD provider. We might have
>> instances where this could be sub-sub node of the the original provider
>> for other providers. Also I do not want to bring in Provider specifics
>> layout into nvmem bindings.
>>
>> I can not make myself any clearer than this, Its going to be a NAK from
>> my side for the above reasons!
>
> I fully understand you concern but I think they are overblown. First I
> highly doubt that more layouts will ever be needed, using a compatible
> string pretty much guarantee that we won't clash with another binding.
> Furthermore even if you consider this extension "MTD specific" the
> amount of code is very small, non intrusive and only run once at
> registration time. I would understand if we were talking about pages of
> code nesting in various place, but not really when it is a single small
> if block with an obvious condition. And finally I don't see that as MTD
> specific as any other device could use this feature without any code
> change.
>
>> Also, patch I shared should give enough flexibility to various range of
>> providers which have different child node layouts without touching the
>> nvmem bindings. If it works please use it.
>
> It works from a code POV but it break the basic guidelines of DT
> bindings. As I want to have this done, I'm going to do a patch as you
> want, but I see a high chance that the binding is going to be rejected
> by the DT maintainers and we'll be back here again.

If you think the sub node is going to be a problem from MTD point of
view then that is worth discussing.
Adding subnode in nvmem bindings is not going to help or make the
situation any better.

Lets see how this goes!

thanks,
srini

>
> Alban
>