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

Message ID 1521933899-362-2-git-send-email-albeu@free.fr
State New
Delegated to: Boris Brezillon
Headers show
Series
  • mtd: Add support for reading MTD devices via the nvmem API
Related show

Commit Message

Alban March 24, 2018, 11:24 p.m.
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(-)

Comments

Rob Herring April 16, 2018, 9:04 p.m. | #1
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.
Alban April 17, 2018, 12:31 p.m. | #2
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
Srinivas Kandagatla April 17, 2018, 12:54 p.m. | #3
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
Alban April 17, 2018, 2:54 p.m. | #4
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
Srinivas Kandagatla April 17, 2018, 3:44 p.m. | #5
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.
Alban April 17, 2018, 4 p.m. | #6
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
Alban April 18, 2018, 11:41 a.m. | #7
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
Srinivas Kandagatla April 18, 2018, 12:12 p.m. | #8
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
>
Alban April 18, 2018, 12:32 p.m. | #9
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
Srinivas Kandagatla April 18, 2018, 12:53 p.m. | #10
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
Alban April 18, 2018, 1:34 p.m. | #11
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

Patch

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