Hi Sughosh, On Wed, 21 Aug 2024 at 01:48, Sughosh Ganu <[email protected]> wrote: > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <[email protected]> wrote: > > > > Hi Sughosh, > > > > On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <[email protected]> wrote: > > > > > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <[email protected]> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <[email protected]> > > > > 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 <[email protected]> > > > > > --- > > > > > 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 <[email protected]> > > > > > > > > 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
