[PATCH] env: mmc: specify that the address is a block address when erasing

[Quentin Schulz]

Hi E,

On 6/27/25 9:15 PM, E Shattow wrote:
> 
> 
> On 6/27/25 07:04, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz at cherry.de>
>>
>> I got very confused by the address printed when erasing the environment
>> because I clearly didn't remember setting it at that location.
>>
>> It turns out, it's the block offset and not the byte offset that is
>> reported, so let's make this a bit more obvious to the user.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz at cherry.de>
>> ---
>> Another option would be to report the byte offset by multiplying
>> blk_start by desc->blksz I guess. The result needs to be stored in a u64
>> though instead of uint otherwise we'll be limited to 4GiB offset (which
>> should be fine in most cases but eh).
>> ---
>>   env/mmc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/env/mmc.c b/env/mmc.c
>> index 8848371eb4f5df58da2be5398f159fdd0a00da21..f16926dc74c594b15f0bc82301aa89e1d4456d59 100644
>> --- a/env/mmc.c
>> +++ b/env/mmc.c
>> @@ -383,7 +383,7 @@ static inline int erase_env(struct mmc *mmc, unsigned long size,
>>   	blk_cnt = ALIGN(size, erase_size) / desc->blksz;
>>   
>>   	n = blk_derase(desc, blk_start, blk_cnt);
>> -	printf("%d blocks erased at 0x%x: %s\n", n, blk_start,
>> +	printf("%d blocks erased starting from block at 0x%x: %s\n", n, blk_start,
>>   	       (n == blk_cnt) ? "OK" : "ERROR");
>>   
>>   	return (n == blk_cnt) ? 0 : 1;
>>
>> ---
>> base-commit: 359e3012921f2fc2d43f3c4e320a752173f82b82
>> change-id: 20250627-env-mmc-erase-blk-7a46a3e63c78
>>
>> Best regards,
> 
> NAK. This is more text on the line and less readable; by doing this here

I can suggest "%d blocks erased from block 0x%x: %s\n" instead which I 
still find more helpful than what we currently have?

> and nowhere else the overall effect is adding to the confusion.
> 
> Look at this (topically related) snippet output:
> 
> "Bytes transferred = 534773760 (1fe00000 hex)
> 
> MMC read: dev # 0, block # 8355840, count 1044480 ... 1044480 blocks
> read: OK
> Total of 534773760 byte(s) were the same"
> 
> What units and packet lengths are these numbers? It's hopeless. All of

dev # 0 is the device number, it is unlikely you'll have more than 10 
MMC devices, so whether it's hex or dec base doesn't matter much as the 
value in both bases would be the same.

Block number doesn't contain any hex character, so I would assume that 
to be dec but as you rightfully reported, we have a mix of hex and dec 
values all over the code base with and without the 0x prefix to 
differentiate which is which. Nothing I can do about that though. MMC 
has a block size of 512B, considering the total transfer size in bytes 
is 534773760 which matches the decimal number in the Bytes transferred 
line, it is decimal and 1044480 is in decimal format as well since 
1044480 * 512 = 534773760.

I didn't read the code and got the information anyway, it's not great UX 
but aside from the block number, it should be fine?

> that output is garbage to me as the user, missing context required to
> make sense of it.  Adding word-qualifiers I suppose attempts to add
> clarity but the result lacks any meaning. The command which does get it
> right about hexadecimal v. decimal units does so with a displaying a
> literal word qualifier "hex" suffix, not consistent with what we're
> doing elsewhere.
> 

There's no consistency in U-Boot's code base AFAIK.

> I do agree with your premise that hex address locations and sizes, and
> block counts, are presented in a confusing mix-and-match to the user.
> Decimal numbering is the exception and not the rule but we somehow all
> decide that it's *not important* to make this obvious to the user who
> will have to input everything (except only some things, some times, but
> non-obviously) as hexadecimal. We insist on showing "0x" prefix but user
> input without this prefix is still considered as hexadecimal which is
> just terrible UX all around.
> 

Again, not consistent across the code base AFAIR, so it's even worse 
than that.

> Instead of this one patch in one place and nowhere else - I suggest to
> fix the problem which is U-Boot code base (and code style) has no
> discernible convention for documenting and displaying these outputs. I
> understand the technical limitations of decimal user input/output
> exceeding the memory type sizes, but...
> 
> 
> #1 rule: DO NOT show me decimal output while only accepting hexadecimal
> at input, it is ridiculous.
> 
> also #1 rule: Packet length not-equal-to-one? Display the packet length
> with the output.
> 
> Our consistency of units in both documentation and display of
> information is the problem - which I really do want to see resolved...
> can we?
> 

Anything that is displayed and not parsed should be ok, but then you'd 
have commands such as

u-boot> commandx bebe
Successfully did something with 48830

Is that really less confusing?

Also, what you are asking is to check and rewrite all printfs, log 
messages, etc... from all drivers, subsystems, commands, etc... which

1) isn't a small feat
2) would require all maintainers to agree on that (and a single one 
disagreeing would still make everything confusing enough as it would be 
rule for thee but not for me)

Also, this is entirely unrelated to this patch as I'm providing the unit 
of the value and not the base (which for once is explicit).

Cheers,
Quentin