[PATCH v3 0/3] mtd: Add support for reading MTD devices via the nvmem API

[PATCH v3 0/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban Bedel]

Hi all,

this series add support for reading MTD devices via the nvmem API, this
is mostly needed on embedded devices where things like MAC address and
calibration data is often stored in a partition on the main flash device.

Adding support for the nvmem API to the MTD core is trivial, however there
is a clash in the OF binding used by both subsystems. The current nvmem
binding expect nvmem cell to be subnode of the nvmem device, without any
compatible string. But MTD devices used a similar scheme for partition in
the past, so a subnode from an MTD device could be a partition using the
old binding or an nvmem cell.

To avoid this problem we update the nvmem cell binding to use a 'nvmem-cells'
subnode with compatible string to hold the list of nvmem cells. This new
binding make sure that any kind of device can be used as nvmem provider.

Alban Bedel (3):
  nvmem: Update the OF binding to use a subnode for the cells list
  doc: bindings: Add bindings documentation for mtd nvmem
  mtd: Add support for reading MTD devices via the nvmem API

 .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 27 ++++++++++
 Documentation/devicetree/bindings/nvmem/nvmem.txt  | 55 +++++++++++++-------
 drivers/mtd/Kconfig                                |  1 +
 drivers/mtd/mtdcore.c                              | 59 ++++++++++++++++++++++
 drivers/nvmem/core.c                               | 10 ++++
 include/linux/mtd/mtd.h                            |  2 +
 6 files changed, 137 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt

-- 
2.7.4

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

[Alban Bedel]

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 <albeu@free.fr>
---
 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.
 
 Optional properties:
  read-only: Mark the provider as read only.
 
+= Data cells list =
+The data cells list node should be named 'nvmem-cells' and have a
+child node for each data cell.
+
+Required properties:
+ compatible: Must be "nvmem-cells"
+ #address-cells: <1> if the provider use 32 bit addressing,
+                 <2> for 64 bits addressing
+ #size-cells: <1> if the provider use 32 bit sizes,
+              <2> for 64 bits sizes
+
 = Data cells =
-These are the child nodes of the provider which contain data cell
+These are the child nodes of the nvmem-cells node which contain data cell
 information like offset and size in nvmem provider.
 
 Required properties:
@@ -37,24 +52,30 @@ For example:
 		...
 
 		/* Data cells */
-		tsens_calibration: calib@404 {
-			reg = <0x404 0x10>;
-		};
+		nvmem-cells {
+			compatible = "nvmem-cells";
+			#address-cells = <1>;
+			#size-cells = <1>;
 
-		tsens_calibration_bckp: calib_bckp@504 {
-			reg = <0x504 0x11>;
-			bits = <6 128>
-		};
+			tsens_calibration: calib@404 {
+				reg = <0x404 0x10>;
+			};
 
-		pvs_version: pvs-version@6 {
-			reg = <0x6 0x2>
-			bits = <7 2>
-		};
+			tsens_calibration_bckp: calib_bckp@504 {
+				reg = <0x504 0x11>;
+				bits = <6 128>
+			};
 
-		speed_bin: speed-bin@c{
-			reg = <0xc 0x1>;
-			bits = <2 3>;
+			pvs_version: pvs-version@6 {
+				reg = <0x6 0x2>
+				bits = <7 2>
+			};
 
+			speed_bin: speed-bin@c{
+				reg = <0xc 0x1>;
+				bits = <2 3>;
+
+			};
 		};
 		...
 	};
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 78051f0..a59195c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -783,6 +783,16 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 	if (!nvmem_np)
 		return ERR_PTR(-EINVAL);
 
+	/* Devices using the new binding have all the cells in
+	 * a subnode with compatible = "nvmem-cells". In this
+	 * case the device will be the parent of this node.
+	 */
+	if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+		nvmem_np = of_get_next_parent(nvmem_np);
+		if (!nvmem_np)
+			return ERR_PTR(-EINVAL);
+	}
+
 	nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
 	of_node_put(nvmem_np);
 	if (IS_ERR(nvmem))
-- 
2.7.4

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

[Rob Herring]

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 <albeu@free.fr>
> ---
>  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.

>  
>  Optional properties:
>   read-only: Mark the provider as read only.
>  
> += Data cells list =
> +The data cells list node should be named 'nvmem-cells' and have a
> +child node for each data cell.
> +
> +Required properties:
> + compatible: Must be "nvmem-cells"
> + #address-cells: <1> if the provider use 32 bit addressing,
> +                 <2> for 64 bits addressing
> + #size-cells: <1> if the provider use 32 bit sizes,
> +              <2> for 64 bits sizes
> +
>  = Data cells =
> -These are the child nodes of the provider which contain data cell
> +These are the child nodes of the nvmem-cells node which contain data cell
>  information like offset and size in nvmem provider.

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Mon, 16 Apr 2018 16:04:29 -0500
Rob Herring <robh@kernel.org> 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 <albeu@free.fr>
> > ---
> >  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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

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!!

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?

Doing this way will make the binding very much specific to the provider 
rather than changing nvmem core bindings.

thanks,
srini

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

On Tue, 17 Apr 2018 13:54:07 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

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.

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Tue, 17 Apr 2018 16:44:01 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> 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!

TBH I just copied what have been done to fix the same problem with the
MTD partitions. But yes we could also just extend the current binding
to require a compatible string on each nvmem-cell, which would not
require any code change to support.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Tue, 17 Apr 2018 18:00:40 +0200
Alban <albeu@free.fr> wrote:

> On Tue, 17 Apr 2018 16:44:01 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
> > 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!  
> 
> TBH I just copied what have been done to fix the same problem with the
> MTD partitions. But yes we could also just extend the current binding
> to require a compatible string on each nvmem-cell, which would not
> require any code change to support.

However this scheme will not work if the device node binding already
have subnodes with addresses. The addressing, as specified by
#address-cells and #size-cells, might be incompatible or might overlap.
Using the nvmem-cells subnode solve this problem.

Alban


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 18/04/18 12:41, Alban wrote:
> On Tue, 17 Apr 2018 18:00:40 +0200
> Alban <albeu@free.fr> wrote:
> 
>> On Tue, 17 Apr 2018 16:44:01 +0100
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>>
>>> 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!
>>
>> TBH I just copied what have been done to fix the same problem with the
>> MTD partitions. But yes we could also just extend the current binding
>> to require a compatible string on each nvmem-cell, which would not
>> require any code change to support.
> 
> However this scheme will not work if the device node binding already
> have subnodes with addresses. The addressing, as specified by
> #address-cells and #size-cells, might be incompatible or might overlap.
> Using the nvmem-cells subnode solve this problem.
> 

I was also suggesting you to use nvmem-cell subnode, but make it a 
proper nvmem provider device, rather than reusing its parent device.

You would end up some thing like this in dt.

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

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

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

--srini

> Alban
> 

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

On Wed, 18 Apr 2018 13:12:48 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 12:41, Alban wrote:
> > On Tue, 17 Apr 2018 18:00:40 +0200
> > Alban <albeu@free.fr> wrote:
> >   
> >> On Tue, 17 Apr 2018 16:44:01 +0100
> >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> >>  
> >>> 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!  
> >>
> >> TBH I just copied what have been done to fix the same problem with the
> >> MTD partitions. But yes we could also just extend the current binding
> >> to require a compatible string on each nvmem-cell, which would not
> >> require any code change to support.  
> > 
> > However this scheme will not work if the device node binding already
> > have subnodes with addresses. The addressing, as specified by
> > #address-cells and #size-cells, might be incompatible or might overlap.
> > Using the nvmem-cells subnode solve this problem.
> >   
> 
> I was also suggesting you to use nvmem-cell subnode, but make it a 
> proper nvmem provider device, rather than reusing its parent device.
> 
> You would end up some thing like this in dt.
> 
> flash@0 {
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	compatible = "s25sl064a";
> 	reg = <0>;
> 
> 	nvmem-cells {
> 		compatible = "mtd-nvmem";
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		calibration: calib@404 {
> 			reg = <0x404 0x10>;
> 		};
> 	};
> };

But the root cause is in the nvmem binding, this conflict could exists
with any device type, not just MTD. I don't understand why we would want
such workarounds instead of just fixing the problem once and for all.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 18/04/18 13:32, Alban wrote:
>> I was also suggesting you to use nvmem-cell subnode, but make it a
>> proper nvmem provider device, rather than reusing its parent device.
>>
>> You would end up some thing like this in dt.
>>
>> flash@0 {
>> 	#address-cells = <1>;
>> 	#size-cells = <1>;
>> 	compatible = "s25sl064a";
>> 	reg = <0>;
>>
>> 	nvmem-cells {
>> 		compatible = "mtd-nvmem";
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>>
>> 		calibration: calib@404 {
>> 			reg = <0x404 0x10>;
>> 		};
>> 	};
>> };
> But the root cause is in the nvmem binding, this conflict could exists
No, the root cause is because of passing wrong device instance to nvmem 
core. And trying to workaround is the actual issue.

> with any device type, not just MTD. I don't understand why we would want
> such workarounds instead of just fixing the problem once and for all.
AFAIU, This is not a workaround, this is how nvmem provider bindings are 
and all providers should try to follow it.

I still do not understand what is the issue in making nvmem-cells node a 
proper nvmem provider device?

--srini

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

On Wed, 18 Apr 2018 13:53:56 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 13:32, Alban wrote:
> >> I was also suggesting you to use nvmem-cell subnode, but make it a
> >> proper nvmem provider device, rather than reusing its parent device.
> >>
> >> You would end up some thing like this in dt.
> >>
> >> flash@0 {
> >> 	#address-cells = <1>;
> >> 	#size-cells = <1>;
> >> 	compatible = "s25sl064a";
> >> 	reg = <0>;
> >>
> >> 	nvmem-cells {
> >> 		compatible = "mtd-nvmem";
> >> 		#address-cells = <1>;
> >> 		#size-cells = <1>;
> >>
> >> 		calibration: calib@404 {
> >> 			reg = <0x404 0x10>;
> >> 		};
> >> 	};
> >> };  
> > But the root cause is in the nvmem binding, this conflict could exists  
> No, the root cause is because of passing wrong device instance to nvmem 
> core. And trying to workaround is the actual issue.

The data is stored on the MTD, so the nvmem provider is the MTD device.
I don't think it is a good idea to have a virtual device in the DT to
accommodate the nvmem API.

> > with any device type, not just MTD. I don't understand why we would want
> > such workarounds instead of just fixing the problem once and for all.  
> AFAIU, This is not a workaround, this is how nvmem provider bindings are 
> and all providers should try to follow it.
> 
> I still do not understand what is the issue in making nvmem-cells node a 
> proper nvmem provider device?

It is doable, but beside making the code more complex, AFAIU that would
goes against DT best practice as no such device exists in the hardware.
The DT should only represent that this device contains data that can be
requested by a driver, and ideally in the same way for all kind of
storages.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 18/04/18 14:34, Alban wrote:
> On Wed, 18 Apr 2018 13:53:56 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 18/04/18 13:32, Alban wrote:
>>>> I was also suggesting you to use nvmem-cell subnode, but make it a
>>>> proper nvmem provider device, rather than reusing its parent device.
>>>>
>>>> You would end up some thing like this in dt.
>>>>
>>>> flash@0 {
>>>> 	#address-cells = <1>;
>>>> 	#size-cells = <1>;
>>>> 	compatible = "s25sl064a";
>>>> 	reg = <0>;
>>>>
>>>> 	nvmem-cells {
>>>> 		compatible = "mtd-nvmem";
>>>> 		#address-cells = <1>;
>>>> 		#size-cells = <1>;
>>>>
>>>> 		calibration: calib@404 {
>>>> 			reg = <0x404 0x10>;
>>>> 		};
>>>> 	};
>>>> };
>>> But the root cause is in the nvmem binding, this conflict could exists
>> No, the root cause is because of passing wrong device instance to nvmem
>> core. And trying to workaround is the actual issue.
> 
> The data is stored on the MTD, so the nvmem provider is the MTD device.
> I don't think it is a good idea to have a virtual device in the DT to
> accommodate the nvmem API.
> 
Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem 
provider too.

However, I would like to see this moving forward.

I can think of one possible solution here, which is, adding 
"nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell. The 
problem you mentioned regarding #address-cells and #size-cells with 
provider need to be addressed in nvmem core.

Currently nvmem core only support offsets of 32 bits, if you are 
expecting a 64 bit offsets then we should add that as a feature to nvmem 
core.

nvmem core as it is today should work fine with 32 bit offsets for mtd 
cases.

what do you think?

thanks,
srini

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

On Tue, 1 May 2018 17:49:03 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 14:34, Alban wrote:
> > On Wed, 18 Apr 2018 13:53:56 +0100
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> >   
> >> On 18/04/18 13:32, Alban wrote:  
> >>>> I was also suggesting you to use nvmem-cell subnode, but make it a
> >>>> proper nvmem provider device, rather than reusing its parent device.
> >>>>
> >>>> You would end up some thing like this in dt.
> >>>>
> >>>> flash@0 {
> >>>> 	#address-cells = <1>;
> >>>> 	#size-cells = <1>;
> >>>> 	compatible = "s25sl064a";
> >>>> 	reg = <0>;
> >>>>
> >>>> 	nvmem-cells {
> >>>> 		compatible = "mtd-nvmem";
> >>>> 		#address-cells = <1>;
> >>>> 		#size-cells = <1>;
> >>>>
> >>>> 		calibration: calib@404 {
> >>>> 			reg = <0x404 0x10>;
> >>>> 		};
> >>>> 	};
> >>>> };  
> >>> But the root cause is in the nvmem binding, this conflict could exists  
> >> No, the root cause is because of passing wrong device instance to nvmem
> >> core. And trying to workaround is the actual issue.  
> > 
> > The data is stored on the MTD, so the nvmem provider is the MTD device.
> > I don't think it is a good idea to have a virtual device in the DT to
> > accommodate the nvmem API.
> >   
> Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem 
> provider too.
> 
> However, I would like to see this moving forward.
> 
> I can think of one possible solution here, which is, adding 
> "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell.

I would definitely use "nvmem-cell", from the binding point of view it
doesn't matter what the underlaying storage is.

> The problem you mentioned regarding #address-cells and #size-cells with 
> provider need to be addressed in nvmem core.
> 
> Currently nvmem core only support offsets of 32 bits, if you are 
> expecting a 64 bit offsets then we should add that as a feature to nvmem 
> core.
> 
> nvmem core as it is today should work fine with 32 bit offsets for mtd 
> cases.

That's not what I meant, 32 bit should be more that enough for now.
What I meant is that if a binding already has children nodes using
unit-address, then we would end up with two different uses of the same
"address space".

> what do you think?

AFAIU the only thing that we disagree on now is if the nodes
representing the cells should be direct children of the provider
or in a dedicated subnode. For the MTD case both solution would solve
the binding clash. I would really appreciate if the DT people could
chip in so that we can settle this and get the MTD support merged.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 07/06/18 17:41, Alban wrote:
> AFAIU the only thing that we disagree on now is if the nodes
> representing the cells should be direct children of the provider
> or in a dedicated subnode. For the MTD case both solution would solve
> the binding clash. I would really appreciate if the DT people could
Am reluctant in changing the nvmem generic bindings for a special case.

Can you try this with your original subnode proposal:
just pass the subnode node pointer in np of nvmem_config:

------------------------->cut<------------------------------------
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b05aa8e81303..c9621632bbfb 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct 
nvmem_config *config)
         nvmem->priv = config->priv;
         nvmem->reg_read = config->reg_read;
         nvmem->reg_write = config->reg_write;
-       nvmem->dev.of_node = config->dev->of_node;
+
+       if (config->np)
+               nvmem->dev.of_node = config->np;
+       else
+               nvmem->dev.of_node = config->dev->of_node;

         if (config->id == -1 && config->name) {
                 dev_set_name(&nvmem->dev, "%s", config->name);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index f89598bc4e1c..743345ffe2c8 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, unsigned 
int offset,
   */
  struct nvmem_config {
         struct device           *dev;
+       struct device_node      *np;
         const char              *name;
         int                     id;
         struct module           *owner;

------------------------->cut<------------------------------------

thanks,
srini

> chip in so that we can settle this and get the MTD support merged.

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On Thu, 7 Jun 2018 18:03:16 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 07/06/18 17:41, Alban wrote:
> > AFAIU the only thing that we disagree on now is if the nodes
> > representing the cells should be direct children of the provider
> > or in a dedicated subnode. For the MTD case both solution would solve
> > the binding clash. I would really appreciate if the DT people could  
> Am reluctant in changing the nvmem generic bindings for a special case.

Where I think the generic binding is fundamentally flawed, as this
problem will most probably appear again. But do note that my proposal
doesn't require updating the dts using the original binding, both are
still supported. I proposed deprecating the current binding because I
think it is flawed, but we could also "officially" support both style.

> Can you try this with your original subnode proposal:
> just pass the subnode node pointer in np of nvmem_config:
> 
> ------------------------->cut<------------------------------------  
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..c9621632bbfb 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct 
> nvmem_config *config)
>          nvmem->priv = config->priv;
>          nvmem->reg_read = config->reg_read;
>          nvmem->reg_write = config->reg_write;
> -       nvmem->dev.of_node = config->dev->of_node;
> +
> +       if (config->np)
> +               nvmem->dev.of_node = config->np;
> +       else
> +               nvmem->dev.of_node = config->dev->of_node;
> 
>          if (config->id == -1 && config->name) {
>                  dev_set_name(&nvmem->dev, "%s", config->name);
> diff --git a/include/linux/nvmem-provider.h
> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> 100644 --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> unsigned int offset,nvmem_device_get(
>    */
>   struct nvmem_config {
>          struct device           *dev;
> +       struct device_node      *np;
>          const char              *name;
>          int                     id;
>          struct module           *owner;
> 
> ------------------------->cut<------------------------------------  

That should work just fine to allow next to any kind of binding.
I'll do a new patch using this approach for the code side and leaving
the generic binding as it is.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 08/06/18 11:59, Alban wrote:
> On Thu, 7 Jun 2018 18:03:16 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 07/06/18 17:41, Alban wrote:
>>> AFAIU the only thing that we disagree on now is if the nodes
>>> representing the cells should be direct children of the provider
>>> or in a dedicated subnode. For the MTD case both solution would solve
>>> the binding clash. I would really appreciate if the DT people could
>> Am reluctant in changing the nvmem generic bindings for a special case.
> 
> Where I think the generic binding is fundamentally flawed, as this
> problem will most probably appear again. But do note that my proposal

It depends from which side you are looking at this!!

There are more than 16 nvmem providers in kernel which use this bindings 
and they are fine!!

I obviously take some of your concerns w.r.t bindings and there is a 
scope of making the bindings much robust, most importantly the #address 
and #size-cells. But 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 
into this bindings. If single level of subnode works for your case and 
the below patch works for you then I would recommend lets move on and 
try to get this going.

> doesn't require updating the dts using the original binding, both are
> still supported. I proposed deprecating the current binding because I
> think it is flawed, but we could also "officially" support both style.
> 
>> Can you try this with your original subnode proposal:
>> just pass the subnode node pointer in np of nvmem_config:
>>
>> ------------------------->cut<------------------------------------
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index b05aa8e81303..c9621632bbfb 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
>> nvmem_config *config)
>>           nvmem->priv = config->priv;
>>           nvmem->reg_read = config->reg_read;
>>           nvmem->reg_write = config->reg_write;
>> -       nvmem->dev.of_node = config->dev->of_node;
>> +
>> +       if (config->np)
>> +               nvmem->dev.of_node = config->np;
>> +       else
>> +               nvmem->dev.of_node = config->dev->of_node;
>>
>>           if (config->id == -1 && config->name) {
>>                   dev_set_name(&nvmem->dev, "%s", config->name);
>> diff --git a/include/linux/nvmem-provider.h
>> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
>> 100644 --- a/include/linux/nvmem-provider.h
>> +++ b/include/linux/nvmem-provider.h
>> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
>> unsigned int offset,nvmem_device_get(
>>     */
>>    struct nvmem_config {
>>           struct device           *dev;
>> +       struct device_node      *np;
>>           const char              *name;
>>           int                     id;
>>           struct module           *owner;
>>
>> ------------------------->cut<------------------------------------
> 
> That should work just fine to allow next to any kind of binding.
> I'll do a new patch using this approach for the code side and leaving
> the generic binding as it is.
Sure!!
This will give more flexibility to other provider drivers!


thanks,
srini
> 
> Alban
> 

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 4499 bytes --]

On Fri, 8 Jun 2018 12:34:12 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> >> Can you try this with your original subnode proposal:
> >> just pass the subnode node pointer in np of nvmem_config:
> >>  
> >> ------------------------->cut<------------------------------------  
> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >> index b05aa8e81303..c9621632bbfb 100644
> >> --- a/drivers/nvmem/core.c
> >> +++ b/drivers/nvmem/core.c
> >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> >> nvmem_config *config)
> >>           nvmem->priv = config->priv;
> >>           nvmem->reg_read = config->reg_read;
> >>           nvmem->reg_write = config->reg_write;
> >> -       nvmem->dev.of_node = config->dev->of_node;
> >> +
> >> +       if (config->np)
> >> +               nvmem->dev.of_node = config->np;
> >> +       else
> >> +               nvmem->dev.of_node = config->dev->of_node;
> >>
> >>           if (config->id == -1 && config->name) {
> >>                   dev_set_name(&nvmem->dev, "%s", config->name);
> >> diff --git a/include/linux/nvmem-provider.h
> >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> >> 100644 --- a/include/linux/nvmem-provider.h
> >> +++ b/include/linux/nvmem-provider.h
> >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> >> unsigned int offset,nvmem_device_get(
> >>     */
> >>    struct nvmem_config {
> >>           struct device           *dev;
> >> +       struct device_node      *np;
> >>           const char              *name;
> >>           int                     id;
> >>           struct module           *owner;
> >>  
> >> ------------------------->cut<------------------------------------  
> > 
> > That should work just fine to allow next to any kind of binding.
> > I'll do a new patch using this approach for the code side and leaving
> > the generic binding as it is.  
> Sure!!
> This will give more flexibility to other provider drivers!

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.

What I would suggest now is to just change the wording. We don't
deprecate the current binding, but we extend it to allow grouping the
cells in a child node if required. The code to support this is trivial,
(4 lines including error handling) so even if we expect very few
bindings to make use of it it is not going to be maintenance drag.
That would look like this:

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09..085d042 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -19,7 +19,10 @@ Optional properties:
 
 = Data cells =
 These are the child nodes of the provider which contain data cell
-information like offset and size in nvmem provider.
+information like offset and size in nvmem provider. Alternatively the data
+cells can be grouped in a node that has a compatible property set to
+"nvmem-cells".
+
 
 Required properties:
 reg:   specifies the offset in byte within the storage device.
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e94a78..3e1369c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
        if (!nvmem_np)
                return ERR_PTR(-EINVAL);
 
+       /* bindings that already have anonymous child nodes can instead put
+        * their cells in a child node with an nvmem-cells compatible. */
+       if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+               nvmem_np = of_get_next_parent(nvmem_np);
+               if (!nvmem_np)
+                       return ERR_PTR(-EINVAL);
+       }
+
        nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
        of_node_put(nvmem_np);
        if (IS_ERR(nvmem))

What about it?

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 08/06/18 18:07, Alban wrote:
> On Fri, 8 Jun 2018 12:34:12 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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!


> What I would suggest now is to just change the wording. We don't
> deprecate the current binding, but we extend it to allow grouping the
> cells in a child node if required. The code to support this is trivial,
> (4 lines including error handling) so even if we expect very few
> bindings to make use of it it is not going to be maintenance drag.
> That would look like this:

> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> index fd06c09..085d042 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> @@ -19,7 +19,10 @@ Optional properties:
>   
>   = Data cells =
>   These are the child nodes of the provider which contain data cell
> -information like offset and size in nvmem provider.
> +information like offset and size in nvmem provider. Alternatively the data
> +cells can be grouped in a node that has a compatible property set to
> +"nvmem-cells".
> +
>   
>   Required properties:
>   reg:   specifies the offset in byte within the storage device.
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 4e94a78..3e1369c 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>          if (!nvmem_np)
>                  return ERR_PTR(-EINVAL);
>   
> +       /* bindings that already have anonymous child nodes can instead put
> +        * their cells in a child node with an nvmem-cells compatible. */
> +       if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> +               nvmem_np = of_get_next_parent(nvmem_np);
> +               if (!nvmem_np)
> +                       return ERR_PTR(-EINVAL);
> +       }
> +
>          nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
>          of_node_put(nvmem_np);
>          if (IS_ERR(nvmem))
> 
> What about it?
Let me repeat what I have said in my previous emails:

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!

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.


thanks,
srini
> 
> Alban
> 

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

[Alban]

[-- Attachment #1: Type: text/plain, Size: 5213 bytes --]

On Sun, 10 Jun 2018 11:32:36 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 08/06/18 18:07, Alban wrote:
> > On Fri, 8 Jun 2018 12:34:12 +0100
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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.
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.

> > What I would suggest now is to just change the wording. We don't
> > deprecate the current binding, but we extend it to allow grouping the
> > cells in a child node if required. The code to support this is trivial,
> > (4 lines including error handling) so even if we expect very few
> > bindings to make use of it it is not going to be maintenance drag.
> > That would look like this:  
> 
> > 
> > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > index fd06c09..085d042 100644
> > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > @@ -19,7 +19,10 @@ Optional properties:
> >   
> >   = Data cells =
> >   These are the child nodes of the provider which contain data cell
> > -information like offset and size in nvmem provider.
> > +information like offset and size in nvmem provider. Alternatively the data
> > +cells can be grouped in a node that has a compatible property set to
> > +"nvmem-cells".
> > +
> >   
> >   Required properties:
> >   reg:   specifies the offset in byte within the storage device.
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 4e94a78..3e1369c 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> >          if (!nvmem_np)
> >                  return ERR_PTR(-EINVAL);
> >   
> > +       /* bindings that already have anonymous child nodes can instead put
> > +        * their cells in a child node with an nvmem-cells compatible. */
> > +       if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> > +               nvmem_np = of_get_next_parent(nvmem_np);
> > +               if (!nvmem_np)
> > +                       return ERR_PTR(-EINVAL);
> > +       }
> > +
> >          nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
> >          of_node_put(nvmem_np);
> >          if (IS_ERR(nvmem))
> > 
> > What about it?  
> Let me repeat what I have said in my previous emails:
> 
> 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.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Srinivas Kandagatla]

On 10/06/18 12:36, Alban wrote:
> On Sun, 10 Jun 2018 11:32:36 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 08/06/18 18:07, Alban wrote:
>>> On Fri, 8 Jun 2018 12:34:12 +0100
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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
> 

[PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban Bedel]

Config data for drivers, like MAC addresses, is often stored in MTD.
Add a binding that define how such data storage can be represented in
device tree.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
Changelog:
v2: * Added a "Required properties" section with the nvmem-provider
      property
v3: * Fixed my name in From and Signed-off-by
    * Moved to the new nvmem binding with the nvmem-cells subnode
---
 .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt

diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
new file mode 100644
index 0000000..c819a69
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
@@ -0,0 +1,27 @@
+= NVMEM in MTD =
+
+Config data for drivers, like MAC addresses, is often stored in MTD.
+An MTD device, or one of its partition, can be defined as a NVMEM provider
+by having an 'nvmem-cells' subnode as defined in nvmem.txt.
+
+Example:
+
+	flash@0 {
+		...
+
+		partition@2 {
+			label = "art";
+			reg = <0x7F0000 0x010000>;
+			read-only;
+
+			nvmem-cells {
+				compatible = "nvmem-cells";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				eeprom@1000 {
+					reg = <0x1000 0x1000>;
+				};
+			};
+		};
+	};
-- 
2.7.4

Re: [PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem

[Rob Herring]

On Sun, Mar 25, 2018 at 12:24:58AM +0100, Alban Bedel wrote:
> Config data for drivers, like MAC addresses, is often stored in MTD.
> Add a binding that define how such data storage can be represented in
> device tree.
> 
> Signed-off-by: Alban Bedel <albeu@free.fr>
> ---
> Changelog:
> v2: * Added a "Required properties" section with the nvmem-provider
>       property
> v3: * Fixed my name in From and Signed-off-by
>     * Moved to the new nvmem binding with the nvmem-cells subnode
> ---
>  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..c819a69
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,27 @@
> += NVMEM in MTD =
> +
> +Config data for drivers, like MAC addresses, is often stored in MTD.
> +An MTD device, or one of its partition, can be defined as a NVMEM provider
> +by having an 'nvmem-cells' subnode as defined in nvmem.txt.
> +
> +Example:
> +
> +	flash@0 {
> +		...
> +
> +		partition@2 {

This unit address is not correct...

> +			label = "art";
> +			reg = <0x7F0000 0x010000>;

Lowercase hex.

> +			read-only;
> +
> +			nvmem-cells {
> +				compatible = "nvmem-cells";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +
> +				eeprom@1000 {

"eeprom" isn't specific data. The purpose of the nvmem binding is to 
provide specific data fields like MAC addresseses.

Plus "eeprom" is the node name for EEPROM devices.

> +					reg = <0x1000 0x1000>;
> +				};
> +			};
> +		};
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Mon, 16 Apr 2018 16:08:51 -0500
Rob Herring <robh@kernel.org> wrote:

> On Sun, Mar 25, 2018 at 12:24:58AM +0100, Alban Bedel wrote:
> > Config data for drivers, like MAC addresses, is often stored in MTD.
> > Add a binding that define how such data storage can be represented in
> > device tree.
> > 
> > Signed-off-by: Alban Bedel <albeu@free.fr>
> > ---
> > Changelog:
> > v2: * Added a "Required properties" section with the nvmem-provider
> >       property
> > v3: * Fixed my name in From and Signed-off-by
> >     * Moved to the new nvmem binding with the nvmem-cells subnode
> > ---
> >  .../devicetree/bindings/nvmem/mtd-nvmem.txt        | 27 ++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 0000000..c819a69
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,27 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > +An MTD device, or one of its partition, can be defined as a NVMEM provider
> > +by having an 'nvmem-cells' subnode as defined in nvmem.txt.
> > +
> > +Example:
> > +
> > +	flash@0 {
> > +		...
> > +
> > +		partition@2 {  
> 
> This unit address is not correct...

Is it because I ellipsed the #address-cells and #size-cells? I will add
them.
 
> > +			label = "art";
> > +			reg = <0x7F0000 0x010000>;  
> 
> Lowercase hex.

Will do.

> > +			read-only;
> > +
> > +			nvmem-cells {
> > +				compatible = "nvmem-cells";
> > +				#address-cells = <1>;
> > +				#size-cells = <1>;
> > +
> > +				eeprom@1000 {  
> 
> "eeprom" isn't specific data. The purpose of the nvmem binding is to 
> provide specific data fields like MAC addresseses.
> 
> Plus "eeprom" is the node name for EEPROM devices.

This example is from a board using an ath9k chip and this field contains
what the driver names EEPROM, although it is in fact only a config
binary blob. However I agree that it is misleading for such an example,
I will replace it with something like "wifi-config-data".

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 3/3] mtd: Add support for reading MTD devices via the nvmem API

[Alban Bedel]

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.
On OF systems only devices that have the 'nvmem-provider' property
are registered, on non-OF system all MTD devices are registered.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
Changelog:
v2: * Moved to the MTD core instead of using notifiers
    * Fixed the Kconfig description
v3: * Rebased on current kernel
    * Moved the code to mtdcore.c and removed the conditional
      compilation as suggested by Boris Brezillon
    * Fixed my name in From and Signed-off-by
    * Only allow root to read from the nvmem sysfs interface
---
 drivers/mtd/Kconfig     |  1 +
 drivers/mtd/mtdcore.c   | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |  2 ++
 3 files changed, 62 insertions(+)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 2a8ac68..911d869 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -1,5 +1,6 @@
 menuconfig MTD
 	tristate "Memory Technology Device (MTD) support"
+	imply NVMEM
 	help
 	  Memory Technology Devices are flash, RAM and similar chips, often
 	  used for solid state file systems on embedded devices. This option
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 28553c8..d2a127c 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -478,6 +478,48 @@ int mtd_pairing_groups(struct mtd_info *mtd)
 }
 EXPORT_SYMBOL_GPL(mtd_pairing_groups);
 
+static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct mtd_info *mtd = priv;
+	size_t retlen;
+	int err;
+
+	err = mtd_read(mtd, offset, bytes, &retlen, val);
+	if (err && err != -EUCLEAN)
+		return err;
+
+	return retlen == bytes ? 0 : -EIO;
+}
+
+static int mtd_nvmem_add(struct mtd_info *mtd)
+{
+	struct nvmem_config config = {};
+
+	config.dev = &mtd->dev;
+	config.owner = THIS_MODULE;
+	config.reg_read = mtd_nvmem_reg_read;
+	config.size = mtd->size;
+	config.word_size = 1;
+	config.stride = 1;
+	config.read_only = true;
+	config.root_only = true;
+	config.priv = mtd;
+
+	mtd->nvmem = nvmem_register(&config);
+	if (IS_ERR(mtd->nvmem)) {
+		/* Just ignore if there is no NVMEM support in the kernel */
+		if (PTR_ERR(mtd->nvmem) == -ENOSYS) {
+			mtd->nvmem = NULL;
+		} else {
+			dev_err(&mtd->dev, "Failed to register NVMEM device\n");
+			return PTR_ERR(mtd->nvmem);
+		}
+	}
+
+	return 0;
+}
+
 static struct dentry *dfs_dir_mtd;
 
 /**
@@ -560,6 +602,11 @@ int add_mtd_device(struct mtd_info *mtd)
 	if (error)
 		goto fail_added;
 
+	/* Add the nvmem provider */
+	error = mtd_nvmem_add(mtd);
+	if (error)
+		goto fail_nvmem_add;
+
 	if (!IS_ERR_OR_NULL(dfs_dir_mtd)) {
 		mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(&mtd->dev), dfs_dir_mtd);
 		if (IS_ERR_OR_NULL(mtd->dbg.dfs_dir)) {
@@ -585,6 +632,8 @@ int add_mtd_device(struct mtd_info *mtd)
 	__module_get(THIS_MODULE);
 	return 0;
 
+fail_nvmem_add:
+	device_unregister(&mtd->dev);
 fail_added:
 	of_node_put(mtd_get_of_node(mtd));
 	idr_remove(&mtd_idr, i);
@@ -627,6 +676,16 @@ int del_mtd_device(struct mtd_info *mtd)
 		       mtd->index, mtd->name, mtd->usecount);
 		ret = -EBUSY;
 	} else {
+		/* Try to remove the NVMEM provider */
+		if (mtd->nvmem) {
+			ret = nvmem_unregister(mtd->nvmem);
+			if (ret) {
+				dev_err(&mtd->dev,
+					"Failed to unregister NVMEM device\n");
+				goto out_error;
+			}
+		}
+
 		device_unregister(&mtd->dev);
 
 		idr_remove(&mtd_idr, mtd->index);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 205eded..660b8e7 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -25,6 +25,7 @@
 #include <linux/notifier.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/nvmem-provider.h>
 
 #include <mtd/mtd-abi.h>
 
@@ -352,6 +353,7 @@ struct mtd_info {
 	struct device dev;
 	int usecount;
 	struct mtd_debug_info dbg;
+	struct nvmem_device *nvmem;
 };
 
 int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
-- 
2.7.4