Hi Christian,
Thanks for working on this, it will be useful for me as well.
Some comments inside.
On Sun, Sep 29, 2024 at 04:06:19PM +0200, Christian Marangi wrote:
> 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 with
> "fixed-partitions" compatible 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. If label is not declared, the node name
> is used as fallback. Eventually is also possible to declare the read-only
> property to flag the partition as read-only.
>
> For eMMC block, 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 as supported in
> JEDEC 4.4+. If this suffix is detected, "partitions-boot0" or
> "partitions-boot1" are used instead of the generic "partitions" for the
> relevant disk.
>
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
> block/partitions/Kconfig | 8 ++
> block/partitions/Makefile | 1 +
> block/partitions/check.h | 1 +
> block/partitions/core.c | 3 +
> block/partitions/of.c | 151 ++++++++++++++++++++++++++++++++++++++
> 5 files changed, 164 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
Should be "device tree partition support".
> + 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/of.c b/block/partitions/of.c
> new file mode 100644
> index 000000000000..bc6200eb86b3
> --- /dev/null
> +++ b/block/partitions/of.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/blkdev.h>
> +#include <linux/major.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,
> + struct gendisk *disk)
> +{
> + const char *node_name = "partitions";
> +
> + /*
> + * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> + * present on every eMMC. These additional partition are always hardcoded
> + * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> + * store keys and exposed as a char device, the other 2 are exposed as
> + * real separate disk with the boot0/1 appended to the disk name.
> + *
> + * Here we parse the disk_name in search for such suffix and select
> + * the correct partition node.
> + */
> + if (disk->major == MMC_BLOCK_MAJOR) {
> + const char *disk_name = disk->disk_name;
> +
> + 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 int validate_of_partition(struct device_node *np, int slot)
> +{
> + int a_cells, s_cells;
> + 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);
> +
The corresponding mtd ofpart parser validates a_cells + s_cells against
len, like this:
if (len / 4 != a_cells + s_cells) {
pr_debug("%s: ofpart partition %pOF (%pOF) error parsing reg property.\n",
master->name, pp,
mtd_node);
goto ofpart_fail;
}
I think you should do it here as well.
> + /*
> + * Validate offset conversion from bytes to sectors.
> + * Only the first partition is allowed to have offset 0.
> + */
Where is this constraint coming from? I would put the partitions in
order into the device tree as well, but the binding doesn't enforce it
and I see no reason to do so.
> + offset = of_read_number(reg, a_cells);
> + if (do_div(offset, SECTOR_SIZE) ||
How about (offset % SECTOR_SIZE) or (offset & (SECTOR_SIZE - 1))? Might
be a bit more intuitive to read.
> + (slot > 1 && !offset))
> + return -EINVAL;
> +
> + /* Validate size conversion from bytes to sectors */
> + size = of_read_number(reg + a_cells, s_cells);
> + if (do_div(size, SECTOR_SIZE) || !size)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +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);
> +
> + /* Convert bytes to sector size */
> + offset = of_read_number(reg, a_cells) / SECTOR_SIZE;
> + size = of_read_number(reg + a_cells, s_cells) / SECTOR_SIZE;
> +
> + put_partition(state, slot, offset, size);
> +
> + if (of_property_read_bool(np, "read-only"))
> + state->parts[slot].flags |= ADDPART_FLAG_READONLY;
> +
> + /*
> + * Follow MTD label logic, search for label property,
> + * fallback to node name if not found.
> + */
> + info = &state->parts[slot].info;
> + partname = of_get_property(np, "label", &len);
> + if (!partname)
> + partname = of_get_property(np, "name", &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;
> +
> + disk_np = of_node_get(ddev->parent->of_node);
> + if (!disk_np)
> + return 0;
> +
> + partitions_np = get_partitions_node(disk_np, state->disk);
> + if (!partitions_np ||
> + !of_device_is_compatible(partitions_np, "fixed-partitions"))
> + return 0;
of_node_put(disk_np) missing here before return.
> +
> + /* Check if child are over the limit */
> + slot = of_get_child_count(partitions_np);
> + if (slot >= state->limit)
> + goto err;
Other partition parsers just silently ignore the partitions
exceeding state->limit instead of throwing an error. Maybe do the same
here?
> +
> + slot = 1;
> + /* Validate parition offset and size */
> + for_each_child_of_node(partitions_np, np) {
> + if (validate_of_partition(np, slot))
> + goto err;
> +
> + slot++;
> + }
> +
> + slot = 1;
> + for_each_child_of_node(partitions_np, np) {
> + add_of_partition(state, slot, np);
> +
> + slot++;
> + }
> +
> + strlcat(state->pp_buf, "\n", PAGE_SIZE);
> +
> + return 1;
> +err:
> + of_node_put(partitions_np);
> + of_node_put(disk_np);
You should put the nodes for the non error case as well.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
