[PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl

[PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Rasmus Villemoes]

We have a board with several revisions. The older ones use a nor flash
with 64k erase size, while the newer have a flash with 4k sectors. The
environment size is 8k.

Currently, we have to put a column containing 0x10000 (64k) in
fw_env.config in order for it to work on the older boards. But that
ends up wasting quite a lot of time on the newer boards that could
just erase the 8k occupied by the environment - strace says the 64k
erase takes 0.405 seconds. With this patch, as expected, that's about
an 8-fold better, at 0.043 seconds.

Having different fw_env.config files for the different revisions is
highly impractical, and the correct information is already available
right at our fingertips. So use the erasesize returned by the
MEMGETINFO ioctl when the fourth column is absent or contains a 0.

As I'm only testing this on a NOR flash, I'm only changing the logic
for that case, though I think it should be possible for the other
types as well.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/env/fw_env.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 381739d28d..87c3504315 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1647,6 +1647,8 @@ static int check_device_config(int dev)
 			goto err;
 		}
 		DEVTYPE(dev) = mtdinfo.type;
+		if (DEVESIZE(dev) == 0 && mtdinfo.type == MTD_NORFLASH)
+			DEVESIZE(dev) = mtdinfo.erasesize;
 		if (DEVESIZE(dev) == 0)
 			/* Assume the erase size is the same as the env-size */
 			DEVESIZE(dev) = ENVSIZE(dev);
-- 
2.23.0

[PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Wolfgang Denk]

Dear Rasmus,

In message <20200320140414.19689-1-rasmus.villemoes@prevas.dk> you wrote:
>
> Having different fw_env.config files for the different revisions is
> highly impractical, and the correct information is already available
> right at our fingertips. So use the erasesize returned by the
> MEMGETINFO ioctl when the fourth column is absent or contains a 0.
>
> As I'm only testing this on a NOR flash, I'm only changing the logic
> for that case, though I think it should be possible for the other
> types as well.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/env/fw_env.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 381739d28d..87c3504315 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1647,6 +1647,8 @@ static int check_device_config(int dev)
>  			goto err;
>  		}
>  		DEVTYPE(dev) = mtdinfo.type;
> +		if (DEVESIZE(dev) == 0 && mtdinfo.type == MTD_NORFLASH)
> +			DEVESIZE(dev) = mtdinfo.erasesize;
>  		if (DEVESIZE(dev) == 0)
>  			/* Assume the erase size is the same as the env-size */
>  			DEVESIZE(dev) = ENVSIZE(dev);

What happens if you - say - have an environment size of 16 KiB and
an erase block size of 4 KiB?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A memorandum is written not to inform the reader, but to protect  the
writer.                                               -- Dean Acheson

[PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Rasmus Villemoes]

On 20/03/2020 15.11, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200320140414.19689-1-rasmus.villemoes@prevas.dk> you wrote:
>>
>> Having different fw_env.config files for the different revisions is
>> highly impractical, and the correct information is already available
>> right at our fingertips. So use the erasesize returned by the
>> MEMGETINFO ioctl when the fourth column is absent or contains a 0.
>>
>> As I'm only testing this on a NOR flash, I'm only changing the logic
>> for that case, though I think it should be possible for the other
>> types as well.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  tools/env/fw_env.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>> index 381739d28d..87c3504315 100644
>> --- a/tools/env/fw_env.c
>> +++ b/tools/env/fw_env.c
>> @@ -1647,6 +1647,8 @@ static int check_device_config(int dev)
>>  			goto err;
>>  		}
>>  		DEVTYPE(dev) = mtdinfo.type;
>> +		if (DEVESIZE(dev) == 0 && mtdinfo.type == MTD_NORFLASH)
>> +			DEVESIZE(dev) = mtdinfo.erasesize;
>>  		if (DEVESIZE(dev) == 0)
>>  			/* Assume the erase size is the same as the env-size */
>>  			DEVESIZE(dev) = ENVSIZE(dev);
> 
> What happens if you - say - have an environment size of 16 KiB and
> an erase block size of 4 KiB?

The right thing - just as in my test case of an environment of 8K and
erase block size of 4K, ENVSECTORS gets computed as 2 (or 4 in your
example). And if the environment has a size which is not a multiple of
the erase block size, all the same logic as usual applies.

It really works just the same as if one explicitly put in an erase block
size in fw_env.config of 4K, but as I wrote in the commit log, there are
cases where one has to support different flashes with different erase
block sizes.

Now, if somebody has put in 0 for the erase block size (relying on the
env size being used) and a 1 in the ENVSECTORS column, then yes, this
will break. I can certainly add an additional check for ENVSECTORS(dev)
being 0 in order to apply the mtdinfo.erasesize.

Rasmus

[PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Rasmus Villemoes]

On 20/03/2020 15.27, Rasmus Villemoes wrote:
> On 20/03/2020 15.11, Wolfgang Denk wrote:
>> Dear Rasmus,
>>
>> In message <20200320140414.19689-1-rasmus.villemoes@prevas.dk> you wrote:
>>>
>>> Having different fw_env.config files for the different revisions is
>>> highly impractical, and the correct information is already available
>>> right at our fingertips. So use the erasesize returned by the
>>> MEMGETINFO ioctl when the fourth column is absent or contains a 0.
>>>
>>> As I'm only testing this on a NOR flash, I'm only changing the logic
>>> for that case, though I think it should be possible for the other
>>> types as well.
>>>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>  tools/env/fw_env.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>>> index 381739d28d..87c3504315 100644
>>> --- a/tools/env/fw_env.c
>>> +++ b/tools/env/fw_env.c
>>> @@ -1647,6 +1647,8 @@ static int check_device_config(int dev)
>>>  			goto err;
>>>  		}
>>>  		DEVTYPE(dev) = mtdinfo.type;
>>> +		if (DEVESIZE(dev) == 0 && mtdinfo.type == MTD_NORFLASH)
>>> +			DEVESIZE(dev) = mtdinfo.erasesize;
>>>  		if (DEVESIZE(dev) == 0)
>>>  			/* Assume the erase size is the same as the env-size */
>>>  			DEVESIZE(dev) = ENVSIZE(dev);
>>
>> What happens if you - say - have an environment size of 16 KiB and
>> an erase block size of 4 KiB?
> 
> The right thing - just as in my test case of an environment of 8K and
> erase block size of 4K, ENVSECTORS gets computed as 2 (or 4 in your
> example). And if the environment has a size which is not a multiple of
> the erase block size, all the same logic as usual applies.
> 
> It really works just the same as if one explicitly put in an erase block
> size in fw_env.config of 4K, but as I wrote in the commit log, there are
> cases where one has to support different flashes with different erase
> block sizes.
> 
> Now, if somebody has put in 0 for the erase block size (relying on the
> env size being used) and a 1 in the ENVSECTORS column, then yes, this
> will break.

But, it won't break silently - there's already a check

	if (ENVSIZE(dev) > ENVSECTORS(dev) * DEVESIZE(dev)) {

Rasmus

[PATCH v2] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Rasmus Villemoes]

We have a board with several revisions. The older ones use a nor flash
with 64k erase size, while the newer have a flash with 4k sectors. The
environment size is 8k.

Currently, we have to put a column containing 0x10000 (64k) in
fw_env.config in order for it to work on the older boards. But that
ends up wasting quite a lot of time on the newer boards that could
just erase the 8k occupied by the environment - strace says the 64k
erase takes 0.405 seconds. With this patch, as expected, that's about
an 8-fold better, at 0.043 seconds.

Having different fw_env.config files for the different revisions is
highly impractical, and the correct information is already available
right at our fingertips. So use the erasesize returned by the
MEMGETINFO ioctl when the fourth and fifth columns (sector size and
#sectors, respectively) are absent or contain 0, a case where the
logic previously used to use the environment size as erase size (and
consequently computed ENVSECTORS(dev) as 1).

As I'm only testing this on a NOR flash, I'm only changing the logic
for that case, though I think it should be possible for the other
types as well.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

v2: Also require ENVSECTORS(dev) to be absent (or 0) to apply
this, in order to prevent regressions in the somewhat odd case
where someone used 0 as sector size (and hence used the
environment size as sector size) and explicitly listed 1 as
#sectors. 

 tools/env/fw_env.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 381739d28d..8734663cd4 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1647,6 +1647,9 @@ static int check_device_config(int dev)
 			goto err;
 		}
 		DEVTYPE(dev) = mtdinfo.type;
+		if (DEVESIZE(dev) == 0 && ENVSECTORS(dev) == 0 &&
+		    mtdinfo.type == MTD_NORFLASH)
+			DEVESIZE(dev) = mtdinfo.erasesize;
 		if (DEVESIZE(dev) == 0)
 			/* Assume the erase size is the same as the env-size */
 			DEVESIZE(dev) = ENVSIZE(dev);
-- 
2.23.0

[PATCH v2] tools: fw_env: use erasesize from MEMGETINFO ioctl

[Tom Rini]

On Tue, Mar 24, 2020 at 01:57:40PM +0100, Rasmus Villemoes wrote:

> We have a board with several revisions. The older ones use a nor flash
> with 64k erase size, while the newer have a flash with 4k sectors. The
> environment size is 8k.
> 
> Currently, we have to put a column containing 0x10000 (64k) in
> fw_env.config in order for it to work on the older boards. But that
> ends up wasting quite a lot of time on the newer boards that could
> just erase the 8k occupied by the environment - strace says the 64k
> erase takes 0.405 seconds. With this patch, as expected, that's about
> an 8-fold better, at 0.043 seconds.
> 
> Having different fw_env.config files for the different revisions is
> highly impractical, and the correct information is already available
> right at our fingertips. So use the erasesize returned by the
> MEMGETINFO ioctl when the fourth and fifth columns (sector size and
> #sectors, respectively) are absent or contain 0, a case where the
> logic previously used to use the environment size as erase size (and
> consequently computed ENVSECTORS(dev) as 1).
> 
> As I'm only testing this on a NOR flash, I'm only changing the logic
> for that case, though I think it should be possible for the other
> types as well.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200428/02d437c5/attachment.sig>