Hi Sughosh,
On Wed, 21 Aug 2024 at 05:00, Sughosh Ganu <sughosh.g...@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.g...@linaro.org>
> ---
> Changes since V2:
> * Change the logic to accomodate new value of LMB_NONE(0).
> * s/print_region_flags()/lmb_print_region_flags().
> * s/"LMB_NOMAP"/"no-map".
> * s/"LMB_NOOVERWRITE"/"no-overwrite".
>
> lib/lmb.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass <s...@chromium.org>
nit below
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 11959760b6..0379798837 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -26,6 +26,19 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static struct lmb lmb;
>
> +static void lmb_print_region_flags(enum lmb_flags flags)
> +{
> + uint64_t bitpos;
> + const char *flag_str[] = { "none", "no-map", "no-overwrite" };
This implicitly follows the enum values, but I think that is fine
given that this is debug / dump code.
> +
> + do {
> + bitpos = flags ? fls(flags) - 1 : 0;
> + printf("%s", flag_str[bitpos]);
> + flags &= ~(1ull << bitpos);
> + flags ? puts(", ") : puts("\n");
That line is pretty uncommon C :-) How about putting the ? inside the puts()?
> + } 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);
> + lmb_print_region_flags(flags);
> }
> }
>
> --
> 2.34.1
>
Regards,
Simon
