[PATCH 1/2] dt-bindings: mtd: explicitly describe nesting partitions

[PATCH 1/2] dt-bindings: mtd: explicitly describe nesting partitions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Documentation was already saying that fixed and dynamic partitioning can
be mixed but was missing a clear description and examples. This commit
adds a proper description of how partitions can be nested and how layout
descriptions can be mixed.

This addition is important for partitions that contain subpartitions.
It's useful to have parent partition registered (e.g. for overwriting
purposes) as well as children ones (for accessing data). It's also
required when a single partition uses different partitioning method
(e.g. vendor custom "firmware" partition with kernel + rootfs).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Another example (I couldn't put in Documentation yet) could be:

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

		partition@0 {
			label = "bootloader";
			reg = <0x0000000 0x0100000>;
			read-only;
		};

		firmware@100000 {
			label = "firmware";
			reg = <0x0100000 0x0800000>;
			compatible = "brcm,trx";
		};
	};
};

It's probably even more realistic one, but we don't describe "brcm,trx"
binding yet.

The purpose of above description would be to:
1) Specify fixed partitions (they never change)
2) Tell operating system that "firmware" partition uses Broadcom's TRX
   format which is a container for 2 or 3 subpartitions (usually: kernel
   and rootfs).
---
 .../devicetree/bindings/mtd/partition.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index a8f382642ba9..2214167ce88a 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -14,6 +14,13 @@ method is used for a given flash device. To describe the method there should be
 a subnode of the flash device that is named 'partitions'. It must have a
 'compatible' property, which is used to identify the method to use.
 
+When a single partition is represented with a DT node (it depends on a used
+format) it may also be described using above rules ('compatible' and optionally
+some extra properties / subnodes). It allows describing more complex,
+hierarchical (multi-level) layouts and should be used if there is some
+significant relation between partitions or some partition internally uses
+another partitioning method.
+
 Available bindings are listed in the "partitions" subdirectory.
 
 
@@ -73,6 +80,24 @@ flash@0 {
 		uimage@100000 {
 			reg = <0x0100000 0x200000>;
 		};
+
+		calibration@200000 {
+			label = "calibration";
+			reg = <0x0200000 0x100000>;
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			partition@0 {
+				label = "wifi0";
+				reg = <0x000000 0x080000>;
+			};
+
+			partition@80000 {
+				label = "wifi1";
+				reg = <0x080000 0x080000>;
+			};
+		};
 	};
 };
 
-- 
2.13.6


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: partitions: use DT info for parsing partitions with specified type

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This supports nested partitions in a DT. If selected partition has a
"compatible" property specified it will be parsed looking for
subpartitions.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/mtdpart.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index f8d3a015cdad..52e2cb35fc79 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p)
 	kfree(p);
 }
 
-/**
- * mtd_parse_part - parse MTD partition looking for subpartitions
- *
- * @slave: part that is supposed to be a container and should be parsed
- * @types: NULL-terminated array with names of partition parsers to try
- *
- * Some partitions are kind of containers with extra subpartitions (volumes).
- * There can be various formats of such containers. This function tries to use
- * specified parsers to analyze given partition and registers found
- * subpartitions on success.
- */
-static int mtd_parse_part(struct mtd_part *slave, const char *const *types)
-{
-	return parse_mtd_partitions(&slave->mtd, types, NULL);
-}
-
 static struct mtd_part *allocate_partition(struct mtd_info *parent,
 			const struct mtd_partition *part, int partno,
 			uint64_t cur_offset)
@@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master,
 
 		add_mtd_device(&slave->mtd);
 		mtd_add_partition_attrs(slave);
-		if (parts[i].types)
-			mtd_parse_part(slave, parts[i].types);
+		/* Look for subpartitions */
+		parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
 
 		cur_offset = slave->offset + slave->mtd.size;
 	}
@@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = {
 	NULL
 };
 
+/* Check DT only when looking for subpartitions. */
+static const char * const default_subpartition_types[] = {
+	"ofpart",
+	NULL
+};
+
 static int mtd_part_do_parse(struct mtd_part_parser *parser,
 			     struct mtd_info *master,
 			     struct mtd_partitions *pparts,
@@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master,
 	const char *fixed = "fixed-partitions";
 	int ret, err = 0;
 
-	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
+	np = mtd_get_of_node(master);
+	if (!mtd_is_partition(master))
+		np = of_get_child_by_name(np, "partitions");
 	of_property_for_each_string(np, "compatible", prop, compat) {
 		parser = mtd_part_get_compatible_parser(compat);
 		if (!parser)
@@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	int ret, err = 0;
 
 	if (!types)
-		types = default_mtd_part_types;
+		types = mtd_is_partition(master) ? default_subpartition_types :
+			default_mtd_part_types;
 
 	for ( ; *types; types++) {
 		/*
-- 
2.13.6


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: partitions: use DT info for parsing partitions with specified type

[Boris Brezillon]

On Wed, 23 May 2018 19:14:48 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This supports nested partitions in a DT. If selected partition has a
> "compatible" property specified it will be parsed looking for
> subpartitions.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/mtdpart.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index f8d3a015cdad..52e2cb35fc79 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p)
>  	kfree(p);
>  }
>  
> -/**
> - * mtd_parse_part - parse MTD partition looking for subpartitions
> - *
> - * @slave: part that is supposed to be a container and should be parsed
> - * @types: NULL-terminated array with names of partition parsers to try
> - *
> - * Some partitions are kind of containers with extra subpartitions (volumes).
> - * There can be various formats of such containers. This function tries to use
> - * specified parsers to analyze given partition and registers found
> - * subpartitions on success.
> - */
> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types)
> -{
> -	return parse_mtd_partitions(&slave->mtd, types, NULL);
> -}
> -
>  static struct mtd_part *allocate_partition(struct mtd_info *parent,
>  			const struct mtd_partition *part, int partno,
>  			uint64_t cur_offset)
> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master,
>  
>  		add_mtd_device(&slave->mtd);
>  		mtd_add_partition_attrs(slave);
> -		if (parts[i].types)
> -			mtd_parse_part(slave, parts[i].types);
> +		/* Look for subpartitions */
> +		parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
>  
>  		cur_offset = slave->offset + slave->mtd.size;
>  	}
> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = {
>  	NULL
>  };
>  
> +/* Check DT only when looking for subpartitions. */
> +static const char * const default_subpartition_types[] = {
> +	"ofpart",
> +	NULL
> +};
> +
>  static int mtd_part_do_parse(struct mtd_part_parser *parser,
>  			     struct mtd_info *master,
>  			     struct mtd_partitions *pparts,
> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master,
>  	const char *fixed = "fixed-partitions";
>  	int ret, err = 0;
>  
> -	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
> +	np = mtd_get_of_node(master);
> +	if (!mtd_is_partition(master))
> +		np = of_get_child_by_name(np, "partitions");
>  	of_property_for_each_string(np, "compatible", prop, compat) {
>  		parser = mtd_part_get_compatible_parser(compat);
>  		if (!parser)
> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	int ret, err = 0;
>  
>  	if (!types)
> -		types = default_mtd_part_types;
> +		types = mtd_is_partition(master) ? default_subpartition_types :
> +			default_mtd_part_types;

Hm, that means the subparts inherit the parser types from their parent
if types != NULL? Is that really what we want? And if that's what we
want, why don't we do the same for types == NULL?

>  
>  	for ( ; *types; types++) {
>  		/*


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: partitions: use DT info for parsing partitions with specified type

[Rafał Miłecki]

On 23.05.2018 20:24, Boris Brezillon wrote:
> On Wed, 23 May 2018 19:14:48 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
> 
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This supports nested partitions in a DT. If selected partition has a
>> "compatible" property specified it will be parsed looking for
>> subpartitions.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   drivers/mtd/mtdpart.c | 33 +++++++++++++--------------------
>>   1 file changed, 13 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> index f8d3a015cdad..52e2cb35fc79 100644
>> --- a/drivers/mtd/mtdpart.c
>> +++ b/drivers/mtd/mtdpart.c
>> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p)
>>   	kfree(p);
>>   }
>>   
>> -/**
>> - * mtd_parse_part - parse MTD partition looking for subpartitions
>> - *
>> - * @slave: part that is supposed to be a container and should be parsed
>> - * @types: NULL-terminated array with names of partition parsers to try
>> - *
>> - * Some partitions are kind of containers with extra subpartitions (volumes).
>> - * There can be various formats of such containers. This function tries to use
>> - * specified parsers to analyze given partition and registers found
>> - * subpartitions on success.
>> - */
>> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types)
>> -{
>> -	return parse_mtd_partitions(&slave->mtd, types, NULL);
>> -}
>> -
>>   static struct mtd_part *allocate_partition(struct mtd_info *parent,
>>   			const struct mtd_partition *part, int partno,
>>   			uint64_t cur_offset)
>> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master,
>>   
>>   		add_mtd_device(&slave->mtd);
>>   		mtd_add_partition_attrs(slave);
>> -		if (parts[i].types)
>> -			mtd_parse_part(slave, parts[i].types);
>> +		/* Look for subpartitions */
>> +		parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
>>   
>>   		cur_offset = slave->offset + slave->mtd.size;
>>   	}
>> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = {
>>   	NULL
>>   };
>>   
>> +/* Check DT only when looking for subpartitions. */
>> +static const char * const default_subpartition_types[] = {
>> +	"ofpart",
>> +	NULL
>> +};
>> +
>>   static int mtd_part_do_parse(struct mtd_part_parser *parser,
>>   			     struct mtd_info *master,
>>   			     struct mtd_partitions *pparts,
>> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master,
>>   	const char *fixed = "fixed-partitions";
>>   	int ret, err = 0;
>>   
>> -	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
>> +	np = mtd_get_of_node(master);
>> +	if (!mtd_is_partition(master))
>> +		np = of_get_child_by_name(np, "partitions");
>>   	of_property_for_each_string(np, "compatible", prop, compat) {
>>   		parser = mtd_part_get_compatible_parser(compat);
>>   		if (!parser)
>> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>>   	int ret, err = 0;
>>   
>>   	if (!types)
>> -		types = default_mtd_part_types;
>> +		types = mtd_is_partition(master) ? default_subpartition_types :
>> +			default_mtd_part_types;
> 
> Hm, that means the subparts inherit the parser types from their parent
> if types != NULL? Is that really what we want? And if that's what we
> want, why don't we do the same for types == NULL?

No, unless I'm missing something. In add_mtd_partitions() there is now
a following call:
parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);

In most cases parts[i].types is NULL, unless you're using some
exceptional driver (like bcm47xxpart) which sets "types".

So for each partition parse_mtd_partitions() will be called with "types"
argument almost always set to NULL (no inheriting). That will result in
picking default_subpartition_types.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: partitions: use DT info for parsing partitions with specified type

[Boris Brezillon]

On Thu, 24 May 2018 07:50:10 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> On 23.05.2018 20:24, Boris Brezillon wrote:
> > On Wed, 23 May 2018 19:14:48 +0200
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >   
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> This supports nested partitions in a DT. If selected partition has a
> >> "compatible" property specified it will be parsed looking for
> >> subpartitions.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >>   drivers/mtd/mtdpart.c | 33 +++++++++++++--------------------
> >>   1 file changed, 13 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> >> index f8d3a015cdad..52e2cb35fc79 100644
> >> --- a/drivers/mtd/mtdpart.c
> >> +++ b/drivers/mtd/mtdpart.c
> >> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p)
> >>   	kfree(p);
> >>   }
> >>   
> >> -/**
> >> - * mtd_parse_part - parse MTD partition looking for subpartitions
> >> - *
> >> - * @slave: part that is supposed to be a container and should be parsed
> >> - * @types: NULL-terminated array with names of partition parsers to try
> >> - *
> >> - * Some partitions are kind of containers with extra subpartitions (volumes).
> >> - * There can be various formats of such containers. This function tries to use
> >> - * specified parsers to analyze given partition and registers found
> >> - * subpartitions on success.
> >> - */
> >> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types)
> >> -{
> >> -	return parse_mtd_partitions(&slave->mtd, types, NULL);
> >> -}
> >> -
> >>   static struct mtd_part *allocate_partition(struct mtd_info *parent,
> >>   			const struct mtd_partition *part, int partno,
> >>   			uint64_t cur_offset)
> >> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master,
> >>   
> >>   		add_mtd_device(&slave->mtd);
> >>   		mtd_add_partition_attrs(slave);
> >> -		if (parts[i].types)
> >> -			mtd_parse_part(slave, parts[i].types);
> >> +		/* Look for subpartitions */
> >> +		parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
> >>   
> >>   		cur_offset = slave->offset + slave->mtd.size;
> >>   	}
> >> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = {
> >>   	NULL
> >>   };
> >>   
> >> +/* Check DT only when looking for subpartitions. */
> >> +static const char * const default_subpartition_types[] = {
> >> +	"ofpart",
> >> +	NULL
> >> +};
> >> +
> >>   static int mtd_part_do_parse(struct mtd_part_parser *parser,
> >>   			     struct mtd_info *master,
> >>   			     struct mtd_partitions *pparts,
> >> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master,
> >>   	const char *fixed = "fixed-partitions";
> >>   	int ret, err = 0;
> >>   
> >> -	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
> >> +	np = mtd_get_of_node(master);
> >> +	if (!mtd_is_partition(master))
> >> +		np = of_get_child_by_name(np, "partitions");
> >>   	of_property_for_each_string(np, "compatible", prop, compat) {
> >>   		parser = mtd_part_get_compatible_parser(compat);
> >>   		if (!parser)
> >> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
> >>   	int ret, err = 0;
> >>   
> >>   	if (!types)
> >> -		types = default_mtd_part_types;
> >> +		types = mtd_is_partition(master) ? default_subpartition_types :
> >> +			default_mtd_part_types;  
> > 
> > Hm, that means the subparts inherit the parser types from their parent
> > if types != NULL? Is that really what we want? And if that's what we
> > want, why don't we do the same for types == NULL?  
> 
> No, unless I'm missing something. In add_mtd_partitions() there is now
> a following call:
> parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
> 
> In most cases parts[i].types is NULL, unless you're using some
> exceptional driver (like bcm47xxpart) which sets "types".
> 
> So for each partition parse_mtd_partitions() will be called with "types"
> argument almost always set to NULL (no inheriting). That will result in
> picking default_subpartition_types.

And that's the "almost always" I'm not happy with. Either you don't
want to inherit the types from the parent and you do that for both !=
NULL and == NULL, or you inherit it for both.

Also, I'd really like to improve the 'part parser types' selection logic
by letting the boards select the part parsers instead of having the MTD
drivers do it.

Maybe we could have an approach similar to the gpio-lookup-table where
board files could assign a part-parser list to an MTD name, and then let
the core find whether there's a part parser list attached to the MTD
dev at registration time. And for DT-based setup, this information
would just be extracted from the compatible of the 'partitions' node (or
the node directly if the MTD device is a partition).

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] dt-bindings: mtd: explicitly describe nesting partitions

[Rob Herring]

On Wed, May 23, 2018 at 07:14:47PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Documentation was already saying that fixed and dynamic partitioning can
> be mixed but was missing a clear description and examples. This commit
> adds a proper description of how partitions can be nested and how layout
> descriptions can be mixed.
> 
> This addition is important for partitions that contain subpartitions.
> It's useful to have parent partition registered (e.g. for overwriting
> purposes) as well as children ones (for accessing data). It's also
> required when a single partition uses different partitioning method
> (e.g. vendor custom "firmware" partition with kernel + rootfs).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> Another example (I couldn't put in Documentation yet) could be:
> 
> flash@0 {
> 	partitions {
> 		compatible = "fixed-partitions";
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		partition@0 {
> 			label = "bootloader";
> 			reg = <0x0000000 0x0100000>;
> 			read-only;
> 		};
> 
> 		firmware@100000 {
> 			label = "firmware";
> 			reg = <0x0100000 0x0800000>;
> 			compatible = "brcm,trx";
> 		};
> 	};
> };
> 
> It's probably even more realistic one, but we don't describe "brcm,trx"
> binding yet.

This one makes more sense to me than the one you've added because if you 
only have fixed partitions, it seems like most times you could just 
flatten them to 1 level. I suppose having some levels could make doing 
updates easier.

> The purpose of above description would be to:
> 1) Specify fixed partitions (they never change)
> 2) Tell operating system that "firmware" partition uses Broadcom's TRX
>    format which is a container for 2 or 3 subpartitions (usually: kernel
>    and rootfs).
> ---
>  .../devicetree/bindings/mtd/partition.txt          | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> index a8f382642ba9..2214167ce88a 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -14,6 +14,13 @@ method is used for a given flash device. To describe the method there should be
>  a subnode of the flash device that is named 'partitions'. It must have a
>  'compatible' property, which is used to identify the method to use.
>  
> +When a single partition is represented with a DT node (it depends on a used
> +format) it may also be described using above rules ('compatible' and optionally
> +some extra properties / subnodes). It allows describing more complex,
> +hierarchical (multi-level) layouts and should be used if there is some
> +significant relation between partitions or some partition internally uses
> +another partitioning method.
> +
>  Available bindings are listed in the "partitions" subdirectory.
>  
>  
> @@ -73,6 +80,24 @@ flash@0 {
>  		uimage@100000 {
>  			reg = <0x0100000 0x200000>;
>  		};
> +
> +		calibration@200000 {
> +			label = "calibration";
> +			reg = <0x0200000 0x100000>;
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;

You are missing 'ranges' here.

> +
> +			partition@0 {
> +				label = "wifi0";
> +				reg = <0x000000 0x080000>;
> +			};
> +
> +			partition@80000 {
> +				label = "wifi1";
> +				reg = <0x080000 0x080000>;
> +			};
> +		};
>  	};
>  };
>  
> -- 
> 2.13.6
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/