Re: [PATCH v4] cmd: mem: add function for getting ram size for use in scripts

[Frank Wunderlich]

Hi

Thank you for your review

Am 19. Januar 2026 11:22:07 MEZ schrieb Quentin Schulz <quentin.schulz at cherry.de>:
>Hi Frank,
>
>On 1/18/26 11:50 AM, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w at public-files.de>
>> 
>> Add a command for getting detected ram size with possibility to write
>> to environment variable.
>> 
>> example usage:
>> 
>> BPI-R4> msize
>> 4294967296
>> BPI-R4> msize m
>> 4096m
>> BPI-R4> msize g
>> 4g
>> BPI-R4> msize g ramsize
>> BPI-R4> printenv ramsize
>> ramsize=4
>> BPI-R4>
>> 
>> board with 8GB ram:
>> 
>> BPI-R4> msize
>> 8589934592
>> BPI-R4> msize m
>> 8192m
>> BPI-R4> msize g
>> 8g
>> BPI-R4> msize g ramsize
>> BPI-R4> printenv ramsize
>> ramsize=8
>> BPI-R4>
>> 
>> Signed-off-by: Frank Wunderlich <frank-w at public-files.de>
>> ---
>> v4: drop rounding to full MB/GB as it leads to wrong display
>> v3: add missing ifdefs
>> v2: add Kconfig entry
>> ---
>>   cmd/Kconfig |  5 +++++
>>   cmd/mem.c   | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
>> 
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 5c611fb3016e..b82b17195b7e 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -974,6 +974,11 @@ config CMD_RANDOM
>>   	help
>>   	  random - fill memory with random data
>>   +config CMD_MEMSIZE
>> +	bool "memsize"
>> +	help
>> +	  Get RAM via command for use in scripts.
>> +
>
>Only available if CMD_MEMORY is set, so need a depends on.
>
>I'm wondering if this new command isn't more suited in cmd/meminfo.c? Maybe even extend the meminfo command instead of creating a new one?

Can put it in meminfo too with depency cmd_memory.

>>   config CMD_MEMTEST
>>   	bool "memtest"
>>   	help
>> diff --git a/cmd/mem.c b/cmd/mem.c
>> index d5d7ca2790bd..9dcfeacbbcc5 100644
>> --- a/cmd/mem.c
>> +++ b/cmd/mem.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/compiler.h>
>>   #include <linux/ctype.h>
>>   #include <linux/delay.h>
>> +#include <linux/sizes.h>
>>     DECLARE_GLOBAL_DATA_PTR;
>>   @@ -711,6 +712,29 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag, int argc,
>>   }
>>   #endif /* CONFIG_LOOPW */
>>   +#ifdef CONFIG_CMD_MEMSIZE
>> +static int do_mem_size(struct cmd_tbl *cmdtp, int flag, int argc,
>> +		       char *const argv[])
>> +{
>> +	u64 memsize = gd->ram_size;
>> +
>> +	if (argc > 1) {
>> +		if (!strcmp(argv[1], "m"))
>> +			memsize = memsize / SZ_1M;
>> +		else if (!strcmp(argv[1], "g"))
>> +			memsize = memsize / SZ_1G;
>> +		if (argc > 2)
>> +			env_set_ulong(argv[2], memsize);
>> +		else
>> +			printf("%lld%s\n", memsize, argv[1]);
>> +	} else {
>> +		printf("%lld\n", memsize);
>> +	}
>
>Flatten this like so:
>
>"""
>if (argc <= 1) {
>    printf("%lld\n", memsize);
>    return 0;
>}
>
>if (!strcmp(argv[1], "m"))
>    memsize = memsize / SZ_1M;
>else if (!strcmp(argv[1], "g"))
>    memsize = memsize / SZ_1G;
>if (argc > 2)
>    env_set_ulong(argv[2], memsize);
>else
>    printf("%lld%s\n", memsize, argv[1]);
>return 0;
>"""
>
>Otherwise I see a few issues:
>- rounding not made explicit, (also it's truncating and not rounding I believe here?). Why not print the actual value here as a float (of course, not using floats)? I vaguely remember some of my Rockchip board printing 1.5GiB when booting up (cannot verify because this happens due to a bug in the SDRAM init and isn't easily reproducible). We do have some boards with 256, 512MiB of RAM so we should be clear whether we are printing 0 or 0.25 since that will influence how to do math operations with the variable containing the value.

I have dropped my previous round because it leads to wrong values and i wanted to avoid floating point numbers. But yes 2 decimal digits seem to be needed to give correct output. I guess var is then string,not any floating point type.

So for GB sonething like this:

u64 ram_gb_x100 = GB_X100(gd->ram_size);
char buf[16];

snprintf(buf, sizeof(buf), "%llu.%02llu",
         ram_gb_x100 / 100,
         ram_gb_x100 % 100);

env_set("ram_size_gb", buf);

>- what if I want to save the byte size in a variable? With the current implementation, only mebibyte or gibibyte sizes can be stored. I would suggest to argc-- if m or g is passed, and then compare against argc > 1 in which case you'd use this third (in case m or g is passed) or second (if they aren't) argument to store the value.

Good idea, thinking about how to implement this the correct way :) as varname could be "g" or "b" too.

>- missing documentation on the base for the environment variable. We have some commands setting hex digits in variables, some others in decimal, so this needs to be made explicit so that people know if they need to translate stuff before using it (or how to compare two numbers against each other). Here it's decimal according to the code (maybe we should have it in hex instead? I am not sure what is the expected base for variables :/).

As i want to use it in scripts to choose correct bl2 file for bananapi r4. Imho decimal is better to read here,but it should be mentioned in doc.

>> +
>> +	return 0;
>> +}
>> +#endif /* CONFIG_CMD_MEMSIZE */
>> +
>>   #ifdef CONFIG_CMD_MEMTEST
>>   static ulong mem_test_alt(volatile ulong *buf, ulong start_addr, ulong end_addr,
>>   			  volatile ulong *dummy)
>> @@ -1404,6 +1428,14 @@ U_BOOT_CMD(
>>   );
>>   #endif /* CONFIG_LOOPW */
>>   +#ifdef CONFIG_CMD_MEMSIZE
>> +U_BOOT_CMD(
>> +	msize,	3,	1,	do_mem_size,
>> +	"get detected ram size, optional set env variable with value",
>> +	"[m, g] [envvar]"
>
>Explain m and g so the user knows when typing help msize.
>
>We need tests in test/cmd/ and a new entry in doc/usage/cmd/ for this new command.

I see this note in checkpatch,but as i rely on dynamic value and i saw no test for mem yet.

>Cheers,
>Quentin


regards Frank