Hi Thank you for your review
Am 19. Januar 2026 11:22:07 MEZ schrieb Quentin Schulz <[email protected]>: >Hi Frank, > >On 1/18/26 11:50 AM, Frank Wunderlich wrote: >> From: Frank Wunderlich <[email protected]> >> >> 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 <[email protected]> >> --- >> 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
