[PATCH 2/7] lmb: Tidy up structure access

[Simon Glass]

Hi Heinrich,

On Wed, 23 Aug 2023 at 09:44, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 23.08.23 15:41, Simon Glass wrote:
> > In some cases it helps to define a local variable pointing to the
> > structure being accessed. This avoids lots of repeated code.
> >
> > There is no need to individually assign each struct member, so use a
> > structure assignment instead.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >   lib/lmb.c | 54 +++++++++++++++++++++++++-----------------------------
> >   1 file changed, 25 insertions(+), 29 deletions(-)
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index ae1969893f00..8b9a611c5216 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -23,20 +23,19 @@ DECLARE_GLOBAL_DATA_PTR;
> >
> >   static void lmb_dump_region(struct lmb_region *rgn, char *name)
> >   {
> > -     unsigned long long base, size, end;
> > -     enum lmb_flags flags;
> >       int i;
> >
> >       printf(" %s.cnt = 0x%lx / max = 0x%lx\n", name, rgn->cnt, rgn->max);
> >
> >       for (i = 0; i < rgn->cnt; i++) {
> > -             base = rgn->region[i].base;
> > -             size = rgn->region[i].size;
> > -             end = base + size - 1;
> > -             flags = rgn->region[i].flags;
> > +             struct lmb_property *prop = &rgn->region[i];
> > +             unsigned long long end;
> > +
> > +             end = prop->base + prop->size - 1;
> >
> >               printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n",
> > -                    name, i, base, end, size, flags);
> > +                    name, i, (unsigned long long)prop->base, end,
> > +                    (unsigned long long)prop->size, prop->flags);
> >       }
> >   }
> >
> > @@ -89,11 +88,8 @@ static void lmb_remove_region(struct lmb_region *rgn, unsigned long r)
> >   {
> >       unsigned long i;
> >
> > -     for (i = r; i < rgn->cnt - 1; i++) {
> > -             rgn->region[i].base = rgn->region[i + 1].base;
> > -             rgn->region[i].size = rgn->region[i + 1].size;
> > -             rgn->region[i].flags = rgn->region[i + 1].flags;
> > -     }
> > +     for (i = r; i < rgn->cnt - 1; i++)
> > +             rgn->region[i] = rgn->region[i + 1];
> >       rgn->cnt--;
> >   }
> >
> > @@ -122,6 +118,7 @@ void lmb_init(struct lmb *lmb)
> >
> >   void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
> >   {
> > +     struct bd_info *bd = gd->bd;
> >       ulong bank_end;
> >       int bank;
> >
> > @@ -135,12 +132,10 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
> >       /* adjust sp by 4K to be safe */
> >       sp -= align;
> >       for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > -             if (!gd->bd->bi_dram[bank].size ||
> > -                 sp < gd->bd->bi_dram[bank].start)
> > +             if (!bd->bi_dram[bank].size || sp < bd->bi_dram[bank].start)
> >                       continue;
> >               /* Watch out for RAM at end of address space! */
> > -             bank_end = gd->bd->bi_dram[bank].start +
> > -                     gd->bd->bi_dram[bank].size - 1;
> > +             bank_end = bd->bi_dram[bank].start + bd->bi_dram[bank].size - 1;
> >               if (sp > bank_end)
> >                       continue;
> >               if (bank_end > end)
> > @@ -244,9 +239,11 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
> >
> >       /* First try and coalesce this LMB with another. */
> >       for (i = 0; i < rgn->cnt; i++) {
> > -             phys_addr_t rgnbase = rgn->region[i].base;
> > -             phys_size_t rgnsize = rgn->region[i].size;
> > -             phys_size_t rgnflags = rgn->region[i].flags;
> > +             struct lmb_property *prop = &rgn->region[i];
>
> Why call a region prop? Can't we call it "region" or "rgn_ptr" or if you
> want to allude to the position in the array "pos"? This would avoid
> confusion.

We always have rgn so that would be hopelessly confusing.

I am using prop just because that is the struct name.

I did not invent the confusion:

struct lmb_region {
   unsigned long cnt;
   unsigned long max;
   struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
};

There is a region within a region, and also:

struct lmb {
   struct lmb_region memory;
   struct lmb_region reserved;
   struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
   struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
};

I did consider trying to rename one of them...but 'region' is used
extensively in the C code to mean the inner region. We could perhaps
rename the outer 'region' to an 'area'?

Regards,
Simon