[PATCH u-boot-dm + u-boot-spi v2 3/7] mtd: add support for parsing partitions defined in OF

["Pali Rohár" <pali@kernel.org>]

On Tuesday 09 February 2021 15:44:48 Marek Beh?n wrote:
> Add support for parsing partitions defined in device-trees via the
> `partitions` node with `fixed-partitions` compatible.
> 
> The `mtdparts`/`mtdids` mechanism takes precedence. If some partitions
> are defined for a MTD device via this mechanism, the code won't register
> partitions for that MTD device from OF, even if they are defined.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/mtd/mtd_uboot.c | 106 +++++++++++++++++++++++-----------------
>  drivers/mtd/mtdpart.c   |  60 +++++++++++++++++++++++
>  include/linux/mtd/mtd.h |   9 ++++
>  3 files changed, 131 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
> index 9360d4ed17..7fb72eb1f4 100644
> --- a/drivers/mtd/mtd_uboot.c
> +++ b/drivers/mtd/mtd_uboot.c
> @@ -197,53 +197,11 @@ static void mtd_del_all_parts(void)
>  	} while (ret > 0);
>  }
>  
> -int mtd_probe_devices(void)
> +static int parse_mtdparts(const char *mtdparts, const char *mtdids)
>  {
> -	static char *old_mtdparts;
> -	static char *old_mtdids;
> -	const char *mtdparts = get_mtdparts();
> -	const char *mtdids = get_mtdids();
> -	const char *mtdparts_next = mtdparts;
> +	const char *mtdparts_next;
>  	struct mtd_info *mtd;
>  
> -	mtd_probe_uclass_mtd_devs();
> -
> -	/*
> -	 * Check if mtdparts/mtdids changed, if the MTD dev list was updated
> -	 * or if our previous attempt to delete existing partititions failed.
> -	 * In any of these cases we want to update the partitions, otherwise,
> -	 * everything is up-to-date and we can return 0 directly.
> -	 */
> -	if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||
> -	    (mtdparts && old_mtdparts && mtdids && old_mtdids &&
> -	     !mtd_dev_list_updated() && !mtd_del_all_parts_failed &&
> -	     !strcmp(mtdparts, old_mtdparts) &&
> -	     !strcmp(mtdids, old_mtdids)))
> -		return 0;
> -
> -	/* Update the local copy of mtdparts */
> -	free(old_mtdparts);
> -	free(old_mtdids);
> -	old_mtdparts = strdup(mtdparts);
> -	old_mtdids = strdup(mtdids);
> -
> -	/*
> -	 * Remove all old parts. Note that partition removal can fail in case
> -	 * one of the partition is still being used by an MTD user, so this
> -	 * does not guarantee that all old partitions are gone.
> -	 */
> -	mtd_del_all_parts();
> -
> -	/*
> -	 * Call mtd_dev_list_updated() to clear updates generated by our own
> -	 * parts removal loop.
> -	 */
> -	mtd_dev_list_updated();
> -
> -	/* If either mtdparts or mtdids is empty, then exit */
> -	if (!mtdparts || !mtdids)
> -		return 0;
> -
>  	/* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */
>  	if (!strncmp(mtdparts, "mtdparts=", sizeof("mtdparts=") - 1))
>  		mtdparts += 9;
> @@ -342,6 +300,66 @@ int mtd_probe_devices(void)
>  		put_mtd_device(mtd);
>  	}
>  
> +	return 0;
> +}
> +
> +int mtd_probe_devices(void)
> +{
> +	static char *old_mtdparts;
> +	static char *old_mtdids;
> +	const char *mtdparts = get_mtdparts();
> +	const char *mtdids = get_mtdids();
> +	struct mtd_info *mtd;
> +
> +	mtd_probe_uclass_mtd_devs();
> +
> +	/*
> +	 * Check if mtdparts/mtdids changed, if the MTD dev list was updated
> +	 * or if our previous attempt to delete existing partititions failed.
> +	 * In any of these cases we want to update the partitions, otherwise,
> +	 * everything is up-to-date and we can return 0 directly.
> +	 */
> +	if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||
> +	    (mtdparts && old_mtdparts && mtdids && old_mtdids &&
> +	     !mtd_dev_list_updated() && !mtd_del_all_parts_failed &&
> +	     !strcmp(mtdparts, old_mtdparts) &&
> +	     !strcmp(mtdids, old_mtdids)))
> +		return 0;
> +
> +	/* Update the local copy of mtdparts */
> +	free(old_mtdparts);
> +	free(old_mtdids);
> +	old_mtdparts = strdup(mtdparts);
> +	old_mtdids = strdup(mtdids);
> +
> +	/*
> +	 * Remove all old parts. Note that partition removal can fail in case
> +	 * one of the partition is still being used by an MTD user, so this
> +	 * does not guarantee that all old partitions are gone.
> +	 */
> +	mtd_del_all_parts();
> +
> +	/*
> +	 * Call mtd_dev_list_updated() to clear updates generated by our own
> +	 * parts removal loop.
> +	 */
> +	mtd_dev_list_updated();
> +
> +	/* If both mtdparts and mtdids are non-empty, parse */
> +	if (mtdparts && mtdids) {
> +		if (parse_mtdparts(mtdparts, mtdids) < 0)
> +			printf("Failed parsing MTD partitions from mtdparts!\n");
> +	}
> +
> +	/* Fallback to OF partitions */
> +	mtd_for_each_device(mtd) {
> +		if (list_empty(&mtd->partitions)) {
> +			if (add_mtd_partitions_of(mtd) < 0)
> +				printf("Failed parsing MTD %s OF partitions!\n",
> +					mtd->name);
> +		}
> +	}
> +
>  	/*
>  	 * Call mtd_dev_list_updated() to clear updates generated by our own
>  	 * parts registration loop.
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index d064ac3048..2ebbf74d92 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -892,6 +892,66 @@ int add_mtd_partitions(struct mtd_info *master,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_DM
> +int add_mtd_partitions_of(struct mtd_info *master)
> +{
> +	ofnode parts, child;
> +	int i = 0;
> +
> +	if (!master->dev)
> +		return 0;
> +
> +	parts = ofnode_find_subnode(mtd_get_ofnode(master), "partitions");
> +	if (!ofnode_valid(parts) || !ofnode_is_available(parts) ||
> +	    !ofnode_device_is_compatible(parts, "fixed-partitions"))
> +		return 0;
> +
> +	ofnode_for_each_subnode(child, parts) {
> +		struct mtd_partition part = { 0 };
> +		struct mtd_info *slave;
> +		fdt_addr_t offset, size;
> +
> +		if (!ofnode_is_available(child))
> +			continue;
> +
> +		if (ofnode_get_property(child, "compatible", NULL))
> +			continue;

I think this check is incorrect. Partition node may have another
"compatible" property with another node. Linux kernel supports defining
partitions on partitions. E.g.

    partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        partition at 0 {
            label = "bootloader";
            reg = <0x000000 0x100000>;
            read-only;
        };

        firmware at 100000 {
            compatible = "brcm,trx";
            label = "firmware";
            reg = <0x100000 0xe00000>;
        };

        calibration at f00000 {
            compatible = "fixed-partitions";
            label = "calibration";
            reg = <0xf00000 0x100000>;
            ranges = <0 0xf00000 0x100000>;
            #address-cells = <1>;
            #size-cells = <1>;

            partition at 0 {
                label = "wifi0";
                reg = <0x000000 0x080000>;
            };

            partition at 80000 {
                label = "wifi1";
                reg = <0x080000 0x080000>;
            };
        };
    };

See Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml

> +
> +		offset = ofnode_get_addr_size_index_notrans(child, 0, &size);
> +		if (offset == FDT_ADDR_T_NONE || !size) {
> +			debug("Missing partition offset/size on \"%s\" partition\n",
> +			      master->name);
> +			continue;
> +		}
> +
> +		part.name = ofnode_read_string(child, "label");
> +		if (!part.name)
> +			part.name = ofnode_read_string(child, "name");
> +
> +		if (ofnode_read_bool(child, "read-only"))
> +			part.mask_flags |= MTD_WRITEABLE;
> +		if (ofnode_read_bool(child, "lock"))
> +			part.mask_flags |= MTD_POWERUP_LOCK;

I would suggest to add some explanation why setting these two mask flags
according to those DT properties is correct as it is not obvious.

mask_flags contains flags which have to be removed from partition.
DT "read-only" means that partition is read-only, so removing writable
is correct. DR "lock" means to *not* unlock the partition at
initialization time and MTD_POWERUP_LOCK means that partition is always
locked after reset and needs to be (always) unlocked. So masking this
flag means that it is never unlocked.

> +
> +		part.offset = offset;
> +		part.size = size;
> +		part.ecclayout = master->ecclayout;
> +
> +		slave = allocate_partition(master, &part, i++, 0);
> +		if (IS_ERR(slave))
> +			return PTR_ERR(slave);
> +
> +		mutex_lock(&mtd_partitions_mutex);
> +		list_add_tail(&slave->node, &master->partitions);
> +		mutex_unlock(&mtd_partitions_mutex);
> +
> +		add_mtd_device(slave);
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
>  #ifndef __UBOOT__
>  static DEFINE_SPINLOCK(part_parser_lock);
>  static LIST_HEAD(part_parsers);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 927854950a..ec9331b804 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -581,6 +581,15 @@ static inline int del_mtd_partitions(struct mtd_info *mtd)
>  }
>  #endif
>  
> +#if defined(CONFIG_MTD_PARTITIONS) && defined(CONFIG_DM)
> +int add_mtd_partitions_of(struct mtd_info *master);
> +#else
> +static inline int add_mtd_partitions_of(struct mtd_info *master)
> +{
> +	return 0;
> +}
> +#endif
> +
>  struct mtd_info *__mtd_next_device(int i);
>  #define mtd_for_each_device(mtd)			\
>  	for ((mtd) = __mtd_next_device(0);		\
> -- 
> 2.26.2
>