[PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Rafał Miłecki]

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

This change simplifies main parsing loop logic a bit. In future it may
be useful for moving TRX support to separated module / parser (if we
implement support for them at some point).
Finally parsing TRX at the end puts us in a better position as we have
better flash layout knowledge. It may be useful e.g. if it appears there
is more than 1 TRX partition.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/bcm47xxpart.c | 124 ++++++++++++++++++++++++++++------------------
 1 file changed, 77 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index 283ff7e..d7d1b6e 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -83,6 +83,70 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
 	return "rootfs";
 }
 
+static int bcm47xxpart_parse_trx(struct mtd_info *master,
+				 struct mtd_partition *trx,
+				 struct mtd_partition *parts,
+				 size_t parts_len)
+{
+	struct mtd_partition *part;
+	struct trx_header header;
+	size_t bytes_read;
+	int curr_part = 0;
+	int i, err;
+
+	if (parts_len < 3) {
+		pr_warn("No enough space to add TRX partitions!\n");
+		return -ENOMEM;
+	}
+
+	err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
+		       (uint8_t *)&header);
+	if (err && !mtd_is_bitflip(err)) {
+		pr_err("mtd_read error while reading TRX header: %d\n", err);
+		return err;
+	}
+
+	i = 0;
+
+	/* We have LZMA loader if offset[2] points to sth */
+	if (header.offset[2]) {
+		part = &parts[curr_part++];
+		part->name = "loader";
+		part->offset = trx->offset + header.offset[i];
+		i++;
+	}
+
+	if (header.offset[i]) {
+		part = &parts[curr_part++];
+		part->name = "linux";
+		part->offset = trx->offset + header.offset[i];
+		i++;
+	}
+
+	if (header.offset[i]) {
+		size_t offset = trx->offset + header.offset[i];
+
+		part = &parts[curr_part++];
+		part->name = bcm47xxpart_trx_data_part_name(master, offset);
+		part->offset = offset;
+		i++;
+	}
+
+	/*
+	 * Assume that every partition ends at the beginning of the one it is
+	 * followed by.
+	 */
+	for (i = 0; i < curr_part; i++) {
+		u64 next_part_offset = (i < curr_part - 1) ?
+					parts[i + 1].offset :
+					trx->offset + trx->size;
+
+		parts[i].size = next_part_offset - parts[i].offset;
+	}
+
+	return curr_part;
+}
+
 static int bcm47xxpart_parse(struct mtd_info *master,
 			     const struct mtd_partition **pparts,
 			     struct mtd_part_parser_data *data)
@@ -93,9 +157,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	size_t bytes_read;
 	uint32_t offset;
 	uint32_t blocksize = master->erasesize;
-	struct trx_header *trx;
 	int trx_part = -1;
-	int last_trx_part = -1;
 	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
 	int err;
 
@@ -182,54 +244,14 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 
 		/* TRX */
 		if (buf[0x000 / 4] == TRX_MAGIC) {
-			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
-				pr_warn("Not enough partitions left to register trx, scanning stopped!\n");
-				break;
-			}
-
-			trx = (struct trx_header *)buf;
+			struct trx_header *trx;
 
 			trx_part = curr_part;
 			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
 					     offset, 0);
 
-			i = 0;
-			/* We have LZMA loader if offset[2] points to sth */
-			if (trx->offset[2]) {
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     "loader",
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			if (trx->offset[i]) {
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     "linux",
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			/*
-			 * Pure rootfs size is known and can be calculated as:
-			 * trx->length - trx->offset[i]. We don't fill it as
-			 * we want to have jffs2 (overlay) in the same mtd.
-			 */
-			if (trx->offset[i]) {
-				const char *name;
-
-				name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]);
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     name,
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			last_trx_part = curr_part - 1;
-
 			/* Jump to the end of TRX */
+			trx = (struct trx_header *)buf;
 			offset = roundup(offset + trx->length, blocksize);
 			/* Next loop iteration will increase the offset */
 			offset -= blocksize;
@@ -307,9 +329,17 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 				       parts[i + 1].offset : master->size;
 
 		parts[i].size = next_part_offset - parts[i].offset;
-		if (i == last_trx_part && trx_part >= 0)
-			parts[trx_part].size = next_part_offset -
-					       parts[trx_part].offset;
+	}
+
+	/* If there was TRX parse it now */
+	if (trx_part >= 0) {
+		int num_parts;
+
+		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
+						  parts + curr_part,
+						  BCM47XXPART_MAX_PARTS - curr_part);
+		if (num_parts > 0)
+			curr_part += num_parts;
 	}
 
 	*pparts = parts;
-- 
2.10.1

[PATCH 2/2] mtd: bcm47xxpart: support layouts with multiple TRX partitions

[Rafał Miłecki]

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

Some devices may have an extra TRX partition used as failsafe one. If
we detect such partition we should set a proper name for it and don't
parse it.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/bcm47xxpart.c | 56 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index d7d1b6e..95cfd88 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -9,6 +9,7 @@
  *
  */
 
+#include <linux/bcm47xx_nvram.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -147,6 +148,30 @@ static int bcm47xxpart_parse_trx(struct mtd_info *master,
 	return curr_part;
 }
 
+/**
+ * bcm47xxpart_bootpartition - gets index of TRX partition used by bootloader
+ *
+ * Some devices may have more than one TRX partition. In such case one of them
+ * is the main one and another a failsafe one. Bootloader may fallback to the
+ * failsafe firmware if it detects corruption of the main image.
+ *
+ * This function provides info about currently used TRX partition. It's the one
+ * containing kernel started by the bootloader.
+ */
+static int bcm47xxpart_bootpartition(void)
+{
+	char buf[4];
+	int bootpartition;
+
+	/* Check CFE environment variable */
+	if (bcm47xx_nvram_getenv("bootpartition", buf, sizeof(buf)) > 0) {
+		if (!kstrtoint(buf, 0, &bootpartition))
+			return bootpartition;
+	}
+
+	return 0;
+}
+
 static int bcm47xxpart_parse(struct mtd_info *master,
 			     const struct mtd_partition **pparts,
 			     struct mtd_part_parser_data *data)
@@ -157,7 +182,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	size_t bytes_read;
 	uint32_t offset;
 	uint32_t blocksize = master->erasesize;
-	int trx_part = -1;
+	int trx_parts[2]; /* Array with indexes of TRX partitions */
+	int trx_num = 0; /* Number of found TRX partitions */
 	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
 	int err;
 
@@ -246,7 +272,11 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 		if (buf[0x000 / 4] == TRX_MAGIC) {
 			struct trx_header *trx;
 
-			trx_part = curr_part;
+			if (trx_num >= ARRAY_SIZE(trx_parts))
+				pr_warn("No enough space to store another TRX found at 0x%X\n",
+					offset);
+			else
+				trx_parts[trx_num++] = curr_part;
 			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
 					     offset, 0);
 
@@ -332,14 +362,20 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	}
 
 	/* If there was TRX parse it now */
-	if (trx_part >= 0) {
-		int num_parts;
-
-		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
-						  parts + curr_part,
-						  BCM47XXPART_MAX_PARTS - curr_part);
-		if (num_parts > 0)
-			curr_part += num_parts;
+	for (i = 0; i < trx_num; i++) {
+		struct mtd_partition *trx = &parts[trx_parts[i]];
+
+		if (i == bcm47xxpart_bootpartition()) {
+			int num_parts;
+
+			num_parts = bcm47xxpart_parse_trx(master, trx,
+							  parts + curr_part,
+							  BCM47XXPART_MAX_PARTS - curr_part);
+			if (num_parts > 0)
+				curr_part += num_parts;
+		} else {
+			trx->name = "failsafe";
+		}
 	}
 
 	*pparts = parts;
-- 
2.10.1

Re: [PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 01/10/2017 12:50 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This change simplifies main parsing loop logic a bit. In future it may
> be useful for moving TRX support to separated module / parser (if we
> implement support for them at some point).
> Finally parsing TRX at the end puts us in a better position as we have
> better flash layout knowledge. It may be useful e.g. if it appears there
> is more than 1 TRX partition.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/bcm47xxpart.c | 124 ++++++++++++++++++++++++++++------------------
>  1 file changed, 77 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> index 283ff7e..d7d1b6e 100644
> --- a/drivers/mtd/bcm47xxpart.c
> +++ b/drivers/mtd/bcm47xxpart.c
> @@ -83,6 +83,70 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>  	return "rootfs";
>  }
>  
> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
> +				 struct mtd_partition *trx,
> +				 struct mtd_partition *parts,
> +				 size_t parts_len)
> +{
> +	struct mtd_partition *part;
> +	struct trx_header header;
> +	size_t bytes_read;
> +	int curr_part = 0;
> +	int i, err;
> +
> +	if (parts_len < 3) {
> +		pr_warn("No enough space to add TRX partitions!\n");
> +		return -ENOMEM;
> +	}
> +
> +	err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
> +		       (uint8_t *)&header);
> +	if (err && !mtd_is_bitflip(err)) {
> +		pr_err("mtd_read error while reading TRX header: %d\n", err);
> +		return err;
> +	}
> +
> +	i = 0;
> +
> +	/* We have LZMA loader if offset[2] points to sth */
> +	if (header.offset[2]) {
> +		part = &parts[curr_part++];
> +		part->name = "loader";
> +		part->offset = trx->offset + header.offset[i];
> +		i++;
> +	}
> +
> +	if (header.offset[i]) {
> +		part = &parts[curr_part++];
> +		part->name = "linux";
> +		part->offset = trx->offset + header.offset[i];
> +		i++;
> +	}
> +
> +	if (header.offset[i]) {
> +		size_t offset = trx->offset + header.offset[i];
> +
> +		part = &parts[curr_part++];
> +		part->name = bcm47xxpart_trx_data_part_name(master, offset);
> +		part->offset = offset;

Why don't you still use bcm47xxpart_add_part() here ?

> +		i++;
> +	}
> +
> +	/*
> +	 * Assume that every partition ends at the beginning of the one it is
> +	 * followed by.
> +	 */
> +	for (i = 0; i < curr_part; i++) {
> +		u64 next_part_offset = (i < curr_part - 1) ?
> +					parts[i + 1].offset :
> +					trx->offset + trx->size;
> +
> +		parts[i].size = next_part_offset - parts[i].offset;
> +	}
> +
> +	return curr_part;
> +}
> +
>  static int bcm47xxpart_parse(struct mtd_info *master,
>  			     const struct mtd_partition **pparts,
>  			     struct mtd_part_parser_data *data)
> @@ -93,9 +157,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  	size_t bytes_read;
>  	uint32_t offset;
>  	uint32_t blocksize = master->erasesize;
> -	struct trx_header *trx;
>  	int trx_part = -1;
> -	int last_trx_part = -1;
>  	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
>  	int err;
>  
> @@ -182,54 +244,14 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  
>  		/* TRX */
>  		if (buf[0x000 / 4] == TRX_MAGIC) {
> -			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
> -				pr_warn("Not enough partitions left to register trx, scanning stopped!\n");
> -				break;
> -			}
> -
> -			trx = (struct trx_header *)buf;
> +			struct trx_header *trx;
>  
>  			trx_part = curr_part;
>  			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
>  					     offset, 0);
>  
> -			i = 0;
> -			/* We have LZMA loader if offset[2] points to sth */
> -			if (trx->offset[2]) {
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     "loader",
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			if (trx->offset[i]) {
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     "linux",
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			/*
> -			 * Pure rootfs size is known and can be calculated as:
> -			 * trx->length - trx->offset[i]. We don't fill it as
> -			 * we want to have jffs2 (overlay) in the same mtd.
> -			 */
> -			if (trx->offset[i]) {
> -				const char *name;
> -
> -				name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]);
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     name,
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			last_trx_part = curr_part - 1;
> -
>  			/* Jump to the end of TRX */
> +			trx = (struct trx_header *)buf;
>  			offset = roundup(offset + trx->length, blocksize);
>  			/* Next loop iteration will increase the offset */
>  			offset -= blocksize;
> @@ -307,9 +329,17 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  				       parts[i + 1].offset : master->size;
>  
>  		parts[i].size = next_part_offset - parts[i].offset;
> -		if (i == last_trx_part && trx_part >= 0)
> -			parts[trx_part].size = next_part_offset -
> -					       parts[trx_part].offset;
> +	}
> +
> +	/* If there was TRX parse it now */
> +	if (trx_part >= 0) {
> +		int num_parts;
> +
> +		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
> +						  parts + curr_part,
> +						  BCM47XXPART_MAX_PARTS - curr_part);
> +		if (num_parts > 0)
> +			curr_part += num_parts;
>  	}
>  
>  	*pparts = parts;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Rafał Miłecki]

On 10 January 2017 at 17:19, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/10/2017 12:50 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This change simplifies main parsing loop logic a bit. In future it may
>> be useful for moving TRX support to separated module / parser (if we
>> implement support for them at some point).
>> Finally parsing TRX at the end puts us in a better position as we have
>> better flash layout knowledge. It may be useful e.g. if it appears there
>> is more than 1 TRX partition.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  drivers/mtd/bcm47xxpart.c | 124 ++++++++++++++++++++++++++++------------------
>>  1 file changed, 77 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>> index 283ff7e..d7d1b6e 100644
>> --- a/drivers/mtd/bcm47xxpart.c
>> +++ b/drivers/mtd/bcm47xxpart.c
>> @@ -83,6 +83,70 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>       return "rootfs";
>>  }
>>
>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>> +                              struct mtd_partition *trx,
>> +                              struct mtd_partition *parts,
>> +                              size_t parts_len)
>> +{
>> +     struct mtd_partition *part;
>> +     struct trx_header header;
>> +     size_t bytes_read;
>> +     int curr_part = 0;
>> +     int i, err;
>> +
>> +     if (parts_len < 3) {
>> +             pr_warn("No enough space to add TRX partitions!\n");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>> +                    (uint8_t *)&header);
>> +     if (err && !mtd_is_bitflip(err)) {
>> +             pr_err("mtd_read error while reading TRX header: %d\n", err);
>> +             return err;
>> +     }
>> +
>> +     i = 0;
>> +
>> +     /* We have LZMA loader if offset[2] points to sth */
>> +     if (header.offset[2]) {
>> +             part = &parts[curr_part++];
>> +             part->name = "loader";
>> +             part->offset = trx->offset + header.offset[i];
>> +             i++;
>> +     }
>> +
>> +     if (header.offset[i]) {
>> +             part = &parts[curr_part++];
>> +             part->name = "linux";
>> +             part->offset = trx->offset + header.offset[i];
>> +             i++;
>> +     }
>> +
>> +     if (header.offset[i]) {
>> +             size_t offset = trx->offset + header.offset[i];
>> +
>> +             part = &parts[curr_part++];
>> +             part->name = bcm47xxpart_trx_data_part_name(master, offset);
>> +             part->offset = offset;
>
> Why don't you still use bcm47xxpart_add_part() here ?

Good point. I think I got two reasons:
1) Making this function more generic to it can be moved to separated
module/parser as some point (without dependency)
2) Sadly I'm not sure how bcm47xxpart_add_part is useful at all. Do
you think it really simplifies code?

-- 
Rafał

Re: [PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 01/10/2017 05:27 PM, Rafał Miłecki wrote:
> On 10 January 2017 at 17:19, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/10/2017 12:50 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This change simplifies main parsing loop logic a bit. In future it may
>>> be useful for moving TRX support to separated module / parser (if we
>>> implement support for them at some point).
>>> Finally parsing TRX at the end puts us in a better position as we have
>>> better flash layout knowledge. It may be useful e.g. if it appears there
>>> is more than 1 TRX partition.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>  drivers/mtd/bcm47xxpart.c | 124 ++++++++++++++++++++++++++++------------------
>>>  1 file changed, 77 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>>> index 283ff7e..d7d1b6e 100644
>>> --- a/drivers/mtd/bcm47xxpart.c
>>> +++ b/drivers/mtd/bcm47xxpart.c
>>> @@ -83,6 +83,70 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>>       return "rootfs";
>>>  }
>>>
>>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>>> +                              struct mtd_partition *trx,
>>> +                              struct mtd_partition *parts,
>>> +                              size_t parts_len)
>>> +{
>>> +     struct mtd_partition *part;
>>> +     struct trx_header header;
>>> +     size_t bytes_read;
>>> +     int curr_part = 0;
>>> +     int i, err;
>>> +
>>> +     if (parts_len < 3) {
>>> +             pr_warn("No enough space to add TRX partitions!\n");
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>>> +                    (uint8_t *)&header);
>>> +     if (err && !mtd_is_bitflip(err)) {
>>> +             pr_err("mtd_read error while reading TRX header: %d\n", err);
>>> +             return err;
>>> +     }
>>> +
>>> +     i = 0;
>>> +
>>> +     /* We have LZMA loader if offset[2] points to sth */
>>> +     if (header.offset[2]) {
>>> +             part = &parts[curr_part++];
>>> +             part->name = "loader";
>>> +             part->offset = trx->offset + header.offset[i];
>>> +             i++;
>>> +     }
>>> +
>>> +     if (header.offset[i]) {
>>> +             part = &parts[curr_part++];
>>> +             part->name = "linux";
>>> +             part->offset = trx->offset + header.offset[i];
>>> +             i++;
>>> +     }
>>> +
>>> +     if (header.offset[i]) {
>>> +             size_t offset = trx->offset + header.offset[i];
>>> +
>>> +             part = &parts[curr_part++];
>>> +             part->name = bcm47xxpart_trx_data_part_name(master, offset);
>>> +             part->offset = offset;
>>
>> Why don't you still use bcm47xxpart_add_part() here ?
> 
> Good point. I think I got two reasons:
> 1) Making this function more generic to it can be moved to separated
> module/parser as some point (without dependency)
> 2) Sadly I'm not sure how bcm47xxpart_add_part is useful at all. Do
> you think it really simplifies code?

Not really, but at least it's consistent.

-- 
Best regards,
Marek Vasut

[PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Rafał Miłecki]

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

This change simplifies main parsing loop logic a bit. In future it may
be useful for moving TRX support to separated module / parser (if we
implement support for them at some point).
Finally parsing TRX at the end puts us in a better position as we have
better flash layout knowledge. It may be useful e.g. if it appears there
is more than 1 TRX partition.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Keep using bcm47xxpart_add_part in new parsing function
---
 drivers/mtd/bcm47xxpart.c | 121 ++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index 283ff7e..1093025 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -83,6 +83,67 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
 	return "rootfs";
 }
 
+static int bcm47xxpart_parse_trx(struct mtd_info *master,
+				 struct mtd_partition *trx,
+				 struct mtd_partition *parts,
+				 size_t parts_len)
+{
+	struct trx_header header;
+	size_t bytes_read;
+	int curr_part = 0;
+	int i, err;
+
+	if (parts_len < 3) {
+		pr_warn("No enough space to add TRX partitions!\n");
+		return -ENOMEM;
+	}
+
+	err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
+		       (uint8_t *)&header);
+	if (err && !mtd_is_bitflip(err)) {
+		pr_err("mtd_read error while reading TRX header: %d\n", err);
+		return err;
+	}
+
+	i = 0;
+
+	/* We have LZMA loader if offset[2] points to sth */
+	if (header.offset[2]) {
+		bcm47xxpart_add_part(&parts[curr_part++], "loader",
+				     trx->offset + header.offset[i], 0);
+		i++;
+	}
+
+	if (header.offset[i]) {
+		bcm47xxpart_add_part(&parts[curr_part++], "linux",
+				     trx->offset + header.offset[i], 0);
+		i++;
+	}
+
+	if (header.offset[i]) {
+		size_t offset = trx->offset + header.offset[i];
+		const char *name = bcm47xxpart_trx_data_part_name(master,
+								  offset);
+
+		bcm47xxpart_add_part(&parts[curr_part++], name, offset, 0);
+		i++;
+	}
+
+	/*
+	 * Assume that every partition ends at the beginning of the one it is
+	 * followed by.
+	 */
+	for (i = 0; i < curr_part; i++) {
+		u64 next_part_offset = (i < curr_part - 1) ?
+					parts[i + 1].offset :
+					trx->offset + trx->size;
+
+		parts[i].size = next_part_offset - parts[i].offset;
+	}
+
+	return curr_part;
+}
+
 static int bcm47xxpart_parse(struct mtd_info *master,
 			     const struct mtd_partition **pparts,
 			     struct mtd_part_parser_data *data)
@@ -93,9 +154,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	size_t bytes_read;
 	uint32_t offset;
 	uint32_t blocksize = master->erasesize;
-	struct trx_header *trx;
 	int trx_part = -1;
-	int last_trx_part = -1;
 	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
 	int err;
 
@@ -182,54 +241,14 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 
 		/* TRX */
 		if (buf[0x000 / 4] == TRX_MAGIC) {
-			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
-				pr_warn("Not enough partitions left to register trx, scanning stopped!\n");
-				break;
-			}
-
-			trx = (struct trx_header *)buf;
+			struct trx_header *trx;
 
 			trx_part = curr_part;
 			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
 					     offset, 0);
 
-			i = 0;
-			/* We have LZMA loader if offset[2] points to sth */
-			if (trx->offset[2]) {
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     "loader",
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			if (trx->offset[i]) {
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     "linux",
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			/*
-			 * Pure rootfs size is known and can be calculated as:
-			 * trx->length - trx->offset[i]. We don't fill it as
-			 * we want to have jffs2 (overlay) in the same mtd.
-			 */
-			if (trx->offset[i]) {
-				const char *name;
-
-				name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]);
-				bcm47xxpart_add_part(&parts[curr_part++],
-						     name,
-						     offset + trx->offset[i],
-						     0);
-				i++;
-			}
-
-			last_trx_part = curr_part - 1;
-
 			/* Jump to the end of TRX */
+			trx = (struct trx_header *)buf;
 			offset = roundup(offset + trx->length, blocksize);
 			/* Next loop iteration will increase the offset */
 			offset -= blocksize;
@@ -307,9 +326,17 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 				       parts[i + 1].offset : master->size;
 
 		parts[i].size = next_part_offset - parts[i].offset;
-		if (i == last_trx_part && trx_part >= 0)
-			parts[trx_part].size = next_part_offset -
-					       parts[trx_part].offset;
+	}
+
+	/* If there was TRX parse it now */
+	if (trx_part >= 0) {
+		int num_parts;
+
+		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
+						  parts + curr_part,
+						  BCM47XXPART_MAX_PARTS - curr_part);
+		if (num_parts > 0)
+			curr_part += num_parts;
 	}
 
 	*pparts = parts;
-- 
2.10.1

[PATCH V2 2/2] mtd: bcm47xxpart: support layouts with multiple TRX partitions

[Rafał Miłecki]

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

Some devices may have an extra TRX partition used as failsafe one. If
we detect such partition we should set a proper name for it and don't
parse it.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/bcm47xxpart.c | 56 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index 1093025..d10fa6c 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -9,6 +9,7 @@
  *
  */
 
+#include <linux/bcm47xx_nvram.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -144,6 +145,30 @@ static int bcm47xxpart_parse_trx(struct mtd_info *master,
 	return curr_part;
 }
 
+/**
+ * bcm47xxpart_bootpartition - gets index of TRX partition used by bootloader
+ *
+ * Some devices may have more than one TRX partition. In such case one of them
+ * is the main one and another a failsafe one. Bootloader may fallback to the
+ * failsafe firmware if it detects corruption of the main image.
+ *
+ * This function provides info about currently used TRX partition. It's the one
+ * containing kernel started by the bootloader.
+ */
+static int bcm47xxpart_bootpartition(void)
+{
+	char buf[4];
+	int bootpartition;
+
+	/* Check CFE environment variable */
+	if (bcm47xx_nvram_getenv("bootpartition", buf, sizeof(buf)) > 0) {
+		if (!kstrtoint(buf, 0, &bootpartition))
+			return bootpartition;
+	}
+
+	return 0;
+}
+
 static int bcm47xxpart_parse(struct mtd_info *master,
 			     const struct mtd_partition **pparts,
 			     struct mtd_part_parser_data *data)
@@ -154,7 +179,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	size_t bytes_read;
 	uint32_t offset;
 	uint32_t blocksize = master->erasesize;
-	int trx_part = -1;
+	int trx_parts[2]; /* Array with indexes of TRX partitions */
+	int trx_num = 0; /* Number of found TRX partitions */
 	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
 	int err;
 
@@ -243,7 +269,11 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 		if (buf[0x000 / 4] == TRX_MAGIC) {
 			struct trx_header *trx;
 
-			trx_part = curr_part;
+			if (trx_num >= ARRAY_SIZE(trx_parts))
+				pr_warn("No enough space to store another TRX found at 0x%X\n",
+					offset);
+			else
+				trx_parts[trx_num++] = curr_part;
 			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
 					     offset, 0);
 
@@ -329,14 +359,20 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	}
 
 	/* If there was TRX parse it now */
-	if (trx_part >= 0) {
-		int num_parts;
-
-		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
-						  parts + curr_part,
-						  BCM47XXPART_MAX_PARTS - curr_part);
-		if (num_parts > 0)
-			curr_part += num_parts;
+	for (i = 0; i < trx_num; i++) {
+		struct mtd_partition *trx = &parts[trx_parts[i]];
+
+		if (i == bcm47xxpart_bootpartition()) {
+			int num_parts;
+
+			num_parts = bcm47xxpart_parse_trx(master, trx,
+							  parts + curr_part,
+							  BCM47XXPART_MAX_PARTS - curr_part);
+			if (num_parts > 0)
+				curr_part += num_parts;
+		} else {
+			trx->name = "failsafe";
+		}
 	}
 
 	*pparts = parts;
-- 
2.10.1

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Rafał Miłecki]

On 10 January 2017 at 23:15, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This change simplifies main parsing loop logic a bit. In future it may
> be useful for moving TRX support to separated module / parser (if we
> implement support for them at some point).
> Finally parsing TRX at the end puts us in a better position as we have
> better flash layout knowledge. It may be useful e.g. if it appears there
> is more than 1 TRX partition.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Marek: you were commenting on V1, what do you think about V2? Could you Ack it?

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 01/10/2017 11:15 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This change simplifies main parsing loop logic a bit. In future it may
> be useful for moving TRX support to separated module / parser (if we
> implement support for them at some point).
> Finally parsing TRX at the end puts us in a better position as we have
> better flash layout knowledge. It may be useful e.g. if it appears there
> is more than 1 TRX partition.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Keep using bcm47xxpart_add_part in new parsing function
> ---
>  drivers/mtd/bcm47xxpart.c | 121 ++++++++++++++++++++++++++++------------------
>  1 file changed, 74 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> index 283ff7e..1093025 100644
> --- a/drivers/mtd/bcm47xxpart.c
> +++ b/drivers/mtd/bcm47xxpart.c
> @@ -83,6 +83,67 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>  	return "rootfs";
>  }
>  
> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
> +				 struct mtd_partition *trx,
> +				 struct mtd_partition *parts,
> +				 size_t parts_len)
> +{
> +	struct trx_header header;
> +	size_t bytes_read;
> +	int curr_part = 0;
> +	int i, err;
> +
> +	if (parts_len < 3) {
> +		pr_warn("No enough space to add TRX partitions!\n");
> +		return -ENOMEM;
> +	}
> +
> +	err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
> +		       (uint8_t *)&header);
> +	if (err && !mtd_is_bitflip(err)) {
> +		pr_err("mtd_read error while reading TRX header: %d\n", err);

Minor nit -- Can we make this dev_err() ?

> +		return err;
> +	}
> +
> +	i = 0;
> +
> +	/* We have LZMA loader if offset[2] points to sth */
> +	if (header.offset[2]) {
> +		bcm47xxpart_add_part(&parts[curr_part++], "loader",
> +				     trx->offset + header.offset[i], 0);
> +		i++;
> +	}
> +
> +	if (header.offset[i]) {
> +		bcm47xxpart_add_part(&parts[curr_part++], "linux",
> +				     trx->offset + header.offset[i], 0);
> +		i++;
> +	}
> +
> +	if (header.offset[i]) {
> +		size_t offset = trx->offset + header.offset[i];
> +		const char *name = bcm47xxpart_trx_data_part_name(master,
> +								  offset);
> +
> +		bcm47xxpart_add_part(&parts[curr_part++], name, offset, 0);
> +		i++;
> +	}
> +
> +	/*
> +	 * Assume that every partition ends at the beginning of the one it is
> +	 * followed by.
> +	 */
> +	for (i = 0; i < curr_part; i++) {
> +		u64 next_part_offset = (i < curr_part - 1) ?
> +					parts[i + 1].offset :
> +					trx->offset + trx->size;
> +
> +		parts[i].size = next_part_offset - parts[i].offset;
> +	}
> +
> +	return curr_part;
> +}
> +
>  static int bcm47xxpart_parse(struct mtd_info *master,
>  			     const struct mtd_partition **pparts,
>  			     struct mtd_part_parser_data *data)
> @@ -93,9 +154,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  	size_t bytes_read;
>  	uint32_t offset;
>  	uint32_t blocksize = master->erasesize;
> -	struct trx_header *trx;
>  	int trx_part = -1;
> -	int last_trx_part = -1;
>  	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
>  	int err;
>  
> @@ -182,54 +241,14 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  
>  		/* TRX */
>  		if (buf[0x000 / 4] == TRX_MAGIC) {
> -			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
> -				pr_warn("Not enough partitions left to register trx, scanning stopped!\n");
> -				break;
> -			}
> -
> -			trx = (struct trx_header *)buf;
> +			struct trx_header *trx;
>  
>  			trx_part = curr_part;
>  			bcm47xxpart_add_part(&parts[curr_part++], "firmware",
>  					     offset, 0);
>  
> -			i = 0;
> -			/* We have LZMA loader if offset[2] points to sth */
> -			if (trx->offset[2]) {
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     "loader",
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			if (trx->offset[i]) {
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     "linux",
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			/*
> -			 * Pure rootfs size is known and can be calculated as:
> -			 * trx->length - trx->offset[i]. We don't fill it as
> -			 * we want to have jffs2 (overlay) in the same mtd.
> -			 */
> -			if (trx->offset[i]) {
> -				const char *name;
> -
> -				name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]);
> -				bcm47xxpart_add_part(&parts[curr_part++],
> -						     name,
> -						     offset + trx->offset[i],
> -						     0);
> -				i++;
> -			}
> -
> -			last_trx_part = curr_part - 1;
> -
>  			/* Jump to the end of TRX */
> +			trx = (struct trx_header *)buf;
>  			offset = roundup(offset + trx->length, blocksize);
>  			/* Next loop iteration will increase the offset */
>  			offset -= blocksize;
> @@ -307,9 +326,17 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>  				       parts[i + 1].offset : master->size;
>  
>  		parts[i].size = next_part_offset - parts[i].offset;
> -		if (i == last_trx_part && trx_part >= 0)
> -			parts[trx_part].size = next_part_offset -
> -					       parts[trx_part].offset;
> +	}
> +
> +	/* If there was TRX parse it now */
> +	if (trx_part >= 0) {
> +		int num_parts;
> +
> +		num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part],
> +						  parts + curr_part,
> +						  BCM47XXPART_MAX_PARTS - curr_part);
> +		if (num_parts > 0)
> +			curr_part += num_parts;
>  	}
>  
>  	*pparts = parts;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 02/09/2017 05:54 PM, Rafał Miłecki wrote:
> On 10 January 2017 at 23:15, Rafał Miłecki <zajec5@gmail.com> wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This change simplifies main parsing loop logic a bit. In future it may
>> be useful for moving TRX support to separated module / parser (if we
>> implement support for them at some point).
>> Finally parsing TRX at the end puts us in a better position as we have
>> better flash layout knowledge. It may be useful e.g. if it appears there
>> is more than 1 TRX partition.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> 
> Marek: you were commenting on V1, what do you think about V2? Could you Ack it?
> 
Minor nit on 1/2 , but otherwise the whole series is

Acked-by: Marek Vasut <marek.vasut@gmail.com>

I will not claim I understand the TRX parsing though.

-- 
Best regards,
Marek Vasut

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Rafał Miłecki]

On 2017-02-09 18:28, Marek Vasut wrote:
> On 01/10/2017 11:15 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> This change simplifies main parsing loop logic a bit. In future it may
>> be useful for moving TRX support to separated module / parser (if we
>> implement support for them at some point).
>> Finally parsing TRX at the end puts us in a better position as we have
>> better flash layout knowledge. It may be useful e.g. if it appears 
>> there
>> is more than 1 TRX partition.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Keep using bcm47xxpart_add_part in new parsing function
>> ---
>>  drivers/mtd/bcm47xxpart.c | 121 
>> ++++++++++++++++++++++++++++------------------
>>  1 file changed, 74 insertions(+), 47 deletions(-)
>> 
>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>> index 283ff7e..1093025 100644
>> --- a/drivers/mtd/bcm47xxpart.c
>> +++ b/drivers/mtd/bcm47xxpart.c
>> @@ -83,6 +83,67 @@ static const char 
>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>  	return "rootfs";
>>  }
>> 
>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>> +				 struct mtd_partition *trx,
>> +				 struct mtd_partition *parts,
>> +				 size_t parts_len)
>> +{
>> +	struct trx_header header;
>> +	size_t bytes_read;
>> +	int curr_part = 0;
>> +	int i, err;
>> +
>> +	if (parts_len < 3) {
>> +		pr_warn("No enough space to add TRX partitions!\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>> +		       (uint8_t *)&header);
>> +	if (err && !mtd_is_bitflip(err)) {
>> +		pr_err("mtd_read error while reading TRX header: %d\n", err);
> 
> Minor nit -- Can we make this dev_err() ?

The problem is parsers receive struct mtd_info (as an argument) with has 
empty
dev. I just tried
dev_info(&master->dev, "TEST TEST TEST :)\n");
and got this:
[    0.491576]  (null): TEST TEST TEST :)

I guess that's the reason why parse_mtd_partitions and 
mtd_device_parse_register
also use pr_* functions.

So this should be improved at mtd core subsystem first, then we can use 
it in
drivers as well.

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 02/09/2017 07:05 PM, Rafał Miłecki wrote:
> On 2017-02-09 18:28, Marek Vasut wrote:
>> On 01/10/2017 11:15 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This change simplifies main parsing loop logic a bit. In future it may
>>> be useful for moving TRX support to separated module / parser (if we
>>> implement support for them at some point).
>>> Finally parsing TRX at the end puts us in a better position as we have
>>> better flash layout knowledge. It may be useful e.g. if it appears there
>>> is more than 1 TRX partition.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> V2: Keep using bcm47xxpart_add_part in new parsing function
>>> ---
>>>  drivers/mtd/bcm47xxpart.c | 121
>>> ++++++++++++++++++++++++++++------------------
>>>  1 file changed, 74 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>>> index 283ff7e..1093025 100644
>>> --- a/drivers/mtd/bcm47xxpart.c
>>> +++ b/drivers/mtd/bcm47xxpart.c
>>> @@ -83,6 +83,67 @@ static const char
>>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>>      return "rootfs";
>>>  }
>>>
>>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>>> +                 struct mtd_partition *trx,
>>> +                 struct mtd_partition *parts,
>>> +                 size_t parts_len)
>>> +{
>>> +    struct trx_header header;
>>> +    size_t bytes_read;
>>> +    int curr_part = 0;
>>> +    int i, err;
>>> +
>>> +    if (parts_len < 3) {
>>> +        pr_warn("No enough space to add TRX partitions!\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>>> +               (uint8_t *)&header);
>>> +    if (err && !mtd_is_bitflip(err)) {
>>> +        pr_err("mtd_read error while reading TRX header: %d\n", err);
>>
>> Minor nit -- Can we make this dev_err() ?
> 
> The problem is parsers receive struct mtd_info (as an argument) with has
> empty
> dev. I just tried
> dev_info(&master->dev, "TEST TEST TEST :)\n");
> and got this:
> [    0.491576]  (null): TEST TEST TEST :)
> 
> I guess that's the reason why parse_mtd_partitions and
> mtd_device_parse_register
> also use pr_* functions.
> 
> So this should be improved at mtd core subsystem first, then we can use
> it in
> drivers as well.

Isn't this patch fixing just that?
 [PATCH] mtd: Add partition device node to mtd partition devices

-- 
Best regards,
Marek Vasut

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Boris Brezillon]

On Thu, 9 Feb 2017 19:21:03 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 02/09/2017 07:05 PM, Rafał Miłecki wrote:
> > On 2017-02-09 18:28, Marek Vasut wrote:  
> >> On 01/10/2017 11:15 PM, Rafał Miłecki wrote:  
> >>> From: Rafał Miłecki <rafal@milecki.pl>
> >>>
> >>> This change simplifies main parsing loop logic a bit. In future it may
> >>> be useful for moving TRX support to separated module / parser (if we
> >>> implement support for them at some point).
> >>> Finally parsing TRX at the end puts us in a better position as we have
> >>> better flash layout knowledge. It may be useful e.g. if it appears there
> >>> is more than 1 TRX partition.
> >>>
> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >>> ---
> >>> V2: Keep using bcm47xxpart_add_part in new parsing function
> >>> ---
> >>>  drivers/mtd/bcm47xxpart.c | 121
> >>> ++++++++++++++++++++++++++++------------------
> >>>  1 file changed, 74 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> >>> index 283ff7e..1093025 100644
> >>> --- a/drivers/mtd/bcm47xxpart.c
> >>> +++ b/drivers/mtd/bcm47xxpart.c
> >>> @@ -83,6 +83,67 @@ static const char
> >>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
> >>>      return "rootfs";
> >>>  }
> >>>
> >>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
> >>> +                 struct mtd_partition *trx,
> >>> +                 struct mtd_partition *parts,
> >>> +                 size_t parts_len)
> >>> +{
> >>> +    struct trx_header header;
> >>> +    size_t bytes_read;
> >>> +    int curr_part = 0;
> >>> +    int i, err;
> >>> +
> >>> +    if (parts_len < 3) {
> >>> +        pr_warn("No enough space to add TRX partitions!\n");
> >>> +        return -ENOMEM;
> >>> +    }
> >>> +
> >>> +    err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
> >>> +               (uint8_t *)&header);
> >>> +    if (err && !mtd_is_bitflip(err)) {
> >>> +        pr_err("mtd_read error while reading TRX header: %d\n", err);  
> >>
> >> Minor nit -- Can we make this dev_err() ?  
> > 
> > The problem is parsers receive struct mtd_info (as an argument) with has
> > empty
> > dev. I just tried
> > dev_info(&master->dev, "TEST TEST TEST :)\n");
> > and got this:
> > [    0.491576]  (null): TEST TEST TEST :)
> > 
> > I guess that's the reason why parse_mtd_partitions and
> > mtd_device_parse_register
> > also use pr_* functions.
> > 
> > So this should be improved at mtd core subsystem first, then we can use
> > it in
> > drivers as well.  
> 
> Isn't this patch fixing just that?
>  [PATCH] mtd: Add partition device node to mtd partition devices
> 


No, this patch is only adding a pointer to a device_node object (DT
node).
To solve the problem, we need to have the master dev name set before we
register its partitions, and this is currently only the case if you
enable CONFIG_MTD_PARTITIONED_MASTER.

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 02/09/2017 08:11 PM, Boris Brezillon wrote:
> On Thu, 9 Feb 2017 19:21:03 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 02/09/2017 07:05 PM, Rafał Miłecki wrote:
>>> On 2017-02-09 18:28, Marek Vasut wrote:  
>>>> On 01/10/2017 11:15 PM, Rafał Miłecki wrote:  
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> This change simplifies main parsing loop logic a bit. In future it may
>>>>> be useful for moving TRX support to separated module / parser (if we
>>>>> implement support for them at some point).
>>>>> Finally parsing TRX at the end puts us in a better position as we have
>>>>> better flash layout knowledge. It may be useful e.g. if it appears there
>>>>> is more than 1 TRX partition.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>> V2: Keep using bcm47xxpart_add_part in new parsing function
>>>>> ---
>>>>>  drivers/mtd/bcm47xxpart.c | 121
>>>>> ++++++++++++++++++++++++++++------------------
>>>>>  1 file changed, 74 insertions(+), 47 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>>>>> index 283ff7e..1093025 100644
>>>>> --- a/drivers/mtd/bcm47xxpart.c
>>>>> +++ b/drivers/mtd/bcm47xxpart.c
>>>>> @@ -83,6 +83,67 @@ static const char
>>>>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>>>>      return "rootfs";
>>>>>  }
>>>>>
>>>>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>>>>> +                 struct mtd_partition *trx,
>>>>> +                 struct mtd_partition *parts,
>>>>> +                 size_t parts_len)
>>>>> +{
>>>>> +    struct trx_header header;
>>>>> +    size_t bytes_read;
>>>>> +    int curr_part = 0;
>>>>> +    int i, err;
>>>>> +
>>>>> +    if (parts_len < 3) {
>>>>> +        pr_warn("No enough space to add TRX partitions!\n");
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>>>>> +               (uint8_t *)&header);
>>>>> +    if (err && !mtd_is_bitflip(err)) {
>>>>> +        pr_err("mtd_read error while reading TRX header: %d\n", err);  
>>>>
>>>> Minor nit -- Can we make this dev_err() ?  
>>>
>>> The problem is parsers receive struct mtd_info (as an argument) with has
>>> empty
>>> dev. I just tried
>>> dev_info(&master->dev, "TEST TEST TEST :)\n");
>>> and got this:
>>> [    0.491576]  (null): TEST TEST TEST :)
>>>
>>> I guess that's the reason why parse_mtd_partitions and
>>> mtd_device_parse_register
>>> also use pr_* functions.
>>>
>>> So this should be improved at mtd core subsystem first, then we can use
>>> it in
>>> drivers as well.  
>>
>> Isn't this patch fixing just that?
>>  [PATCH] mtd: Add partition device node to mtd partition devices
>>
> 
> 
> No, this patch is only adding a pointer to a device_node object (DT
> node).
> To solve the problem, we need to have the master dev name set before we
> register its partitions, and this is currently only the case if you
> enable CONFIG_MTD_PARTITIONED_MASTER.
> 
Ah hmmmm , then leave the pr_err() there , this can be fixed later.

Acked-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

Re: [PATCH V2 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Brian Norris]

On Tue, Jan 10, 2017 at 11:15:24PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This change simplifies main parsing loop logic a bit. In future it may
> be useful for moving TRX support to separated module / parser (if we
> implement support for them at some point).
> Finally parsing TRX at the end puts us in a better position as we have
> better flash layout knowledge. It may be useful e.g. if it appears there
> is more than 1 TRX partition.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Keep using bcm47xxpart_add_part in new parsing function

Applied both to l2-mtd.git