[PATCH v2 0/1] Add more support for NXP's mfgtool

[PATCH v2 0/1] Add more support for NXP's mfgtool

[Angus Ainslie]

NXP's mfgtool needs to query the block size for sparse image upload

Changes since v1:

* Dropped the "all" parition type patch
* Added more detail to the patch commit message
* Only define getvar_logical_blocksize when FASTBOOT_FLASH_MMC is defined
* Combined fb_get_block_size into getvar_logical_blocksize
* Fixed fastboot returned error message

Angus Ainslie (1):
  fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

 drivers/fastboot/fb_getvar.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
2.25.1

[PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Angus Ainslie]

NXP's mfgtool queies the mmc blocksize and splits a sparse image into
blocksize size pieces for the upload.

Signed-off-by: Angus Ainslie <angus@akkea.ca>
---
 drivers/fastboot/fb_getvar.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index d43f2cfee6..3cb72b8a54 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -26,6 +26,7 @@ static void getvar_has_slot(char *var_parameter, char *response);
 #endif
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
 static void getvar_partition_type(char *part_name, char *response);
+static void getvar_logical_blocksize(char *var_parameter, char *response);
 #endif
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
 static void getvar_partition_size(char *part_name, char *response);
@@ -72,6 +73,9 @@ static const struct {
 	}, {
 		.variable = "partition-type",
 		.dispatch = getvar_partition_type
+	}, {
+		.variable = "logical-block-size",
+		.dispatch = getvar_logical_blocksize
 #endif
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
 	}, {
@@ -232,6 +236,23 @@ static void getvar_partition_type(char *part_name, char *response)
 			fastboot_okay(fs_get_type_name(), response);
 	}
 }
+
+static void getvar_logical_blocksize(char *var_parameter, char *response)
+{
+	u32 blksz;
+	struct blk_desc *dev_desc;
+
+	dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
+
+	if (!dev_desc) {
+		pr_err("** Block device mmc %d not supported\n",
+		       CONFIG_FASTBOOT_FLASH_MMC_DEV);
+		fastboot_fail("Get logical_blocksize failed", response);
+		return;
+	}
+
+	fastboot_response("OKAY", response, "0x%08x", dev_desc->blksz);
+}
 #endif
 
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
-- 
2.25.1

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Sean Anderson]

Hi Angus,

> NXP's mfgtool queies the mmc blocksize and splits a sparse image into
> blocksize size pieces for the upload.

It's still not clear to me why this is necessary. fastboot (for example)
transfers in blocks of max-download-size. Using the actual block size
seems like it would result in unnecessary overhead.

--Sean

> Signed-off-by: Angus Ainslie <angus@akkea.ca>
> ---
>   drivers/fastboot/fb_getvar.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index d43f2cfee6..3cb72b8a54 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -26,6 +26,7 @@ static void getvar_has_slot(char *var_parameter, char *response);
>   #endif
>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
>   static void getvar_partition_type(char *part_name, char *response);
> +static void getvar_logical_blocksize(char *var_parameter, char *response);
>   #endif
>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>   static void getvar_partition_size(char *part_name, char *response);
> @@ -72,6 +73,9 @@ static const struct {
>   	}, {
>   		.variable = "partition-type",
>   		.dispatch = getvar_partition_type
> +	}, {
> +		.variable = "logical-block-size",
> +		.dispatch = getvar_logical_blocksize
>   #endif
>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>   	}, {
> @@ -232,6 +236,23 @@ static void getvar_partition_type(char *part_name, char *response)
>   			fastboot_okay(fs_get_type_name(), response);
>   	}
>   }
> +
> +static void getvar_logical_blocksize(char *var_parameter, char *response)
> +{
> +	u32 blksz;
> +	struct blk_desc *dev_desc;
> +
> +	dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
> +
> +	if (!dev_desc) {
> +		pr_err("** Block device mmc %d not supported\n",
> +		       CONFIG_FASTBOOT_FLASH_MMC_DEV);
> +		fastboot_fail("Get logical_blocksize failed", response);
> +		return;
> +	}
> +
> +	fastboot_response("OKAY", response, "0x%08x", dev_desc->blksz);
> +}
>   #endif
>
>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Angus Ainslie]

Hi Sean,

On 2021-12-28 08:59, Sean Anderson wrote:
> Hi Angus,
> 
>> NXP's mfgtool queies the mmc blocksize and splits a sparse image into
>> blocksize size pieces for the upload.
> 
> It's still not clear to me why this is necessary. fastboot (for 
> example)
> transfers in blocks of max-download-size. Using the actual block size
> seems like it would result in unnecessary overhead.
> 

The version of uuu that we are using requires the block-size for the 
sparse upload

https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501

It looks like the upstream version will default to 4096 if the 
block-size is not provided

https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642

Instead of making assumptions about the block size wouldn't it be better 
to provide one if requested ?

We could also remove that being a required parameter from the uuu that 
we are using but that still leaves versions out there that won't work 
with a mainline u-boot.

Thanks
Angus

> --Sean
> 
>> Signed-off-by: Angus Ainslie <angus@akkea.ca>
>> ---
>>   drivers/fastboot/fb_getvar.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>> 
>> diff --git a/drivers/fastboot/fb_getvar.c 
>> b/drivers/fastboot/fb_getvar.c
>> index d43f2cfee6..3cb72b8a54 100644
>> --- a/drivers/fastboot/fb_getvar.c
>> +++ b/drivers/fastboot/fb_getvar.c
>> @@ -26,6 +26,7 @@ static void getvar_has_slot(char *var_parameter, 
>> char *response);
>>   #endif
>>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
>>   static void getvar_partition_type(char *part_name, char *response);
>> +static void getvar_logical_blocksize(char *var_parameter, char 
>> *response);
>>   #endif
>>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>>   static void getvar_partition_size(char *part_name, char *response);
>> @@ -72,6 +73,9 @@ static const struct {
>>   	}, {
>>   		.variable = "partition-type",
>>   		.dispatch = getvar_partition_type
>> +	}, {
>> +		.variable = "logical-block-size",
>> +		.dispatch = getvar_logical_blocksize
>>   #endif
>>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>>   	}, {
>> @@ -232,6 +236,23 @@ static void getvar_partition_type(char 
>> *part_name, char *response)
>>   			fastboot_okay(fs_get_type_name(), response);
>>   	}
>>   }
>> +
>> +static void getvar_logical_blocksize(char *var_parameter, char 
>> *response)
>> +{
>> +	u32 blksz;
>> +	struct blk_desc *dev_desc;
>> +
>> +	dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
>> +
>> +	if (!dev_desc) {
>> +		pr_err("** Block device mmc %d not supported\n",
>> +		       CONFIG_FASTBOOT_FLASH_MMC_DEV);
>> +		fastboot_fail("Get logical_blocksize failed", response);
>> +		return;
>> +	}
>> +
>> +	fastboot_response("OKAY", response, "0x%08x", dev_desc->blksz);
>> +}
>>   #endif
>> 
>>   #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>> 

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Oleh Kravchenko]

Hello Angus,

29.12.21 15:35, Angus Ainslie пише:
> The version of uuu that we are using requires the block-size for the sparse upload
> 
> https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501

This version is outdated (2 years old).
Maybe it is time to upgrade to the latest one?

> It looks like the upstream version will default to 4096 if the block-size is not provided
> 
> https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642
> 
> Instead of making assumptions about the block size wouldn't it be better to provide one if requested ?

+1 if it works faster ^_^

> We could also remove that being a required parameter from the uuu that we are using but that still leaves versions out there that won't work with a mainline u-boot.

Why does mainline use fixed block size 4096?


Excuse me for direct questions I'm just curious.

-- 
Best regards,
Oleh Kravchenko

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Angus Ainslie]

Hi Oleh,

On 2021-12-29 07:07, Oleh Kravchenko wrote:
> Hello Angus,
> 
> 29.12.21 15:35, Angus Ainslie пише:
>> The version of uuu that we are using requires the block-size for the 
>> sparse upload
>> 
>> https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501
> 
> This version is outdated (2 years old).
> Maybe it is time to upgrade to the latest one?
> 

That was one of my suggested solutions but it would still leave older 
uuu implementations out there that won't work with mainline u-boot.

>> It looks like the upstream version will default to 4096 if the 
>> block-size is not provided
>> 
>> https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642
>> 
>> Instead of making assumptions about the block size wouldn't it be 
>> better to provide one if requested ?
> 
> +1 if it works faster ^_^
> 

I haven't done any speed tests so I can't really comment about the 
speed.

>> We could also remove that being a required parameter from the uuu that 
>> we are using but that still leaves versions out there that won't work 
>> with a mainline u-boot.
> 
> Why does mainline use fixed block size 4096?
> 

That's a fall back if the fastboot implementation doesn't provide a 
block-size , probably so that it can work with mainline u-boot.

> 
> Excuse me for direct questions I'm just curious.

NP that's what review is for.

Angus

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Sean Anderson]

On 12/29/21 8:35 AM, Angus Ainslie wrote:
> Hi Sean,
>
> On 2021-12-28 08:59, Sean Anderson wrote:
>> Hi Angus,
>>
>>> NXP's mfgtool queies the mmc blocksize and splits a sparse image into
>>> blocksize size pieces for the upload.
>>
>> It's still not clear to me why this is necessary. fastboot (for example)
>> transfers in blocks of max-download-size. Using the actual block size
>> seems like it would result in unnecessary overhead.
>>
>
> The version of uuu that we are using requires the block-size for the sparse upload
>
> https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501
>
> It looks like the upstream version will default to 4096 if the block-size is not provided
>
> https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642
>
> Instead of making assumptions about the block size wouldn't it be better to provide one if requested ?

The block size is for the sparse image. This determines the granularity
of the sections of the image. For example, if the block size is 1K, then
all sizes will be multiples of 1K. So if you have a block with 1 byte of
data and 1023 bytes of 0 then the whole block will be transferred
instead of being replaced with a fill block.

However, the sparse file block size size completely orthogonal to
the block size of the device being written to. The only thing it affects
is the efficiency of the sparse image. For example, I generate my sparse
files with a block size of 1M, because it is a nice convenient number.

--Sean

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Angus Ainslie]

Hi Sean,

On 2021-12-30 08:21, Sean Anderson wrote:
> On 12/29/21 8:35 AM, Angus Ainslie wrote:
>> Hi Sean,
>> 
>> On 2021-12-28 08:59, Sean Anderson wrote:
>>> Hi Angus,
>>> 
>>>> NXP's mfgtool queies the mmc blocksize and splits a sparse image 
>>>> into
>>>> blocksize size pieces for the upload.
>>> 
>>> It's still not clear to me why this is necessary. fastboot (for 
>>> example)
>>> transfers in blocks of max-download-size. Using the actual block size
>>> seems like it would result in unnecessary overhead.
>>> 
>> 
>> The version of uuu that we are using requires the block-size for the 
>> sparse upload
>> 
>> https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501
>> 
>> It looks like the upstream version will default to 4096 if the 
>> block-size is not provided
>> 
>> https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642
>> 
>> Instead of making assumptions about the block size wouldn't it be 
>> better to provide one if requested ?
> 
> The block size is for the sparse image. This determines the granularity
> of the sections of the image. For example, if the block size is 1K, 
> then
> all sizes will be multiples of 1K. So if you have a block with 1 byte 
> of
> data and 1023 bytes of 0 then the whole block will be transferred
> instead of being replaced with a fill block.
> 

Thanks for the explanation on how it works.

> However, the sparse file block size size completely orthogonal to
> the block size of the device being written to. The only thing it 
> affects
> is the efficiency of the sparse image. For example, I generate my 
> sparse
> files with a block size of 1M, because it is a nice convenient number.
> 

Ok so the way the NXP's uuu uses the blocksize is not correct. However 
the tool is already out there in the wild.

Can we add the block-size response to support that tool or will it be 
required that uuu needs to be upgraded to work with mainline u-boot ?

Thanks
Angus

> --Sean

Re: [PATCH v2 1/1] fastboot: fb_getvar: Add getvar_logical_blocksize for NXP mfgtool

[Sean Anderson]

On 1/3/22 8:27 AM, Angus Ainslie wrote:
> Hi Sean,
>
> On 2021-12-30 08:21, Sean Anderson wrote:
>> On 12/29/21 8:35 AM, Angus Ainslie wrote:
>>> Hi Sean,
>>>
>>> On 2021-12-28 08:59, Sean Anderson wrote:
>>>> Hi Angus,
>>>>
>>>>> NXP's mfgtool queies the mmc blocksize and splits a sparse image into
>>>>> blocksize size pieces for the upload.
>>>>
>>>> It's still not clear to me why this is necessary. fastboot (for example)
>>>> transfers in blocks of max-download-size. Using the actual block size
>>>> seems like it would result in unnecessary overhead.
>>>>
>>>
>>> The version of uuu that we are using requires the block-size for the sparse upload
>>>
>>> https://source.puri.sm/Librem5/mfgtools/-/blob/pureos/amber/libuuu/fastboot.cpp#L501
>>>
>>> It looks like the upstream version will default to 4096 if the block-size is not provided
>>>
>>> https://github.com/NXPmicro/mfgtools/blob/5397913ad97db422c1d70f314dedff4cb7d976b9/libuuu/fastboot.cpp#L642
>>>
>>> Instead of making assumptions about the block size wouldn't it be better to provide one if requested ?
>>
>> The block size is for the sparse image. This determines the granularity
>> of the sections of the image. For example, if the block size is 1K, then
>> all sizes will be multiples of 1K. So if you have a block with 1 byte of
>> data and 1023 bytes of 0 then the whole block will be transferred
>> instead of being replaced with a fill block.
>>
>
> Thanks for the explanation on how it works.
>
>> However, the sparse file block size size completely orthogonal to
>> the block size of the device being written to. The only thing it affects
>> is the efficiency of the sparse image. For example, I generate my sparse
>> files with a block size of 1M, because it is a nice convenient number.
>>
>
> Ok so the way the NXP's uuu uses the blocksize is not correct. However the tool is already out there in the wild.
>
> Can we add the block-size response to support that tool or will it be required that uuu needs to be upgraded to work with mainline u-boot ?

IMO this should be fixed in uuu. And as I understand it, this is only an
issue when you transfer a raw image with uuu. Perhaps you can generate
a fastboot sparse image (with img2simg) and transfer that instead.

--Sean