On Tue, Jan 20, 2026 at 01:47:00PM +0100, Quentin Schulz wrote: > 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.
Docs are always valuable, tests are sometimes valuable. -- Tom
signature.asc
Description: PGP signature
