[RFC PATCH 0/4] block: partition table OF support

[RFC PATCH 0/4] block: partition table OF support

[Christian Marangi]

Hi,
this is an initial proposal to complete support for manually defining
partition table.

Some background on this. Many OEM on embedded device (modem, router...)
are starting to migrate from NOR/NAND flash to eMMC. The reason for this
is that OEM are starting to require more and more space for the firmware
and price difference is becoming so little that using eMMC is only benefits
and no cons.

Given these reason, OEM are also using very custom way to provide a
partition table and doesn't relay on common method like writing a table
on the eMMC.

One way that is commonly used is to hardcode the partition table and
pass it to the system via various way (cmdline, special glue driver,
block2mtd...)
This way is also used on Android where the partition table
is passed from the bootloader via cmdline.

One reason to use this method is to save space on the device and to
permit more flexibility on partition handling.

What this series does is complete support for this feature.
It's possible to use the cmdline to define a partition table similar
to how it's done for MTD but this is problematic for a number of device
where tweaking the cmdline is not possible. This series adds OF support
to make it possible to define a partition table in the Device Tree.

We implement a similar schema to the MTD fixed-partition, where we define
a "label" and a "reg" with "offset" and "size".

A new block partition parser is introduced that check if the block device
have an OF node attached and check if a fixed-partition table is defined.

If a correct node is found, then partition table is filled. cmdline will
still have priority to this new parser.

Some block device also implement boot1 and boot2 additional disk. Similar
to the cmdline parser, these disk can have OF support using the
"partitions-boot0" and "partitions-boot1" additional node.

It's also completed support for declaring partition as read-only as this
feature was introduced but never finished in the cmdline parser.

Posting as RFC for any comments or additional checks on OF parser code.

I hope this solution is better accepted as downstream this is becoming
a real problem with a growing number of strange solution for the simple
task of providing a fixed partition table.

Christian Marangi (4):
  block: add support for defining read-only partitions
  docs: block: Document support for read-only partition in cmdline part
  block: add support for partition table defined in OF
  dt-bindings: mmc: Document support for partition table in mmc-card

 Documentation/block/cmdline-partition.rst     |  5 +-
 .../devicetree/bindings/mmc/mmc-card.yaml     | 75 ++++++++++++++++
 block/blk.h                                   |  1 +
 block/partitions/Kconfig                      |  8 ++
 block/partitions/Makefile                     |  1 +
 block/partitions/cmdline.c                    |  3 +
 block/partitions/core.c                       |  3 +
 block/partitions/of.c                         | 85 +++++++++++++++++++
 8 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 block/partitions/of.c

-- 
2.45.2

[RFC PATCH 1/4] block: add support for defining read-only partitions

[Christian Marangi]

Add support for defining read-only partitions and complete support for
it in the cmdline partition parser as the additional "ro" after a
partition is scanned but never actually applied.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 block/blk.h                | 1 +
 block/partitions/cmdline.c | 3 +++
 block/partitions/core.c    | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/block/blk.h b/block/blk.h
index c718e4291db0..f300212d3e98 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -558,6 +558,7 @@ void blk_free_ext_minor(unsigned int minor);
 #define ADDPART_FLAG_NONE	0
 #define ADDPART_FLAG_RAID	1
 #define ADDPART_FLAG_WHOLEDISK	2
+#define ADDPART_FLAG_READONLY	4
 int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
 		sector_t length);
 int bdev_del_partition(struct gendisk *disk, int partno);
diff --git a/block/partitions/cmdline.c b/block/partitions/cmdline.c
index 152c85df92b2..da3e719d8e51 100644
--- a/block/partitions/cmdline.c
+++ b/block/partitions/cmdline.c
@@ -237,6 +237,9 @@ static int add_part(int slot, struct cmdline_subpart *subpart,
 	put_partition(state, slot, subpart->from >> 9,
 		      subpart->size >> 9);
 
+	if (subpart->flags & PF_RDONLY)
+		state->parts[slot].flags |= ADDPART_FLAG_READONLY;
+
 	info = &state->parts[slot].info;
 
 	strscpy(info->volname, subpart->name, sizeof(info->volname));
diff --git a/block/partitions/core.c b/block/partitions/core.c
index ab76e64f0f6c..abad6c83db8f 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -373,6 +373,9 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 			goto out_del;
 	}
 
+	if (flags & ADDPART_FLAG_READONLY)
+		bdev_set_flag(bdev, BD_READ_ONLY);
+
 	/* everything is up and running, commence */
 	err = xa_insert(&disk->part_tbl, partno, bdev, GFP_KERNEL);
 	if (err)
-- 
2.45.2

Re: [RFC PATCH 1/4] block: add support for defining read-only partitions

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[RFC PATCH 2/4] docs: block: Document support for read-only partition in cmdline part

[Christian Marangi]

Document support for read-only partition in cmdline partition for block
devices by appending "ro" after the (partition name).

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/block/cmdline-partition.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/block/cmdline-partition.rst b/Documentation/block/cmdline-partition.rst
index 530bedff548a..526ba201dddc 100644
--- a/Documentation/block/cmdline-partition.rst
+++ b/Documentation/block/cmdline-partition.rst
@@ -39,13 +39,16 @@ blkdevparts=<blkdev-def>[;<blkdev-def>]
     create a link to block device partition with the name "PARTNAME".
     User space application can access partition by partition name.
 
+ro
+    read-only. Flag the partition as read-only.
+
 Example:
 
     eMMC disk names are "mmcblk0" and "mmcblk0boot0".
 
   bootargs::
 
-    'blkdevparts=mmcblk0:1G(data0),1G(data1),-;mmcblk0boot0:1m(boot),-(kernel)'
+    'blkdevparts=mmcblk0:1G(data0),1G(data1),-;mmcblk0boot0:1m(boot)ro,-(kernel)'
 
   dmesg::
 
-- 
2.45.2

[RFC PATCH 3/4] block: add support for partition table defined in OF

[Christian Marangi]

Add support for partition table defined in Device Tree. Similar to how
it's done with MTD, add support for defining a fixed partition table in
device tree.

A common scenario for this is fixed block (eMMC) embedded devices that
have no MBR or GPT partition table to save storage space. Bootloader
access the block device with absolute address of data.

This is to complete the functionality with an equivalent implementation
with providing partition table with bootargs, for case where the booargs
can't be modified and tweaking the Device Tree is the only solution to
have an usabe partition table.

The implementation follow the fixed-partitions parser used on MTD
devices where a "partitions" node is expected to be declared in the OF
node of the disk device (mmc-card for eMMC for example) and each child
node declare a label and a reg with offset and size. Eventually is also
possible to declare the read-only property to flag the partition as
read-only.

The driver scan the disk name and check if it's suffixed with boot0 or
boot1. This is to handle the additional disk provided by eMMC or UFS
devices in general. If this suffix is detected, "partitions-boot0" or
"partitions-boot1" are used instead of the generic "partitions"

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 block/partitions/Kconfig  |  8 ++++
 block/partitions/Makefile |  1 +
 block/partitions/of.c     | 85 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 block/partitions/of.c

diff --git a/block/partitions/Kconfig b/block/partitions/Kconfig
index 7aff4eb81c60..8534f7544f26 100644
--- a/block/partitions/Kconfig
+++ b/block/partitions/Kconfig
@@ -270,4 +270,12 @@ config CMDLINE_PARTITION
 	  Say Y here if you want to read the partition table from bootargs.
 	  The format for the command line is just like mtdparts.
 
+config OF_PARTITION
+	bool "Command line partition support" if PARTITION_ADVANCED
+	depends on OF
+	help
+	  Say Y here if you want to enable support for partition table
+	  defined in Device Tree. (mainly for eMMC)
+	  The format for the command line is just like MTD fixed-partition schema.
+
 endmenu
diff --git a/block/partitions/Makefile b/block/partitions/Makefile
index a7f05cdb02a8..25d424922c6e 100644
--- a/block/partitions/Makefile
+++ b/block/partitions/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_CMDLINE_PARTITION) += cmdline.o
 obj-$(CONFIG_MAC_PARTITION) += mac.o
 obj-$(CONFIG_LDM_PARTITION) += ldm.o
 obj-$(CONFIG_MSDOS_PARTITION) += msdos.o
+obj-$(CONFIG_OF_PARTITION) += of.o
 obj-$(CONFIG_OSF_PARTITION) += osf.o
 obj-$(CONFIG_SGI_PARTITION) += sgi.o
 obj-$(CONFIG_SUN_PARTITION) += sun.o
diff --git a/block/partitions/of.c b/block/partitions/of.c
new file mode 100644
index 000000000000..1c420b7f53c0
--- /dev/null
+++ b/block/partitions/of.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/blkdev.h>
+#include <linux/of.h>
+#include "check.h"
+
+#define BOOT0_STR	"boot0"
+#define BOOT1_STR	"boot1"
+
+static struct device_node *get_partitions_node(struct device_node *disk_np,
+					       const char *disk_name)
+{
+	const char *node_name = "partitions";
+
+	/* Check if we are parsing boot0 or boot1 */
+	if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT0_STR),
+		    BOOT0_STR, sizeof(BOOT0_STR)))
+		node_name = "partitions-boot0";
+	if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT1_STR),
+		    BOOT1_STR, sizeof(BOOT1_STR)))
+		node_name = "partitions-boot1";
+
+	return of_get_child_by_name(disk_np, node_name);
+}
+
+static void add_of_partition(struct parsed_partitions *state, int slot,
+			     struct device_node *np)
+{
+	struct partition_meta_info *info;
+	char tmp[sizeof(info->volname) + 4];
+	int a_cells, s_cells;
+	const char *partname;
+	const __be32 *reg;
+	u64 offset, size;
+	int len;
+
+	reg = of_get_property(np, "reg", &len);
+
+	a_cells = of_n_addr_cells(np);
+	s_cells = of_n_size_cells(np);
+
+	offset = of_read_number(reg, a_cells);
+	size = of_read_number(reg + a_cells, s_cells);
+
+	put_partition(state, slot, offset, size);
+
+	if (of_property_read_bool(pp, "read-only"))
+		state->parts[slot].flags |= ADDPART_FLAG_READONLY;
+
+	info = &state->parts[slot].info;
+	partname = of_get_property(np, "label", &len);
+	strscpy(info->volname, partname, sizeof(info->volname));
+
+	snprintf(tmp, sizeof(tmp), "(%s)", info->volname);
+	strlcat(state->pp_buf, tmp, PAGE_SIZE);
+}
+
+int of_partition(struct parsed_partitions *state)
+{
+	struct device_node *disk_np, *partitions_np, *np;
+	struct device *ddev = disk_to_dev(state->disk);
+	int slot = 1;
+
+	disk_np = of_node_get(ddev->parent->of_node);
+	if (!disk_np)
+		return 0;
+
+	partitions_np = get_partitions_node(disk_np,
+					    state->disk->disk_name);
+	if (!partitions_np)
+		return 0;
+
+	for_each_child_of_node(partitions_np, np) {
+		if (slot >= state->limit)
+			return -1;
+
+		add_of_partition(state, slot, np);
+
+		slot++;
+	}
+
+	strlcat(state->pp_buf, "\n", PAGE_SIZE);
+
+	return 1;
+}
-- 
2.45.2

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christoph Hellwig]

On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote:
> +#define BOOT0_STR	"boot0"
> +#define BOOT1_STR	"boot1"
> +

This boot0/1 stuff looks like black magic, so it should probably be
documented at very least.

> +	partitions_np = get_partitions_node(disk_np,
> +					    state->disk->disk_name);

disk->disk_name is not a stable identifier and can change from boot to
boot due to async probing.  You'll need to check a uuid or label instead.

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christian Marangi]

On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote:
> On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote:
> > +#define BOOT0_STR	"boot0"
> > +#define BOOT1_STR	"boot1"
> > +
> 
> This boot0/1 stuff looks like black magic, so it should probably be
> documented at very least.
>

It is but from what I have read in the spec for flash in general (this
is not limited to eMMC but also apply to UFS) these are hardware
partition. If the version is high enough these are always present and
have boot0 and boot1 name hardcoded by the driver.

> > +	partitions_np = get_partitions_node(disk_np,
> > +					    state->disk->disk_name);
> 
> disk->disk_name is not a stable identifier and can change from boot to
> boot due to async probing.  You'll need to check a uuid or label instead.

This is really for the 2 special partition up to check the suffix, we
don't really care about the name. I guess it's acceptable to use
unstable identifier?

Thanks a lot for the review!

-- 
	Ansuel

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christoph Hellwig]

On Tue, Sep 24, 2024 at 12:17:36PM +0200, Christian Marangi wrote:
> On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote:
> > On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote:
> > > +#define BOOT0_STR	"boot0"
> > > +#define BOOT1_STR	"boot1"
> > > +
> > 
> > This boot0/1 stuff looks like black magic, so it should probably be
> > documented at very least.
> >
> 
> It is but from what I have read in the spec for flash in general (this
> is not limited to eMMC but also apply to UFS) these are hardware
> partition. If the version is high enough these are always present and
> have boot0 and boot1 name hardcoded by the driver.

How does this belong into generic block layer code?

> > > +	partitions_np = get_partitions_node(disk_np,
> > > +					    state->disk->disk_name);
> > 
> > disk->disk_name is not a stable identifier and can change from boot to
> > boot due to async probing.  You'll need to check a uuid or label instead.
> 
> This is really for the 2 special partition up to check the suffix, we
> don't really care about the name. I guess it's acceptable to use
> unstable identifier?

No.  ->disk_name is in no way reliable, we can't hardcode that into
a partition parser.

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christian Marangi]

On Tue, Oct 01, 2024 at 01:37:05AM -0700, Christoph Hellwig wrote:
> On Tue, Sep 24, 2024 at 12:17:36PM +0200, Christian Marangi wrote:
> > On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote:
> > > On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote:
> > > > +#define BOOT0_STR	"boot0"
> > > > +#define BOOT1_STR	"boot1"
> > > > +
> > > 
> > > This boot0/1 stuff looks like black magic, so it should probably be
> > > documented at very least.
> > >
> > 
> > It is but from what I have read in the spec for flash in general (this
> > is not limited to eMMC but also apply to UFS) these are hardware
> > partition. If the version is high enough these are always present and
> > have boot0 and boot1 name hardcoded by the driver.
> 
> How does this belong into generic block layer code?
>

(just as an info, we are at v4 where I added more info about this)

The cmdline partition parser supports this already, just not clearly
stated in the code but described in the Documentation example and info.

> > > > +	partitions_np = get_partitions_node(disk_np,
> > > > +					    state->disk->disk_name);
> > > 
> > > disk->disk_name is not a stable identifier and can change from boot to
> > > boot due to async probing.  You'll need to check a uuid or label instead.
> > 
> > This is really for the 2 special partition up to check the suffix, we
> > don't really care about the name. I guess it's acceptable to use
> > unstable identifier?
> 
> No.  ->disk_name is in no way reliable, we can't hardcode that into
> a partition parser.
> 

Then any hint on this or alternative way?
Again this is how it's done with cmdline partition so I'm just following
how it's already done.

Also I feel it's not clear enough that we really don't care about the
identifier, eMMC driver hardcode and always append to disk_name boot0, boot1,
the fact that one disk or another might have a different identifier and
they change on different boot is not important for the task needed here.

I can drop this thing entirely and make the implementation very simple
but there are already request and happy dev that would benefits for the
additional hardware partition supported by this.

-- 
	Ansuel

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christoph Hellwig]

On Tue, Oct 01, 2024 at 11:26:22AM +0200, Christian Marangi wrote:
> > No.  ->disk_name is in no way reliable, we can't hardcode that into
> > a partition parser.
> > 
> 
> Then any hint on this or alternative way?

The normal way would be to use eui/ngui/uuid provided by the storage
device.  We have a interface for that in the block layer support by
scsi and nvme, but I don't know how to wire that up for eMMC which
I suspect is what you care about.

Re: [RFC PATCH 3/4] block: add support for partition table defined in OF

[Christian Marangi]

On Wed, Oct 02, 2024 at 12:45:37AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 01, 2024 at 11:26:22AM +0200, Christian Marangi wrote:
> > > No.  ->disk_name is in no way reliable, we can't hardcode that into
> > > a partition parser.
> > > 
> > 
> > Then any hint on this or alternative way?
> 
> The normal way would be to use eui/ngui/uuid provided by the storage
> device.  We have a interface for that in the block layer support by
> scsi and nvme, but I don't know how to wire that up for eMMC which
> I suspect is what you care about.
> 

I think I have found a better way to handle it, please check v5, did you
receive the series?

The new series just make the driver pass the partition node and the OF
code just take it and use it.

This way we don't have to parse the disk name at all and it's driver
specific work to select the "partitions" node if multiple are present.

I feel your hint produced a much better implementation without having to
pollute the block code with specific case.

-- 
	Ansuel

[RFC PATCH 4/4] dt-bindings: mmc: Document support for partition table in mmc-card

[Christian Marangi]

Document support for defining a partition table in the mmc-card node.

This is needed if the eMMC doesn't have a partition table written and
the bootloader of the device load data by using absolute offset of the
block device. This is common on embedded device that have eMMC installed
to save space and have non removable block devices.

eMMC provide a generic disk for user data and if supported also provide
one or two additional disk (boot0 and boot1) for special usage of boot
operation where normally is stored the bootloader or boot info.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/mmc/mmc-card.yaml     | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.yaml b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
index fd347126449a..fab9fa5c170a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-card.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
@@ -13,6 +13,10 @@ description: |
   This documents describes the devicetree bindings for a mmc-host controller
   child node describing a mmc-card / an eMMC.
 
+  It's possible to define a fixed partition table for an eMMC for the user
+  partition and one of the 2 boot partition (boot0/boot1) if supported by the
+  eMMC.
+
 properties:
   compatible:
     const: mmc-card
@@ -26,6 +30,48 @@ properties:
       Use this to indicate that the mmc-card has a broken hpi
       implementation, and that hpi should not be used.
 
+  "#address-cells": true
+
+  "#size-cells": true
+
+patternProperties:
+  "^partitions(-boot[01])?$":
+    type: object
+
+    properties:
+      "#address-cells": true
+
+      "#size-cells": true
+
+    patternProperties:
+      "@[0-9a-f]+$":
+        type: object
+
+        properties:
+          reg:
+            description: partition's offset and size within the flash (in sector
+              block, 512byte)
+            maxItems: 1
+
+
+          label:
+            description: The label / name for this partition.
+
+          read-only:
+            description: This parameter, if present, is a hint that this partition
+              should only be mounted read-only. This is usually used for flash
+              partitions containing early-boot firmware images or data which should
+              not be clobbered.
+            type: boolean
+
+        required:
+          - reg
+          - label
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 required:
   - compatible
   - reg
@@ -42,6 +88,35 @@ examples:
             compatible = "mmc-card";
             reg = <0>;
             broken-hpi;
+
+            #address-cells = <0>;
+            #size-cells = <0>;
+
+            partitions {
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                partition@0 {
+                    label = "kernel"; /* Kernel */
+                    reg = <0x0 0x10000>; /* 32 MB */
+                };
+
+                partition@3400 {
+                    label = "rootfs";
+                    reg = <0x3400 0x200000>; /* 1GB */
+                };
+            };
+
+            partitions-boot0 {
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                partition@0 {
+                    label = "bl";
+                    reg = <0x0 0x10000>; /* 32MB */
+                    read-only;
+                };
+            };
         };
     };
 
-- 
2.45.2

Re: [RFC PATCH 4/4] dt-bindings: mmc: Document support for partition table in mmc-card

[Rob Herring]

On Mon, Sep 23, 2024 at 12:59:33PM +0200, Christian Marangi wrote:
> Document support for defining a partition table in the mmc-card node.
> 
> This is needed if the eMMC doesn't have a partition table written and
> the bootloader of the device load data by using absolute offset of the
> block device. This is common on embedded device that have eMMC installed
> to save space and have non removable block devices.

What if the partition table is written? What does one use? One of them 
or both and merge them?

> eMMC provide a generic disk for user data and if supported also provide
> one or two additional disk (boot0 and boot1) for special usage of boot
> operation where normally is stored the bootloader or boot info.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/mmc/mmc-card.yaml     | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.yaml b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> index fd347126449a..fab9fa5c170a 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> @@ -13,6 +13,10 @@ description: |
>    This documents describes the devicetree bindings for a mmc-host controller
>    child node describing a mmc-card / an eMMC.
>  
> +  It's possible to define a fixed partition table for an eMMC for the user
> +  partition and one of the 2 boot partition (boot0/boot1) if supported by the
> +  eMMC.
> +
>  properties:
>    compatible:
>      const: mmc-card
> @@ -26,6 +30,48 @@ properties:
>        Use this to indicate that the mmc-card has a broken hpi
>        implementation, and that hpi should not be used.
>  
> +  "#address-cells": true
> +
> +  "#size-cells": true
> +
> +patternProperties:
> +  "^partitions(-boot[01])?$":
> +    type: object

You don't define this is fixed partitions with a fixed-partitions 
compatible. Why not reuse that? Then this all goes away with a 
reference to it.

> +
> +    properties:
> +      "#address-cells": true
> +
> +      "#size-cells": true
> +
> +    patternProperties:
> +      "@[0-9a-f]+$":
> +        type: object
> +
> +        properties:
> +          reg:
> +            description: partition's offset and size within the flash (in sector
> +              block, 512byte)

Units are sectors? Use bytes instead because everything else does in DT. 

> +            maxItems: 1
> +
> +
> +          label:
> +            description: The label / name for this partition.
> +
> +          read-only:
> +            description: This parameter, if present, is a hint that this partition
> +              should only be mounted read-only. This is usually used for flash
> +              partitions containing early-boot firmware images or data which should
> +              not be clobbered.
> +            type: boolean
> +
> +        required:
> +          - reg
> +          - label
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false

Put the indented cases of additionalProperties/unevaluatedProperties 
before 'properties'. Easier to see what they apply to that way.

Rob

Re: [RFC PATCH 4/4] dt-bindings: mmc: Document support for partition table in mmc-card

[Christian Marangi]

On Tue, Sep 24, 2024 at 05:53:43PM -0500, Rob Herring wrote:
> On Mon, Sep 23, 2024 at 12:59:33PM +0200, Christian Marangi wrote:
> > Document support for defining a partition table in the mmc-card node.
> > 
> > This is needed if the eMMC doesn't have a partition table written and
> > the bootloader of the device load data by using absolute offset of the
> > block device. This is common on embedded device that have eMMC installed
> > to save space and have non removable block devices.
> 
> What if the partition table is written? What does one use? One of them 
> or both and merge them?
>

Hi Rob,
thanks a lot for the review!

The block code parse partition table with some kind of priority system.

Example if cmdline is found, then anything else is ignored. (simple
logic, first parser that match an expected structure win)

We apply the same logic. So with partition table defined in OF, then
anything written will be ignored.

> > eMMC provide a generic disk for user data and if supported also provide
> > one or two additional disk (boot0 and boot1) for special usage of boot
> > operation where normally is stored the bootloader or boot info.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/mmc/mmc-card.yaml     | 75 +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.yaml b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> > index fd347126449a..fab9fa5c170a 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
> > @@ -13,6 +13,10 @@ description: |
> >    This documents describes the devicetree bindings for a mmc-host controller
> >    child node describing a mmc-card / an eMMC.
> >  
> > +  It's possible to define a fixed partition table for an eMMC for the user
> > +  partition and one of the 2 boot partition (boot0/boot1) if supported by the
> > +  eMMC.
> > +
> >  properties:
> >    compatible:
> >      const: mmc-card
> > @@ -26,6 +30,48 @@ properties:
> >        Use this to indicate that the mmc-card has a broken hpi
> >        implementation, and that hpi should not be used.
> >  
> > +  "#address-cells": true
> > +
> > +  "#size-cells": true
> > +
> > +patternProperties:
> > +  "^partitions(-boot[01])?$":
> > +    type: object
> 
> You don't define this is fixed partitions with a fixed-partitions 
> compatible. Why not reuse that? Then this all goes away with a 
> reference to it.
> 

My problem is that the fixed-partition schema in MTD have some
additional property that can't be supported.

Ideally I should define a generic schema that can be shared and then
expand it in MTD. Any hint on the directory structure tho?

Where should I put this generic schema?

> > +
> > +    properties:
> > +      "#address-cells": true
> > +
> > +      "#size-cells": true
> > +
> > +    patternProperties:
> > +      "@[0-9a-f]+$":
> > +        type: object
> > +
> > +        properties:
> > +          reg:
> > +            description: partition's offset and size within the flash (in sector
> > +              block, 512byte)
> 
> Units are sectors? Use bytes instead because everything else does in DT. 
> 

Ok will try to convert value to bytes internally.

> > +            maxItems: 1
> > +
> > +
> > +          label:
> > +            description: The label / name for this partition.
> > +
> > +          read-only:
> > +            description: This parameter, if present, is a hint that this partition
> > +              should only be mounted read-only. This is usually used for flash
> > +              partitions containing early-boot firmware images or data which should
> > +              not be clobbered.
> > +            type: boolean
> > +
> > +        required:
> > +          - reg
> > +          - label
> > +
> > +        additionalProperties: false
> > +
> > +    additionalProperties: false
> 
> Put the indented cases of additionalProperties/unevaluatedProperties 
> before 'properties'. Easier to see what they apply to that way.
> 

ack.

-- 
	Ansuel