[PATCH v2 0/3] Implement nvmem support for mtd

[PATCH v2 0/3] Implement nvmem support for mtd

[Ansuel Smith]

The mtd support for the nvmem api has been stalled from 2018 with a patch
half pushed hoping that a scheme is found for the mtd name later. This
patchset try to address this.

The solution is simple.
New partitions scheme should always have the partitions {} structure and 
declare subnodes as partitions is deprecated and should not be used anymore.
Fixed-partitions parser is changed to parse direct subnode as partitions
only if the appropriate compatible is used. This change make possible
the use of nvmem-partitions compatible and the entire partition node can
be parsed by the nvmem of framework.
The current code register the partition to the nvmem framework every time
but skip actually of_node parting. The new nvmem-partitions compatible is
used to enable of_node parsing on the desired partitions.

Ansuel Smith (3):
  mtd: partitions: ofpart: skip subnodes parse with compatible
  mtd: core: add nvmem-partitions compatible to parse mtd as nvmem cells
  dt-bindings: mtd: Document use of nvmem-partitions compatible

 .../mtd/partitions/nvmem-partitions.yaml      | 105 ++++++++++++++++++
 drivers/mtd/mtdcore.c                         |   3 +-
 drivers/mtd/parsers/ofpart.c                  |   5 +
 3 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/nvmem-partitions.yaml

-- 
2.30.0

[PATCH v2 1/3] mtd: partitions: ofpart: skip subnodes parse with compatible

[Ansuel Smith]

If a partitions structure is not used, parse direct subnodes as
fixed-partitions only if a compatible is not found or is of type
fixed-partition. A parser can be used directly on the subnode and
subnodes should not be parsed as fixed-partitions by default.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/mtd/parsers/ofpart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/parsers/ofpart.c b/drivers/mtd/parsers/ofpart.c
index daf507c123e6..4b363dd0311c 100644
--- a/drivers/mtd/parsers/ofpart.c
+++ b/drivers/mtd/parsers/ofpart.c
@@ -50,6 +50,11 @@ static int parse_fixed_partitions(struct mtd_info *master,
 			 master->name, mtd_node);
 		ofpart_node = mtd_node;
 		dedicated = false;
+
+		/* Skip parsing direct subnodes if a compatible is found and is not fixed-partitions */
+		if (node_has_compatible(ofpart_node) &&
+		    !of_device_is_compatible(ofpart_node, "fixed-partitions"))
+			return 0;
 	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
 		/* The 'partitions' subnode might be used by another parser */
 		return 0;
-- 
2.30.0

Re: [PATCH v2 1/3] mtd: partitions: ofpart: skip subnodes parse with compatible

[Rafał Miłecki]

On 16.02.2021 22:26, Ansuel Smith wrote:
> If a partitions structure is not used, parse direct subnodes as
> fixed-partitions only if a compatible is not found or is of type
> fixed-partition. A parser can be used directly on the subnode and
> subnodes should not be parsed as fixed-partitions by default.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/mtd/parsers/ofpart.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/parsers/ofpart.c b/drivers/mtd/parsers/ofpart.c
> index daf507c123e6..4b363dd0311c 100644
> --- a/drivers/mtd/parsers/ofpart.c
> +++ b/drivers/mtd/parsers/ofpart.c
> @@ -50,6 +50,11 @@ static int parse_fixed_partitions(struct mtd_info *master,
>   			 master->name, mtd_node);
>   		ofpart_node = mtd_node;
>   		dedicated = false;
> +
> +		/* Skip parsing direct subnodes if a compatible is found and is not fixed-partitions */
> +		if (node_has_compatible(ofpart_node) &&
> +		    !of_device_is_compatible(ofpart_node, "fixed-partitions"))
> +			return 0;
>   	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
>   		/* The 'partitions' subnode might be used by another parser */
>   		return 0;

I admit I'm not familiar with the old binding, so let me know if my
understanding is incorrect.

It seems to me however that your change will break parsing in cases
like:

spi-flash@0 {
	compatible = "jedec,spi-nor";
	reg = <0x0>;

	partition@0 {
		label = "bootloader";
		reg = <0x0 0x100000>;
	};
};

nandcs@0 {
	compatible = "brcm,nandcs";
	reg = <0>;

	partition@0 {
		label = "bootloader";
		reg = <0x0000000 0x10000>;
	};
};

Did we ever use "fixed-partitions" without partitions { } subnode?

Re: [PATCH v2 1/3] mtd: partitions: ofpart: skip subnodes parse with compatible

[Ansuel Smith]

On Tue, Mar 02, 2021 at 05:53:54PM +0100, Rafał Miłecki wrote:
> On 16.02.2021 22:26, Ansuel Smith wrote:
> > If a partitions structure is not used, parse direct subnodes as
> > fixed-partitions only if a compatible is not found or is of type
> > fixed-partition. A parser can be used directly on the subnode and
> > subnodes should not be parsed as fixed-partitions by default.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >   drivers/mtd/parsers/ofpart.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/mtd/parsers/ofpart.c b/drivers/mtd/parsers/ofpart.c
> > index daf507c123e6..4b363dd0311c 100644
> > --- a/drivers/mtd/parsers/ofpart.c
> > +++ b/drivers/mtd/parsers/ofpart.c
> > @@ -50,6 +50,11 @@ static int parse_fixed_partitions(struct mtd_info *master,
> >   			 master->name, mtd_node);
> >   		ofpart_node = mtd_node;
> >   		dedicated = false;
> > +
> > +		/* Skip parsing direct subnodes if a compatible is found and is not fixed-partitions */
> > +		if (node_has_compatible(ofpart_node) &&
> > +		    !of_device_is_compatible(ofpart_node, "fixed-partitions"))
> > +			return 0;
> >   	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
> >   		/* The 'partitions' subnode might be used by another parser */
> >   		return 0;
> 
> I admit I'm not familiar with the old binding, so let me know if my
> understanding is incorrect.
> 
> It seems to me however that your change will break parsing in cases
> like:
> 
> spi-flash@0 {
> 	compatible = "jedec,spi-nor";
> 	reg = <0x0>;
> 
> 	partition@0 {
> 		label = "bootloader";
> 		reg = <0x0 0x100000>;
> 	};
> };
> 
> nandcs@0 {
> 	compatible = "brcm,nandcs";
> 	reg = <0>;
> 
> 	partition@0 {
> 		label = "bootloader";
> 		reg = <0x0000000 0x10000>;
> 	};
> };
> 
> Did we ever use "fixed-partitions" without partitions { } subnode?

Hi, very good point. You are right and I didin't think about this case.
I don't want to assume false statement, but since the ofpart parser and
the partitions structure should have been pushed at the same time, there
shouldn't be any use of "fixed-partitions" without partitions { }
subnodes. With this assumtion, the current implementation looks to be the 
cleanest way to skip parsing. (if the parsing is dubious, don't parse at
all... The idea was that)
The hacky and IMHO dirty way to solve this is add a bool to directly
skip the subnode parsing and check for that. Something like
"no-fixed-partition" that would disable the ofnode parser with no
partitions { } subnode would accomplish the same result of this patch
and keep compatibility with nodes scheme you pointed out.

[PATCH v2 2/3] mtd: core: add nvmem-partitions compatible to parse mtd as nvmem cells

[Ansuel Smith]

Partitions that contains the nvmem-partitions compatible will register
their direct subonodes as nvmem cells and the node will be treated as a
nvmem provider.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/mtd/mtdcore.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 2d6423d89a17..29d257678a86 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -531,6 +531,7 @@ static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
 
 static int mtd_nvmem_add(struct mtd_info *mtd)
 {
+	struct device_node *node = mtd_get_of_node(mtd);
 	struct nvmem_config config = {};
 
 	config.id = -1;
@@ -543,7 +544,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 	config.stride = 1;
 	config.read_only = true;
 	config.root_only = true;
-	config.no_of_node = true;
+	config.no_of_node = !of_device_is_compatible(node, "nvmem-partitions");
 	config.priv = mtd;
 
 	mtd->nvmem = nvmem_register(&config);
-- 
2.30.0

Re: [PATCH v2 2/3] mtd: core: add nvmem-partitions compatible to parse mtd as nvmem cells

[Rafał Miłecki]

On 16.02.2021 22:26, Ansuel Smith wrote:
> Partitions that contains the nvmem-partitions compatible will register
> their direct subonodes as nvmem cells and the node will be treated as a
> nvmem provider.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Tested-by: Rafał Miłecki <rafal@milecki.pl>


I applied this patch on top of the:
[PATCH] mtd: parsers: ofpart: limit parsing of deprecated DT syntax

I succesfully used NVMEM cell defined in bootloader mtd partition for
reading device MAC address.

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

	partition@0 {
		compatible = "nvmem-partitions";
		label = "bootloader";
		reg = <0x0 0x100000>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0 0x0 0x100000>;

		base_mac_addr: mac@106a0 {
			reg = <0x106a0 0x6>;
		};
	};
}

[PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Ansuel Smith]

Document nvmem-partitions compatible used to treat mtd partitions as a
nvmem provider.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../mtd/partitions/nvmem-partitions.yaml      | 105 ++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/nvmem-partitions.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/nvmem-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/nvmem-partitions.yaml
new file mode 100644
index 000000000000..1ff283febcaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/nvmem-partitions.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/partitions/nvmem-partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nvmem partitions
+
+description: |
+  This binding can be used to treat the specific partition as a nvmem provider.
+  Each direct subnodes represents the nvmem cells and won't be parsed as fixed-partitions.
+  Fixed-partitions bindings described in fixed-partitions.yaml apply to the nvmem provider node.
+
+maintainers:
+  - Ansuel Smith <ansuelsmth@gmail.com>
+
+properties:
+  compatible:
+    const: nvmem-partitions
+
+  "#address-cells": true
+
+  "#size-cells": true
+
+  reg:
+    description: partition's offset and size within the flash
+    maxItems: 1
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+  - reg
+
+additionalProperties: true
+
+examples:
+  - |
+    partitions {
+      compatible = "fixed-partitions";
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      /* ... */
+
+      };
+      art: art@1200000 {
+        compatible = "nvmem-partitions";
+        reg = <0x1200000 0x0140000>;
+        label = "art";
+        read-only;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        macaddr_gmac1: macaddr_gmac1@0 {
+          reg = <0x0 0x6>;
+        };
+
+        macaddr_gmac2: macaddr_gmac2@6 {
+          reg = <0x6 0x6>;
+        };
+
+        pre_cal_24g: pre_cal_24g@1000 {
+          reg = <0x1000 0x2f20>;
+        };
+
+        pre_cal_5g: pre_cal_5g@5000{
+          reg = <0x5000 0x2f20>;
+        };
+      };
+  - |
+    partitions {
+        compatible = "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            label = "bootloader";
+            reg = <0x000000 0x100000>;
+            read-only;
+        };
+
+        firmware@100000 {
+            compatible = "brcm,trx";
+            label = "firmware";
+            reg = <0x100000 0xe00000>;
+        };
+
+        calibration@f00000 {
+            compatible = "nvmem-partitions";
+            label = "calibration";
+            reg = <0xf00000 0x100000>;
+            ranges = <0 0xf00000 0x100000>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            wifi0@0 {
+                reg = <0x000000 0x080000>;
+            };
+
+            wifi1@80000 {
+                reg = <0x080000 0x080000>;
+            };
+        };
+    };
-- 
2.30.0

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rafał Miłecki]

[Rob: please advise]

On 16.02.2021 22:26, Ansuel Smith wrote:
> Document nvmem-partitions compatible used to treat mtd partitions as a
> nvmem provider.

Until now we were using "compatible" string in partition node only for
parsers (looking for subpartitions). We need to think if this change can
break anything from DT / Linux perspective.

Compatible strings should be unique, so there is no risk of conflict
between NVMEM and parsers.

Now: can we ever need mtd partition to:
1. Contain subpartitions
2. Provide NVMEM
at the same time?

Let's say:

partition@0 {
	compatible = "vendor,dynamic-firmware-partitions", "nvmem-partitions";
	label = "firmware";
	reg = <0x0 0x100000>;
	#address-cells = <1>;
	#size-cells = <1>;
	ranges = <0 0x0 0x100000>;

	firmware-version@10 {
		reg = <0x10 0x4>;
	};

	firmware-date@10 {
		reg = <0x20 0x4>;
	};
};

Is that allowed to respect both "compatible" strings and have:
1. Linux parser parse "firmware" for subpartitions
2. Linux MTD register "firmware" as NVMEM device
?

If not, what other options do we have? Is that allowed to have a
dangling MTD NVMEM node with phandle to MTD partition?

firmware: partition@0 {
	compatible = "vendor,dynamic-firmware-partitions";
	label = "firmware";
	reg = <0x0 0x100000>;
};

(...)

firmware-version@10 {
	compatible = "mtd-nvmem";
	reg = <0x10 0x4>;
	mtd = <&firmware>;
};

firmware-date@10 {
	compatible = "mtd-nvmem";
	reg = <0x20 0x4>;
	mtd = <&firmware>;
};


Rob: I'd really appreciate your input & help here.

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rob Herring]

On Wed, Mar 03, 2021 at 11:01:55AM +0100, Rafał Miłecki wrote:
> [Rob: please advise]
> 
> On 16.02.2021 22:26, Ansuel Smith wrote:
> > Document nvmem-partitions compatible used to treat mtd partitions as a
> > nvmem provider.
> 
> Until now we were using "compatible" string in partition node only for
> parsers (looking for subpartitions). We need to think if this change can
> break anything from DT / Linux perspective.
> 
> Compatible strings should be unique, so there is no risk of conflict
> between NVMEM and parsers.
> 
> Now: can we ever need mtd partition to:
> 1. Contain subpartitions
> 2. Provide NVMEM
> at the same time?
> 
> Let's say:
> 
> partition@0 {
> 	compatible = "vendor,dynamic-firmware-partitions", "nvmem-partitions";

I think you'd want the "vendor,dynamic-firmware-partitions" parser/code 
to serve up any nvmem regions. Whether you have a fallback here depends 
if an OS could make use of the regions knowing nothing about 
"vendor,dynamic-firmware-partitions".

> 	label = "firmware";
> 	reg = <0x0 0x100000>;
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges = <0 0x0 0x100000>;
> 
> 	firmware-version@10 {
> 		reg = <0x10 0x4>;
> 	};
> 
> 	firmware-date@10 {
> 		reg = <0x20 0x4>;
> 	};
> };
> 
> Is that allowed to respect both "compatible" strings and have:
> 1. Linux parser parse "firmware" for subpartitions
> 2. Linux MTD register "firmware" as NVMEM device
> ?
> 
> If not, what other options do we have? Is that allowed to have a
> dangling MTD NVMEM node with phandle to MTD partition?
> 
> firmware: partition@0 {
> 	compatible = "vendor,dynamic-firmware-partitions";
> 	label = "firmware";
> 	reg = <0x0 0x100000>;
> };
> 
> (...)
> 
> firmware-version@10 {
> 	compatible = "mtd-nvmem";
> 	reg = <0x10 0x4>;
> 	mtd = <&firmware>;
> };
> 
> firmware-date@10 {
> 	compatible = "mtd-nvmem";
> 	reg = <0x20 0x4>;
> 	mtd = <&firmware>;
> };

This, I would not like to see.

Rob

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rafał Miłecki]

On 05.03.2021 23:23, Rob Herring wrote:
> On Wed, Mar 03, 2021 at 11:01:55AM +0100, Rafał Miłecki wrote:
>> [Rob: please advise]
>>
>> On 16.02.2021 22:26, Ansuel Smith wrote:
>>> Document nvmem-partitions compatible used to treat mtd partitions as a
>>> nvmem provider.
>>
>> Until now we were using "compatible" string in partition node only for
>> parsers (looking for subpartitions). We need to think if this change can
>> break anything from DT / Linux perspective.
>>
>> Compatible strings should be unique, so there is no risk of conflict
>> between NVMEM and parsers.
>>
>> Now: can we ever need mtd partition to:
>> 1. Contain subpartitions
>> 2. Provide NVMEM
>> at the same time?
>>
>> Let's say:
>>
>> partition@0 {
>> 	compatible = "vendor,dynamic-firmware-partitions", "nvmem-partitions";
> 
> I think you'd want the "vendor,dynamic-firmware-partitions" parser/code
> to serve up any nvmem regions. Whether you have a fallback here depends
> if an OS could make use of the regions knowing nothing about
> "vendor,dynamic-firmware-partitions".

Perfect! I didn't think that driver handling
"vendor,dynamic-firmware-partitions" may also take care of NVMEM.

Thank you.

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rafał Miłecki]

On 16.02.2021 22:26, Ansuel Smith wrote:
> Document nvmem-partitions compatible used to treat mtd partitions as a
> nvmem provider.

I'm just wondering if "nvmem-partitions" is accurate enough. Partitions
bit sounds a bit ambiguous in the mtd context.

What do you think about "mtd-nvmem-cells" or just "nvmem-cells"?

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Ansuel Smith]

On Mon, Mar 08, 2021 at 10:48:32AM +0100, Rafał Miłecki wrote:
> On 16.02.2021 22:26, Ansuel Smith wrote:
> > Document nvmem-partitions compatible used to treat mtd partitions as a
> > nvmem provider.
> 
> I'm just wondering if "nvmem-partitions" is accurate enough. Partitions
> bit sounds a bit ambiguous in the mtd context.
> 
> What do you think about "mtd-nvmem-cells" or just "nvmem-cells"?

I read somewhere that mtd is not so standard so "nvmem-cells" should be the
right compatible.
To sum up, with v2 I should change the compatible name and just push the
2 and 3 patch. (waiting your fix to be accepted) Correct?

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rafał Miłecki]

On 07.03.2021 18:04, Ansuel Smith wrote:
> On Mon, Mar 08, 2021 at 10:48:32AM +0100, Rafał Miłecki wrote:
>> On 16.02.2021 22:26, Ansuel Smith wrote:
>>> Document nvmem-partitions compatible used to treat mtd partitions as a
>>> nvmem provider.
>>
>> I'm just wondering if "nvmem-partitions" is accurate enough. Partitions
>> bit sounds a bit ambiguous in the mtd context.
>>
>> What do you think about "mtd-nvmem-cells" or just "nvmem-cells"?
> 
> I read somewhere that mtd is not so standard so "nvmem-cells" should be the
> right compatible.
> To sum up, with v2 I should change the compatible name and just push the
> 2 and 3 patch. (waiting your fix to be accepted) Correct?

Yes, I believe so

Re: [PATCH v2 3/3] dt-bindings: mtd: Document use of nvmem-partitions compatible

[Rafał Miłecki]

On 07.03.2021 18:04, Ansuel Smith wrote:
> On Mon, Mar 08, 2021 at 10:48:32AM +0100, Rafał Miłecki wrote:
>> On 16.02.2021 22:26, Ansuel Smith wrote:
>>> Document nvmem-partitions compatible used to treat mtd partitions as a
>>> nvmem provider.
>>
>> I'm just wondering if "nvmem-partitions" is accurate enough. Partitions
>> bit sounds a bit ambiguous in the mtd context.
>>
>> What do you think about "mtd-nvmem-cells" or just "nvmem-cells"?
> 
> I read somewhere that mtd is not so standard so "nvmem-cells" should be the
> right compatible.
> To sum up, with v2 I should change the compatible name and just push the
> 2 and 3 patch. (waiting your fix to be accepted) Correct?

I'm also quite sure you're fine to send V2 now, if you just let
maintainers know (e.g. in a comment below a --- tear line) that it
depends on the:
[PATCH] mtd: parsers: ofpart: limit parsing of deprecated DT syntax

In other words: you don't need to wait for my patch to get accepted.