Re: [PATCH v2 01/16] lib: fdtdec: Handle multiple memory nodes

[Yao Zi <me@ziyao.cc>]

On Thu, Jul 02, 2026 at 11:22:14AM +0100, Simon Glass wrote:
> Hi Jiaxun,
> 
> On 2026-07-01T11:17:53, Yao Zi <me@ziyao.cc> wrote:
> > lib: fdtdec: Handle multiple memory nodes
> >
> > Current code only tries to fetch the first memory node found in
> > fdt tree and determine memory banks from multiple reg properties.
> >
> > Specification allows multiple memory nodes in devicetree, rework
> > fdtdec_setup_mem_size_base_lowest and fdtdec_setup_memory_banksize
> > to iterate over all memory nodes.
> >
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > Signed-off-by: Yao Zi <me@ziyao.cc>
> >
> > include/asm-generic/global_data.h |   2 +-
> >  lib/fdtdec.c                      | 129 ++++++++++++++++++++++----------------
> >  2 files changed, 76 insertions(+), 55 deletions(-)
> 
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > @@ -1115,90 +1116,110 @@ ofnode fdtdec_get_next_memory_node(ofnode mem)
> > +too_may_memory_banks:
> > +     log_warning("%s: Too many memory banks\n", __func__);
> > +     return -EINVAL;
> > +}
> 
> Typo - too_may_memory_banks should be too_many_memory_banks. While
> you are here, please drop the goto... bank is bounded, so a single
> check after the loops (or bank < CONFIG_NR_DRAM_BANKS in the inner
> while) gives a single return path.

Thanks for the advice. I don't think it's okay to have only a check
after the loops, in which case we might iterate over the boundary of
gd->dram[]; and if we use bank < CONFIG_NR_DRAM_BANKS for the condition
of the inner loop, another additional check is still necessary after
the loops because we want to give out the message and a correct return
value indicating the error, right?

Alternatively, I think we could drop the goto statement and instead
return immediately in the inner loop if CONFIG_NR_DRAM_BANKS is not
sufficient, is it okay for you?

> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > @@ -1115,90 +1116,110 @@ ofnode fdtdec_get_next_memory_node(ofnode mem)
> > +                     log_debug("%s: DRAM Bank #%d %s.%d: start = 0x%llx, size = 0x%llx\n",
> > +                               __func__, bank, ofnode_get_name(mem), reg,
> > +                              (unsigned long long)gd->dram[bank].start,
> > +                              (unsigned long long)gd->dram[bank].size);
> 
> The current code uses %pap with &gd->dram[bank].start, which is the
> idiomatic way to print phys_addr_t/phys_size_t iand should avoid
> needing casts. Can you keep %pap here (and in the message below)?

Sure, this makes sense. When rebasing, I noted the change in current
master, but had only remembered to update the log_debug() message in
fdtdec_setup_mem_size_base_lowest() to use %pap.

> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > @@ -1115,90 +1116,110 @@ ofnode fdtdec_get_next_memory_node(ofnode mem)
> > +     while (true) {
> > +             struct resource res;
> > +             phys_size_t base, size;
> > +             int reg = 0;
> 
> You should really use phys_addr_t for base

Ok.

> Regards,
> Simon

Best regards,
Yao Zi