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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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;
>   }
>
>