| Message ID | 20240522-loongarch-v1-1-1407e0b69678@flygoat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | LoongArch initial support | expand |
On 22.05.24 17:34, Jiaxun Yang wrote: > Current code only tries to fetch the first memory node found in > fdt tree and determine memory banks from multiple reg properties. > > Linux do allow multiple memory nodes in devicetree, rework %/do allow/allows/ It is not Linux that allows it but Devicetree Specification, Release v0.4 https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf > 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> > --- > lib/fdtdec.c | 137 ++++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 83 insertions(+), 54 deletions(-) > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index b2c59ab3818b..403b363043d6 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1075,90 +1075,119 @@ ofnode get_next_memory_node(ofnode mem) > return mem; > } > > +static void sort_memory_banks(int num) > +{ > + int i, j; > + phys_addr_t tmp_start; > + phys_size_t tmp_size; > + struct bd_info *bd = gd->bd; > + > + for (i = 0; i < num - 1; i++) { > + for (j = i + 1; j < num; j++) { > + if (bd->bi_dram[i].start > bd->bi_dram[j].start) { > + tmp_start = bd->bi_dram[i].start; > + tmp_size = bd->bi_dram[i].size; > + bd->bi_dram[i].start = bd->bi_dram[j].start; > + bd->bi_dram[i].size = bd->bi_dram[j].size; > + bd->bi_dram[j].start = tmp_start; > + bd->bi_dram[j].size = tmp_size; > + } > + } > + } Please, use qsort() instead. > +} > + > int fdtdec_setup_memory_banksize(void) > { > - int bank, ret, reg = 0; > - struct resource res; > + int bank = 0; > ofnode mem = ofnode_null(); > > - mem = get_next_memory_node(mem); > - if (!ofnode_valid(mem)) { > - debug("%s: Missing /memory node\n", __func__); > - return -EINVAL; > - } > + while (true) { > + struct resource res; > + int reg = 0; > > - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { > - ret = ofnode_read_resource(mem, reg++, &res); > - if (ret < 0) { > - reg = 0; > - mem = get_next_memory_node(mem); > - if (!ofnode_valid(mem)) > - break; > + mem = get_next_memory_node(mem); > + if (!ofnode_valid(mem)) > + break; > > - ret = ofnode_read_resource(mem, reg++, &res); > + while (true) { > + int ret = ofnode_read_resource(mem, reg, &res); Please, leave a blank line after declarations. > if (ret < 0) > break; > - } > > - if (ret != 0) > - return -EINVAL; > + if (bank >= CONFIG_VAL(NR_DRAM_BANKS)) > + goto too_may_memory_banks; > > - gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; The conversion is superfluous. > - gd->bd->bi_dram[bank].size = > - (phys_size_t)(res.end - res.start + 1); > + gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; > + gd->bd->bi_dram[bank].size = > + (phys_size_t)(res.end - res.start + 1); > > - debug("%s: DRAM Bank #%d: start = 0x%llx, size = 0x%llx\n", > - __func__, bank, > - (unsigned long long)gd->bd->bi_dram[bank].start, > - (unsigned long long)gd->bd->bi_dram[bank].size); > + 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->bd->bi_dram[bank].start, > + (unsigned long long)gd->bd->bi_dram[bank].size); > + reg++; > + bank++; > + } > } > > + if (!bank) { > + log_warning("%s: Missing /memory node\n", __func__); > + return -EINVAL; > + } > + > + sort_memory_banks(bank); > + > return 0; > + > +too_may_memory_banks: > + log_warning("%s: Too many memory banks\n", __func__); > + return -EINVAL; > } > > int fdtdec_setup_mem_size_base_lowest(void) > { > - int bank, ret, reg = 0; > - struct resource res; > - unsigned long base; > - phys_size_t size; > + int bank = 0; > ofnode mem = ofnode_null(); > + __maybe_unused const char *final_name; > + __maybe_unused int final_reg; '__maybe_unused' is superfluous. > > gd->ram_base = (unsigned long)~0; gd->ram_base = ULONG_MAX; > > - mem = get_next_memory_node(mem); > - if (!ofnode_valid(mem)) { > - debug("%s: Missing /memory node\n", __func__); > - return -EINVAL; > - } > + while (true) { > + struct resource res; > + phys_size_t base, size; > + int reg = 0; > > - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { > - ret = ofnode_read_resource(mem, reg++, &res); > - if (ret < 0) { > - reg = 0; > - mem = get_next_memory_node(mem); > - if (!ofnode_valid(mem)) > - break; > + mem = get_next_memory_node(mem); > + if (!ofnode_valid(mem)) > + break; > > - ret = ofnode_read_resource(mem, reg++, &res); > + while (true) { > + int ret = ofnode_read_resource(mem, reg, &res); > if (ret < 0) > break; > + base = res.start; > + size = res.end - res.start + 1; > + if (gd->ram_base > base && size) { > + gd->ram_base = base; > + gd->ram_size = size; > + final_name = ofnode_get_name(mem); > + final_reg = reg; > + } > + reg++; > + bank++; > } > + } > > - if (ret != 0) > - return -EINVAL; > - > - base = (unsigned long)res.start; > - size = (phys_size_t)(res.end - res.start + 1); > - > - if (gd->ram_base > base && size) { > - gd->ram_base = base; > - gd->ram_size = size; > - debug("%s: Initial DRAM base %lx size %lx\n", > - __func__, base, (unsigned long)size); > - } > + if (!bank) { > + log_warning("%s: Missing /memory node\n", __func__); > + return -EINVAL; > } > > + log_debug("%s: Initial DRAM %s.%d: base %lx size %lx\n", > + __func__, final_name, final_reg, > + (ulong)gd->ram_base, (ulong)gd->ram_size); ram_base is already defined as unsigned long. include/asm-generic/global_data.h:154: unsigned long ram_base; Best regards Heinrich > + > return 0; > } > >
在2024年6月11日六月 下午1:26，Heinrich Schuchardt写道： Hi Heinrich, Thanks for your comments, will fix most of them in next reversion. [...] >> - gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; > > The conversion is superfluous. This is necessary as phys_addr_t can be unsigned long or unsigned long long. > [...] >> + __maybe_unused const char *final_name; >> + __maybe_unused int final_reg; > > '__maybe_unused' is superfluous. This is necessary as when LOG_DEBUG is disabled these two variables won't be referenced. Compilers would complain about it. > >> >> gd->ram_base = (unsigned long)~0; > > gd->ram_base = ULONG_MAX; > >> [...] Thanks
Hi Jiaxun, On Tue, 11 Jun 2024 at 07:48, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > > > 在2024年6月11日六月 下午1:26，Heinrich Schuchardt写道： > Hi Heinrich, > > Thanks for your comments, will fix most of them in next reversion. > > [...] > >> - gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; > > > > The conversion is superfluous. > > This is necessary as phys_addr_t can be unsigned long or unsigned long long. > > > > [...] > >> + __maybe_unused const char *final_name; > >> + __maybe_unused int final_reg; > > > > '__maybe_unused' is superfluous. > > This is necessary as when LOG_DEBUG is disabled these two variables won't > be referenced. Compilers would complain about it. > > > > >> > >> gd->ram_base = (unsigned long)~0; > > > > gd->ram_base = ULONG_MAX; > > > >> > [...] > > Thanks > -- Can you please add a test for this function? Perhaps test/dm/fdtdec.c might be a good place? Regards, Simon > - Jiaxun
Am 11. Juni 2024 20:51:55 MESZ schrieb Simon Glass <sjg@chromium.org>: >Hi Jiaxun, > >On Tue, 11 Jun 2024 at 07:48, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >> >> >> >> 在2024年6月11日六月 下午1:26，Heinrich Schuchardt写道： >> Hi Heinrich, >> >> Thanks for your comments, will fix most of them in next reversion. >> >> [...] >> >> - gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; >> > >> > The conversion is superfluous. >> >> This is necessary as phys_addr_t can be unsigned long or unsigned long long. In C++ you would get an error. In C an assignment to an integer type of different size does not lead to a warning. Best regards Heinrich >> >> > >> [...] >> >> + __maybe_unused const char *final_name; >> >> + __maybe_unused int final_reg; >> > >> > '__maybe_unused' is superfluous. >> >> This is necessary as when LOG_DEBUG is disabled these two variables won't >> be referenced. Compilers would complain about it. >> >> > >> >> >> >> gd->ram_base = (unsigned long)~0; >> > >> > gd->ram_base = ULONG_MAX; >> > >> >> >> [...] >> >> Thanks >> -- > >Can you please add a test for this function? Perhaps test/dm/fdtdec.c >might be a good place? > >Regards, >Simon > >> - Jiaxun
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818b..403b363043d6 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1075,90 +1075,119 @@ ofnode get_next_memory_node(ofnode mem) return mem; } +static void sort_memory_banks(int num) +{ + int i, j; + phys_addr_t tmp_start; + phys_size_t tmp_size; + struct bd_info *bd = gd->bd; + + for (i = 0; i < num - 1; i++) { + for (j = i + 1; j < num; j++) { + if (bd->bi_dram[i].start > bd->bi_dram[j].start) { + tmp_start = bd->bi_dram[i].start; + tmp_size = bd->bi_dram[i].size; + bd->bi_dram[i].start = bd->bi_dram[j].start; + bd->bi_dram[i].size = bd->bi_dram[j].size; + bd->bi_dram[j].start = tmp_start; + bd->bi_dram[j].size = tmp_size; + } + } + } +} + int fdtdec_setup_memory_banksize(void) { - int bank, ret, reg = 0; - struct resource res; + int bank = 0; ofnode mem = ofnode_null(); - mem = get_next_memory_node(mem); - if (!ofnode_valid(mem)) { - debug("%s: Missing /memory node\n", __func__); - return -EINVAL; - } + while (true) { + struct resource res; + int reg = 0; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - ret = ofnode_read_resource(mem, reg++, &res); - if (ret < 0) { - reg = 0; - mem = get_next_memory_node(mem); - if (!ofnode_valid(mem)) - break; + mem = get_next_memory_node(mem); + if (!ofnode_valid(mem)) + break; - ret = ofnode_read_resource(mem, reg++, &res); + while (true) { + int ret = ofnode_read_resource(mem, reg, &res); if (ret < 0) break; - } - if (ret != 0) - return -EINVAL; + if (bank >= CONFIG_VAL(NR_DRAM_BANKS)) + goto too_may_memory_banks; - gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; - gd->bd->bi_dram[bank].size = - (phys_size_t)(res.end - res.start + 1); + gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; + gd->bd->bi_dram[bank].size = + (phys_size_t)(res.end - res.start + 1); - debug("%s: DRAM Bank #%d: start = 0x%llx, size = 0x%llx\n", - __func__, bank, - (unsigned long long)gd->bd->bi_dram[bank].start, - (unsigned long long)gd->bd->bi_dram[bank].size); + 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->bd->bi_dram[bank].start, + (unsigned long long)gd->bd->bi_dram[bank].size); + reg++; + bank++; + } } + if (!bank) { + log_warning("%s: Missing /memory node\n", __func__); + return -EINVAL; + } + + sort_memory_banks(bank); + return 0; + +too_may_memory_banks: + log_warning("%s: Too many memory banks\n", __func__); + return -EINVAL; } int fdtdec_setup_mem_size_base_lowest(void) { - int bank, ret, reg = 0; - struct resource res; - unsigned long base; - phys_size_t size; + int bank = 0; ofnode mem = ofnode_null(); + __maybe_unused const char *final_name; + __maybe_unused int final_reg; gd->ram_base = (unsigned long)~0; - mem = get_next_memory_node(mem); - if (!ofnode_valid(mem)) { - debug("%s: Missing /memory node\n", __func__); - return -EINVAL; - } + while (true) { + struct resource res; + phys_size_t base, size; + int reg = 0; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - ret = ofnode_read_resource(mem, reg++, &res); - if (ret < 0) { - reg = 0; - mem = get_next_memory_node(mem); - if (!ofnode_valid(mem)) - break; + mem = get_next_memory_node(mem); + if (!ofnode_valid(mem)) + break; - ret = ofnode_read_resource(mem, reg++, &res); + while (true) { + int ret = ofnode_read_resource(mem, reg, &res); if (ret < 0) break; + base = res.start; + size = res.end - res.start + 1; + if (gd->ram_base > base && size) { + gd->ram_base = base; + gd->ram_size = size; + final_name = ofnode_get_name(mem); + final_reg = reg; + } + reg++; + bank++; } + } - if (ret != 0) - return -EINVAL; - - base = (unsigned long)res.start; - size = (phys_size_t)(res.end - res.start + 1); - - if (gd->ram_base > base && size) { - gd->ram_base = base; - gd->ram_size = size; - debug("%s: Initial DRAM base %lx size %lx\n", - __func__, base, (unsigned long)size); - } + if (!bank) { + log_warning("%s: Missing /memory node\n", __func__); + return -EINVAL; } + log_debug("%s: Initial DRAM %s.%d: base %lx size %lx\n", + __func__, final_name, final_reg, + (ulong)gd->ram_base, (ulong)gd->ram_size); + return 0; }
Current code only tries to fetch the first memory node found in fdt tree and determine memory banks from multiple reg properties. Linux do allow 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> --- lib/fdtdec.c | 137 ++++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 83 insertions(+), 54 deletions(-)