diff mbox series

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

Message ID 20210216212638.28382-4-ansuelsmth@gmail.com
State Superseded
Headers show
Series Implement nvmem support for mtd | expand

Commit Message

Ansuel Smith Feb. 16, 2021, 9:26 p.m. UTC
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

Comments

Rafał Miłecki March 3, 2021, 10:01 a.m. UTC | #1
[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.
Rob Herring March 5, 2021, 10:23 p.m. UTC | #2
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
Ansuel Smith March 7, 2021, 5:04 p.m. UTC | #3
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?
Rafał Miłecki March 8, 2021, 9:45 a.m. UTC | #4
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.
Rafał Miłecki March 8, 2021, 9:48 a.m. UTC | #5
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"?
Rafał Miłecki March 8, 2021, 1:28 p.m. UTC | #6
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
Rafał Miłecki March 8, 2021, 1:32 p.m. UTC | #7
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.
diff mbox series

Patch

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>;
+            };
+        };
+    };