Hi Frank,
On 1/20/26 1:09 PM, Frank Wunderlich wrote:
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:
[...]
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);
I believe we should be using GiB and MiB, so multiples of 1024, which
makes the part after the dot more cumbersome to calculate.
Likely something like
gib_per = (gd->ram_size % SZ_1G) * 1000 / 1024
gib = gd->ram_size / SZ_1G
This should give us
250
1
for 1280MiB for example (1.250GiB). Still.. that would truncate at the
MiB level (but is there really DRAM chips which aren't multiples of
1MiB???).
I'm not sure this is worth it, especially since that would make the
variable a string and I don't think string comparisons are appropriate
for comparing two numbers :)
We just need to make it explicit that the value is truncated.
- 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.
You could also force the use of the first argument to be able to dump to
a variable.
memsize
memsize b|m|g [varname]
memsize would be an alias to memsize b.
memsize b var stores the byte size in var.
memsize m var stores the (truncated) mebibyte size in var.
memsize g var stores the (truncated) gibibyte size in var.
You could also decide the prefix your size argument with a dash and if
no argument with a dash is passed, it means it's a variable name and the
default is in byte size.
- 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.
OK. Considering test
(https://docs.u-boot.org/en/latest/usage/cmd/test.html#numbers) compares
numbers assuming they're decimal if they don't start with 0x, this is
fine to do (we would need to prefix the number with 0x if we were to
store it as an hex value).
Can you tell us more about the initial issue and how you plan on fixing
it here?
That sounds like a board file (board/....) would be potentially more
appropriate for this? What is this bl2, why does it need to be different
based on DRAM size, isn't there anyway for U-boot to pass the DRAM size
to bl2 and only have one bl2 binary for example?
+
+ 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.
There's one for meminfo, but honestly I'm not sure it's a test that
makes sense (just checking whether some fixed strings are printed and
not associated values that are printed afterwards). I haven't written a
C unit test in U-Boot so far so won't be able to help there. But we
really do want docs in doc/usage/cmd for any new command.
Cheers,
Quentin
