| Message ID | 20240814110009.45310-33-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make LMB memory map global and persistent | expand |
Hi Sughosh, On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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"? > + > + do { > + bitpos = fls(flags) - 1; > + printf("%s", flag_str[bitpos]); > + flags &= ~(1ull << bitpos); > + flags ? puts(", ") : puts("\n"); > + } while (flags); > +} > + > static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) > { > struct lmb_region *rgn = lmb_rgn_lst->data; > @@ -41,8 +54,9 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) > end = base + size - 1; > flags = rgn[i].flags; > > - printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n", > - name, i, base, end, size, flags); > + printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", > + name, i, base, end, size); > + print_region_flags(flags); > } > } > > diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c > index 7dd3f7ca5b..887defc28a 100644 > --- a/test/cmd/bdinfo.c > +++ b/test/cmd/bdinfo.c > @@ -120,8 +120,8 @@ static int lmb_test_dump_region(struct unit_test_state *uts, > ut_assert_nextlinen(" %s[%d]\t[", name, i); > continue; > } > - ut_assert_nextline(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x", > - name, i, base, end, size, flags); > + ut_assert_nextlinen(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", > + name, i, base, end, size); > } > > return 0; > -- > 2.34.1 > Regards, Simon
On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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? -sughosh > > > + > > + do { > > + bitpos = fls(flags) - 1; > > + printf("%s", flag_str[bitpos]); > > + flags &= ~(1ull << bitpos); > > + flags ? puts(", ") : puts("\n"); > > + } while (flags); > > +} > > + > > static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) > > { > > struct lmb_region *rgn = lmb_rgn_lst->data; > > @@ -41,8 +54,9 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) > > end = base + size - 1; > > flags = rgn[i].flags; > > > > - printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n", > > - name, i, base, end, size, flags); > > + printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", > > + name, i, base, end, size); > > + print_region_flags(flags); > > } > > } > > > > diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c > > index 7dd3f7ca5b..887defc28a 100644 > > --- a/test/cmd/bdinfo.c > > +++ b/test/cmd/bdinfo.c > > @@ -120,8 +120,8 @@ static int lmb_test_dump_region(struct unit_test_state *uts, > > ut_assert_nextlinen(" %s[%d]\t[", name, i); > > continue; > > } > > - ut_assert_nextline(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x", > > - name, i, base, end, size, flags); > > + ut_assert_nextlinen(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", > > + name, i, base, end, size); > > } > > > > return 0; > > -- > > 2.34.1 > > > > Regards, > Simon
Hi Sughosh, On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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. [..] Regards, Simon
On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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
Hi Sughosh, On Wed, 21 Aug 2024 at 01:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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
On Wed, 21 Aug 2024 at 19:30, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 21 Aug 2024 at 01:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 20 Aug 2024 at 04:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu@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@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@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. This is precisely what I am doing in the latest version(3) that I have sent. Thanks. -sughosh
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" }; + + do { + bitpos = fls(flags) - 1; + printf("%s", flag_str[bitpos]); + flags &= ~(1ull << bitpos); + flags ? puts(", ") : puts("\n"); + } while (flags); +} + static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) { struct lmb_region *rgn = lmb_rgn_lst->data; @@ -41,8 +54,9 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) end = base + size - 1; flags = rgn[i].flags; - printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n", - name, i, base, end, size, flags); + printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", + name, i, base, end, size); + print_region_flags(flags); } } diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c index 7dd3f7ca5b..887defc28a 100644 --- a/test/cmd/bdinfo.c +++ b/test/cmd/bdinfo.c @@ -120,8 +120,8 @@ static int lmb_test_dump_region(struct unit_test_state *uts, ut_assert_nextlinen(" %s[%d]\t[", name, i); continue; } - ut_assert_nextline(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x", - name, i, base, end, size, flags); + ut_assert_nextlinen(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ", + name, i, base, end, size); } return 0;
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@linaro.org> --- Changes since V1: New patch lib/lmb.c | 18 ++++++++++++++++-- test/cmd/bdinfo.c | 4 ++-- 2 files changed, 18 insertions(+), 4 deletions(-)