Hi Quentin, Am 20. Januar 2026 13:47:00 MEZ schrieb Quentin Schulz <[email protected]>: >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.
I could also drop the first param and always handle as MiB...uboot also prints ram as MiB and not GiB on initialization. So only choose between print or set variable and no "floating point" calculation needs to be done...and result is simply an int. Afaik there should be no more boards with less than 1MB ram,or am i wrong? I think this makes most sense..and keeps code small and clean. >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). But this means numbers can be no fload and filesize is wrong as it returns hex value without 0x prefix. >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? BL2 is bootloader after bootrom (ATF). The board can have 2 different ram hw configurations which require BL2 compiled with different options set. The purpose for uboot command is checking available ram for updating the BL2 via tftp and picking the right file based on current installed bl2 via detected ramsize. The issue is that 4gb board does not boot with 8gb ram bl2, 8gb board boots with 4gb bl2,but cannot use the 8gb (only 4gb). So i tried to make a detection mechanism to select the right file (tftp,usb,...) for current ram configuration. >>>> + >>>> + 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. Not sure if such test makes sense except checking if function returns a string based on unknown gd->ram_size (which depends on the host running the test i guess),or can it set before runing the test so that i can just check the formatting/calculation of the string? >Cheers, >Quentin regards Frank
