[PATCH 2/2] rockchip: rk3588: Add Support for RAM Defines from ATAGs

[Quentin Schulz]

Hi Chris,

On 3/26/24 21:49, Chris Morgan wrote:
> From: Chris Morgan <macromorgan at hotmail.com>
> 
> Add support for defining the usable RAM from ATAGs provided by the
> Rockchip binary TPL loader. This allows us to automatically account
> for necessary memory holes on RK3588 devices with 16GB of RAM or
> more, as well as ensure we can use the full amount of RAM available.
> 
> In the event we can't cleanly read the ATAG values from RAM or are
> not running an RK3588 board, simply fall back to the old method of
> detecting the RAM.
> 
> Tested on Indiedroid Nova with 4GB and 16GB of RAM.
> 
> Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> ---
>   arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++
>   1 file changed, 58 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c
> index 0d9a0aef6f..58b78466b0 100644
> --- a/arch/arm/mach-rockchip/sdram.c
> +++ b/arch/arm/mach-rockchip/sdram.c
> @@ -10,6 +10,7 @@
>   #include <ram.h>
>   #include <asm/global_data.h>
>   #include <asm/io.h>
> +#include <asm/arch-rockchip/atags.h>
>   #include <asm/arch-rockchip/sdram.h>
>   #include <dm/uclass-internal.h>
>   
> @@ -35,12 +36,69 @@ struct tos_parameter_t {
>   	s64 reserve[8];
>   };
>   
> +/*
> + * Read the ATAGs to identify all the memory banks. If we can't do it
> + * cleanly return 1 to note an unsuccessful attempt, otherwise return
> + * 0 for a successful attempt.

Return a valid error code instead? Negative if possible?

> + */
> +int rockchip_atag_ram_banks(void)
> +{
> +	struct tag *t;
> +	int bank_cnt;
> +	size_t tmp;
> +
> +	if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588))
> +		return 1;
> +
> +	t = atags_get_tag(ATAG_DDR_MEM);

I believe this will not compile for non RK3588 because this function is 
not defined.

There are a few ways to handle this:
- always compile atags.c but have ifdefs there with inline functions 
returning -ENOTSUPP or something like that.

> +	if (!t)
> +		return 1;
> +

-ENOENT? -ENOXIO?

> +	bank_cnt = t->u.ddr_mem.count;
> +
> +	/*
> +	 * Check to make sure the first bank ends at 0xf0000000, if it

Explain what 0xf0000000 represents here.

> +	 * does not fall back to the other methods of RAM bank
> +	 * detection.

Do we really need the first bank to ends exactly at 0xf0000000? Or 
should we rather make sure that it doesn't go into that space? (so 
anything below that would be fine?). I assume this is the result of some 
experiments, what did you learn from those boards?

> +	 */
> +	if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000)
> +		return 1;
> +

-EINVAL?

> +	/*
> +	 * Iterate over the RAM banks. If the start address of bank 0
> +	 * is less than or equal to 0x200000, set it to 0x200000 to
> +	 * reserve room for A-TF. Make sure the size of bank 0 doesn't
> +	 * bleed into the address space for hardware (starting at
> +	 * 0xf0000000). Banks 1 and on can be defined as-is.
> +	 */
> +	for (int i = 0; i < (t->u.ddr_mem.count); i++) {

This may result in out-of-bounds access. Indeed gd->bd->bi_dram is a 
build-time allocated array of size CONFIG_NR_DRAM_BANKS.

So I would recommend printing an error message if t->u.ddr_mem.count is 
bigger than CONFIG_NR_DRAM_BANKS. I assume we may want to still boot but 
with less RAM in that case? If we do, then only loop for the min between 
t->u.ddr_mem.count and CONFIG_NR_DRAM_BANKS.

> +		if (i == 0) {
> +			if (t->u.ddr_mem.bank[i] <= 0x200000)
> +				gd->bd->bi_dram[i].start = 0x200000;
> +			else
> +				gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];
> +			tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)];

This is incorrect, it may be offsetting up to 0x200000. If it's offset, 
you need to remove it from the address size.

"""
if (t->u.ddr_mem.bank[i] <= 0x200000)
     tmp -= 0x200000;
"""

> +			if (tmp > 0xf0000000)
> +				gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start;

Shouldn't we do this check for all declared address spaces, not only the 
first one?

> +			else
> +				gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];

You may need to remove the 0x200000 offset here as well, e.g. with

"""

if (tmp > 0xf0000000)
     tmp = 0xf0000000;

gd->bd->bi_dram[i].size = tmp - gd->bd->bi_dram[i].start;
"""

Renaming tmp into end would probably also help figure out what it's 
supposed to contain.

> +		} else {
> +			gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];
> +			gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];
> +		}
> +	};
> +

You can make this for-loop logic a bit more readable (well, to me :) ) with:

"""
/* Iterate over the RAM banks. */
/* If the start address of bank 0
  * is less than or equal to 0x200000, set it to 0x200000 to
  * reserve room for A-TF. Make sure the size of bank 0 doesn't
  * bleed into the address space for hardware (starting at
  * 0xf0000000).
  */
if (t->u.ddr_mem.bank[0] <= 0x200000)
     gd->bd->bi_dram[0].start = 0x200000;
else
     gd->bd->bi_dram[0].start = t->u.ddr_mem.bank[0];

tmp = gd->bd->bi_dram[0].start + t->u.ddr_mem.bank[bank_cnt];

if (tmp > 0xf0000000)
     gd->bd->bi_dram[0].size = 0xf0000000 - gd->bd->bi_dram[0].start;
else
     gd->bd->bi_dram[0].size = t->u.ddr_mem.bank[bank_cnt];

for (int i = 1; i < (t->u.ddr_mem.count); i++) {
     gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];
     gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];
}
"""

Side question, do gd->bd->bi_dram have to store banks in a specific 
order (e.g. from lowest (0) to highest (0xfffffff) location in memory) 
or can it be random? If the former, is Rockchip guaranteeing us that the 
ATAGS will always be ordered properly or should we order them at runtime 
too?

Cheers,
Quentin