dt-bindings: mtd-partitions: Export special values

There are special "offset" and "size" values defined and documented in
linux/mtd/partitions.h:

// consume as much as possible, leaving size after the end of partition.

// the partition will start at the next erase block.

// the partition will start where the previous one ended.

(Though not explicitly, they are compared against variables in uint64_t
in drivers/mtd/mtdpart.c, so they had better be considered as such.)

// the partition will extend to the end of the master MTD device.

These special values could be used to define partitions automatically
fitting to the size of the master MTD device at runtime.

However, these values used not to be exported to dt-bindings, thus
seldom used before, since they might have been only used in numeric form,
such as "(-1) (-3)" for MTDPART_OFS_RETAIN.

Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
values, so 2-cell addressed should be defined to use special offset values,
such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.

Signed-off-by: Edward Chow <equu@openmail.cc>
---
 .../mtd/partitions/fixed-partitions.yaml      | 29 +++++++++++++++++++
 MAINTAINERS                                   |  2 ++
 include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
 3 files changed, 46 insertions(+)
 create mode 100644 include/dt-bindings/mtd/partitions.h

Hi Edward,

Thanks for the proposal!

equu@openmail.cc wrote on Wed, 25 Oct 2023 13:29:37 +0800:

> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 
> // consume as much as possible, leaving size after the end of partition.
> 
> // the partition will start at the next erase block.
> 
> // the partition will start where the previous one ended.
> 
> (Though not explicitly, they are compared against variables in uint64_t
> in drivers/mtd/mtdpart.c, so they had better be considered as such.)
> 
> // the partition will extend to the end of the master MTD device.
> 
> These special values could be used to define partitions automatically
> fitting to the size of the master MTD device at runtime.
> 
> However, these values used not to be exported to dt-bindings, thus
> seldom used before, since they might have been only used in numeric form,
> such as "(-1) (-3)" for MTDPART_OFS_RETAIN.
> 
> Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
> values, so 2-cell addressed should be defined to use special offset values,
> such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
> linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.

I don't think this has ever been used in DT, it comes from a previous
era where everything was declared in machine code and was never
intended to be part of the official bindings.

We've been trying to clarify the partitions binding to make them clean
and easily usable and I believe this comes from old times and is
way too legacy and backwards to be exposed.

Typically, this comes from times where the storage devices were so
small that an erase block boundary would be way too big and one would
want to store different "partitions" in a single block. It is close to
impossible to find such devices today so I don't think it really
matters and, as a general advice, should not be used anymore.

> Signed-off-by: Edward Chow <equu@openmail.cc>
> ---
>  .../mtd/partitions/fixed-partitions.yaml      | 29 +++++++++++++++++++
>  MAINTAINERS                                   |  2 ++
>  include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 include/dt-bindings/mtd/partitions.h
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> index 331e564f29dc..a939fb52ef76 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> @@ -164,3 +164,32 @@ examples:
>              read-only;
>          };
>      };
> +
> +  - |
> +    partitions {
> +        compatible = "fixed-partitions";
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        partition@0 {
> +            label = "bootloader";
> +            reg = <0 0x000000 0x020000>;
> +            read-only;
> +        };
> +
> +        firmware@1 {
> +            label = "firmware";
> +            /* From the end of the last partition, occupying as mush
> +             * as possible, retaining 0x010000 after it,
> +             * "MTDPART_OFS_SPECIAL MTDPART_OFS_NXTBLK" similar to
> +             * this, but always beginning at erase block boundary. */
> +            reg = <MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN 0x010000>;
> +        };
> +
> +        calibration@2 {
> +            compatible = "fixed-partitions";
> +            label = "calibration";
> +            /* Appending to the last partition, occupying 0x010000 */
> +            reg = <MTDPART_OFS_SPECIAL MTDPART_OFS_APPEND 0x010000>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 668d1e24452d..7d6beadc8b36 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13771,9 +13771,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next
>  F:	Documentation/devicetree/bindings/mtd/
>  F:	drivers/mtd/
> +F:	include/dt-bindings/mtd/
>  F:	include/linux/mtd/
>  F:	include/uapi/mtd/

This is a nice addition but totally unrelated, please make a separated
patch.

>  
> +
>  MEMSENSING MICROSYSTEMS MSA311 DRIVER
>  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
>  L:	linux-iio@vger.kernel.org
> diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h
> new file mode 100644
> index 000000000000..456a54a1259a
> --- /dev/null
> +++ b/include/dt-bindings/mtd/partitions.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Device Tree constants identical to those in include/linux/mtd/partitions.h
> + */
> +
> +#ifndef _DT_BINDINGS_MTD_PARTITIONS_H
> +#define _DT_BINDINGS_MTD_PARTITIONS_H
> +
> +#define MTDPART_OFS_SPECIAL	(-1)

This one does not exist, maybe -1 is hardcoded in the code and defining
it close to the existing definitions (not shared with the bindings)
make sense, but that is a different patch.

> +#define MTDPART_OFS_RETAIN	(-3)
> +#define MTDPART_OFS_NXTBLK	(-2)
> +#define MTDPART_OFS_APPEND	(-1)
> +#define MTDPART_SIZ_FULL	(0)

Just as an FYI, I would have expected the existing definitions to be
dropped if we were to take the patch.

Thanks,
Miquèl

On 25/10/2023 07:29, Edward Chow wrote:
> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 
> // consume as much as possible, leaving size after the end of partition.
> 
> // the partition will start at the next erase block.
> 
> // the partition will start where the previous one ended.
> 
> (Though not explicitly, they are compared against variables in uint64_t
> in drivers/mtd/mtdpart.c, so they had better be considered as such.)
> 
> // the partition will extend to the end of the master MTD device.
> 
> These special values could be used to define partitions automatically
> fitting to the size of the master MTD device at runtime.
> 
> However, these values used not to be exported to dt-bindings, thus
> seldom used before, since they might have been only used in numeric form,
> such as "(-1) (-3)" for MTDPART_OFS_RETAIN.
> 
> Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
> values, so 2-cell addressed should be defined to use special offset values,
> such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
> linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.
> 
> Signed-off-by: Edward Chow <equu@openmail.cc>
> ---
>  .../mtd/partitions/fixed-partitions.yaml      | 29 +++++++++++++++++++
>  MAINTAINERS                                   |  2 ++
>  include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 include/dt-bindings/mtd/partitions.h
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> index 331e564f29dc..a939fb52ef76 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> @@ -164,3 +164,32 @@ examples:
>              read-only;
>          };
>      };
> +
> +  - |
> +    partitions {
> +        compatible = "fixed-partitions";
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        partition@0 {
> +            label = "bootloader";
> +            reg = <0 0x000000 0x020000>;
> +            read-only;
> +        };
> +
> +        firmware@1 {
> +            label = "firmware";
> +            /* From the end of the last partition, occupying as mush

Use Linux coding style comments.

> +             * as possible, retaining 0x010000 after it,
> +             * "MTDPART_OFS_SPECIAL MTDPART_OFS_NXTBLK" similar to
> +             * this, but always beginning at erase block boundary. */
> +            reg = <MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN 0x010000>;
> +        };
> +
> +        calibration@2 {
> +            compatible = "fixed-partitions";
> +            label = "calibration";
> +            /* Appending to the last partition, occupying 0x010000 */
> +            reg = <MTDPART_OFS_SPECIAL MTDPART_OFS_APPEND 0x010000>;

And where is any user of this? Example in the bindings is not user. I
would expect that you will change at least one other DTS.

> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 668d1e24452d..7d6beadc8b36 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13771,9 +13771,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next
>  F:	Documentation/devicetree/bindings/mtd/
>  F:	drivers/mtd/
> +F:	include/dt-bindings/mtd/
>  F:	include/linux/mtd/
>  F:	include/uapi/mtd/
>  
> +

Drop.

>  MEMSENSING MICROSYSTEMS MSA311 DRIVER
>  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
>  L:	linux-iio@vger.kernel.org
> diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h
> new file mode 100644
> index 000000000000..456a54a1259a
> --- /dev/null
> +++ b/include/dt-bindings/mtd/partitions.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

Dual license, as asked by checkpatch.

Best regards,
Krzysztof

On Wed, 25 Oct 2023 13:29:37 +0800, Edward Chow wrote:
> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 
> // consume as much as possible, leaving size after the end of partition.
> 
> // the partition will start at the next erase block.
> 
> // the partition will start where the previous one ended.
> 
> (Though not explicitly, they are compared against variables in uint64_t
> in drivers/mtd/mtdpart.c, so they had better be considered as such.)
> 
> // the partition will extend to the end of the master MTD device.
> 
> These special values could be used to define partitions automatically
> fitting to the size of the master MTD device at runtime.
> 
> However, these values used not to be exported to dt-bindings, thus
> seldom used before, since they might have been only used in numeric form,
> such as "(-1) (-3)" for MTDPART_OFS_RETAIN.
> 
> Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
> values, so 2-cell addressed should be defined to use special offset values,
> such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
> linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.
> 
> Signed-off-by: Edward Chow <equu@openmail.cc>
> ---
>  .../mtd/partitions/fixed-partitions.yaml      | 29 +++++++++++++++++++
>  MAINTAINERS                                   |  2 ++
>  include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 include/dt-bindings/mtd/partitions.h
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.example.dts:225.24-25 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231025052937.830813-1-equu@openmail.cc

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

There are special "offset" and "size" values defined and documented in
linux/mtd/partitions.h:

/* consume as much as possible, leaving size after the end of partition. */
 #define MTDPART_OFS_RETAIN (uint64_t)(-3)

/* the partition will start at the next erase block. */
 #define MTDPART_OFS_NXTBLK (uint64_t)(-2)

/* the partition will start where the previous one ended. */
 #define MTDPART_OFS_APPEND (uint64_t)(-1)

(Though not explicitly, they are compared against variables in uint64_t
in drivers/mtd/mtdpart.c, so they had better be considered as such.)

/* the partition will extend to the end of the master MTD device. */
 #define MTDPART_SIZ_FULL (0)

These special values could be used to define partitions automatically
fitting to the size of the master MTD device at runtime.

However, these values used not to be exported to dt-bindings, thus
seldom used before, since they might have been only used in numeric form,
such as "(-1) (-3)" for MTDPART_OFS_RETAIN.

Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
values, so 2-cell addressed should be defined to use special offset values,
such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.

Edward Chow (2):
  dt-bindings: mtd: partitions: Export special values
  dt-bindings: mtd: partitions: Document special values

 .../mtd/partitions/fixed-partitions.yaml      | 30 +++++++++++++++++++
 MAINTAINERS                                   |  2 ++
 include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
 3 files changed, 47 insertions(+)
 create mode 100644 include/dt-bindings/mtd/partitions.h

--
2.42.0

On 27/10/2023 11:46, Edward Chow wrote:
> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 


Missing changelog. I already see you ignored my comments :/

Best regards,
Krzysztof

On 27/10/2023 11:46, Edward Chow wrote:
> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 
> /* consume as much as possible, leaving size after the end of partition. */
>  #define MTDPART_OFS_RETAIN (uint64_t)(-3)
> 

Also:

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

Best regards,
Krzysztof

Hi Edward,

equu@openmail.cc wrote on Fri, 27 Oct 2023 17:46:08 +0800:

> There are special "offset" and "size" values defined and documented in
> linux/mtd/partitions.h:
> 
> /* consume as much as possible, leaving size after the end of partition. */
>  #define MTDPART_OFS_RETAIN (uint64_t)(-3)
> 
> /* the partition will start at the next erase block. */
>  #define MTDPART_OFS_NXTBLK (uint64_t)(-2)
> 
> /* the partition will start where the previous one ended. */
>  #define MTDPART_OFS_APPEND (uint64_t)(-1)
> 
> (Though not explicitly, they are compared against variables in uint64_t
> in drivers/mtd/mtdpart.c, so they had better be considered as such.)
> 
> /* the partition will extend to the end of the master MTD device. */
>  #define MTDPART_SIZ_FULL (0)
> 
> These special values could be used to define partitions automatically
> fitting to the size of the master MTD device at runtime.
> 
> However, these values used not to be exported to dt-bindings, thus
> seldom used before, since they might have been only used in numeric form,
> such as "(-1) (-3)" for MTDPART_OFS_RETAIN.
> 
> Now, they are exported in dt-bindings/mtd/partitions.h as 32-bit cell
> values, so 2-cell addressed should be defined to use special offset values,
> such as "MTDPART_OFS_SPECIAL MTDPART_OFS_RETAIN" for MTDPART_OFS_RETAIN in
> linux/mtd/partitions.h. An example is added to fixed-partitions.yaml.
> 
> Edward Chow (2):
>   dt-bindings: mtd: partitions: Export special values
>   dt-bindings: mtd: partitions: Document special values
> 
>  .../mtd/partitions/fixed-partitions.yaml      | 30 +++++++++++++++++++
>  MAINTAINERS                                   |  2 ++
>  include/dt-bindings/mtd/partitions.h          | 15 ++++++++++
>  3 files changed, 47 insertions(+)
>  create mode 100644 include/dt-bindings/mtd/partitions.h
> 
> --
> 2.42.0

I've expressed my feelings regarding this series in your v1 but it
feels like you are completely ignoring them. As a reminder, I am
opposed to exporting these flags. They are outdated, legacy, have
never been used in DT and were never meant to be.

Thanks, Miquèl