[PATCH 0/3] mtd: Add support for Airoha partition scheme

[PATCH 0/3] mtd: Add support for Airoha partition scheme

[Christian Marangi]

This is split from a different series called "mtd: improve block2mtd +
airoha parser" with only the parser changes.

This series add support for the Airoha partition table where
the last partition is always ART and is placed at the end of the flash.

This require dynamic calculation of the offset as some dedicated
driver for bad block management might be used that reserve some space
at the end of the flash for bad block accounting. This special driver
change the end offset of the flash hence a dynamic parser is needed.

New aarch64 Airoha SoC (EN7581) make use of this partition scheme or
calibration data is not accessible.

Relevant Changes from old series:
Changes v3:
- Add compatible to partitions.yaml

Christian Marangi (3):
  of: also export of_update_property
  dt-bindings: mtd: Add Documentation for Airoha fixed-partitions
  mtd: parser: add support for Airoha parser

 .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
 .../bindings/mtd/partitions/partitions.yaml   |  1 +
 drivers/mtd/parsers/Kconfig                   | 10 +++
 drivers/mtd/parsers/Makefile                  |  1 +
 drivers/mtd/parsers/ofpart_airoha.c           | 57 +++++++++++++
 drivers/mtd/parsers/ofpart_airoha.h           | 18 +++++
 drivers/mtd/parsers/ofpart_core.c             |  6 ++
 drivers/of/base.c                             |  1 +
 8 files changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
 create mode 100644 drivers/mtd/parsers/ofpart_airoha.c
 create mode 100644 drivers/mtd/parsers/ofpart_airoha.h

-- 
2.45.2

[PATCH 1/3] of: also export of_update_property

[Christian Marangi]

Similar to how is done for of_add_property and of_remove_property, also
export of_update_property to permit kmod to use this additional function
and correctly update entry in DT.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/of/base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20603d3c9931..b2d523cf4925 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1746,6 +1746,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(of_update_property);
 
 static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 			 int id, const char *stem, int stem_len)
-- 
2.45.2

[PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

Add Documentation for Airoha fixed-partitions compatibles.

Airoha based SoC declare a dedicated partition at the end of the flash to
store calibration and device specific data, in addition to fixed
partitions.

The offset of this special partition is not well defined as a custom bad
block management driver is used that reserve space at the end of the flash.

This binding allows defining all fixed partitions and marking the last one
to detect the correct offset.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
 .../bindings/mtd/partitions/partitions.yaml   |  1 +
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
new file mode 100644
index 000000000000..a45df51065af
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha SoC partitioning
+
+description: |
+  Airoha based SoC declare a dedicated partition at the end of the flash to
+  store calibration and device specific data, in addition to fixed partitions.
+
+  The offset of this special partition is not well defined as a custom bad block
+  management driver is used that reserve space at the end of the flash.
+
+  This binding allows defining all fixed partitions and marking the last one to
+  detect the correct offset from the new end of the flash.
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+
+select: false
+
+properties:
+  compatible:
+    const: airoha,fixed-partitions
+
+  "#address-cells":
+    enum: [ 1, 2 ]
+
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+patternProperties:
+  "^partition@[0-9a-f]+$":
+    $ref: partition.yaml#
+    properties:
+      compatible:
+        const: airoha,dynamic-art
+    unevaluatedProperties: false
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    partitions {
+        compatible = "airoha,fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+          label = "bootloader";
+          reg = <0x00000000 0x00080000>;
+        };
+
+        partition@80000 {
+          label = "tclinux";
+          reg = <0x00080000 0x02800000>;
+        };
+
+        partition@2880000 {
+          label = "tclinux_slave";
+          reg = <0x02880000 0x02800000>;
+        };
+
+        partition@5080000 {
+          label = "rootfs_data";
+          reg = <0x5080000 0x00800000>;
+        };
+
+        partition@ffffffff {
+          compatible = "airoha,dynamic-art";
+          label = "art";
+          reg = <0xffffffff 0x00300000>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
index 1dda2c80747b..ec254e03adf0 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
@@ -14,6 +14,7 @@ maintainers:
   - Miquel Raynal <miquel.raynal@bootlin.com>
 
 oneOf:
+  - $ref: airoha,fixed-partitions.yaml
   - $ref: arm,arm-firmware-suite.yaml
   - $ref: brcm,bcm4908-partitions.yaml
   - $ref: brcm,bcm947xx-cfe-partitions.yaml
-- 
2.45.2

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:

> Add Documentation for Airoha fixed-partitions compatibles.
> 
> Airoha based SoC declare a dedicated partition at the end of the flash to
> store calibration and device specific data, in addition to fixed
> partitions.
> 
> The offset of this special partition is not well defined as a custom bad
> block management driver is used that reserve space at the end of the flash.
> 
> This binding allows defining all fixed partitions and marking the last one
> to detect the correct offset.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
>  .../bindings/mtd/partitions/partitions.yaml   |  1 +
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> new file mode 100644
> index 000000000000..a45df51065af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha SoC partitioning
> +
> +description: |
> +  Airoha based SoC declare a dedicated partition at the end of the flash to
> +  store calibration and device specific data, in addition to fixed partitions.
> +
> +  The offset of this special partition is not well defined as a custom bad block
> +  management driver is used that reserve space at the end of the flash.
> +
> +  This binding allows defining all fixed partitions and marking the last one to
> +  detect the correct offset from the new end of the flash.
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +select: false
> +
> +properties:
> +  compatible:
> +    const: airoha,fixed-partitions
> +
> +  "#address-cells":
> +    enum: [ 1, 2 ]
> +
> +  "#size-cells":
> +    enum: [ 1, 2 ]
> +
> +patternProperties:
> +  "^partition@[0-9a-f]+$":
> +    $ref: partition.yaml#
> +    properties:
> +      compatible:
> +        const: airoha,dynamic-art
> +    unevaluatedProperties: false
> +
> +required:
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    partitions {
> +        compatible = "airoha,fixed-partitions";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        partition@0 {
> +          label = "bootloader";
> +          reg = <0x00000000 0x00080000>;
> +        };
> +
> +        partition@80000 {
> +          label = "tclinux";
> +          reg = <0x00080000 0x02800000>;
> +        };
> +
> +        partition@2880000 {
> +          label = "tclinux_slave";
> +          reg = <0x02880000 0x02800000>;
> +        };
> +
> +        partition@5080000 {
> +          label = "rootfs_data";
> +          reg = <0x5080000 0x00800000>;
> +        };
> +
> +        partition@ffffffff {
> +          compatible = "airoha,dynamic-art";
> +          label = "art";
> +          reg = <0xffffffff 0x00300000>;

I'm a little bit puzzled by this kind of information which is known to
be wrong. As the partition offset and size must be dynamically
calculated, this reg property (as well as the size parameter of the
previous one) are notably wrong. I guess we are not fully constrained
by the fixed-partitions schema here, so could we avoid the reg property
in the airoha,dynamic-art partition? Maybe we also need a #define for a
specific placeholder in the penultimate reg property too (for the size).

Thanks,
Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> 
> > Add Documentation for Airoha fixed-partitions compatibles.
> > 
> > Airoha based SoC declare a dedicated partition at the end of the flash to
> > store calibration and device specific data, in addition to fixed
> > partitions.
> > 
> > The offset of this special partition is not well defined as a custom bad
> > block management driver is used that reserve space at the end of the flash.
> > 
> > This binding allows defining all fixed partitions and marking the last one
> > to detect the correct offset.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> >  2 files changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > new file mode 100644
> > index 000000000000..a45df51065af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Airoha SoC partitioning
> > +
> > +description: |
> > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > +  store calibration and device specific data, in addition to fixed partitions.
> > +
> > +  The offset of this special partition is not well defined as a custom bad block
> > +  management driver is used that reserve space at the end of the flash.
> > +
> > +  This binding allows defining all fixed partitions and marking the last one to
> > +  detect the correct offset from the new end of the flash.
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +select: false
> > +
> > +properties:
> > +  compatible:
> > +    const: airoha,fixed-partitions
> > +
> > +  "#address-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  "#size-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +patternProperties:
> > +  "^partition@[0-9a-f]+$":
> > +    $ref: partition.yaml#
> > +    properties:
> > +      compatible:
> > +        const: airoha,dynamic-art
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    partitions {
> > +        compatible = "airoha,fixed-partitions";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        partition@0 {
> > +          label = "bootloader";
> > +          reg = <0x00000000 0x00080000>;
> > +        };
> > +
> > +        partition@80000 {
> > +          label = "tclinux";
> > +          reg = <0x00080000 0x02800000>;
> > +        };
> > +
> > +        partition@2880000 {
> > +          label = "tclinux_slave";
> > +          reg = <0x02880000 0x02800000>;
> > +        };
> > +
> > +        partition@5080000 {
> > +          label = "rootfs_data";
> > +          reg = <0x5080000 0x00800000>;
> > +        };
> > +
> > +        partition@ffffffff {
> > +          compatible = "airoha,dynamic-art";
> > +          label = "art";
> > +          reg = <0xffffffff 0x00300000>;
> 
> I'm a little bit puzzled by this kind of information which is known to
> be wrong. As the partition offset and size must be dynamically
> calculated, this reg property (as well as the size parameter of the
> previous one) are notably wrong. I guess we are not fully constrained
> by the fixed-partitions schema here, so could we avoid the reg property
> in the airoha,dynamic-art partition? Maybe we also need a #define for a
> specific placeholder in the penultimate reg property too (for the size).
>

Maybe instead of reg we can use a property like size?

Can you better elaborate the suggestion about the #define?

Do you mean for case where the last partition might overlap
with the penultimate? Honestly in such case I would error hard, that
case happen when too much space is reserved and that is a
misconfiguration of the system (developer error)

-- 
	Ansuel

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:

> On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:
> > Hi Christian,
> > 
> > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> >   
> > > Add Documentation for Airoha fixed-partitions compatibles.
> > > 
> > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > store calibration and device specific data, in addition to fixed
> > > partitions.
> > > 
> > > The offset of this special partition is not well defined as a custom bad
> > > block management driver is used that reserve space at the end of the flash.
> > > 
> > > This binding allows defining all fixed partitions and marking the last one
> > > to detect the correct offset.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > >  2 files changed, 81 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > new file mode 100644
> > > index 000000000000..a45df51065af
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Airoha SoC partitioning
> > > +
> > > +description: |
> > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > +  store calibration and device specific data, in addition to fixed partitions.
> > > +
> > > +  The offset of this special partition is not well defined as a custom bad block
> > > +  management driver is used that reserve space at the end of the flash.
> > > +
> > > +  This binding allows defining all fixed partitions and marking the last one to
> > > +  detect the correct offset from the new end of the flash.
> > > +
> > > +maintainers:
> > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > +
> > > +select: false
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: airoha,fixed-partitions
> > > +
> > > +  "#address-cells":
> > > +    enum: [ 1, 2 ]
> > > +
> > > +  "#size-cells":
> > > +    enum: [ 1, 2 ]
> > > +
> > > +patternProperties:
> > > +  "^partition@[0-9a-f]+$":
> > > +    $ref: partition.yaml#
> > > +    properties:
> > > +      compatible:
> > > +        const: airoha,dynamic-art
> > > +    unevaluatedProperties: false
> > > +
> > > +required:
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    partitions {
> > > +        compatible = "airoha,fixed-partitions";
> > > +        #address-cells = <1>;
> > > +        #size-cells = <1>;
> > > +
> > > +        partition@0 {
> > > +          label = "bootloader";
> > > +          reg = <0x00000000 0x00080000>;
> > > +        };
> > > +
> > > +        partition@80000 {
> > > +          label = "tclinux";
> > > +          reg = <0x00080000 0x02800000>;
> > > +        };
> > > +
> > > +        partition@2880000 {
> > > +          label = "tclinux_slave";
> > > +          reg = <0x02880000 0x02800000>;
> > > +        };
> > > +
> > > +        partition@5080000 {
> > > +          label = "rootfs_data";
> > > +          reg = <0x5080000 0x00800000>;
> > > +        };
> > > +
> > > +        partition@ffffffff {
> > > +          compatible = "airoha,dynamic-art";
> > > +          label = "art";
> > > +          reg = <0xffffffff 0x00300000>;  
> > 
> > I'm a little bit puzzled by this kind of information which is known to
> > be wrong. As the partition offset and size must be dynamically
> > calculated, this reg property (as well as the size parameter of the
> > previous one) are notably wrong. I guess we are not fully constrained
> > by the fixed-partitions schema here, so could we avoid the reg property
> > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > specific placeholder in the penultimate reg property too (for the size).
> >  
> 
> Maybe instead of reg we can use a property like size?
> 
> Can you better elaborate the suggestion about the #define?
> 
> Do you mean for case where the last partition might overlap
> with the penultimate? Honestly in such case I would error hard, that
> case happen when too much space is reserved and that is a
> misconfiguration of the system (developer error)

That's not what I mean.

In the above case you say partition "partition@5080000" is 0x800000
bytes long. This is obviously wrong otherwise you would know where the
art partition starts. And right after you're saying partition
"partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
This is also wrong because 0xffffffff is not a valid start address and
IIUC 0x300000 is also unknown and dynamically derived.

So for the art partition my advise if you know nothing about the
start/length is to just skip the reg property. For the previous
partition I'd maybe use a definition (whose name is to discuss) instead
of the wrong size argument (the start offset being correct on his side).

Thanks, Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

On Wed, Sep 25, 2024 at 01:52:56PM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:
> 
> > On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:
> > > Hi Christian,
> > > 
> > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> > >   
> > > > Add Documentation for Airoha fixed-partitions compatibles.
> > > > 
> > > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > store calibration and device specific data, in addition to fixed
> > > > partitions.
> > > > 
> > > > The offset of this special partition is not well defined as a custom bad
> > > > block management driver is used that reserve space at the end of the flash.
> > > > 
> > > > This binding allows defining all fixed partitions and marking the last one
> > > > to detect the correct offset.
> > > > 
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > > >  2 files changed, 81 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > new file mode 100644
> > > > index 000000000000..a45df51065af
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > @@ -0,0 +1,80 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Airoha SoC partitioning
> > > > +
> > > > +description: |
> > > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > +  store calibration and device specific data, in addition to fixed partitions.
> > > > +
> > > > +  The offset of this special partition is not well defined as a custom bad block
> > > > +  management driver is used that reserve space at the end of the flash.
> > > > +
> > > > +  This binding allows defining all fixed partitions and marking the last one to
> > > > +  detect the correct offset from the new end of the flash.
> > > > +
> > > > +maintainers:
> > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > +
> > > > +select: false
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: airoha,fixed-partitions
> > > > +
> > > > +  "#address-cells":
> > > > +    enum: [ 1, 2 ]
> > > > +
> > > > +  "#size-cells":
> > > > +    enum: [ 1, 2 ]
> > > > +
> > > > +patternProperties:
> > > > +  "^partition@[0-9a-f]+$":
> > > > +    $ref: partition.yaml#
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: airoha,dynamic-art
> > > > +    unevaluatedProperties: false
> > > > +
> > > > +required:
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    partitions {
> > > > +        compatible = "airoha,fixed-partitions";
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <1>;
> > > > +
> > > > +        partition@0 {
> > > > +          label = "bootloader";
> > > > +          reg = <0x00000000 0x00080000>;
> > > > +        };
> > > > +
> > > > +        partition@80000 {
> > > > +          label = "tclinux";
> > > > +          reg = <0x00080000 0x02800000>;
> > > > +        };
> > > > +
> > > > +        partition@2880000 {
> > > > +          label = "tclinux_slave";
> > > > +          reg = <0x02880000 0x02800000>;
> > > > +        };
> > > > +
> > > > +        partition@5080000 {
> > > > +          label = "rootfs_data";
> > > > +          reg = <0x5080000 0x00800000>;
> > > > +        };
> > > > +
> > > > +        partition@ffffffff {
> > > > +          compatible = "airoha,dynamic-art";
> > > > +          label = "art";
> > > > +          reg = <0xffffffff 0x00300000>;  
> > > 
> > > I'm a little bit puzzled by this kind of information which is known to
> > > be wrong. As the partition offset and size must be dynamically
> > > calculated, this reg property (as well as the size parameter of the
> > > previous one) are notably wrong. I guess we are not fully constrained
> > > by the fixed-partitions schema here, so could we avoid the reg property
> > > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > > specific placeholder in the penultimate reg property too (for the size).
> > >  
> > 
> > Maybe instead of reg we can use a property like size?
> > 
> > Can you better elaborate the suggestion about the #define?
> > 
> > Do you mean for case where the last partition might overlap
> > with the penultimate? Honestly in such case I would error hard, that
> > case happen when too much space is reserved and that is a
> > misconfiguration of the system (developer error)
> 
> That's not what I mean.
> 
> In the above case you say partition "partition@5080000" is 0x800000
> bytes long. This is obviously wrong otherwise you would know where the
> art partition starts. And right after you're saying partition
> "partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
> This is also wrong because 0xffffffff is not a valid start address and
> IIUC 0x300000 is also unknown and dynamically derived.
> 
> So for the art partition my advise if you know nothing about the
> start/length is to just skip the reg property. For the previous
> partition I'd maybe use a definition (whose name is to discuss) instead
> of the wrong size argument (the start offset being correct on his side).
>

Ok probably the description isn't clear enough. The missing info that
require this parser is the flash end.

Following the example we know the size of rootfs_data and start offset
AND we know the size of the ART partition.

There might be a space in the middle unused between the rootfs_data
partition and the art partition. What is derived is the starting offset
of the art partition that is flash end - art partition size.
(where flash end change and is not always the same due to how the special
bad block managament table reserved space is handled)

This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
runtime. On second tought tho maybe using this dummy offset is wrong and
I should just have something like

length = <0x300000>;

Is it clear now? Sorry for any confusion.

-- 
	Ansuel

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 14:06:11 +0200:

> On Wed, Sep 25, 2024 at 01:52:56PM +0200, Miquel Raynal wrote:
> > Hi Christian,
> > 
> > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:
> >   
> > > On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:  
> > > > Hi Christian,
> > > > 
> > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> > > >     
> > > > > Add Documentation for Airoha fixed-partitions compatibles.
> > > > > 
> > > > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > store calibration and device specific data, in addition to fixed
> > > > > partitions.
> > > > > 
> > > > > The offset of this special partition is not well defined as a custom bad
> > > > > block management driver is used that reserve space at the end of the flash.
> > > > > 
> > > > > This binding allows defining all fixed partitions and marking the last one
> > > > > to detect the correct offset.
> > > > > 
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > ---
> > > > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > > > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > > > >  2 files changed, 81 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..a45df51065af
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > @@ -0,0 +1,80 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Airoha SoC partitioning
> > > > > +
> > > > > +description: |
> > > > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > +  store calibration and device specific data, in addition to fixed partitions.
> > > > > +
> > > > > +  The offset of this special partition is not well defined as a custom bad block
> > > > > +  management driver is used that reserve space at the end of the flash.
> > > > > +
> > > > > +  This binding allows defining all fixed partitions and marking the last one to
> > > > > +  detect the correct offset from the new end of the flash.
> > > > > +
> > > > > +maintainers:
> > > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > > +
> > > > > +select: false
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: airoha,fixed-partitions
> > > > > +
> > > > > +  "#address-cells":
> > > > > +    enum: [ 1, 2 ]
> > > > > +
> > > > > +  "#size-cells":
> > > > > +    enum: [ 1, 2 ]
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^partition@[0-9a-f]+$":
> > > > > +    $ref: partition.yaml#
> > > > > +    properties:
> > > > > +      compatible:
> > > > > +        const: airoha,dynamic-art
> > > > > +    unevaluatedProperties: false
> > > > > +
> > > > > +required:
> > > > > +  - "#address-cells"
> > > > > +  - "#size-cells"
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    partitions {
> > > > > +        compatible = "airoha,fixed-partitions";
> > > > > +        #address-cells = <1>;
> > > > > +        #size-cells = <1>;
> > > > > +
> > > > > +        partition@0 {
> > > > > +          label = "bootloader";
> > > > > +          reg = <0x00000000 0x00080000>;
> > > > > +        };
> > > > > +
> > > > > +        partition@80000 {
> > > > > +          label = "tclinux";
> > > > > +          reg = <0x00080000 0x02800000>;
> > > > > +        };
> > > > > +
> > > > > +        partition@2880000 {
> > > > > +          label = "tclinux_slave";
> > > > > +          reg = <0x02880000 0x02800000>;
> > > > > +        };
> > > > > +
> > > > > +        partition@5080000 {
> > > > > +          label = "rootfs_data";
> > > > > +          reg = <0x5080000 0x00800000>;
> > > > > +        };
> > > > > +
> > > > > +        partition@ffffffff {
> > > > > +          compatible = "airoha,dynamic-art";
> > > > > +          label = "art";
> > > > > +          reg = <0xffffffff 0x00300000>;    
> > > > 
> > > > I'm a little bit puzzled by this kind of information which is known to
> > > > be wrong. As the partition offset and size must be dynamically
> > > > calculated, this reg property (as well as the size parameter of the
> > > > previous one) are notably wrong. I guess we are not fully constrained
> > > > by the fixed-partitions schema here, so could we avoid the reg property
> > > > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > > > specific placeholder in the penultimate reg property too (for the size).
> > > >    
> > > 
> > > Maybe instead of reg we can use a property like size?
> > > 
> > > Can you better elaborate the suggestion about the #define?
> > > 
> > > Do you mean for case where the last partition might overlap
> > > with the penultimate? Honestly in such case I would error hard, that
> > > case happen when too much space is reserved and that is a
> > > misconfiguration of the system (developer error)  
> > 
> > That's not what I mean.
> > 
> > In the above case you say partition "partition@5080000" is 0x800000
> > bytes long. This is obviously wrong otherwise you would know where the
> > art partition starts. And right after you're saying partition
> > "partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
> > This is also wrong because 0xffffffff is not a valid start address and
> > IIUC 0x300000 is also unknown and dynamically derived.
> > 
> > So for the art partition my advise if you know nothing about the
> > start/length is to just skip the reg property. For the previous
> > partition I'd maybe use a definition (whose name is to discuss) instead
> > of the wrong size argument (the start offset being correct on his side).
> >  
> 
> Ok probably the description isn't clear enough. The missing info that
> require this parser is the flash end.
> 
> Following the example we know the size of rootfs_data and start offset
> AND we know the size of the ART partition.
> 
> There might be a space in the middle unused between the rootfs_data
> partition and the art partition. What is derived is the starting offset
> of the art partition that is flash end - art partition size.
> (where flash end change and is not always the same due to how the special
> bad block managament table reserved space is handled)
> 
> This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> runtime. On second tought tho maybe using this dummy offset is wrong and
> I should just have something like
> 
> length = <0x300000>;
> 
> Is it clear now? Sorry for any confusion.

I'm sorry but not really. You know the end of the physical device and
the size of the ART partition, so you must know its start as well?

Thanks,
Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

On Mon, Sep 30, 2024 at 11:48:19AM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 14:06:11 +0200:
> 
> > On Wed, Sep 25, 2024 at 01:52:56PM +0200, Miquel Raynal wrote:
> > > Hi Christian,
> > > 
> > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:
> > >   
> > > > On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:  
> > > > > Hi Christian,
> > > > > 
> > > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> > > > >     
> > > > > > Add Documentation for Airoha fixed-partitions compatibles.
> > > > > > 
> > > > > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > store calibration and device specific data, in addition to fixed
> > > > > > partitions.
> > > > > > 
> > > > > > The offset of this special partition is not well defined as a custom bad
> > > > > > block management driver is used that reserve space at the end of the flash.
> > > > > > 
> > > > > > This binding allows defining all fixed partitions and marking the last one
> > > > > > to detect the correct offset.
> > > > > > 
> > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > ---
> > > > > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > > > > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > > > > >  2 files changed, 81 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..a45df51065af
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > @@ -0,0 +1,80 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Airoha SoC partitioning
> > > > > > +
> > > > > > +description: |
> > > > > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > +  store calibration and device specific data, in addition to fixed partitions.
> > > > > > +
> > > > > > +  The offset of this special partition is not well defined as a custom bad block
> > > > > > +  management driver is used that reserve space at the end of the flash.
> > > > > > +
> > > > > > +  This binding allows defining all fixed partitions and marking the last one to
> > > > > > +  detect the correct offset from the new end of the flash.
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > > > +
> > > > > > +select: false
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: airoha,fixed-partitions
> > > > > > +
> > > > > > +  "#address-cells":
> > > > > > +    enum: [ 1, 2 ]
> > > > > > +
> > > > > > +  "#size-cells":
> > > > > > +    enum: [ 1, 2 ]
> > > > > > +
> > > > > > +patternProperties:
> > > > > > +  "^partition@[0-9a-f]+$":
> > > > > > +    $ref: partition.yaml#
> > > > > > +    properties:
> > > > > > +      compatible:
> > > > > > +        const: airoha,dynamic-art
> > > > > > +    unevaluatedProperties: false
> > > > > > +
> > > > > > +required:
> > > > > > +  - "#address-cells"
> > > > > > +  - "#size-cells"
> > > > > > +
> > > > > > +additionalProperties: false
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    partitions {
> > > > > > +        compatible = "airoha,fixed-partitions";
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <1>;
> > > > > > +
> > > > > > +        partition@0 {
> > > > > > +          label = "bootloader";
> > > > > > +          reg = <0x00000000 0x00080000>;
> > > > > > +        };
> > > > > > +
> > > > > > +        partition@80000 {
> > > > > > +          label = "tclinux";
> > > > > > +          reg = <0x00080000 0x02800000>;
> > > > > > +        };
> > > > > > +
> > > > > > +        partition@2880000 {
> > > > > > +          label = "tclinux_slave";
> > > > > > +          reg = <0x02880000 0x02800000>;
> > > > > > +        };
> > > > > > +
> > > > > > +        partition@5080000 {
> > > > > > +          label = "rootfs_data";
> > > > > > +          reg = <0x5080000 0x00800000>;
> > > > > > +        };
> > > > > > +
> > > > > > +        partition@ffffffff {
> > > > > > +          compatible = "airoha,dynamic-art";
> > > > > > +          label = "art";
> > > > > > +          reg = <0xffffffff 0x00300000>;    
> > > > > 
> > > > > I'm a little bit puzzled by this kind of information which is known to
> > > > > be wrong. As the partition offset and size must be dynamically
> > > > > calculated, this reg property (as well as the size parameter of the
> > > > > previous one) are notably wrong. I guess we are not fully constrained
> > > > > by the fixed-partitions schema here, so could we avoid the reg property
> > > > > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > > > > specific placeholder in the penultimate reg property too (for the size).
> > > > >    
> > > > 
> > > > Maybe instead of reg we can use a property like size?
> > > > 
> > > > Can you better elaborate the suggestion about the #define?
> > > > 
> > > > Do you mean for case where the last partition might overlap
> > > > with the penultimate? Honestly in such case I would error hard, that
> > > > case happen when too much space is reserved and that is a
> > > > misconfiguration of the system (developer error)  
> > > 
> > > That's not what I mean.
> > > 
> > > In the above case you say partition "partition@5080000" is 0x800000
> > > bytes long. This is obviously wrong otherwise you would know where the
> > > art partition starts. And right after you're saying partition
> > > "partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
> > > This is also wrong because 0xffffffff is not a valid start address and
> > > IIUC 0x300000 is also unknown and dynamically derived.
> > > 
> > > So for the art partition my advise if you know nothing about the
> > > start/length is to just skip the reg property. For the previous
> > > partition I'd maybe use a definition (whose name is to discuss) instead
> > > of the wrong size argument (the start offset being correct on his side).
> > >  
> > 
> > Ok probably the description isn't clear enough. The missing info that
> > require this parser is the flash end.
> > 
> > Following the example we know the size of rootfs_data and start offset
> > AND we know the size of the ART partition.
> > 
> > There might be a space in the middle unused between the rootfs_data
> > partition and the art partition. What is derived is the starting offset
> > of the art partition that is flash end - art partition size.
> > (where flash end change and is not always the same due to how the special
> > bad block managament table reserved space is handled)
> > 
> > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > runtime. On second tought tho maybe using this dummy offset is wrong and
> > I should just have something like
> > 
> > length = <0x300000>;
> > 
> > Is it clear now? Sorry for any confusion.
> 
> I'm sorry but not really. You know the end of the physical device and
> the size of the ART partition, so you must know its start as well?
>

Before the system boot we know:
- size of the ART partition
- real size of the physical device (512mb... 1G... 64mb...)

When the physical device is probed (nand) a special driver is loaded
(before mtd parsing logic) that change the physical size of the device
(mtd->size) as at the end of the nand some space is reserved for bad
block management and other metadata info.

So on the mtd parsing logic we know:
- size of the ART partitiomn
- new size of the physical device (512-reserved space...)

And we calculate the start offset of the ART partition.

It's very difficult to know what is the new size of the physical device
after the driver change it as it might change based on the internal
configuration of the driver itself.

-- 
	Ansuel

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Mon, 30 Sep 2024 12:10:21 +0200:

> On Mon, Sep 30, 2024 at 11:48:19AM +0200, Miquel Raynal wrote:
> > Hi Christian,
> > 
> > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 14:06:11 +0200:
> >   
> > > On Wed, Sep 25, 2024 at 01:52:56PM +0200, Miquel Raynal wrote:  
> > > > Hi Christian,
> > > > 
> > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:
> > > >     
> > > > > On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:    
> > > > > > Hi Christian,
> > > > > > 
> > > > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> > > > > >       
> > > > > > > Add Documentation for Airoha fixed-partitions compatibles.
> > > > > > > 
> > > > > > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > > store calibration and device specific data, in addition to fixed
> > > > > > > partitions.
> > > > > > > 
> > > > > > > The offset of this special partition is not well defined as a custom bad
> > > > > > > block management driver is used that reserve space at the end of the flash.
> > > > > > > 
> > > > > > > This binding allows defining all fixed partitions and marking the last one
> > > > > > > to detect the correct offset.
> > > > > > > 
> > > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > > ---
> > > > > > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > > > > > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > > > > > >  2 files changed, 81 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..a45df51065af
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > @@ -0,0 +1,80 @@
> > > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Airoha SoC partitioning
> > > > > > > +
> > > > > > > +description: |
> > > > > > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > > +  store calibration and device specific data, in addition to fixed partitions.
> > > > > > > +
> > > > > > > +  The offset of this special partition is not well defined as a custom bad block
> > > > > > > +  management driver is used that reserve space at the end of the flash.
> > > > > > > +
> > > > > > > +  This binding allows defining all fixed partitions and marking the last one to
> > > > > > > +  detect the correct offset from the new end of the flash.
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > > > > +
> > > > > > > +select: false
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: airoha,fixed-partitions
> > > > > > > +
> > > > > > > +  "#address-cells":
> > > > > > > +    enum: [ 1, 2 ]
> > > > > > > +
> > > > > > > +  "#size-cells":
> > > > > > > +    enum: [ 1, 2 ]
> > > > > > > +
> > > > > > > +patternProperties:
> > > > > > > +  "^partition@[0-9a-f]+$":
> > > > > > > +    $ref: partition.yaml#
> > > > > > > +    properties:
> > > > > > > +      compatible:
> > > > > > > +        const: airoha,dynamic-art
> > > > > > > +    unevaluatedProperties: false
> > > > > > > +
> > > > > > > +required:
> > > > > > > +  - "#address-cells"
> > > > > > > +  - "#size-cells"
> > > > > > > +
> > > > > > > +additionalProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > +  - |
> > > > > > > +    partitions {
> > > > > > > +        compatible = "airoha,fixed-partitions";
> > > > > > > +        #address-cells = <1>;
> > > > > > > +        #size-cells = <1>;
> > > > > > > +
> > > > > > > +        partition@0 {
> > > > > > > +          label = "bootloader";
> > > > > > > +          reg = <0x00000000 0x00080000>;
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        partition@80000 {
> > > > > > > +          label = "tclinux";
> > > > > > > +          reg = <0x00080000 0x02800000>;
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        partition@2880000 {
> > > > > > > +          label = "tclinux_slave";
> > > > > > > +          reg = <0x02880000 0x02800000>;
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        partition@5080000 {
> > > > > > > +          label = "rootfs_data";
> > > > > > > +          reg = <0x5080000 0x00800000>;
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        partition@ffffffff {
> > > > > > > +          compatible = "airoha,dynamic-art";
> > > > > > > +          label = "art";
> > > > > > > +          reg = <0xffffffff 0x00300000>;      
> > > > > > 
> > > > > > I'm a little bit puzzled by this kind of information which is known to
> > > > > > be wrong. As the partition offset and size must be dynamically
> > > > > > calculated, this reg property (as well as the size parameter of the
> > > > > > previous one) are notably wrong. I guess we are not fully constrained
> > > > > > by the fixed-partitions schema here, so could we avoid the reg property
> > > > > > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > > > > > specific placeholder in the penultimate reg property too (for the size).
> > > > > >      
> > > > > 
> > > > > Maybe instead of reg we can use a property like size?
> > > > > 
> > > > > Can you better elaborate the suggestion about the #define?
> > > > > 
> > > > > Do you mean for case where the last partition might overlap
> > > > > with the penultimate? Honestly in such case I would error hard, that
> > > > > case happen when too much space is reserved and that is a
> > > > > misconfiguration of the system (developer error)    
> > > > 
> > > > That's not what I mean.
> > > > 
> > > > In the above case you say partition "partition@5080000" is 0x800000
> > > > bytes long. This is obviously wrong otherwise you would know where the
> > > > art partition starts. And right after you're saying partition
> > > > "partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
> > > > This is also wrong because 0xffffffff is not a valid start address and
> > > > IIUC 0x300000 is also unknown and dynamically derived.
> > > > 
> > > > So for the art partition my advise if you know nothing about the
> > > > start/length is to just skip the reg property. For the previous
> > > > partition I'd maybe use a definition (whose name is to discuss) instead
> > > > of the wrong size argument (the start offset being correct on his side).
> > > >    
> > > 
> > > Ok probably the description isn't clear enough. The missing info that
> > > require this parser is the flash end.
> > > 
> > > Following the example we know the size of rootfs_data and start offset
> > > AND we know the size of the ART partition.
> > > 
> > > There might be a space in the middle unused between the rootfs_data
> > > partition and the art partition. What is derived is the starting offset
> > > of the art partition that is flash end - art partition size.
> > > (where flash end change and is not always the same due to how the special
> > > bad block managament table reserved space is handled)
> > > 
> > > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > > runtime. On second tought tho maybe using this dummy offset is wrong and
> > > I should just have something like
> > > 
> > > length = <0x300000>;
> > > 
> > > Is it clear now? Sorry for any confusion.  
> > 
> > I'm sorry but not really. You know the end of the physical device and
> > the size of the ART partition, so you must know its start as well?
> >  
> 
> Before the system boot we know:
> - size of the ART partition
> - real size of the physical device (512mb... 1G... 64mb...)
> 
> When the physical device is probed (nand) a special driver is loaded
> (before mtd parsing logic) that change the physical size of the device
> (mtd->size) as at the end of the nand some space is reserved for bad
> block management and other metadata info.

Here you are explaining what you intend Linux to do, right? I would
like to understand what you are trying to solve. I dont understand why
you need the size change, I don't understand why you don't know the
start of the ART partition, I don't understand what the data you are
hiding contains and who uses it :-) I'm sorry, this is too unclear yet.

Quoting your cover letter:

	"This require dynamic calculation of the offset as some
	dedicated driver for bad block management might be used that
	reserve some space at the end of the flash for bad block
	accounting. This special driver change the end offset of the
	flash hence a dynamic parser is needed."

I don't know what this "dedicated driver" is, I don't understand why it
is needed.

> So on the mtd parsing logic we know:
> - size of the ART partitiomn
> - new size of the physical device (512-reserved space...)
> 
> And we calculate the start offset of the ART partition.
> 
> It's very difficult to know what is the new size of the physical device
> after the driver change it as it might change based on the internal
> configuration of the driver itself.

Thanks,
Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

On Tue, Oct 01, 2024 at 10:42:25AM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> ansuelsmth@gmail.com wrote on Mon, 30 Sep 2024 12:10:21 +0200:
> 
> > On Mon, Sep 30, 2024 at 11:48:19AM +0200, Miquel Raynal wrote:
> > > Hi Christian,
> > > 
> > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 14:06:11 +0200:
> > >   
> > > > On Wed, Sep 25, 2024 at 01:52:56PM +0200, Miquel Raynal wrote:  
> > > > > Hi Christian,
> > > > > 
> > > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 13:35:38 +0200:
> > > > >     
> > > > > > On Wed, Sep 25, 2024 at 01:30:03PM +0200, Miquel Raynal wrote:    
> > > > > > > Hi Christian,
> > > > > > > 
> > > > > > > ansuelsmth@gmail.com wrote on Wed, 25 Sep 2024 12:13:58 +0200:
> > > > > > >       
> > > > > > > > Add Documentation for Airoha fixed-partitions compatibles.
> > > > > > > > 
> > > > > > > > Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > > > store calibration and device specific data, in addition to fixed
> > > > > > > > partitions.
> > > > > > > > 
> > > > > > > > The offset of this special partition is not well defined as a custom bad
> > > > > > > > block management driver is used that reserve space at the end of the flash.
> > > > > > > > 
> > > > > > > > This binding allows defining all fixed partitions and marking the last one
> > > > > > > > to detect the correct offset.
> > > > > > > > 
> > > > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > > > ---
> > > > > > > >  .../partitions/airoha,fixed-partitions.yaml   | 80 +++++++++++++++++++
> > > > > > > >  .../bindings/mtd/partitions/partitions.yaml   |  1 +
> > > > > > > >  2 files changed, 81 insertions(+)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..a45df51065af
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/airoha,fixed-partitions.yaml
> > > > > > > > @@ -0,0 +1,80 @@
> > > > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > > > +%YAML 1.2
> > > > > > > > +---
> > > > > > > > +$id: http://devicetree.org/schemas/mtd/partitions/airoha,fixed-partitions.yaml#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: Airoha SoC partitioning
> > > > > > > > +
> > > > > > > > +description: |
> > > > > > > > +  Airoha based SoC declare a dedicated partition at the end of the flash to
> > > > > > > > +  store calibration and device specific data, in addition to fixed partitions.
> > > > > > > > +
> > > > > > > > +  The offset of this special partition is not well defined as a custom bad block
> > > > > > > > +  management driver is used that reserve space at the end of the flash.
> > > > > > > > +
> > > > > > > > +  This binding allows defining all fixed partitions and marking the last one to
> > > > > > > > +  detect the correct offset from the new end of the flash.
> > > > > > > > +
> > > > > > > > +maintainers:
> > > > > > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > > > > > +
> > > > > > > > +select: false
> > > > > > > > +
> > > > > > > > +properties:
> > > > > > > > +  compatible:
> > > > > > > > +    const: airoha,fixed-partitions
> > > > > > > > +
> > > > > > > > +  "#address-cells":
> > > > > > > > +    enum: [ 1, 2 ]
> > > > > > > > +
> > > > > > > > +  "#size-cells":
> > > > > > > > +    enum: [ 1, 2 ]
> > > > > > > > +
> > > > > > > > +patternProperties:
> > > > > > > > +  "^partition@[0-9a-f]+$":
> > > > > > > > +    $ref: partition.yaml#
> > > > > > > > +    properties:
> > > > > > > > +      compatible:
> > > > > > > > +        const: airoha,dynamic-art
> > > > > > > > +    unevaluatedProperties: false
> > > > > > > > +
> > > > > > > > +required:
> > > > > > > > +  - "#address-cells"
> > > > > > > > +  - "#size-cells"
> > > > > > > > +
> > > > > > > > +additionalProperties: false
> > > > > > > > +
> > > > > > > > +examples:
> > > > > > > > +  - |
> > > > > > > > +    partitions {
> > > > > > > > +        compatible = "airoha,fixed-partitions";
> > > > > > > > +        #address-cells = <1>;
> > > > > > > > +        #size-cells = <1>;
> > > > > > > > +
> > > > > > > > +        partition@0 {
> > > > > > > > +          label = "bootloader";
> > > > > > > > +          reg = <0x00000000 0x00080000>;
> > > > > > > > +        };
> > > > > > > > +
> > > > > > > > +        partition@80000 {
> > > > > > > > +          label = "tclinux";
> > > > > > > > +          reg = <0x00080000 0x02800000>;
> > > > > > > > +        };
> > > > > > > > +
> > > > > > > > +        partition@2880000 {
> > > > > > > > +          label = "tclinux_slave";
> > > > > > > > +          reg = <0x02880000 0x02800000>;
> > > > > > > > +        };
> > > > > > > > +
> > > > > > > > +        partition@5080000 {
> > > > > > > > +          label = "rootfs_data";
> > > > > > > > +          reg = <0x5080000 0x00800000>;
> > > > > > > > +        };
> > > > > > > > +
> > > > > > > > +        partition@ffffffff {
> > > > > > > > +          compatible = "airoha,dynamic-art";
> > > > > > > > +          label = "art";
> > > > > > > > +          reg = <0xffffffff 0x00300000>;      
> > > > > > > 
> > > > > > > I'm a little bit puzzled by this kind of information which is known to
> > > > > > > be wrong. As the partition offset and size must be dynamically
> > > > > > > calculated, this reg property (as well as the size parameter of the
> > > > > > > previous one) are notably wrong. I guess we are not fully constrained
> > > > > > > by the fixed-partitions schema here, so could we avoid the reg property
> > > > > > > in the airoha,dynamic-art partition? Maybe we also need a #define for a
> > > > > > > specific placeholder in the penultimate reg property too (for the size).
> > > > > > >      
> > > > > > 
> > > > > > Maybe instead of reg we can use a property like size?
> > > > > > 
> > > > > > Can you better elaborate the suggestion about the #define?
> > > > > > 
> > > > > > Do you mean for case where the last partition might overlap
> > > > > > with the penultimate? Honestly in such case I would error hard, that
> > > > > > case happen when too much space is reserved and that is a
> > > > > > misconfiguration of the system (developer error)    
> > > > > 
> > > > > That's not what I mean.
> > > > > 
> > > > > In the above case you say partition "partition@5080000" is 0x800000
> > > > > bytes long. This is obviously wrong otherwise you would know where the
> > > > > art partition starts. And right after you're saying partition
> > > > > "partition@ffffffff" starts at 0xffffffff and is 0x300000 bytes long.
> > > > > This is also wrong because 0xffffffff is not a valid start address and
> > > > > IIUC 0x300000 is also unknown and dynamically derived.
> > > > > 
> > > > > So for the art partition my advise if you know nothing about the
> > > > > start/length is to just skip the reg property. For the previous
> > > > > partition I'd maybe use a definition (whose name is to discuss) instead
> > > > > of the wrong size argument (the start offset being correct on his side).
> > > > >    
> > > > 
> > > > Ok probably the description isn't clear enough. The missing info that
> > > > require this parser is the flash end.
> > > > 
> > > > Following the example we know the size of rootfs_data and start offset
> > > > AND we know the size of the ART partition.
> > > > 
> > > > There might be a space in the middle unused between the rootfs_data
> > > > partition and the art partition. What is derived is the starting offset
> > > > of the art partition that is flash end - art partition size.
> > > > (where flash end change and is not always the same due to how the special
> > > > bad block managament table reserved space is handled)
> > > > 
> > > > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > > > runtime. On second tought tho maybe using this dummy offset is wrong and
> > > > I should just have something like
> > > > 
> > > > length = <0x300000>;
> > > > 
> > > > Is it clear now? Sorry for any confusion.  
> > > 
> > > I'm sorry but not really. You know the end of the physical device and
> > > the size of the ART partition, so you must know its start as well?
> > >  
> > 
> > Before the system boot we know:
> > - size of the ART partition
> > - real size of the physical device (512mb... 1G... 64mb...)
> > 
> > When the physical device is probed (nand) a special driver is loaded
> > (before mtd parsing logic) that change the physical size of the device
> > (mtd->size) as at the end of the nand some space is reserved for bad
> > block management and other metadata info.
> 
> Here you are explaining what you intend Linux to do, right? I would
> like to understand what you are trying to solve. I dont understand why
> you need the size change, I don't understand why you don't know the
> start of the ART partition, I don't understand what the data you are
> hiding contains and who uses it :-) I'm sorry, this is too unclear yet.

Totally not a problem and thanks a lot for you keep asking them... More
than happy to clear things, I'm trying to solve a problem present on
Airoha SoC and upstreaming a correct parser for it.

What I'm trying to solve:

Correct access to this partition at the end of the flash in an automated
way.

The content of this partition is the usual ART partition found on lots of
embedded devices. MAC address, wifi calibration data, serial. Usage is
NVMEM cells and userspace with dd command to extract data from.

Airoha use something also used by some mediatek SoC. They call it BMT
and it's currently used downstream in OpenWrt and they firmware. This is
also used in the bootloader.

The usage of BMT is a custom way to handle bad blocks entirely by
software. At the end of the flash some space is reserved where info
about all the blocks of the flash are put. I'm not 100% sure about the
functionality of this but it can relocate block and do magic things to
handle bad blocks. For the scope of this change, the important info is
that after the BMT is probed, the operation of "reserving space" is done
by reducing the MTD flash size. So from the MTD subsystem, it does see a
smaller flash than it actually is.

The reserved space change! Across SoC or even devices but the BMT is a
must where it's used as bootloader makes use of it and writing to it
might confuse the bootloader corrupting data. (one block might be
flagged as bad ad data moved, BMT driver validates his table and do
operation)

We discover this the hard way at times with firmware getting corrupted
on upgrading it.

The intention of this parser is to handle this problem transparently and
easier.

The Airoha partition scheme always follow this logic:
- bootloader
- fit image (kernel+rootfs)
- fit image backup (kernel+rootfs) (optional)
- rootfs_data
- opaque data (no partition)
- ART partition (end of partition = start of reserved BMT)
- BMT reserved space
- end of flash

What I'm trying to solve is making it easy to calculate the offset of
the partition written before the BMT reserved space.

Feel free to ask more question about this and again thanks the help in
figuring this out.

> 
> Quoting your cover letter:
> 
> 	"This require dynamic calculation of the offset as some
> 	dedicated driver for bad block management might be used that
> 	reserve some space at the end of the flash for bad block
> 	accounting. This special driver change the end offset of the
> 	flash hence a dynamic parser is needed."
> 
> I don't know what this "dedicated driver" is, I don't understand why it
> is needed.

I hope it's clear what is the usage of this driver now. (In short a
software way to handle bad block from Mediatek that propagated to Airoha)

> 
> > So on the mtd parsing logic we know:
> > - size of the ART partitiomn
> > - new size of the physical device (512-reserved space...)
> > 
> > And we calculate the start offset of the ART partition.
> > 
> > It's very difficult to know what is the new size of the physical device
> > after the driver change it as it might change based on the internal
> > configuration of the driver itself.
> 
> Thanks,
> Miquèl

-- 
	Ansuel

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

> > > > > Ok probably the description isn't clear enough. The missing info that
> > > > > require this parser is the flash end.
> > > > > 
> > > > > Following the example we know the size of rootfs_data and start offset
> > > > > AND we know the size of the ART partition.
> > > > > 
> > > > > There might be a space in the middle unused between the rootfs_data
> > > > > partition and the art partition. What is derived is the starting offset
> > > > > of the art partition that is flash end - art partition size.
> > > > > (where flash end change and is not always the same due to how the special
> > > > > bad block managament table reserved space is handled)
> > > > > 
> > > > > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > > > > runtime. On second tought tho maybe using this dummy offset is wrong and
> > > > > I should just have something like
> > > > > 
> > > > > length = <0x300000>;
> > > > > 
> > > > > Is it clear now? Sorry for any confusion.    
> > > > 
> > > > I'm sorry but not really. You know the end of the physical device and
> > > > the size of the ART partition, so you must know its start as well?
> > > >    
> > > 
> > > Before the system boot we know:
> > > - size of the ART partition
> > > - real size of the physical device (512mb... 1G... 64mb...)
> > > 
> > > When the physical device is probed (nand) a special driver is loaded
> > > (before mtd parsing logic) that change the physical size of the device
> > > (mtd->size) as at the end of the nand some space is reserved for bad
> > > block management and other metadata info.  
> > 
> > Here you are explaining what you intend Linux to do, right? I would
> > like to understand what you are trying to solve. I dont understand why
> > you need the size change, I don't understand why you don't know the
> > start of the ART partition, I don't understand what the data you are
> > hiding contains and who uses it :-) I'm sorry, this is too unclear yet.  
> 
> Totally not a problem and thanks a lot for you keep asking them... More
> than happy to clear things, I'm trying to solve a problem present on
> Airoha SoC and upstreaming a correct parser for it.
> 
> What I'm trying to solve:
> 
> Correct access to this partition at the end of the flash in an automated
> way.
> 
> The content of this partition is the usual ART partition found on lots of
> embedded devices. MAC address, wifi calibration data, serial. Usage is
> NVMEM cells and userspace with dd command to extract data from.
> 
> Airoha use something also used by some mediatek SoC. They call it BMT
> and it's currently used downstream in OpenWrt and they firmware. This is
> also used in the bootloader.
> 
> The usage of BMT is a custom way to handle bad blocks entirely by
> software. At the end of the flash some space is reserved where info
> about all the blocks of the flash are put. I'm not 100% sure about the
> functionality of this but it can relocate block and do magic things to
> handle bad blocks. For the scope of this change, the important info is
> that after the BMT is probed, the operation of "reserving space" is done
> by reducing the MTD flash size. So from the MTD subsystem, it does see a
> smaller flash than it actually is.
> 
> The reserved space change! Across SoC or even devices but the BMT is a
> must where it's used as bootloader makes use of it and writing to it
> might confuse the bootloader corrupting data. (one block might be
> flagged as bad ad data moved, BMT driver validates his table and do
> operation)

Ok, I think that's way clearer now.

So the BMT driver does not exist in mainline Linux, but you would like
to skip this part of the MTD device to avoid smashing it. And it is in
use by the vendor Bootloader I guess?

Is it some kind of table that is written by the chip itself in order to
maintain a list of auto-replacement blocks for bad blocks? Can the size
of this table move with the use of the device? (if yes, it's
problematic, we don't want to resize MTD partitions without noticing,
it would break eg. UBI).

I believe this BMT block is going against the bad block handling in
Linux, so I really wonder how one can use both mechanisms in a system.
If the BMT layer takes "one random block" to map a corrupted one on it,
it totally defeats the current bad block model we have in MTD/UBI
and simply cannot be supported at all. Just skipping the
currently-used-for-BMT blocks sounds like a very bad idea that will
break your system, later.

Thanks,
Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Christian Marangi]

On Wed, Oct 02, 2024 at 10:00:06AM +0200, Miquel Raynal wrote:
> Hi Christian,
> 
> > > > > > Ok probably the description isn't clear enough. The missing info that
> > > > > > require this parser is the flash end.
> > > > > > 
> > > > > > Following the example we know the size of rootfs_data and start offset
> > > > > > AND we know the size of the ART partition.
> > > > > > 
> > > > > > There might be a space in the middle unused between the rootfs_data
> > > > > > partition and the art partition. What is derived is the starting offset
> > > > > > of the art partition that is flash end - art partition size.
> > > > > > (where flash end change and is not always the same due to how the special
> > > > > > bad block managament table reserved space is handled)
> > > > > > 
> > > > > > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > > > > > runtime. On second tought tho maybe using this dummy offset is wrong and
> > > > > > I should just have something like
> > > > > > 
> > > > > > length = <0x300000>;
> > > > > > 
> > > > > > Is it clear now? Sorry for any confusion.    
> > > > > 
> > > > > I'm sorry but not really. You know the end of the physical device and
> > > > > the size of the ART partition, so you must know its start as well?
> > > > >    
> > > > 
> > > > Before the system boot we know:
> > > > - size of the ART partition
> > > > - real size of the physical device (512mb... 1G... 64mb...)
> > > > 
> > > > When the physical device is probed (nand) a special driver is loaded
> > > > (before mtd parsing logic) that change the physical size of the device
> > > > (mtd->size) as at the end of the nand some space is reserved for bad
> > > > block management and other metadata info.  
> > > 
> > > Here you are explaining what you intend Linux to do, right? I would
> > > like to understand what you are trying to solve. I dont understand why
> > > you need the size change, I don't understand why you don't know the
> > > start of the ART partition, I don't understand what the data you are
> > > hiding contains and who uses it :-) I'm sorry, this is too unclear yet.  
> > 
> > Totally not a problem and thanks a lot for you keep asking them... More
> > than happy to clear things, I'm trying to solve a problem present on
> > Airoha SoC and upstreaming a correct parser for it.
> > 
> > What I'm trying to solve:
> > 
> > Correct access to this partition at the end of the flash in an automated
> > way.
> > 
> > The content of this partition is the usual ART partition found on lots of
> > embedded devices. MAC address, wifi calibration data, serial. Usage is
> > NVMEM cells and userspace with dd command to extract data from.
> > 
> > Airoha use something also used by some mediatek SoC. They call it BMT
> > and it's currently used downstream in OpenWrt and they firmware. This is
> > also used in the bootloader.
> > 
> > The usage of BMT is a custom way to handle bad blocks entirely by
> > software. At the end of the flash some space is reserved where info
> > about all the blocks of the flash are put. I'm not 100% sure about the
> > functionality of this but it can relocate block and do magic things to
> > handle bad blocks. For the scope of this change, the important info is
> > that after the BMT is probed, the operation of "reserving space" is done
> > by reducing the MTD flash size. So from the MTD subsystem, it does see a
> > smaller flash than it actually is.
> > 
> > The reserved space change! Across SoC or even devices but the BMT is a
> > must where it's used as bootloader makes use of it and writing to it
> > might confuse the bootloader corrupting data. (one block might be
> > flagged as bad ad data moved, BMT driver validates his table and do
> > operation)
> 
> Ok, I think that's way clearer now.
>

Hi sorry for the delay, very happy this is better now.

> So the BMT driver does not exist in mainline Linux, but you would like
> to skip this part of the MTD device to avoid smashing it. And it is in
> use by the vendor Bootloader I guess?

Yes correct, idea is to permit easier access to the partition. I hope
(and assume) this driver will come upstream.

> 
> Is it some kind of table that is written by the chip itself in order to
> maintain a list of auto-replacement blocks for bad blocks? Can the size
> of this table move with the use of the device? (if yes, it's
> problematic, we don't want to resize MTD partitions without noticing,
> it would break eg. UBI).
> 

No chip hw bad block is disabled with this implementation and the table
size doesn't move/change so MTD partitions will stay at the same offset
after the first parse on boot.

> I believe this BMT block is going against the bad block handling in
> Linux, so I really wonder how one can use both mechanisms in a system.
> If the BMT layer takes "one random block" to map a corrupted one on it,
> it totally defeats the current bad block model we have in MTD/UBI
> and simply cannot be supported at all. Just skipping the
> currently-used-for-BMT blocks sounds like a very bad idea that will
> break your system, later.
>

Well we disable it and since it's reserved, from the system side you can
do all kind of magic since the space used for the driver is not
available to the system but I will try to gather more info about this in
the next few days.

-- 
	Ansuel

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Miquel Raynal]

Hi Christian,

ansuelsmth@gmail.com wrote on Wed, 16 Oct 2024 09:33:46 +0200:

> On Wed, Oct 02, 2024 at 10:00:06AM +0200, Miquel Raynal wrote:
> > Hi Christian,
> >   
> > > > > > > Ok probably the description isn't clear enough. The missing info that
> > > > > > > require this parser is the flash end.
> > > > > > > 
> > > > > > > Following the example we know the size of rootfs_data and start offset
> > > > > > > AND we know the size of the ART partition.
> > > > > > > 
> > > > > > > There might be a space in the middle unused between the rootfs_data
> > > > > > > partition and the art partition. What is derived is the starting offset
> > > > > > > of the art partition that is flash end - art partition size.
> > > > > > > (where flash end change and is not always the same due to how the special
> > > > > > > bad block managament table reserved space is handled)
> > > > > > > 
> > > > > > > This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
> > > > > > > runtime. On second tought tho maybe using this dummy offset is wrong and
> > > > > > > I should just have something like
> > > > > > > 
> > > > > > > length = <0x300000>;
> > > > > > > 
> > > > > > > Is it clear now? Sorry for any confusion.      
> > > > > > 
> > > > > > I'm sorry but not really. You know the end of the physical device and
> > > > > > the size of the ART partition, so you must know its start as well?
> > > > > >      
> > > > > 
> > > > > Before the system boot we know:
> > > > > - size of the ART partition
> > > > > - real size of the physical device (512mb... 1G... 64mb...)
> > > > > 
> > > > > When the physical device is probed (nand) a special driver is loaded
> > > > > (before mtd parsing logic) that change the physical size of the device
> > > > > (mtd->size) as at the end of the nand some space is reserved for bad
> > > > > block management and other metadata info.    
> > > > 
> > > > Here you are explaining what you intend Linux to do, right? I would
> > > > like to understand what you are trying to solve. I dont understand why
> > > > you need the size change, I don't understand why you don't know the
> > > > start of the ART partition, I don't understand what the data you are
> > > > hiding contains and who uses it :-) I'm sorry, this is too unclear yet.    
> > > 
> > > Totally not a problem and thanks a lot for you keep asking them... More
> > > than happy to clear things, I'm trying to solve a problem present on
> > > Airoha SoC and upstreaming a correct parser for it.
> > > 
> > > What I'm trying to solve:
> > > 
> > > Correct access to this partition at the end of the flash in an automated
> > > way.
> > > 
> > > The content of this partition is the usual ART partition found on lots of
> > > embedded devices. MAC address, wifi calibration data, serial. Usage is
> > > NVMEM cells and userspace with dd command to extract data from.
> > > 
> > > Airoha use something also used by some mediatek SoC. They call it BMT
> > > and it's currently used downstream in OpenWrt and they firmware. This is
> > > also used in the bootloader.
> > > 
> > > The usage of BMT is a custom way to handle bad blocks entirely by
> > > software. At the end of the flash some space is reserved where info
> > > about all the blocks of the flash are put. I'm not 100% sure about the
> > > functionality of this but it can relocate block and do magic things to
> > > handle bad blocks. For the scope of this change, the important info is
> > > that after the BMT is probed, the operation of "reserving space" is done
> > > by reducing the MTD flash size. So from the MTD subsystem, it does see a
> > > smaller flash than it actually is.
> > > 
> > > The reserved space change! Across SoC or even devices but the BMT is a
> > > must where it's used as bootloader makes use of it and writing to it
> > > might confuse the bootloader corrupting data. (one block might be
> > > flagged as bad ad data moved, BMT driver validates his table and do
> > > operation)  
> > 
> > Ok, I think that's way clearer now.
> >  
> 
> Hi sorry for the delay, very happy this is better now.
> 
> > So the BMT driver does not exist in mainline Linux, but you would like
> > to skip this part of the MTD device to avoid smashing it. And it is in
> > use by the vendor Bootloader I guess?  
> 
> Yes correct, idea is to permit easier access to the partition. I hope
> (and assume) this driver will come upstream.
> 
> > 
> > Is it some kind of table that is written by the chip itself in order to
> > maintain a list of auto-replacement blocks for bad blocks? Can the size
> > of this table move with the use of the device? (if yes, it's
> > problematic, we don't want to resize MTD partitions without noticing,
> > it would break eg. UBI).
> >   
> 
> No chip hw bad block is disabled with this implementation and the table
> size doesn't move/change so MTD partitions will stay at the same offset
> after the first parse on boot.
> 
> > I believe this BMT block is going against the bad block handling in
> > Linux, so I really wonder how one can use both mechanisms in a system.
> > If the BMT layer takes "one random block" to map a corrupted one on it,
> > it totally defeats the current bad block model we have in MTD/UBI
> > and simply cannot be supported at all. Just skipping the
> > currently-used-for-BMT blocks sounds like a very bad idea that will
> > break your system, later.
> >  
> 
> Well we disable it and since it's reserved, from the system side you can
> do all kind of magic since the space used for the driver is not
> available to the system but I will try to gather more info about this in
> the next few days.

I understand, but if you cannot get rid of it, it means "someone" is
using it, presumably the bootloader, right? How can the bootloader use
this feature?

Or maybe you need this table to show the (vendor) bootloader "nothing
changed, use PEB normally, none of them is bad, there is no ongoing
remapping"?

In this case I guess the size of the table is a linear function against
the size of the chip and thus can be statically derived?

Thanks,
Miquèl

Re: [PATCH 2/3] dt-bindings: mtd: Add Documentation for Airoha fixed-partitions

[Andreas Gnau]

Hi Christian,

On 2024-10-16 09:33, Christian Marangi wrote:
> On Wed, Oct 02, 2024 at 10:00:06AM +0200, Miquel Raynal wrote:
>> Hi Christian,
>>
>>>>>>> Ok probably the description isn't clear enough. The missing info that
>>>>>>> require this parser is the flash end.
>>>>>>>
>>>>>>> Following the example we know the size of rootfs_data and start offset
>>>>>>> AND we know the size of the ART partition.
>>>>>>>
>>>>>>> There might be a space in the middle unused between the rootfs_data
>>>>>>> partition and the art partition. What is derived is the starting offset
>>>>>>> of the art partition that is flash end - art partition size.
>>>>>>> (where flash end change and is not always the same due to how the special
>>>>>>> bad block managament table reserved space is handled)
>>>>>>>
>>>>>>> This is why 0xffffffff, used as a dummy offset to signal it will be parsed at
>>>>>>> runtime. On second tought tho maybe using this dummy offset is wrong and
>>>>>>> I should just have something like
>>>>>>>
>>>>>>> length = <0x300000>;
>>>>>>>
>>>>>>> Is it clear now? Sorry for any confusion.
>>>>>>
>>>>>> I'm sorry but not really. You know the end of the physical device and
>>>>>> the size of the ART partition, so you must know its start as well?
>>>>>>     
>>>>>
>>>>> Before the system boot we know:
>>>>> - size of the ART partition
>>>>> - real size of the physical device (512mb... 1G... 64mb...)
>>>>>
>>>>> When the physical device is probed (nand) a special driver is loaded
>>>>> (before mtd parsing logic) that change the physical size of the device
>>>>> (mtd->size) as at the end of the nand some space is reserved for bad
>>>>> block management and other metadata info.
>>>>
>>>> Here you are explaining what you intend Linux to do, right? I would
>>>> like to understand what you are trying to solve. I dont understand why
>>>> you need the size change, I don't understand why you don't know the
>>>> start of the ART partition, I don't understand what the data you are
>>>> hiding contains and who uses it :-) I'm sorry, this is too unclear yet.
>>>
>>> Totally not a problem and thanks a lot for you keep asking them... More
>>> than happy to clear things, I'm trying to solve a problem present on
>>> Airoha SoC and upstreaming a correct parser for it.
>>>
>>> What I'm trying to solve:
>>>
>>> Correct access to this partition at the end of the flash in an automated
>>> way.
>>>
>>> The content of this partition is the usual ART partition found on lots of
>>> embedded devices. MAC address, wifi calibration data, serial. Usage is
>>> NVMEM cells and userspace with dd command to extract data from.
>>>
>>> Airoha use something also used by some mediatek SoC. They call it BMT
>>> and it's currently used downstream in OpenWrt and they firmware. This is
>>> also used in the bootloader.
>>>
>>> The usage of BMT is a custom way to handle bad blocks entirely by
>>> software. At the end of the flash some space is reserved where info
>>> about all the blocks of the flash are put. I'm not 100% sure about the
>>> functionality of this but it can relocate block and do magic things to
>>> handle bad blocks. For the scope of this change, the important info is
>>> that after the BMT is probed, the operation of "reserving space" is done
>>> by reducing the MTD flash size. So from the MTD subsystem, it does see a
>>> smaller flash than it actually is.
>>>
>>> The reserved space change! Across SoC or even devices but the BMT is a
>>> must where it's used as bootloader makes use of it and writing to it
>>> might confuse the bootloader corrupting data. (one block might be
>>> flagged as bad ad data moved, BMT driver validates his table and do
>>> operation)
>>
>> Ok, I think that's way clearer now.
>>
> 
> Hi sorry for the delay, very happy this is better now.
> 
>> So the BMT driver does not exist in mainline Linux, but you would like
>> to skip this part of the MTD device to avoid smashing it. And it is in
>> use by the vendor Bootloader I guess?
> 
> Yes correct, idea is to permit easier access to the partition. I hope
> (and assume) this driver will come upstream.

May I ask for a better understanding what the "complete goal" is? Is the 
goal only compatibility with the Airoha ATF as it is now? Or is the goal 
to read flashes that have been using the Airoha SDK with BL and Linux? 
Airoha bootloader is just software on the flash, it can be changed to 
not write BMT, which we have done.

I am asking, because I consider this BMT to be actually detrimental when 
used together with UBI. Wear-levelling on the UBI side is no longer 
correct when blocks can get re-located by some other entity below due to 
bad-blocks.

We have patched the Airoha bootloader components to not write BMT (and 
of course our U-Boot fork and Linux flash drivers do not use it either).

Before we had patched the bootloader, we had initially marked the BMT as 
bad block, but if I remember correctly the BMT location might also be 
somewhere else in case the BMT block itself is a bad block, which is 
also the reason why we went the safe way and patched ATF.

Just putting my thoughts/experiences out there, mostly because I am a 
bit concerned to about the possible double bad block management (UBI + 
Airoha). You might have more insight into things than we have about how 
exactly things work. So, maybe it is not as big of an issue.

I just think that this BMT can create a lot of other issues as well. And 
maybe, I am a bit of a burnt child dreading the fire because of learning 
about this BMT and how it works step by step the hard way. For example, 
we had an issue where Linux and U-Boot had support for a NAND chip while 
ATF would mis-detect that 256 MB NAND as 128 MB and thus destroy data by 
writing BMT in the middle of the flash. Fun times...

As a side-note: We have also migrated some customer devices that had 
been deployed in the field with Airoha SDK BMT and drivers to our UBI 
flash layout without BMT. Strategy was to load relevant data into RAM 
from Airoha U-Boot and then chainload our U-Boot 2023 that would flash 
everything back.

>> Is it some kind of table that is written by the chip itself in order to
>> maintain a list of auto-replacement blocks for bad blocks? Can the size
>> of this table move with the use of the device? (if yes, it's
>> problematic, we don't want to resize MTD partitions without noticing,
>> it would break eg. UBI).
>>
> 
> No chip hw bad block is disabled with this implementation and the table
> size doesn't move/change so MTD partitions will stay at the same offset
> after the first parse on boot.

If the block that holds the BMT goes bad when BMT is being updated, 
wouldn't the BMT location itself change? At least that is my faint 
understanding (which could be wrong, of course and it has also been some 
time...).

>> I believe this BMT block is going against the bad block handling in
>> Linux, so I really wonder how one can use both mechanisms in a system.
>> If the BMT layer takes "one random block" to map a corrupted one on it,
>> it totally defeats the current bad block model we have in MTD/UBI
>> and simply cannot be supported at all. Just skipping the
>> currently-used-for-BMT blocks sounds like a very bad idea that will
>> break your system, later.
>>
> 
> Well we disable it and since it's reserved, from the system side you can
> do all kind of magic since the space used for the driver is not
> available to the system but I will try to gather more info about this in
> the next few days.
> 

Best Regards,

Andreas Gnau

[PATCH 3/3] mtd: parser: add support for Airoha parser

[Christian Marangi]

Add support for Airoha parser based on a post parse ofpart function.

Airoha partition table follow normal fixed-partition implementation
with a special implementation for the ART partition. This is always the
last partition and is placed from the end of the flash - the partition
size.

To enable this special implementation for ART partition, the relevant
node require the "airoha,dynamic-art" compatible. With that declared,
offset value is ignored and real offset is updated with the calculated
value.

Due to usage of specific bad block management driver, the MTD size might
vary hence the ART partition offset needs to be dynamically parsed and
can't be declared statically.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/mtd/parsers/Kconfig         | 10 +++++
 drivers/mtd/parsers/Makefile        |  1 +
 drivers/mtd/parsers/ofpart_airoha.c | 57 +++++++++++++++++++++++++++++
 drivers/mtd/parsers/ofpart_airoha.h | 18 +++++++++
 drivers/mtd/parsers/ofpart_core.c   |  6 +++
 5 files changed, 92 insertions(+)
 create mode 100644 drivers/mtd/parsers/ofpart_airoha.c
 create mode 100644 drivers/mtd/parsers/ofpart_airoha.h

diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig
index da03ab6efe04..d6c53aa16ea6 100644
--- a/drivers/mtd/parsers/Kconfig
+++ b/drivers/mtd/parsers/Kconfig
@@ -72,6 +72,16 @@ config MTD_OF_PARTS
 	  flash memory node, as described in
 	  Documentation/devicetree/bindings/mtd/mtd.yaml.
 
+config MTD_OF_PARTS_AIROHA
+	bool "Airoha EN7815 partitioning support"
+	depends on MTD_OF_PARTS && (ARCH_AIROHA || COMPILE_TEST)
+	default ARCH_AIROHA
+	help
+	  This provides partitions parser for Airoha EN7815 family devices
+	  that can have dynamic "ART" partition at the end of the flash.
+	  It takes care of finding the correct offset and update property
+	  with it.
+
 config MTD_OF_PARTS_BCM4908
 	bool "BCM4908 partitioning support"
 	depends on MTD_OF_PARTS && (ARCH_BCMBCA || COMPILE_TEST)
diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile
index 9b00c62b837a..d67f9b4d39ac 100644
--- a/drivers/mtd/parsers/Makefile
+++ b/drivers/mtd/parsers/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_MTD_BRCM_U_BOOT)		+= brcm_u-boot.o
 obj-$(CONFIG_MTD_CMDLINE_PARTS)		+= cmdlinepart.o
 obj-$(CONFIG_MTD_OF_PARTS)		+= ofpart.o
 ofpart-y				+= ofpart_core.o
+ofpart-$(CONFIG_MTD_OF_PARTS_AIROHA)	+= ofpart_airoha.o
 ofpart-$(CONFIG_MTD_OF_PARTS_BCM4908)	+= ofpart_bcm4908.o
 ofpart-$(CONFIG_MTD_OF_PARTS_LINKSYS_NS)+= ofpart_linksys_ns.o
 obj-$(CONFIG_MTD_PARSER_IMAGETAG)	+= parser_imagetag.o
diff --git a/drivers/mtd/parsers/ofpart_airoha.c b/drivers/mtd/parsers/ofpart_airoha.c
new file mode 100644
index 000000000000..5e2cc7b6376b
--- /dev/null
+++ b/drivers/mtd/parsers/ofpart_airoha.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Christian Marangi <ansuelsmth@gmail.com>
+ */
+
+#include <linux/mtd/mtd.h>
+#include <linux/slab.h>
+#include <linux/mtd/partitions.h>
+
+#include "ofpart_airoha.h"
+
+int airoha_partitions_post_parse(struct mtd_info *mtd,
+				 struct mtd_partition *parts,
+				 int nr_parts)
+{
+	struct mtd_partition *part;
+	int len, a_cells, s_cells;
+	struct device_node *pp;
+	struct property *prop;
+	const __be32 *reg;
+	__be32 *new_reg;
+
+	part = &parts[nr_parts - 1];
+	pp = part->of_node;
+
+	/* Skip if ART partition have a valid offset instead of a dynamic one */
+	if (!of_device_is_compatible(pp, "airoha,dynamic-art"))
+		return 0;
+
+	/* ART partition is set at the end of flash - size */
+	part->offset = mtd->size - part->size;
+
+	/* Update the offset with the new calculate value in DT */
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return -ENOMEM;
+
+	/* Reg already validated by fixed-partition parser */
+	reg = of_get_property(pp, "reg", &len);
+
+	/* Fixed partition */
+	a_cells = of_n_addr_cells(pp);
+	s_cells = of_n_size_cells(pp);
+
+	prop->name = "reg";
+	prop->length = (a_cells + s_cells) * sizeof(__be32);
+	prop->value = kmemdup(reg, (a_cells + s_cells) * sizeof(__be32),
+			      GFP_KERNEL);
+	new_reg = prop->value;
+	memset(new_reg, 0, a_cells * sizeof(__be32));
+	new_reg[a_cells - 1] = cpu_to_be32(part->offset);
+	if (a_cells > 1)
+		new_reg[0] = cpu_to_be32(part->offset >> 32);
+	of_update_property(pp, prop);
+
+	return 0;
+}
diff --git a/drivers/mtd/parsers/ofpart_airoha.h b/drivers/mtd/parsers/ofpart_airoha.h
new file mode 100644
index 000000000000..3e8a8456c13a
--- /dev/null
+++ b/drivers/mtd/parsers/ofpart_airoha.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __OFPART_AIROHA_H
+#define __OFPART_AIROHA_H
+
+#ifdef CONFIG_MTD_OF_PARTS_AIROHA
+int airoha_partitions_post_parse(struct mtd_info *mtd,
+				 struct mtd_partition *parts,
+				 int nr_parts);
+#else
+static inline int airoha_partitions_post_parse(struct mtd_info *mtd,
+					       struct mtd_partition *parts,
+					       int nr_parts)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+#endif
diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
index abfa68798918..8e86fa4f3bbd 100644
--- a/drivers/mtd/parsers/ofpart_core.c
+++ b/drivers/mtd/parsers/ofpart_core.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/mtd/partitions.h>
 
+#include "ofpart_airoha.h"
 #include "ofpart_bcm4908.h"
 #include "ofpart_linksys_ns.h"
 
@@ -23,6 +24,10 @@ struct fixed_partitions_quirks {
 	int (*post_parse)(struct mtd_info *mtd, struct mtd_partition *parts, int nr_parts);
 };
 
+static struct fixed_partitions_quirks airoha_partitions_quirks = {
+	.post_parse = airoha_partitions_post_parse,
+};
+
 static struct fixed_partitions_quirks bcm4908_partitions_quirks = {
 	.post_parse = bcm4908_partitions_post_parse,
 };
@@ -192,6 +197,7 @@ static const struct of_device_id parse_ofpart_match_table[] = {
 	/* Generic */
 	{ .compatible = "fixed-partitions" },
 	/* Customized */
+	{ .compatible = "airoha,fixed-partitions", .data = &airoha_partitions_quirks, },
 	{ .compatible = "brcm,bcm4908-partitions", .data = &bcm4908_partitions_quirks, },
 	{ .compatible = "linksys,ns-partitions", .data = &linksys_ns_partitions_quirks, },
 	{},
-- 
2.45.2