[PATCH] board_f: show_dram_config: Print also real DRAM size

[Simon Glass]

Hi Pali,

On Mon, 12 Sept 2022 at 15:58, Sean Anderson <sean.anderson at seco.com> wrote:
>
>
>
> On 9/12/22 2:56 PM, Pali Rohár wrote:
> > On Monday 12 September 2022 07:34:47 Simon Glass wrote:
> >> Hi Pali,
> >>
> >> On Sun, 11 Sept 2022 at 03:39, Pali Rohár <pali at kernel.org> wrote:
> >> >
> >> > 32-bit U-Boot builds cannot use more than around 2 GB of DDR memory. But on
> >> > some platforms/boards it is possible to connect also 4 GB SODIMM DDR memory.
> >> > U-Boot currently prints only effective size of RAM which can use, which may
> >> > be misleading as somebody would expect that this line prints total size of
> >> > connected DDR modules. So change show_dram_config code to prints both real
> >> > and effective DRAM size if they are different. If they are same then print
> >> > just one number like before. It is possible that effective size is just few
> >> > bytes smaller than the real size, so print both numbers only in case
> >> > function print_size() prints formats them differently.
> >> >
> >> > Signed-off-by: Pali Rohár <pali at kernel.org>
> >> > ---
> >> >  common/board_f.c | 31 ++++++++++++++++++++++++++++++-
> >> >  1 file changed, 30 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/common/board_f.c b/common/board_f.c
> >> > index 9e34fbee147e..3131a06db940 100644
> >> > --- a/common/board_f.c
> >> > +++ b/common/board_f.c
> >> > @@ -54,6 +54,7 @@
> >> >  #include <asm/sections.h>
> >> >  #include <dm/root.h>
> >> >  #include <linux/errno.h>
> >> > +#include <linux/log2.h>
> >> >
> >> >  /*
> >> >   * Pointer to initial global data area
> >> > @@ -213,6 +214,30 @@ static int announce_dram_init(void)
> >> >         return 0;
> >> >  }
> >> >
> >> > +/*
> >> > + * Check if the sizes in their natural units written in decimal format with
> >> > + * one fraction number are same.
> >> > + */
> >> > +static int sizes_near(unsigned long long size1, unsigned long long size2)
> >> > +{
> >> > +       unsigned int size1_scale = ilog2(size1) / 10 * 10;
> >> > +       unsigned int size1_val = (10 * size1 + ((1ULL << size1_scale) >> 1)) >> size1_scale;
> >> > +       unsigned int size2_scale = ilog2(size2) / 10 * 10;
> >> > +       unsigned int size2_val = (10 * size2 + ((1ULL << size2_scale) >> 1)) >> size2_scale;
> >>
> >> Can you put that expression into a function with a comment, etc.? It
> >> is a bit hard to understand.
> >
> > Ok.
> >
> >> > +
> >> > +       if (size1_val == 10240) {
> >> > +               size1_val = 10;
> >> > +               size1_scale += 10;
> >> > +       }
> >> > +
> >> > +       if (size2_val == 10240) {
> >> > +               size2_val = 10;
> >> > +               size2_scale += 10;
> >> > +       }
> >>
> >> If you are doing the same thing to each, why bother? It should not
> >> affect the expression below, should it? :
> >
> > This is interesting question, and the answer it that it is required and
> > affects comparison expression below. For example for the case when size1
> > is below 1GB limit, size2 is above 1GB limit and both values are near.
> > Imagine that size1 is approaching value 1GB from the left and size2 from
> > the right side.
> >
> >> > +
> >> > +       return size1_scale == size2_scale && size1_val == size2_val;
> >> > +}
> >> > +
> >> >  static int show_dram_config(void)
> >> >  {
> >> >         unsigned long long size;
> >> > @@ -229,7 +254,11 @@ static int show_dram_config(void)
> >> >         }
> >> >         debug("\nDRAM:  ");
> >> >
> >> > -       print_size(size, "");
> >> > +       print_size(gd->ram_size, "");
> >> > +       if (!sizes_near(gd->ram_size, size)) {
> >> > +               printf(" (effective ");
> >> > +               print_size(size, ")");
> >> > +       }
> >> >         board_add_ram_info(0);
> >> >         putc('\n');
> >> >
> >> > --
> >> > 2.20.1
> >> >
> >>
> >> Can we make this testable somehow? You could put the new code into a
> >> lib/ function, perhaps, and call it from a C unit test in test/lib ?
> >>
> >> Regards,
> >> Simon
> >
> > Meh... I do not know how to test such code. Due to size / optimization
> > requirements it is not a good idea to make function outside of board_f.c
> > file.
> >
>
> You can use TEST_STATIC from test/export.h this case.

Good point, Sean.

Also LTO take away most//all of the cost of making a static function global.

Regards,
Simon