[v2,06/29] mtd: Add support for reading MTD devices via the nvmem API

Commit Message

Bartosz Golaszewski Aug. 10, 2018, 8:05 a.m. UTC

From: Alban Bedel <[e-mail]>

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

Signed-off-by: Alban Bedel <[e-mail]>
[Bartosz:
  - use the managed variant of nvmem_register(),
  - set the nvmem name]
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 drivers/mtd/Kconfig     |  1 +
 drivers/mtd/mtdcore.c   | 50 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |  2 ++
 3 files changed, 53 insertions(+)

Comments

Boris Brezillon Aug. 17, 2018, 4:27 p.m. UTC | #1

Hi Bartosz,

On Fri, 10 Aug 2018 10:05:03 +0200
Bartosz Golaszewski <[e-mail]> wrote:

> From: Alban Bedel <[e-mail]>
> 
> Allow drivers that use the nvmem API to read data stored on MTD devices.
> For this the mtd devices are registered as read-only NVMEM providers.
> 
> Signed-off-by: Alban Bedel <[e-mail]>
> [Bartosz:
>   - use the managed variant of nvmem_register(),
>   - set the nvmem name]
> Signed-off-by: Bartosz Golaszewski <[e-mail]>

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

Regards,

Boris

> ---
>  drivers/mtd/Kconfig     |  1 +
>  drivers/mtd/mtdcore.c   | 50 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |  2 ++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 46ab7feec6b6..f5549482d0df 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 42395df06be9..a57302eaceb5 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -488,6 +488,49 @@ 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.name = mtd->name;
> +	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 = devm_nvmem_register(&mtd->dev, &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;
>  
>  /**
> @@ -570,6 +613,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)) {
> @@ -595,6 +643,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);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index a86c4fa93115..8121c6582285 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>
>  
> @@ -339,6 +340,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,

Alban Bedel Aug. 19, 2018, 11:31 a.m. UTC | #2

On Fri, 17 Aug 2018 18:27:20 +0200
Boris Brezillon <[e-mail]> wrote:

> Hi Bartosz,
> 
> On Fri, 10 Aug 2018 10:05:03 +0200
> Bartosz Golaszewski <[e-mail]> wrote:
> 
> > From: Alban Bedel <[e-mail]>
> > 
> > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > For this the mtd devices are registered as read-only NVMEM providers.
> > 
> > Signed-off-by: Alban Bedel <[e-mail]>
> > [Bartosz:
> >   - use the managed variant of nvmem_register(),
> >   - set the nvmem name]
> > Signed-off-by: Bartosz Golaszewski <[e-mail]>  
> 
> What happened to the 2 other patches of Alban's series? I'd really
> like the DT case to be handled/agreed on in the same patchset, but
> IIRC, Alban and Srinivas disagreed on how this should be represented.
> I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> glue has been floating around for quite some time...

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

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

Alban

Boris Brezillon Aug. 19, 2018, 4:46 p.m. UTC | #3

On Sun, 19 Aug 2018 13:31:06 +0200
Alban <[e-mail]> wrote:

> On Fri, 17 Aug 2018 18:27:20 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > Hi Bartosz,
> > 
> > On Fri, 10 Aug 2018 10:05:03 +0200
> > Bartosz Golaszewski <[e-mail]> wrote:
> >   
> > > From: Alban Bedel <[e-mail]>
> > > 
> > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > For this the mtd devices are registered as read-only NVMEM providers.
> > > 
> > > Signed-off-by: Alban Bedel <[e-mail]>
> > > [Bartosz:
> > >   - use the managed variant of nvmem_register(),
> > >   - set the nvmem name]
> > > Signed-off-by: Bartosz Golaszewski <[e-mail]>    
> > 
> > What happened to the 2 other patches of Alban's series? I'd really
> > like the DT case to be handled/agreed on in the same patchset, but
> > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > glue has been floating around for quite some time...  
> 
> These other patches were to fix what I consider a fundamental flaw in
> the generic NVMEM bindings, however we couldn't agree on this point.
> Bartosz later contacted me to take over this series and I suggested to
> just change the MTD NVMEM binding to use a compatible string on the
> NVMEM cells as an alternative solution to fix the clash with the old
> style MTD partition.
> 
> However all this has no impact on the code needed to add NVMEM support
> to MTD, so the above patch didn't change at all.

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

I see several options:

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

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

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

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

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

There are probably other options (some were proposed by Alban and
Srinivas already), but I'd like to get this sorted out before we merge
this patch.

Alban, Srinivas, any opinion?

Srinivas Kandagatla Aug. 20, 2018, 10:43 a.m. UTC | #4

Thanks Boris, for looking into this in more detail.

On 19/08/18 17:46, Boris Brezillon wrote:
> On Sun, 19 Aug 2018 13:31:06 +0200
> Alban <[e-mail]> wrote:
> 
>> On Fri, 17 Aug 2018 18:27:20 +0200
>> Boris Brezillon <[e-mail]> wrote:
>>
>>> Hi Bartosz,
>>>
>>> On Fri, 10 Aug 2018 10:05:03 +0200
>>> Bartosz Golaszewski <[e-mail]> wrote:
>>>    
>>>> From: Alban Bedel <[e-mail]>
>>>>
>>>> Allow drivers that use the nvmem API to read data stored on MTD devices.
>>>> For this the mtd devices are registered as read-only NVMEM providers.
>>>>
>>>> Signed-off-by: Alban Bedel <[e-mail]>
>>>> [Bartosz:
>>>>    - use the managed variant of nvmem_register(),
>>>>    - set the nvmem name]
>>>> Signed-off-by: Bartosz Golaszewski <[e-mail]>
>>>
>>> What happened to the 2 other patches of Alban's series? I'd really
>>> like the DT case to be handled/agreed on in the same patchset, but
>>> IIRC, Alban and Srinivas disagreed on how this should be represented.
>>> I hope this time we'll come to an agreement, because the MTD <-> NVMEM
>>> glue has been floating around for quite some time...
>>
>> These other patches were to fix what I consider a fundamental flaw in
>> the generic NVMEM bindings, however we couldn't agree on this point.
>> Bartosz later contacted me to take over this series and I suggested to
>> just change the MTD NVMEM binding to use a compatible string on the
>> NVMEM cells as an alternative solution to fix the clash with the old
>> style MTD partition.
>>
>> However all this has no impact on the code needed to add NVMEM support
>> to MTD, so the above patch didn't change at all.
> 
> It does have an impact on the supported binding though.
> nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> means people will be able to define their NVMEM cells directly under
> the MTD device and reference them from other nodes (even if it's not
> documented), and as you said, it conflict with the old MTD partition
> bindings. So we'd better agree on this binding before merging this
> patch.
> 

Yes, I agree with you!

> I see several options:
> 
> 1/ provide a way to tell the NVMEM framework not to use parent->of_node
>     even if it's != NULL. This way we really don't support defining
>     NVMEM cells in the DT, and also don't support referencing the nvmem
>     device using a phandle.
> 
Other options look much better than this one!

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

Do we expect that there would be nvmem cells in any of the partitions?
or
nvmem cell are only valid for unpartioned area?

Am sure that the nvmem cells would be in multiple partitions, Is it okay 
to have some parts of partition to be in a separate subnode?

I would like this case to be considered too.

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

> 4/ Add a ->of_xlate() hook that would be called if present by the
>     framework instead of using the default parsing we have right now.
This looks much cleaner! We could hook that up under 
__nvmem_device_get() to do that translation.

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

> 
> There are probably other options (some were proposed by Alban and
> Srinivas already), but I'd like to get this sorted out before we merge
> this patch.
> 
> Alban, Srinivas, any opinion?

Overall am still not able to clear visualize on how MTD bindings with 
nvmem cells would look in both partition and un-partition usecases?
An example DT would be nice here!!

Option 4 looks like much generic solution to me, may be we should try 
this once bindings on MTD side w.r.t nvmem cells are decided.

Thanks,
Srini

>

Boris Brezillon Aug. 20, 2018, 6:20 p.m. UTC | #5

On Mon, 20 Aug 2018 11:43:34 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> 
> Overall am still not able to clear visualize on how MTD bindings with 
> nvmem cells would look in both partition and un-partition usecases?
> An example DT would be nice here!!

Something along those lines:

	mtdnode {
		nvmem-cells {
			#address-cells = <1>;
			#size-cells = <1>;

			cell@0 {
				reg = <0x0 0x14>;
			};
		};

		partitions {
			compatible = "fixed-partitions";
			#address-cells = <1>;
			#size-cells = <1>;

			partition@0 {
				reg = <0x0 0x20000>;

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

					cell@0 {
						reg = <0x0 0x10>;
					};
				};
			};
		};
	};

Bartosz Golaszewski Aug. 20, 2018, 6:50 p.m. UTC | #6

2018-08-20 20:20 GMT+02:00 Boris Brezillon <[e-mail]>:
> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[e-mail]> wrote:
>
>>
>> Overall am still not able to clear visualize on how MTD bindings with
>> nvmem cells would look in both partition and un-partition usecases?
>> An example DT would be nice here!!
>
> Something along those lines:
>
>         mtdnode {
>                 nvmem-cells {
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         cell@0 {
>                                 reg = <0x0 0x14>;
>                         };
>                 };
>
>                 partitions {
>                         compatible = "fixed-partitions";
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         partition@0 {
>                                 reg = <0x0 0x20000>;
>
>                                 nvmem-cells {
>                                         #address-cells = <1>;
>                                         #size-cells = <1>;
>
>                                         cell@0 {
>                                                 reg = <0x0 0x10>;
>                                         };
>                                 };
>                         };
>                 };
>         };

If there'll be an agreement on the bindings: will you be willing to
merge Alban's patch even without support in the code for the above
(with the assumption that it will be added later)? My use-case is on
non-DT systems and creating nvmem devices corresponding to MTD
partitions if fine by me. I also don't have the means to test the
support for these bindings if I were to actually write them myself.

Best regards,
Bartosz Golaszewski

Boris Brezillon Aug. 20, 2018, 7:06 p.m. UTC | #7

On Mon, 20 Aug 2018 20:50:55 +0200
Bartosz Golaszewski <[e-mail]> wrote:

> 2018-08-20 20:20 GMT+02:00 Boris Brezillon <[e-mail]>:
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[e-mail]> wrote:
> >  
> >>
> >> Overall am still not able to clear visualize on how MTD bindings with
> >> nvmem cells would look in both partition and un-partition usecases?
> >> An example DT would be nice here!!  
> >
> > Something along those lines:
> >
> >         mtdnode {
> >                 nvmem-cells {
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> >
> >                         cell@0 {
> >                                 reg = <0x0 0x14>;
> >                         };
> >                 };
> >
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> >
> >                         partition@0 {
> >                                 reg = <0x0 0x20000>;
> >
> >                                 nvmem-cells {
> >                                         #address-cells = <1>;
> >                                         #size-cells = <1>;
> >
> >                                         cell@0 {
> >                                                 reg = <0x0 0x10>;
> >                                         };
> >                                 };
> >                         };
> >                 };
> >         };  
> 
> If there'll be an agreement on the bindings: will you be willing to
> merge Alban's patch even without support in the code for the above
> (with the assumption that it will be added later)?

No, because Alban's patch actually allows people to define and
reference nvmem cells in a DT, but without documenting it (see my first
reply).

> My use-case is on
> non-DT systems and creating nvmem devices corresponding to MTD
> partitions if fine by me.

What you propose is option #1 in my list of proposals, and it requires
some changes to avoid automatically assigning nvmem->dev.of_node to
parent->of_node (which will be != NULL when the MTD device has been
instantiated from a DT node).

> I also don't have the means to test the
> support for these bindings if I were to actually write them myself.

And that's the very reason I proposed #1. I don't want to block this
stuff, but in its current state, I'm not willing to accept it either.
Either we agree on the binding and patch the nvmem framework to support
this new binding, or we find a way to hide the fact that the mtd
device (the nvmem parent) has a DT node attached to it.

Alban Bedel Aug. 20, 2018, 9:27 p.m. UTC | #8

On Mon, 20 Aug 2018 20:20:38 +0200
Boris Brezillon <[e-mail]> wrote:

> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[e-mail]> wrote:
> 
> > 
> > Overall am still not able to clear visualize on how MTD bindings with 
> > nvmem cells would look in both partition and un-partition usecases?
> > An example DT would be nice here!!  
> 
> Something along those lines:

We must also have a compatible string on the nvmem-cells node to make
sure we don't clash with the old style MTD partitions, or some other
device specific binding.

> 
> 	mtdnode {
> 		nvmem-cells {
                        compatible = "nvmem-cells";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			cell@0 {
> 				reg = <0x0 0x14>;
> 			};
> 		};
> 
> 		partitions {
> 			compatible = "fixed-partitions";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			partition@0 {
> 				reg = <0x0 0x20000>;
> 
> 				nvmem-cells {
                                        compatible = "nvmem-cells";
> 					#address-cells = <1>;
> 					#size-cells = <1>;
> 
> 					cell@0 {
> 						reg = <0x0 0x10>;
> 					};
> 				};
> 			};
> 		};
> 	};


Alban

Alban Bedel Aug. 20, 2018, 10:53 p.m. UTC | #9

On Sun, 19 Aug 2018 18:46:09 +0200
Boris Brezillon <[e-mail]> wrote:

> On Sun, 19 Aug 2018 13:31:06 +0200
> Alban <[e-mail]> wrote:
> 
> > On Fri, 17 Aug 2018 18:27:20 +0200
> > Boris Brezillon <[e-mail]> wrote:
> >   
> > > Hi Bartosz,
> > > 
> > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > Bartosz Golaszewski <[e-mail]> wrote:
> > >     
> > > > From: Alban Bedel <[e-mail]>
> > > > 
> > > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > > For this the mtd devices are registered as read-only NVMEM providers.
> > > > 
> > > > Signed-off-by: Alban Bedel <[e-mail]>
> > > > [Bartosz:
> > > >   - use the managed variant of nvmem_register(),
> > > >   - set the nvmem name]
> > > > Signed-off-by: Bartosz Golaszewski <[e-mail]>      
> > > 
> > > What happened to the 2 other patches of Alban's series? I'd really
> > > like the DT case to be handled/agreed on in the same patchset, but
> > > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > > glue has been floating around for quite some time...    
> > 
> > These other patches were to fix what I consider a fundamental flaw in
> > the generic NVMEM bindings, however we couldn't agree on this point.
> > Bartosz later contacted me to take over this series and I suggested to
> > just change the MTD NVMEM binding to use a compatible string on the
> > NVMEM cells as an alternative solution to fix the clash with the old
> > style MTD partition.
> > 
> > However all this has no impact on the code needed to add NVMEM support
> > to MTD, so the above patch didn't change at all.  
> 
> It does have an impact on the supported binding though.
> nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> means people will be able to define their NVMEM cells directly under
> the MTD device and reference them from other nodes (even if it's not
> documented), and as you said, it conflict with the old MTD partition
> bindings. So we'd better agree on this binding before merging this
> patch.

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

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

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

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

This is not good. First the NVMEM device is only a virtual concept of
the Linux kernel, it has no place in the DT. Secondly the NVMEM
provider (here the MTD device) then has to manually parse its DT node to
find this subnode, pass it to the NVMEM framework to later again
resolve it back to the MTD device. Not very complex but still a lot of
useless code, just registering the MTD device is a lot simpler and much
more inline with most other kernel API that register a "service"
available from a device.

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

That sound like a poor workaround. Remember that this problem could
appear with any device that has a binding that use child nodes.

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

That is a bit cleaner, but I don't think it would be worse the
complexity. Furthermore xlate functions are more about converting
from hardware parameters to internal kernel representation than to hide
extra DT parsing.

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

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

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

7/ Just add a compatible string to the nvmem cell. No code change is
needed, however as the nvmem cells have an address space (the offset in
byte in the storage) it might still clash with another address space
used by the main device biding (for example a number of child
functions).

> There are probably other options (some were proposed by Alban and
> Srinivas already), but I'd like to get this sorted out before we merge
> this patch.
> 
> Alban, Srinivas, any opinion?

My preference goes to 6/ as it is trivial to implement, solves all
known shortcomings and is backward compatible with the current binding.
All other solutions have limitations and/or require too complex
implementations compared to what they try to solve.

Alban

Boris Brezillon Aug. 21, 2018, 5:07 a.m. UTC | #10

On Mon, 20 Aug 2018 23:27:48 +0200
Alban <[e-mail]> wrote:

> On Mon, 20 Aug 2018 20:20:38 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[e-mail]> wrote:
> >   
> > > 
> > > Overall am still not able to clear visualize on how MTD bindings with 
> > > nvmem cells would look in both partition and un-partition usecases?
> > > An example DT would be nice here!!    
> > 
> > Something along those lines:  
> 
> We must also have a compatible string on the nvmem-cells node to make
> sure we don't clash with the old style MTD partitions,

That's not possible, because we don't have a reg prop in the
nvmem-cells node.

> or some other
> device specific binding.

This one might happen.

Was Rob okay with this compatible? If he was, I guess we can go for
this binding. Srinivas, any objection?

> 
> > 
> > 	mtdnode {
> > 		nvmem-cells {  
>                         compatible = "nvmem-cells";
> > 			#address-cells = <1>;
> > 			#size-cells = <1>;
> > 
> > 			cell@0 {
> > 				reg = <0x0 0x14>;
> > 			};
> > 		};
> > 
> > 		partitions {
> > 			compatible = "fixed-partitions";
> > 			#address-cells = <1>;
> > 			#size-cells = <1>;
> > 
> > 			partition@0 {
> > 				reg = <0x0 0x20000>;
> > 
> > 				nvmem-cells {  
>                                         compatible = "nvmem-cells";
> > 					#address-cells = <1>;
> > 					#size-cells = <1>;
> > 
> > 					cell@0 {
> > 						reg = <0x0 0x10>;
> > 					};
> > 				};
> > 			};
> > 		};
> > 	};  
> 
> 
> Alban

Boris Brezillon Aug. 21, 2018, 5:44 a.m. UTC | #11

On Tue, 21 Aug 2018 00:53:27 +0200
Alban <[e-mail]> wrote:

> On Sun, 19 Aug 2018 18:46:09 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Sun, 19 Aug 2018 13:31:06 +0200
> > Alban <[e-mail]> wrote:
> >   
> > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > Boris Brezillon <[e-mail]> wrote:
> > >     
> > > > Hi Bartosz,
> > > > 
> > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > Bartosz Golaszewski <[e-mail]> wrote:
> > > >       
> > > > > From: Alban Bedel <[e-mail]>
> > > > > 
> > > > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > > > For this the mtd devices are registered as read-only NVMEM providers.
> > > > > 
> > > > > Signed-off-by: Alban Bedel <[e-mail]>
> > > > > [Bartosz:
> > > > >   - use the managed variant of nvmem_register(),
> > > > >   - set the nvmem name]
> > > > > Signed-off-by: Bartosz Golaszewski <[e-mail]>        
> > > > 
> > > > What happened to the 2 other patches of Alban's series? I'd really
> > > > like the DT case to be handled/agreed on in the same patchset, but
> > > > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > > > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > > > glue has been floating around for quite some time...      
> > > 
> > > These other patches were to fix what I consider a fundamental flaw in
> > > the generic NVMEM bindings, however we couldn't agree on this point.
> > > Bartosz later contacted me to take over this series and I suggested to
> > > just change the MTD NVMEM binding to use a compatible string on the
> > > NVMEM cells as an alternative solution to fix the clash with the old
> > > style MTD partition.
> > > 
> > > However all this has no impact on the code needed to add NVMEM support
> > > to MTD, so the above patch didn't change at all.    
> > 
> > It does have an impact on the supported binding though.
> > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> > means people will be able to define their NVMEM cells directly under
> > the MTD device and reference them from other nodes (even if it's not
> > documented), and as you said, it conflict with the old MTD partition
> > bindings. So we'd better agree on this binding before merging this
> > patch.  
> 
> Unless the nvmem cell node has a compatible string, then it won't be
> considered as a partition by the MTD code. That is were the clash is,
> both bindings allow free named child nodes without a compatible string.

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

> 
> > I see several options:
> > 
> > 1/ provide a way to tell the NVMEM framework not to use parent->of_node
> >    even if it's != NULL. This way we really don't support defining
> >    NVMEM cells in the DT, and also don't support referencing the nvmem
> >    device using a phandle.  
> 
> I really don't get what the point of this would be. Make the whole API
> useless?

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

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

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

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

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

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

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

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

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

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

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

> 
> > 4/ Add a ->of_xlate() hook that would be called if present by the
> >    framework instead of using the default parsing we have right now.  
> 
> That is a bit cleaner, but I don't think it would be worse the
> complexity.

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

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

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

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

If you like.

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

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

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

That's not true!!! What forces people to add this compatible in their
DT? Nothing. I'll tell you what will happen: people will start defining
their nvmem cells directly under the MTD node because that *works*, and
even if the binding is not documented and we consider it invalid, we'll
be stuck supporting it forever. As said above, the very reason for
option #1 to exist is to give you and Srinivas some more time to sort
this out, while unblocking Bartosz in the meantime.

> however as the nvmem cells have an address space (the offset in
> byte in the storage) it might still clash with another address space
> used by the main device biding (for example a number of child
> functions).
> 
> > There are probably other options (some were proposed by Alban and
> > Srinivas already), but I'd like to get this sorted out before we merge
> > this patch.
> > 
> > Alban, Srinivas, any opinion?  
> 
> My preference goes to 6/ as it is trivial to implement, solves all
> known shortcomings and is backward compatible with the current binding.
> All other solutions have limitations and/or require too complex
> implementations compared to what they try to solve.

So we're back to square 1, and you're again blocking everything because
you refuse to consider other options.

There's obviously nothing more I can do to help, and that's unfortunate
because other people are waiting for this feature.

Regards,

Boris

Srinivas Kandagatla Aug. 21, 2018, 9:38 a.m. UTC | #12

Hi Boris/Bartosz,

On 21/08/18 06:44, Boris Brezillon wrote:
>>> 4/ Add a ->of_xlate() hook that would be called if present by the
>>>     framework instead of using the default parsing we have right now.
>> That is a bit cleaner, but I don't think it would be worse the
>> complexity.
> But it's way more flexible than putting everything in the nvmem
> framework. BTW, did you notice that nvmem-cells parsing does not work
> with flashes bigger than 4GB, because the framework assumes
> #address-cells and #size-cells are always 1. That's probably something
> we'll have to fix for the MTD case.
> 

I have hacked up some thing on these lines to add a custom match 
function for nvmem provider and it looks like it can work for mtd case.

This addresses concern #1 "to ignore of_node from dev pointer passed to 
nvmem_config" also provides way to do some sanity checks on nvmem cell node.
In this patch I have just added a simple mtd_nvmem_match() example which 
will be always true, however we can add checks here to see if the np is 
actually a nvmem-cells node or something on those lines to enforce the 
bindings. Please fix and remove this from nvmem-core patch incase you 
plan to use/test this.

We still have one open issue of supporting #address-cells and 
#size-cells in nvmem, which I can look at if you are happy with this 
approach!

----------------------------------->cut<---------------------------------
Author: Srinivas Kandagatla <[e-mail]>
Date:   Tue Aug 21 10:07:24 2018 +0100

     nvmem: core: add custom match function support

     Some nvmem providers might not have a simple DT layout, nvmem cells
     could be part of the unpartioned space or with-in partition or
     even in sub partition of the provider.

     Current matching function is expecting that the provider should be
     immediate parent of the cell, which might not be true for the above
     cases. So allow a custom match function for such devices which can
     validate and match the cell as per the provider specific bindings.

     Signed-off-by: Srinivas Kandagatla <[e-mail]>

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index a57302eaceb5..33541b18ac30 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -502,6 +502,19 @@ static int mtd_nvmem_reg_read(void *priv, unsigned 
int offset,
         return retlen == bytes ? 0 : -EIO;
  }

+static int mtd_nvmem_match(void *priv, struct device *dev,
+                          struct device_node *np)
+{
+       struct mtd_info *mtd = priv;
+
+       /*
+        * Add more checks to make sure device node is inline with
+        * binding if required
+        */
+
+       return &mtd->dev == dev->parent;
+}
+
  static int mtd_nvmem_add(struct mtd_info *mtd)
  {
         struct nvmem_config config = { };
@@ -516,6 +529,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
         config.read_only = true;
         config.root_only = true;
         config.priv = mtd;
+       config.match = mtd_nvmem_match;

         mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
         if (IS_ERR(mtd->nvmem)) {
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 3a8bf832243d..32bc4e70522c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -41,6 +41,7 @@ struct nvmem_device {
         struct device           *base_dev;
         nvmem_reg_read_t        reg_read;
         nvmem_reg_write_t       reg_write;
+       nvmem_match_t           match;
         void *priv;
  };

@@ -265,6 +266,11 @@ static struct bus_type nvmem_bus_type = {

  static int of_nvmem_match(struct device *dev, void *nvmem_np)
  {
+       struct nvmem_device *nvmem = to_nvmem_device(dev);
+
+       if (nvmem->match)
+               return nvmem->match(nvmem->priv, dev, nvmem_np);
+
         return dev->of_node == nvmem_np;
  }

@@ -482,7 +488,9 @@ 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->match)
+               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 24def6ad09bb..b29059bb406e 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -14,6 +14,7 @@

  #include <linux/err.h>
  #include <linux/errno.h>
+#include <linux/of.h>

  struct nvmem_device;
  struct nvmem_cell_info;
@@ -21,6 +22,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned 
int offset,
                                 void *val, size_t bytes);
  typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
                                  void *val, size_t bytes);
+typedef int (*nvmem_match_t)(void *priv, struct device *dev,
+                            struct device_node *np);
+

  /**
   * struct nvmem_config - NVMEM device configuration
@@ -58,6 +62,7 @@ struct nvmem_config {
         bool                    root_only;
         nvmem_reg_read_t        reg_read;
         nvmem_reg_write_t       reg_write;
+       nvmem_match_t           match;
         int     size;
         int     word_size;
         int     stride;

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

thanks,
srini

Srinivas Kandagatla Aug. 21, 2018, 9:50 a.m. UTC | #13

On 20/08/18 19:20, Boris Brezillon wrote:
> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[e-mail]> wrote:
> 
>>
>> Overall am still not able to clear visualize on how MTD bindings with
>> nvmem cells would look in both partition and un-partition usecases?
>> An example DT would be nice here!!
> 
> Something along those lines:
> 
This looks good to me.
> 	mtdnode {
> 		nvmem-cells {
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			cell@0 {
> 				reg = <0x0 0x14>;
> 			};
> 		};
> 
> 		partitions {
> 			compatible = "fixed-partitions";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			partition@0 {
> 				reg = <0x0 0x20000>;
> 
> 				nvmem-cells {
> 					#address-cells = <1>;
> 					#size-cells = <1>;
> 
> 					cell@0 {
> 						reg = <0x0 0x10>;
> 					};
> 				};
> 			};
> 		};
> 	}; >

Just curious...Is there a reason why we can't do it like this?:
Is this because of issue of #address-cells and #size-cells Or mtd 
bindings always prefer subnodes?

	mtdnode {
		reg = <0x0123000 0x40000>;
		#address-cells = <1>;
		#size-cells = <1>;
		cell@0 {
			compatible = "nvmem-cell";
			reg = <0x0 0x14>;
		};

		partitions {
			compatible = "fixed-partitions";
			#address-cells = <1>;
			#size-cells = <1>;

			partition@0 {
				reg = <0x0 0x20000>;
				cell@0 {
					compatible = "nvmem-cell";
					reg = <0x0 0x10>;
				};
			};
		};
	};

Am okay either way!

thanks,
srini

Boris Brezillon Aug. 21, 2018, 9:56 a.m. UTC | #14

On Tue, 21 Aug 2018 10:50:07 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> On 20/08/18 19:20, Boris Brezillon wrote:
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[e-mail]> wrote:
> >   
> >>
> >> Overall am still not able to clear visualize on how MTD bindings with
> >> nvmem cells would look in both partition and un-partition usecases?
> >> An example DT would be nice here!!  
> > 
> > Something along those lines:
> >   
> This looks good to me.
> > 	mtdnode {
> > 		nvmem-cells {
> > 			#address-cells = <1>;
> > 			#size-cells = <1>;
> > 
> > 			cell@0 {
> > 				reg = <0x0 0x14>;
> > 			};
> > 		};
> > 
> > 		partitions {
> > 			compatible = "fixed-partitions";
> > 			#address-cells = <1>;
> > 			#size-cells = <1>;
> > 
> > 			partition@0 {
> > 				reg = <0x0 0x20000>;
> > 
> > 				nvmem-cells {
> > 					#address-cells = <1>;
> > 					#size-cells = <1>;
> > 
> > 					cell@0 {
> > 						reg = <0x0 0x10>;
> > 					};
> > 				};
> > 			};
> > 		};
> > 	}; >  
> 
> Just curious...Is there a reason why we can't do it like this?:
> Is this because of issue of #address-cells and #size-cells Or mtd 
> bindings always prefer subnodes?
> 
> 	mtdnode {
> 		reg = <0x0123000 0x40000>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		cell@0 {
> 			compatible = "nvmem-cell";
> 			reg = <0x0 0x14>;
> 		};
> 
> 		partitions {
> 			compatible = "fixed-partitions";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			partition@0 {
> 				reg = <0x0 0x20000>;
> 				cell@0 {
> 					compatible = "nvmem-cell";
> 					reg = <0x0 0x10>;
> 				};
> 			};
> 		};
> 	};

It's because partitions were initially directly defined under the mtd
node, so, if you have an old DT you might have something like:

	mtdnode {
		reg = <0x0123000 0x40000>;
		#address-cells = <1>;
		#size-cells = <1>;

		partition@0 {
			reg = <0x0 0x20000>;
			...
		};
		...
	};

If we use such a DT with this patch applied, the NVMEM framework will
consider MTD partitions as nvmem cells, which is not what we want.

Srinivas Kandagatla Aug. 21, 2018, 10:11 a.m. UTC | #15

On 21/08/18 10:56, Boris Brezillon wrote:
> On Tue, 21 Aug 2018 10:50:07 +0100
> Srinivas Kandagatla<[e-mail]>  wrote:
> 
>> On 20/08/18 19:20, Boris Brezillon wrote:
>>> On Mon, 20 Aug 2018 11:43:34 +0100
>>> Srinivas Kandagatla<[e-mail]>  wrote:
>>>    
>>>> Overall am still not able to clear visualize on how MTD bindings with
>>>> nvmem cells would look in both partition and un-partition usecases?
>>>> An example DT would be nice here!!
>>> Something along those lines:
>>>    
>> This looks good to me.
>>> 	mtdnode {
>>> 		nvmem-cells {
>>> 			#address-cells = <1>;
>>> 			#size-cells = <1>;
>>>
>>> 			cell@0 {
>>> 				reg = <0x0 0x14>;
>>> 			};
>>> 		};
>>>
>>> 		partitions {
>>> 			compatible = "fixed-partitions";
>>> 			#address-cells = <1>;
>>> 			#size-cells = <1>;
>>>
>>> 			partition@0 {
>>> 				reg = <0x0 0x20000>;
>>>
>>> 				nvmem-cells {
>>> 					#address-cells = <1>;
>>> 					#size-cells = <1>;
>>>
>>> 					cell@0 {
>>> 						reg = <0x0 0x10>;
>>> 					};
>>> 				};
>>> 			};
>>> 		};
>>> 	}; >
>> Just curious...Is there a reason why we can't do it like this?:
>> Is this because of issue of #address-cells and #size-cells Or mtd
>> bindings always prefer subnodes?
>>
>> 	mtdnode {
>> 		reg = <0x0123000 0x40000>;
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		cell@0 {
>> 			compatible = "nvmem-cell";
>> 			reg = <0x0 0x14>;
>> 		};
>>
>> 		partitions {
>> 			compatible = "fixed-partitions";
>> 			#address-cells = <1>;
>> 			#size-cells = <1>;
>>
>> 			partition@0 {
>> 				reg = <0x0 0x20000>;
>> 				cell@0 {
>> 					compatible = "nvmem-cell";
>> 					reg = <0x0 0x10>;
>> 				};
>> 			};
>> 		};
>> 	};
> It's because partitions were initially directly defined under the mtd
> node, so, if you have an old DT you might have something like:
> 
> 	mtdnode {
> 		reg = <0x0123000 0x40000>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		partition@0 {
> 			reg = <0x0 0x20000>;
> 			...
> 		};
> 		...
> 	};
> 
> If we use such a DT with this patch applied, the NVMEM framework will
> consider MTD partitions as nvmem cells, which is not what we want.
Yep, I agree.
TBH, I wanted to add compatible string to nvmem-cell at some point in 
time and it seems more natural update too. One of the reason we 
discussed this in the past was parsers. Looks like mtd can make use of this.

We should be able to add this as an optional flag in nvmem_config to 
enforce this check in case providers wanted to.

Do you think that would help mtd nvmem case?
Also I felt like nvmem-cells subnode seems to be a bit heavy!

thanks,
srini

Boris Brezillon Aug. 21, 2018, 10:43 a.m. UTC | #16

On Tue, 21 Aug 2018 11:11:58 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> On 21/08/18 10:56, Boris Brezillon wrote:
> > On Tue, 21 Aug 2018 10:50:07 +0100
> > Srinivas Kandagatla<[e-mail]>  wrote:
> >   
> >> On 20/08/18 19:20, Boris Brezillon wrote:  
> >>> On Mon, 20 Aug 2018 11:43:34 +0100
> >>> Srinivas Kandagatla<[e-mail]>  wrote:
> >>>      
> >>>> Overall am still not able to clear visualize on how MTD bindings with
> >>>> nvmem cells would look in both partition and un-partition usecases?
> >>>> An example DT would be nice here!!  
> >>> Something along those lines:
> >>>      
> >> This looks good to me.  
> >>> 	mtdnode {
> >>> 		nvmem-cells {
> >>> 			#address-cells = <1>;
> >>> 			#size-cells = <1>;
> >>>
> >>> 			cell@0 {
> >>> 				reg = <0x0 0x14>;
> >>> 			};
> >>> 		};
> >>>
> >>> 		partitions {
> >>> 			compatible = "fixed-partitions";
> >>> 			#address-cells = <1>;
> >>> 			#size-cells = <1>;
> >>>
> >>> 			partition@0 {
> >>> 				reg = <0x0 0x20000>;
> >>>
> >>> 				nvmem-cells {
> >>> 					#address-cells = <1>;
> >>> 					#size-cells = <1>;
> >>>
> >>> 					cell@0 {
> >>> 						reg = <0x0 0x10>;
> >>> 					};
> >>> 				};
> >>> 			};
> >>> 		};
> >>> 	}; >  
> >> Just curious...Is there a reason why we can't do it like this?:
> >> Is this because of issue of #address-cells and #size-cells Or mtd
> >> bindings always prefer subnodes?
> >>
> >> 	mtdnode {
> >> 		reg = <0x0123000 0x40000>;
> >> 		#address-cells = <1>;
> >> 		#size-cells = <1>;
> >> 		cell@0 {
> >> 			compatible = "nvmem-cell";
> >> 			reg = <0x0 0x14>;
> >> 		};
> >>
> >> 		partitions {
> >> 			compatible = "fixed-partitions";
> >> 			#address-cells = <1>;
> >> 			#size-cells = <1>;
> >>
> >> 			partition@0 {
> >> 				reg = <0x0 0x20000>;
> >> 				cell@0 {
> >> 					compatible = "nvmem-cell";
> >> 					reg = <0x0 0x10>;
> >> 				};
> >> 			};
> >> 		};
> >> 	};  
> > It's because partitions were initially directly defined under the mtd
> > node, so, if you have an old DT you might have something like:
> > 
> > 	mtdnode {
> > 		reg = <0x0123000 0x40000>;
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 
> > 		partition@0 {
> > 			reg = <0x0 0x20000>;
> > 			...
> > 		};
> > 		...
> > 	};
> > 
> > If we use such a DT with this patch applied, the NVMEM framework will
> > consider MTD partitions as nvmem cells, which is not what we want.  
> Yep, I agree.
> TBH, I wanted to add compatible string to nvmem-cell at some point in 
> time and it seems more natural update too. One of the reason we 
> discussed this in the past was parsers. Looks like mtd can make use of this.
> 
> We should be able to add this as an optional flag in nvmem_config to 
> enforce this check in case providers wanted to.
> 
> Do you think that would help mtd nvmem case?

Yes, it should work if nvmem cells are defined directly under the mtd
node (or the partition they belong to).

> Also I felt like nvmem-cells subnode seems to be a bit heavy!

I still think grouping nvmem cells in a subnode is cleaner (just like
we do for partitions), but I won't object if all parties (you, Alban
and Rob) agree on this solution.

Boris Brezillon Aug. 21, 2018, 11:31 a.m. UTC | #17

On Tue, 21 Aug 2018 10:38:13 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> Hi Boris/Bartosz,
> 
> On 21/08/18 06:44, Boris Brezillon wrote:
> >>> 4/ Add a ->of_xlate() hook that would be called if present by the
> >>>     framework instead of using the default parsing we have right now.  
> >> That is a bit cleaner, but I don't think it would be worse the
> >> complexity.  
> > But it's way more flexible than putting everything in the nvmem
> > framework. BTW, did you notice that nvmem-cells parsing does not work
> > with flashes bigger than 4GB, because the framework assumes
> > #address-cells and #size-cells are always 1. That's probably something
> > we'll have to fix for the MTD case.
> >   
> 
> I have hacked up some thing on these lines to add a custom match 
> function for nvmem provider and it looks like it can work for mtd case.
> 
> This addresses concern #1 "to ignore of_node from dev pointer passed to 
> nvmem_config" also provides way to do some sanity checks on nvmem cell node.
> In this patch I have just added a simple mtd_nvmem_match() example which 
> will be always true, however we can add checks here to see if the np is 
> actually a nvmem-cells node or something on those lines to enforce the 
> bindings. Please fix and remove this from nvmem-core patch incase you 
> plan to use/test this.
> 
> We still have one open issue of supporting #address-cells and 
> #size-cells in nvmem, which I can look at if you are happy with this 
> approach!
> 
> ----------------------------------->cut<---------------------------------  
> Author: Srinivas Kandagatla <[e-mail]>
> Date:   Tue Aug 21 10:07:24 2018 +0100
> 
>      nvmem: core: add custom match function support
> 
>      Some nvmem providers might not have a simple DT layout, nvmem cells
>      could be part of the unpartioned space or with-in partition or
>      even in sub partition of the provider.
> 
>      Current matching function is expecting that the provider should be
>      immediate parent of the cell, which might not be true for the above
>      cases. So allow a custom match function for such devices which can
>      validate and match the cell as per the provider specific bindings.
> 
>      Signed-off-by: Srinivas Kandagatla <[e-mail]>
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index a57302eaceb5..33541b18ac30 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -502,6 +502,19 @@ static int mtd_nvmem_reg_read(void *priv, unsigned 
> int offset,
>          return retlen == bytes ? 0 : -EIO;
>   }
> 
> +static int mtd_nvmem_match(void *priv, struct device *dev,
> +                          struct device_node *np)
> +{
> +       struct mtd_info *mtd = priv;
> +
> +       /*
> +        * Add more checks to make sure device node is inline with
> +        * binding if required
> +        */
> +
> +       return &mtd->dev == dev->parent;
> +}
> +
>   static int mtd_nvmem_add(struct mtd_info *mtd)
>   {
>          struct nvmem_config config = { };
> @@ -516,6 +529,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>          config.read_only = true;
>          config.root_only = true;
>          config.priv = mtd;
> +       config.match = mtd_nvmem_match;
> 
>          mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
>          if (IS_ERR(mtd->nvmem)) {
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 3a8bf832243d..32bc4e70522c 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -41,6 +41,7 @@ struct nvmem_device {
>          struct device           *base_dev;
>          nvmem_reg_read_t        reg_read;
>          nvmem_reg_write_t       reg_write;
> +       nvmem_match_t           match;
>          void *priv;
>   };
> 
> @@ -265,6 +266,11 @@ static struct bus_type nvmem_bus_type = {
> 
>   static int of_nvmem_match(struct device *dev, void *nvmem_np)
>   {
> +       struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> +       if (nvmem->match)
> +               return nvmem->match(nvmem->priv, dev, nvmem_np);
> +
>          return dev->of_node == nvmem_np;
>   }
> 
> @@ -482,7 +488,9 @@ 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->match)
> +               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 24def6ad09bb..b29059bb406e 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -14,6 +14,7 @@
> 
>   #include <linux/err.h>
>   #include <linux/errno.h>
> +#include <linux/of.h>
> 
>   struct nvmem_device;
>   struct nvmem_cell_info;
> @@ -21,6 +22,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned 
> int offset,
>                                  void *val, size_t bytes);
>   typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
>                                   void *val, size_t bytes);
> +typedef int (*nvmem_match_t)(void *priv, struct device *dev,
> +                            struct device_node *np);
> +
> 
>   /**
>    * struct nvmem_config - NVMEM device configuration
> @@ -58,6 +62,7 @@ struct nvmem_config {
>          bool                    root_only;
>          nvmem_reg_read_t        reg_read;
>          nvmem_reg_write_t       reg_write;
> +       nvmem_match_t           match;
>          int     size;
>          int     word_size;
>          int     stride;
> 

That might work if nvmem cells are defined directly under the mtdnode.
If we go for this approach, I'd recommend replacing this ->match() hook
by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
node, because what we're really after here is knowing which subnode is
an nvmem cell and which subnode is not.

Alban Bedel Aug. 21, 2018, 11:39 a.m. UTC | #18

On Tue, 21 Aug 2018 10:50:07 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> Just curious...Is there a reason why we can't do it like this?:
> Is this because of issue of #address-cells and #size-cells Or mtd 
> bindings always prefer subnodes?
> 
> 	mtdnode {
> 		reg = <0x0123000 0x40000>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		cell@0 {
> 			compatible = "nvmem-cell";
> 			reg = <0x0 0x14>;
> 		};
> 
> 		partitions {
> 			compatible = "fixed-partitions";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			partition@0 {
> 				reg = <0x0 0x20000>;
> 				cell@0 {
> 					compatible = "nvmem-cell";
> 					reg = <0x0 0x10>;
> 				};
> 			};
> 		};
> 	};

That would work, the MTD partitions parser ignore child nodes with a
compatible string when looking for "old style" partitions, see [1].
However we still have the a potential address space clash between the
nvmem cells and the main device binding.

Alban

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/mtd/ofpart.c#L28

Srinivas Kandagatla Aug. 21, 2018, noon UTC | #19

On 21/08/18 12:39, Alban wrote:
> However we still have the a potential address space clash between the
> nvmem cells and the main device binding.
Can you elaborate?

--srini

Alban Bedel Aug. 21, 2018, 12:27 p.m. UTC | #20

On Tue, 21 Aug 2018 07:44:04 +0200
Boris Brezillon <[e-mail]> wrote:

> On Tue, 21 Aug 2018 00:53:27 +0200
> Alban <[e-mail]> wrote:
> 
> > On Sun, 19 Aug 2018 18:46:09 +0200
> > Boris Brezillon <[e-mail]> wrote:
> >   
> > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > Alban <[e-mail]> wrote:
> > >     
> > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > Boris Brezillon <[e-mail]> wrote:
> > > >       
> > > > > Hi Bartosz,
> > > > > 
> > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > Bartosz Golaszewski <[e-mail]> wrote:
> > > > >         
> > > > > > From: Alban Bedel <[e-mail]>
> > > > > > 
> > > > > > Allow drivers that use the nvmem API to read data stored on
> > > > > > MTD devices. For this the mtd devices are registered as
> > > > > > read-only NVMEM providers.
> > > > > > 
> > > > > > Signed-off-by: Alban Bedel <[e-mail]>
> > > > > > [Bartosz:
> > > > > >   - use the managed variant of nvmem_register(),
> > > > > >   - set the nvmem name]
> > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > <[e-mail]>          
> > > > > 
> > > > > What happened to the 2 other patches of Alban's series? I'd
> > > > > really like the DT case to be handled/agreed on in the same
> > > > > patchset, but IIRC, Alban and Srinivas disagreed on how this
> > > > > should be represented. I hope this time we'll come to an
> > > > > agreement, because the MTD <-> NVMEM glue has been floating
> > > > > around for quite some time...        
> > > > 
> > > > These other patches were to fix what I consider a fundamental
> > > > flaw in the generic NVMEM bindings, however we couldn't agree
> > > > on this point. Bartosz later contacted me to take over this
> > > > series and I suggested to just change the MTD NVMEM binding to
> > > > use a compatible string on the NVMEM cells as an alternative
> > > > solution to fix the clash with the old style MTD partition.
> > > > 
> > > > However all this has no impact on the code needed to add NVMEM
> > > > support to MTD, so the above patch didn't change at all.      
> > > 
> > > It does have an impact on the supported binding though.
> > > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node,
> > > which means people will be able to define their NVMEM cells
> > > directly under the MTD device and reference them from other nodes
> > > (even if it's not documented), and as you said, it conflict with
> > > the old MTD partition bindings. So we'd better agree on this
> > > binding before merging this patch.    
> > 
> > Unless the nvmem cell node has a compatible string, then it won't be
> > considered as a partition by the MTD code. That is were the clash
> > is, both bindings allow free named child nodes without a compatible
> > string.  
> 
> Except the current nvmem cells parsing code does not enforce that, and
> existing DTs rely on this behavior, so we're screwed. Or are you
> suggesting to add a new "bool check_cells_compat;" field to
> nvmem_config?

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

> >   
> > > I see several options:
> > > 
> > > 1/ provide a way to tell the NVMEM framework not to use
> > > parent->of_node even if it's != NULL. This way we really don't
> > > support defining NVMEM cells in the DT, and also don't support
> > > referencing the nvmem device using a phandle.    
> > 
> > I really don't get what the point of this would be. Make the whole
> > API useless?  
> 
> No, just allow Bartosz to get his changes merged without waiting for
> you and Srinivas to agree on how to handle the new binding. As I said
> earlier, this mtd <-> nvmem stuff has been around for quite some time,
> and instead of trying to find an approach that makes everyone happy,
> you decided to let the patchset die.

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

> >   
> > > 2/ define a new binding where all nvmem-cells are placed in an
> > >    "nvmem" subnode (just like we have this "partitions" subnode
> > > for partitions), and then add a config->of_node field so that the
> > >    nvmem provider can explicitly specify the DT node representing
> > > the nvmem device. We'll also need to set this field to
> > > ERR_PTR(-ENOENT) in case this node does not exist so that the
> > > nvmem framework knows that it should not assign
> > > nvmem->dev.of_node to parent->of_node    
> > 
> > This is not good. First the NVMEM device is only a virtual concept
> > of the Linux kernel, it has no place in the DT.  
> 
> nvmem-cells is a virtual concept too, still, you define them in the
> DT.

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

> > Secondly the NVMEM
> > provider (here the MTD device) then has to manually parse its DT
> > node to find this subnode, pass it to the NVMEM framework to later
> > again resolve it back to the MTD device.  
> 
> We don't resolve it back to the MTD device, because the MTD device is
> just the parent of the nvmem device.
>
> > Not very complex but still a lot of
> > useless code, just registering the MTD device is a lot simpler and
> > much more inline with most other kernel API that register a
> > "service" available from a device.  
> 
> I'm not a big fan of this option either, but I thought I had to
> propose it.
> 
> >   
> > > 3/ only declare partitions as nvmem providers. This would solve
> > > the problem we have with partitions defined in the DT since
> > >    defining sub-partitions in the DT is not (yet?) supported and
> > >    partition nodes are supposed to be leaf nodes. Still, I'm not
> > > a big fan of this solution because it will prevent us from
> > > supporting sub-partitions if we ever want/need to.    
> > 
> > That sound like a poor workaround.  
> 
> Yes, that's a workaround. And the reason I propose it, is, again,
> because I don't want to block Bartosz.
> 
> > Remember that this problem could
> > appear with any device that has a binding that use child nodes.  
> 
> I'm talking about partitions, and you're talking about mtd devices.
> Right now partitions don't have subnodes, and if we define that
> partition subnodes should describe nvmem-cells, then it becomes part
> of the official binding. So, no, the problem you mention does not
> (yet) exist.

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

> >   
> > > 4/ Add a ->of_xlate() hook that would be called if present by the
> > >    framework instead of using the default parsing we have right
> > > now.    
> > 
> > That is a bit cleaner, but I don't think it would be worse the
> > complexity.  
> 
> But it's way more flexible than putting everything in the nvmem
> framework. BTW, did you notice that nvmem-cells parsing does not work
> with flashes bigger than 4GB, because the framework assumes
> #address-cells and #size-cells are always 1. That's probably something
> we'll have to fix for the MTD case.

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

> > Furthermore xlate functions are more about converting
> > from hardware parameters to internal kernel representation than to
> > hide extra DT parsing.  
> 
> Hm, how is that different? ->of_xlate() is just a way for drivers to
> have their own DT representation, which is exactly what we want here.

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

> >   
> > > 5/ Tell the nvmem framework the name of the subnode containing
> > > nvmem cell definitions (if NULL that means cells are directly
> > > defined under the nvmem provider node). We would set it to
> > > "nvmem-cells" (or whatever you like) for the MTD case.    
> > 
> > If so please match on compatible and not on the node name.  
> 
> If you like.
> 
> > 
> > 6/ Extend the current NVMEM cell lookup to check if the parent node
> > of the cell has a compatible string set to "nvmem-cells". If it
> > doesn't it mean we have the current binding and this node is the
> > NVMEM device. If it does the device node is just the next parent.
> > This is trivial to implement (literally 2 lines of code) and cover
> > all the cases currently known.  
> 
> Except Srinivas was not happy with this solution, and this stalled the
> discussion. I'm trying to find other options and you keep rejecting
> all of them to come back to this one.

Well, I think this is the best solution :/

> > 
> > 7/ Just add a compatible string to the nvmem cell. No code change is
> > needed,  
> 
> That's not true!!!

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

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

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

>  As said above, the
> very reason for option #1 to exist is to give you and Srinivas some
> more time to sort this out, while unblocking Bartosz in the meantime.

I'm fine with #1, I just didn't understood what it was useful for.

> > however as the nvmem cells have an address space (the offset in
> > byte in the storage) it might still clash with another address space
> > used by the main device biding (for example a number of child
> > functions).
> >   
> > > There are probably other options (some were proposed by Alban and
> > > Srinivas already), but I'd like to get this sorted out before we
> > > merge this patch.
> > > 
> > > Alban, Srinivas, any opinion?    
> > 
> > My preference goes to 6/ as it is trivial to implement, solves all
> > known shortcomings and is backward compatible with the current
> > binding. All other solutions have limitations and/or require too
> > complex implementations compared to what they try to solve.  
> 
> So we're back to square 1, and you're again blocking everything
> because you refuse to consider other options.

As I'm not a maintainer so I just can't block anything. But I won't lie
and pretend that I support a solution with known shortcomings.

Alban

Boris Brezillon Aug. 21, 2018, 12:57 p.m. UTC | #21

On Tue, 21 Aug 2018 14:27:16 +0200
Alban <[e-mail]> wrote:

> On Tue, 21 Aug 2018 07:44:04 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Tue, 21 Aug 2018 00:53:27 +0200
> > Alban <[e-mail]> wrote:
> >   
> > > On Sun, 19 Aug 2018 18:46:09 +0200
> > > Boris Brezillon <[e-mail]> wrote:
> > >     
> > > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > > Alban <[e-mail]> wrote:
> > > >       
> > > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > > Boris Brezillon <[e-mail]> wrote:
> > > > >         
> > > > > > Hi Bartosz,
> > > > > > 
> > > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > > Bartosz Golaszewski <[e-mail]> wrote:
> > > > > >           
> > > > > > > From: Alban Bedel <[e-mail]>
> > > > > > > 
> > > > > > > Allow drivers that use the nvmem API to read data stored on
> > > > > > > MTD devices. For this the mtd devices are registered as
> > > > > > > read-only NVMEM providers.
> > > > > > > 
> > > > > > > Signed-off-by: Alban Bedel <[e-mail]>
> > > > > > > [Bartosz:
> > > > > > >   - use the managed variant of nvmem_register(),
> > > > > > >   - set the nvmem name]
> > > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > > <[e-mail]>            
> > > > > > 
> > > > > > What happened to the 2 other patches of Alban's series? I'd
> > > > > > really like the DT case to be handled/agreed on in the same
> > > > > > patchset, but IIRC, Alban and Srinivas disagreed on how this
> > > > > > should be represented. I hope this time we'll come to an
> > > > > > agreement, because the MTD <-> NVMEM glue has been floating
> > > > > > around for quite some time...          
> > > > > 
> > > > > These other patches were to fix what I consider a fundamental
> > > > > flaw in the generic NVMEM bindings, however we couldn't agree
> > > > > on this point. Bartosz later contacted me to take over this
> > > > > series and I suggested to just change the MTD NVMEM binding to
> > > > > use a compatible string on the NVMEM cells as an alternative
> > > > > solution to fix the clash with the old style MTD partition.
> > > > > 
> > > > > However all this has no impact on the code needed to add NVMEM
> > > > > support to MTD, so the above patch didn't change at all.        
> > > > 
> > > > It does have an impact on the supported binding though.
> > > > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node,
> > > > which means people will be able to define their NVMEM cells
> > > > directly under the MTD device and reference them from other nodes
> > > > (even if it's not documented), and as you said, it conflict with
> > > > the old MTD partition bindings. So we'd better agree on this
> > > > binding before merging this patch.      
> > > 
> > > Unless the nvmem cell node has a compatible string, then it won't be
> > > considered as a partition by the MTD code. That is were the clash
> > > is, both bindings allow free named child nodes without a compatible
> > > string.    
> > 
> > Except the current nvmem cells parsing code does not enforce that, and
> > existing DTs rely on this behavior, so we're screwed. Or are you
> > suggesting to add a new "bool check_cells_compat;" field to
> > nvmem_config?  
> 
> There is no nvmem cell parsing at the moment. The DT lookup just
> resolve the phandle to the cell node, take the parent node and search
> for the nvmem provider that has this OF node. So extending it in case
> the node has a *new* compatible string would not break users of the old
> binding, none of them has a compatible string.

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

> 
> > >     
> > > > I see several options:
> > > > 
> > > > 1/ provide a way to tell the NVMEM framework not to use
> > > > parent->of_node even if it's != NULL. This way we really don't
> > > > support defining NVMEM cells in the DT, and also don't support
> > > > referencing the nvmem device using a phandle.      
> > > 
> > > I really don't get what the point of this would be. Make the whole
> > > API useless?    
> > 
> > No, just allow Bartosz to get his changes merged without waiting for
> > you and Srinivas to agree on how to handle the new binding. As I said
> > earlier, this mtd <-> nvmem stuff has been around for quite some time,
> > and instead of trying to find an approach that makes everyone happy,
> > you decided to let the patchset die.  
> 
> As long as that wouldn't prevent using DT in the future I'm fine with
> it.
> 
> > >     
> > > > 2/ define a new binding where all nvmem-cells are placed in an
> > > >    "nvmem" subnode (just like we have this "partitions" subnode
> > > > for partitions), and then add a config->of_node field so that the
> > > >    nvmem provider can explicitly specify the DT node representing
> > > > the nvmem device. We'll also need to set this field to
> > > > ERR_PTR(-ENOENT) in case this node does not exist so that the
> > > > nvmem framework knows that it should not assign
> > > > nvmem->dev.of_node to parent->of_node      
> > > 
> > > This is not good. First the NVMEM device is only a virtual concept
> > > of the Linux kernel, it has no place in the DT.    
> > 
> > nvmem-cells is a virtual concept too, still, you define them in the
> > DT.  
> 
> To be honest I also think that naming this concept "nvmem" in the DT was
> a bad idea. Perhaps something like "driver-data" or "data-cell" would
> have been better as that would make it clear what this is about, nvmem
> is just the Linux implementation of this concept.

I'm fine using a different name.

> 
> > > Secondly the NVMEM
> > > provider (here the MTD device) then has to manually parse its DT
> > > node to find this subnode, pass it to the NVMEM framework to later
> > > again resolve it back to the MTD device.    
> > 
> > We don't resolve it back to the MTD device, because the MTD device is
> > just the parent of the nvmem device.
> >  
> > > Not very complex but still a lot of
> > > useless code, just registering the MTD device is a lot simpler and
> > > much more inline with most other kernel API that register a
> > > "service" available from a device.    
> > 
> > I'm not a big fan of this option either, but I thought I had to
> > propose it.
> >   
> > >     
> > > > 3/ only declare partitions as nvmem providers. This would solve
> > > > the problem we have with partitions defined in the DT since
> > > >    defining sub-partitions in the DT is not (yet?) supported and
> > > >    partition nodes are supposed to be leaf nodes. Still, I'm not
> > > > a big fan of this solution because it will prevent us from
> > > > supporting sub-partitions if we ever want/need to.      
> > > 
> > > That sound like a poor workaround.    
> > 
> > Yes, that's a workaround. And the reason I propose it, is, again,
> > because I don't want to block Bartosz.
> >   
> > > Remember that this problem could
> > > appear with any device that has a binding that use child nodes.    
> > 
> > I'm talking about partitions, and you're talking about mtd devices.
> > Right now partitions don't have subnodes, and if we define that
> > partition subnodes should describe nvmem-cells, then it becomes part
> > of the official binding. So, no, the problem you mention does not
> > (yet) exist.  
> 
> That would add another binding that allow free named child nodes
> without compatible string although experience has repeatedly shown that
> this was a bad idea.

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

> 
> > >     
> > > > 4/ Add a ->of_xlate() hook that would be called if present by the
> > > >    framework instead of using the default parsing we have right
> > > > now.      
> > > 
> > > That is a bit cleaner, but I don't think it would be worse the
> > > complexity.    
> > 
> > But it's way more flexible than putting everything in the nvmem
> > framework. BTW, did you notice that nvmem-cells parsing does not work
> > with flashes bigger than 4GB, because the framework assumes
> > #address-cells and #size-cells are always 1. That's probably something
> > we'll have to fix for the MTD case.  
> 
> Yes, however that's just an implementation limitation which is trivial
> to solve.

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

> 
> > > Furthermore xlate functions are more about converting
> > > from hardware parameters to internal kernel representation than to
> > > hide extra DT parsing.    
> > 
> > Hm, how is that different? ->of_xlate() is just a way for drivers to
> > have their own DT representation, which is exactly what we want here.  
> 
> There is a big difference. DT represent the hardware and the
> relationship between the devices in an OS independent format. We don't
> add extra stuff in there just to map back internal Linux API details.

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

> 
> > >     
> > > > 5/ Tell the nvmem framework the name of the subnode containing
> > > > nvmem cell definitions (if NULL that means cells are directly
> > > > defined under the nvmem provider node). We would set it to
> > > > "nvmem-cells" (or whatever you like) for the MTD case.      
> > > 
> > > If so please match on compatible and not on the node name.    
> > 
> > If you like.
> >   
> > > 
> > > 6/ Extend the current NVMEM cell lookup to check if the parent node
> > > of the cell has a compatible string set to "nvmem-cells". If it
> > > doesn't it mean we have the current binding and this node is the
> > > NVMEM device. If it does the device node is just the next parent.
> > > This is trivial to implement (literally 2 lines of code) and cover
> > > all the cases currently known.    
> > 
> > Except Srinivas was not happy with this solution, and this stalled the
> > discussion. I'm trying to find other options and you keep rejecting
> > all of them to come back to this one.  
> 
> Well, I think this is the best solution :/
> 
> > > 
> > > 7/ Just add a compatible string to the nvmem cell. No code change is
> > > needed,    
> > 
> > That's not true!!!  
> 
> What is not true in this statement? The current nvmem lookup don't care
> about compatible strings, so the cell lookup would just works fine. The
> MTD partition parser won't consider them as a partition because of the
> compatible string. Problem solved!

No because partitions defined the old way (as direct subnodes of the MTD
node) will be considered as NVMEM cells by the NVMEM framework, and I
don't want that. Plus, I don't want people to start defining their
NVMEM cells and forget the compat string (which would work just fine
because the NVMEM framework doesn't care).

> 
> > What forces people to add this compatible in their
> > DT? Nothing. I'll tell you what will happen: people will start
> > defining their nvmem cells directly under the MTD node because that
> > *works*, and even if the binding is not documented and we consider it
> > invalid, we'll be stuck supporting it forever.  
> 
> Do note that undocumented bindings are not allowed. DTS that use
> undocumented bindings (normally) just get rejected.

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

Boris Brezillon Aug. 21, 2018, 1:01 p.m. UTC | #22

On Tue, 21 Aug 2018 13:00:04 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> On 21/08/18 12:39, Alban wrote:
> > However we still have the a potential address space clash between the
> > nvmem cells and the main device binding.  
> Can you elaborate?
> 

Yes, I'd be interested in having a real example too, cause I don't see
what this address space clash is.

Srinivas Kandagatla Aug. 21, 2018, 1:34 p.m. UTC | #23

On 21/08/18 12:31, Boris Brezillon wrote:
>>     * struct nvmem_config - NVMEM device configuration
>> @@ -58,6 +62,7 @@ struct nvmem_config {
>>           bool                    root_only;
>>           nvmem_reg_read_t        reg_read;
>>           nvmem_reg_write_t       reg_write;
>> +       nvmem_match_t           match;
>>           int     size;
>>           int     word_size;
>>           int     stride;
>>
> That might work if nvmem cells are defined directly under the mtdnode.
Layout should not matter! which is the purpose of this callback.

The only purpose of this callback is to tell nvmem core that the 
node(nvmem cell) belongs to that provider or not, if it is then we 
successfully found the provider. Its up to the provider on which layout 
it describes nvmem cells. Additionally the provider can add additional 
sanity checks in this match function to ensure that cell is correctly 
represented.


> If we go for this approach, I'd recommend replacing this ->match() hook
> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
> node, because what we're really after here is knowing which subnode is
> an nvmem cell and which subnode is not.

I agree on passing cell node instead of its parent. Regarding basic 
validating if its nvmem cell or not, we can check compatible string in 
nvmem core if we decide to use "nvmem-cell" compatible.

Also just in case if you missed this, nvmem would not iterate the

--srini

Srinivas Kandagatla Aug. 21, 2018, 1:37 p.m. UTC | #24

On 21/08/18 14:34, Srinivas Kandagatla wrote:
> 
> 
> On 21/08/18 12:31, Boris Brezillon wrote:
>>>     * struct nvmem_config - NVMEM device configuration
>>> @@ -58,6 +62,7 @@ struct nvmem_config {
>>>           bool                    root_only;
>>>           nvmem_reg_read_t        reg_read;
>>>           nvmem_reg_write_t       reg_write;
>>> +       nvmem_match_t           match;
>>>           int     size;
>>>           int     word_size;
>>>           int     stride;
>>>
>> That might work if nvmem cells are defined directly under the mtdnode.
> Layout should not matter! which is the purpose of this callback.
> 
> The only purpose of this callback is to tell nvmem core that the 
> node(nvmem cell) belongs to that provider or not, if it is then we 
> successfully found the provider. Its up to the provider on which layout 
> it describes nvmem cells. Additionally the provider can add additional 
> sanity checks in this match function to ensure that cell is correctly 
> represented.
> 
> 
>> If we go for this approach, I'd recommend replacing this ->match() hook
>> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
>> node, because what we're really after here is knowing which subnode is
>> an nvmem cell and which subnode is not.
> 
> I agree on passing cell node instead of its parent. Regarding basic 
> validating if its nvmem cell or not, we can check compatible string in 
> nvmem core if we decide to use "nvmem-cell" compatible.
> 
> Also just in case if you missed this, nvmem would not iterate the
Sorry !! i hit send button too quickly I guess.

What I meant to say here, is that nvmem core would not iterate the 
provider node in any case.

Only time it looks at the cell node is when a consumer requests for the 
cell.

--srini

Alban Bedel Aug. 21, 2018, 1:57 p.m. UTC | #25

On Tue, 21 Aug 2018 14:57:25 +0200
Boris Brezillon <[e-mail]> wrote:

> On Tue, 21 Aug 2018 14:27:16 +0200
> Alban <[e-mail]> wrote:
> 
> > On Tue, 21 Aug 2018 07:44:04 +0200
> > Boris Brezillon <[e-mail]> wrote:
> >   
> > > On Tue, 21 Aug 2018 00:53:27 +0200
> > > Alban <[e-mail]> wrote:
> > >     
> > > > On Sun, 19 Aug 2018 18:46:09 +0200
> > > > Boris Brezillon <[e-mail]> wrote:
> > > >       
> > > > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > > > Alban <[e-mail]> wrote:
> > > > >         
> > > > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > > > Boris Brezillon <[e-mail]> wrote:
> > > > > >           
> > > > > > > Hi Bartosz,
> > > > > > > 
> > > > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > > > Bartosz Golaszewski <[e-mail]> wrote:
> > > > > > >             
> > > > > > > > From: Alban Bedel <[e-mail]>
> > > > > > > > 
> > > > > > > > Allow drivers that use the nvmem API to read data
> > > > > > > > stored on MTD devices. For this the mtd devices are
> > > > > > > > registered as read-only NVMEM providers.
> > > > > > > > 
> > > > > > > > Signed-off-by: Alban Bedel <[e-mail]>
> > > > > > > > [Bartosz:
> > > > > > > >   - use the managed variant of nvmem_register(),
> > > > > > > >   - set the nvmem name]
> > > > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > > > <[e-mail]>              
> > > > > > > 
> > > > > > > What happened to the 2 other patches of Alban's series?
> > > > > > > I'd really like the DT case to be handled/agreed on in
> > > > > > > the same patchset, but IIRC, Alban and Srinivas disagreed
> > > > > > > on how this should be represented. I hope this time we'll
> > > > > > > come to an agreement, because the MTD <-> NVMEM glue has
> > > > > > > been floating around for quite some time...            
> > > > > > 
> > > > > > These other patches were to fix what I consider a
> > > > > > fundamental flaw in the generic NVMEM bindings, however we
> > > > > > couldn't agree on this point. Bartosz later contacted me to
> > > > > > take over this series and I suggested to just change the
> > > > > > MTD NVMEM binding to use a compatible string on the NVMEM
> > > > > > cells as an alternative solution to fix the clash with the
> > > > > > old style MTD partition.
> > > > > > 
> > > > > > However all this has no impact on the code needed to add
> > > > > > NVMEM support to MTD, so the above patch didn't change at
> > > > > > all.          
> > > > > 
> > > > > It does have an impact on the supported binding though.
> > > > > nvmem->dev.of_node is automatically assigned to
> > > > > mtd->dev.of_node, which means people will be able to define
> > > > > their NVMEM cells directly under the MTD device and reference
> > > > > them from other nodes (even if it's not documented), and as
> > > > > you said, it conflict with the old MTD partition bindings. So
> > > > > we'd better agree on this binding before merging this
> > > > > patch.        
> > > > 
> > > > Unless the nvmem cell node has a compatible string, then it
> > > > won't be considered as a partition by the MTD code. That is
> > > > were the clash is, both bindings allow free named child nodes
> > > > without a compatible string.      
> > > 
> > > Except the current nvmem cells parsing code does not enforce
> > > that, and existing DTs rely on this behavior, so we're screwed.
> > > Or are you suggesting to add a new "bool check_cells_compat;"
> > > field to nvmem_config?    
> > 
> > There is no nvmem cell parsing at the moment. The DT lookup just
> > resolve the phandle to the cell node, take the parent node and
> > search for the nvmem provider that has this OF node. So extending
> > it in case the node has a *new* compatible string would not break
> > users of the old binding, none of them has a compatible string.  
> 
> But we want to enforce the compat check on MTD devices, otherwise old
> MTD partitions (those defined under the MTD node) will be considered
> as NVMEM cells by the NVMEM framework. Hence the bool
> check_cells_compat field.

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

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

> >   
> > > >       
> > > > > I see several options:
> > > > > 
> > > > > 1/ provide a way to tell the NVMEM framework not to use
> > > > > parent->of_node even if it's != NULL. This way we really don't
> > > > > support defining NVMEM cells in the DT, and also don't support
> > > > > referencing the nvmem device using a phandle.        
> > > > 
> > > > I really don't get what the point of this would be. Make the
> > > > whole API useless?      
> > > 
> > > No, just allow Bartosz to get his changes merged without waiting
> > > for you and Srinivas to agree on how to handle the new binding.
> > > As I said earlier, this mtd <-> nvmem stuff has been around for
> > > quite some time, and instead of trying to find an approach that
> > > makes everyone happy, you decided to let the patchset die.    
> > 
> > As long as that wouldn't prevent using DT in the future I'm fine
> > with it.
> >   
> > > >       
> > > > > 2/ define a new binding where all nvmem-cells are placed in an
> > > > >    "nvmem" subnode (just like we have this "partitions"
> > > > > subnode for partitions), and then add a config->of_node field
> > > > > so that the nvmem provider can explicitly specify the DT node
> > > > > representing the nvmem device. We'll also need to set this
> > > > > field to ERR_PTR(-ENOENT) in case this node does not exist so
> > > > > that the nvmem framework knows that it should not assign
> > > > > nvmem->dev.of_node to parent->of_node        
> > > > 
> > > > This is not good. First the NVMEM device is only a virtual
> > > > concept of the Linux kernel, it has no place in the DT.      
> > > 
> > > nvmem-cells is a virtual concept too, still, you define them in
> > > the DT.    
> > 
> > To be honest I also think that naming this concept "nvmem" in the
> > DT was a bad idea. Perhaps something like "driver-data" or
> > "data-cell" would have been better as that would make it clear what
> > this is about, nvmem is just the Linux implementation of this
> > concept.  
> 
> I'm fine using a different name.
> 
> >   
> > > > Secondly the NVMEM
> > > > provider (here the MTD device) then has to manually parse its DT
> > > > node to find this subnode, pass it to the NVMEM framework to
> > > > later again resolve it back to the MTD device.      
> > > 
> > > We don't resolve it back to the MTD device, because the MTD
> > > device is just the parent of the nvmem device.
> > >    
> > > > Not very complex but still a lot of
> > > > useless code, just registering the MTD device is a lot simpler
> > > > and much more inline with most other kernel API that register a
> > > > "service" available from a device.      
> > > 
> > > I'm not a big fan of this option either, but I thought I had to
> > > propose it.
> > >     
> > > >       
> > > > > 3/ only declare partitions as nvmem providers. This would
> > > > > solve the problem we have with partitions defined in the DT
> > > > > since defining sub-partitions in the DT is not (yet?)
> > > > > supported and partition nodes are supposed to be leaf nodes.
> > > > > Still, I'm not a big fan of this solution because it will
> > > > > prevent us from supporting sub-partitions if we ever
> > > > > want/need to.        
> > > > 
> > > > That sound like a poor workaround.      
> > > 
> > > Yes, that's a workaround. And the reason I propose it, is, again,
> > > because I don't want to block Bartosz.
> > >     
> > > > Remember that this problem could
> > > > appear with any device that has a binding that use child
> > > > nodes.      
> > > 
> > > I'm talking about partitions, and you're talking about mtd
> > > devices. Right now partitions don't have subnodes, and if we
> > > define that partition subnodes should describe nvmem-cells, then
> > > it becomes part of the official binding. So, no, the problem you
> > > mention does not (yet) exist.    
> > 
> > That would add another binding that allow free named child nodes
> > without compatible string although experience has repeatedly shown
> > that this was a bad idea.  
> 
> Yes, I agree. Just thought it was important to have this solution in
> the list, even if it's just to reject it.
> 
> >   
> > > >       
> > > > > 4/ Add a ->of_xlate() hook that would be called if present by
> > > > > the framework instead of using the default parsing we have
> > > > > right now.        
> > > > 
> > > > That is a bit cleaner, but I don't think it would be worse the
> > > > complexity.      
> > > 
> > > But it's way more flexible than putting everything in the nvmem
> > > framework. BTW, did you notice that nvmem-cells parsing does not
> > > work with flashes bigger than 4GB, because the framework assumes
> > > #address-cells and #size-cells are always 1. That's probably
> > > something we'll have to fix for the MTD case.    
> > 
> > Yes, however that's just an implementation limitation which is
> > trivial to solve.  
> 
> Agree. I was just pointing it in case you hadn't noticed.
> 
> >   
> > > > Furthermore xlate functions are more about converting
> > > > from hardware parameters to internal kernel representation than
> > > > to hide extra DT parsing.      
> > > 
> > > Hm, how is that different? ->of_xlate() is just a way for drivers
> > > to have their own DT representation, which is exactly what we
> > > want here.    
> > 
> > There is a big difference. DT represent the hardware and the
> > relationship between the devices in an OS independent format. We
> > don't add extra stuff in there just to map back internal Linux API
> > details.  
> 
> And I'm not talking about adding SW information in the DT, I'm talking
> about HW specific description. We have the same solution for pinctrl
> configs (it's HW/driver specific).

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

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

> >   
> > > >       
> > > > > 5/ Tell the nvmem framework the name of the subnode containing
> > > > > nvmem cell definitions (if NULL that means cells are directly
> > > > > defined under the nvmem provider node). We would set it to
> > > > > "nvmem-cells" (or whatever you like) for the MTD case.        
> > > > 
> > > > If so please match on compatible and not on the node name.      
> > > 
> > > If you like.
> > >     
> > > > 
> > > > 6/ Extend the current NVMEM cell lookup to check if the parent
> > > > node of the cell has a compatible string set to "nvmem-cells".
> > > > If it doesn't it mean we have the current binding and this node
> > > > is the NVMEM device. If it does the device node is just the
> > > > next parent. This is trivial to implement (literally 2 lines of
> > > > code) and cover all the cases currently known.      
> > > 
> > > Except Srinivas was not happy with this solution, and this
> > > stalled the discussion. I'm trying to find other options and you
> > > keep rejecting all of them to come back to this one.    
> > 
> > Well, I think this is the best solution :/
> >   
> > > > 
> > > > 7/ Just add a compatible string to the nvmem cell. No code
> > > > change is needed,      
> > > 
> > > That's not true!!!    
> > 
> > What is not true in this statement? The current nvmem lookup don't
> > care about compatible strings, so the cell lookup would just works
> > fine. The MTD partition parser won't consider them as a partition
> > because of the compatible string. Problem solved!  
> 
> No because partitions defined the old way (as direct subnodes of the
> MTD node) will be considered as NVMEM cells by the NVMEM framework,
> and I don't want that.

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

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

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

> >   
> > > What forces people to add this compatible in their
> > > DT? Nothing. I'll tell you what will happen: people will start
> > > defining their nvmem cells directly under the MTD node because
> > > that *works*, and even if the binding is not documented and we
> > > consider it invalid, we'll be stuck supporting it forever.    
> > 
> > Do note that undocumented bindings are not allowed. DTS that use
> > undocumented bindings (normally) just get rejected.  
> 
> Except that's just in theory. In practice, if people can do something
> wrong, they'll complain if you later fix the bug and break their
> setup. So no, if we go for the "nvmem cells have an 'nvmem-cell'
> compat", then I'd like the NVMEM framework to enforce that somehow.

That should be trivial to implement.

Alban

Boris Brezillon Aug. 21, 2018, 1:57 p.m. UTC | #26

On Tue, 21 Aug 2018 14:37:37 +0100
Srinivas Kandagatla <[e-mail]> wrote:

> On 21/08/18 14:34, Srinivas Kandagatla wrote:
> > 
> > 
> > On 21/08/18 12:31, Boris Brezillon wrote:  
> >>>     * struct nvmem_config - NVMEM device configuration
> >>> @@ -58,6 +62,7 @@ struct nvmem_config {
> >>>           bool                    root_only;
> >>>           nvmem_reg_read_t        reg_read;
> >>>           nvmem_reg_write_t       reg_write;
> >>> +       nvmem_match_t           match;
> >>>           int     size;
> >>>           int     word_size;
> >>>           int     stride;
> >>>  
> >> That might work if nvmem cells are defined directly under the mtdnode.  
> > Layout should not matter! which is the purpose of this callback.
> > 
> > The only purpose of this callback is to tell nvmem core that the 
> > node(nvmem cell) belongs to that provider or not, if it is then we 
> > successfully found the provider. Its up to the provider on which layout 
> > it describes nvmem cells. Additionally the provider can add additional 
> > sanity checks in this match function to ensure that cell is correctly 
> > represented.
> > 
> >   
> >> If we go for this approach, I'd recommend replacing this ->match() hook
> >> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
> >> node, because what we're really after here is knowing which subnode is
> >> an nvmem cell and which subnode is not.  
> > 
> > I agree on passing cell node instead of its parent. Regarding basic 
> > validating if its nvmem cell or not, we can check compatible string in 
> > nvmem core if we decide to use "nvmem-cell" compatible.
> > 
> > Also just in case if you missed this, nvmem would not iterate the  
> Sorry !! i hit send button too quickly I guess.
> 
> What I meant to say here, is that nvmem core would not iterate the 
> provider node in any case.
> 
> Only time it looks at the cell node is when a consumer requests for the 
> cell.

I did miss that, indeed. Thanks for the heads up.

So, the "old partitions being considered as nvmem cells" is not really
a problem, because those parts shouldn't be referenced.
This leaves us with the config->force_compat_check topic, which I'd
like to have to ensure that nvmem cells under MTD nodes actually have
compatible = "nvmem-cell" and prevent people from inadvertently
omitting this prop.

And of course, we need Rob's approval on this new binding :-).

Boris Brezillon Aug. 21, 2018, 2:26 p.m. UTC | #27

On Tue, 21 Aug 2018 15:57:06 +0200
Alban <[e-mail]> wrote:

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

Yes, I missed that when briefly looking at the code.

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

Wow, that's not good. I guess we'll want to make that consistent at
some point.

 
> > > > > Furthermore xlate functions are more about converting
> > > > > from hardware parameters to internal kernel representation than
> > > > > to hide extra DT parsing.        
> > > > 
> > > > Hm, how is that different? ->of_xlate() is just a way for drivers
> > > > to have their own DT representation, which is exactly what we
> > > > want here.      
> > > 
> > > There is a big difference. DT represent the hardware and the
> > > relationship between the devices in an OS independent format. We
> > > don't add extra stuff in there just to map back internal Linux API
> > > details.    
> > 
> > And I'm not talking about adding SW information in the DT, I'm talking
> > about HW specific description. We have the same solution for pinctrl
> > configs (it's HW/driver specific).  
> 
> For pinctrl I do understand, these beast can be very different from SoC
> to SoC, having a single biding for all doesn't make much sense.
> 
> However here we are talking about a simple linear storage, nothing
> special at all. I could see the need for an xlate to for example
> support a device with several partitions, but not to just allow each
> driver to have slightly incompatible bindings.

Maybe, but I guess that's up to the subsystem maintainer to decide what
he prefers.
> > 
> > No because partitions defined the old way (as direct subnodes of the
> > MTD node) will be considered as NVMEM cells by the NVMEM framework,
> > and I don't want that.  
> 
> As I explained above that is not currently the case. If the NVMEM,
> framework is ever changed to explicitly parse NVMEM cells in advance
> we can first update the few existing users to add the compatible string.

We're supposed to be backward compatible (compatible with old DTs), so
that's not an option, though we could add a way to check the compat
string afterwards.

> 
> > Plus, I don't want people to start defining their NVMEM cells and
> > forget the compat string (which would work just fine because the
> > NVMEM framework doesn't care).  
> 
> A review of a new DTS should check that it use each binding correctly,
> AFAIK the DT people do that. We could also add a warning when there is
> no compatible string, that would also help pushing people to update
> their DTS.

Yes, but I'd still prefer if we were preventing people from referencing
mtd-nvmem cells if the node does not have an "nvmem-cell" compat.

> 
> > >     
> > > > What forces people to add this compatible in their
> > > > DT? Nothing. I'll tell you what will happen: people will start
> > > > defining their nvmem cells directly under the MTD node because
> > > > that *works*, and even if the binding is not documented and we
> > > > consider it invalid, we'll be stuck supporting it forever.      
> > > 
> > > Do note that undocumented bindings are not allowed. DTS that use
> > > undocumented bindings (normally) just get rejected.    
> > 
> > Except that's just in theory. In practice, if people can do something
> > wrong, they'll complain if you later fix the bug and break their
> > setup. So no, if we go for the "nvmem cells have an 'nvmem-cell'
> > compat", then I'd like the NVMEM framework to enforce that somehow.  
> 
> That should be trivial to implement.

Exactly, and that's why I'm insisting on this point.

Srinivas Kandagatla Aug. 21, 2018, 2:33 p.m. UTC | #28

On 21/08/18 15:26, Boris Brezillon wrote:
>> This also has the side effect that nvmem cells defined in DT don't
>> appear in sysfs, unlike those defined from board code.
> Wow, that's not good. I guess we'll want to make that consistent at
> some point.
> 
support for sysfs entries per cell is not available in nvmem. However 
there is nvmem sysfs entry per provider which can be read by the user 
space if required.


--srini

Alban Bedel Aug. 23, 2018, 10:29 a.m. UTC | #29

On Tue, 21 Aug 2018 15:01:21 +0200
Boris Brezillon <[e-mail]> wrote:

> On Tue, 21 Aug 2018 13:00:04 +0100
> Srinivas Kandagatla <[e-mail]> wrote:
> 
> > On 21/08/18 12:39, Alban wrote:  
> > > However we still have the a potential address space clash between
> > > the nvmem cells and the main device binding.    
> > Can you elaborate?
> >   
> 
> Yes, I'd be interested in having a real example too, cause I don't see
> what this address space clash is.

Let say I have a device that use the following binding:

device {
    compatible = "example-device";
    #address-cells = <2>;
    #size-cells = <1>;

    child@0,0 {
        reg = <0x0 0x0>;
        ...
    };

    child@1,2 {
        reg = <0x1 0x2>;
        ...
    };

};

Now this binding already use the node address space for something,
so putting a nvmem node as direct child would not work. Here it is
quiet clear as we have 2 address cells, however even if the number of
cells and the cells size would match it would still be conceptually
wrong as both bindings then use the same address space for something
different.

Alban

Boris Brezillon Aug. 24, 2018, 2:39 p.m. UTC | #30

On Thu, 23 Aug 2018 12:29:21 +0200
Alban <[e-mail]> wrote:

> On Tue, 21 Aug 2018 15:01:21 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Tue, 21 Aug 2018 13:00:04 +0100
> > Srinivas Kandagatla <[e-mail]> wrote:
> >   
> > > On 21/08/18 12:39, Alban wrote:    
> > > > However we still have the a potential address space clash between
> > > > the nvmem cells and the main device binding.      
> > > Can you elaborate?
> > >     
> > 
> > Yes, I'd be interested in having a real example too, cause I don't see
> > what this address space clash is.  
> 
> Let say I have a device that use the following binding:
> 
> device {
>     compatible = "example-device";
>     #address-cells = <2>;
>     #size-cells = <1>;
> 
>     child@0,0 {
>         reg = <0x0 0x0>;
>         ...
>     };
> 
>     child@1,2 {
>         reg = <0x1 0x2>;
>         ...
>     };
> 
> };
> 
> Now this binding already use the node address space for something,
> so putting a nvmem node as direct child would not work. Here it is
> quiet clear as we have 2 address cells, however even if the number of
> cells and the cells size would match it would still be conceptually
> wrong as both bindings then use the same address space for something
> different.

When would we end up in this situation? MTD/flash nodes are supposed to
be leaf nodes (if you omit the partitions underneath). What would be
the case for defining subnodes there?

Do you know of any devices that uses such a representation?

Srinivas Kandagatla Aug. 28, 2018, 10:20 a.m. UTC | #31

On 23/08/18 11:29, Alban wrote:
> Let say I have a device that use the following binding:
> 
> device {
>      compatible = "example-device";
>      #address-cells = <2>;
>      #size-cells = <1>;
> 
>      child@0,0 {
>          reg = <0x0 0x0>;
>          ...
>      };
> 
>      child@1,2 {
>          reg = <0x1 0x2>;
>          ...
>      };
> 
> };
> 
> Now this binding already use the node address space for something,
> so putting a nvmem node as direct child would not work.

AFAIK, It should work but the we would get a DT warning this, as nvmem 
does not use of_address based apis to parse this. Which should be 
totally fixed!!

As discussed before once we add support to #address-cells and 
#size-cells in nvmem core this should not be a problem.


--srini
  Here it is
> quiet clear as we have 2 address cells, however even if the number of
> cells and the cells size would match it would still be conceptually
> wrong as both bindings then use the same address space for something
> different.

Patch

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 46ab7feec6b6..f5549482d0df 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 42395df06be9..a57302eaceb5 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -488,6 +488,49 @@  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.name = mtd->name;
+	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 = devm_nvmem_register(&mtd->dev, &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;
 
 /**
@@ -570,6 +613,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)) {
@@ -595,6 +643,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);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a86c4fa93115..8121c6582285 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>
 
@@ -339,6 +340,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,