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

[Sughosh Ganu]

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.

-sughosh

>
> [..]
>
> Regards,
> Simon