[PATCH v2 32/32] lmb: add logic to print lmb flag strings

[Simon Glass]

Hi Sughosh,

On Wed, 21 Aug 2024 at 01:48, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > Instead of printing the LMB flags as numerical values, print them as
> > > > > strings. This makes it easier to understand what flags are associated
> > > > > with the lmb region. Also make corresponding changes to the bdinfo
> > > > > command's test code.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > ---
> > > > > Changes since V1: New patch
> > > > >
> > > > >  lib/lmb.c         | 18 ++++++++++++++++--
> > > > >  test/cmd/bdinfo.c |  4 ++--
> > > > >  2 files changed, 18 insertions(+), 4 deletions(-)
> > > >
> > > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > > >
> > > > But see below
> > > >
> > > > >
> > > > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > > > index 37d2a72951..5c5b3e9bb5 100644
> > > > > --- a/lib/lmb.c
> > > > > +++ b/lib/lmb.c
> > > > > @@ -26,6 +26,19 @@ DECLARE_GLOBAL_DATA_PTR;
> > > > >
> > > > >  static struct lmb lmb;
> > > > >
> > > > > +static void print_region_flags(enum lmb_flags flags)
> > > > > +{
> > > > > +       uint64_t bitpos;
> > > > > +       const char *flag_str[] = { "LMB_NONE", "LMB_NOMAP", "LMB_NOOVERWRITE" };
> > > >
> > > > As mentioned, LMB_NONE shouldn't be a flag. For the other two, how
> > > > about "no-map" and "no-overwrite"?
> > >
> > > So, you don't want any value to be shown with LMB_NONE? I guess
> > > LMB_NONE is indicative of the fact that the region does not have any
> > > attributes, no?
> >
> > That's my understanding, yes. This could be a later cleanup I suppose,
> > but since you are adding flags, you may as well remove this one.
>
> Maybe I am not getting your point. But we have the flags member in the
> struct lmb_region, and a region without any flags set will have a
> value of 0. And we are calling that LMB_NONE. So, if we consider the
> boot_fdt_reserve_region() function, we need to pass a flags argument
> to the function. And a value of 0 would mean LMB_NONE. Is it not
> instructive to show a value of 0 as LMB_NONE(okay, maybe not in
> uppercase), instead of having to just print 0? Not printing anything
> is a little confusing imo.

But then why set LMB_NONE to BIT(0)? That has the value of 1 and
suggests it is useful in some way.

You could just leave it as zero. I don't see why having no flags is
confusing, but you can print 'none' if you like.

Regards,
Simon