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

[Jonas Karlman]

Hi Chris and Quentin,

On 2024-03-27 11:51, Quentin Schulz wrote:
> 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))

CONFIG_IS_ENABLED should only be used if there is an xPL variant of the
config symbol, IS_ENABLED should probably be used here. Also I would
possible check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) instead.

>> +		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.

You should use SDRAM_MAX_SIZE instead of 0xf0000000.

> 
>> +	 * 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?

The Rockchip TPL will report differently depending on model/build.

Some report regions that can be used as-is, other report actual memory.
So we must handle the case where first bank is reported for
0 - SDRAM_MAX_SIZE and the other case when full memory, 0 - 8 GiB, is
reported and skips the SDRAM_MAX_SIZE - 4GiB hw regs space.

Here are dumps of ddr_mem atag on RK3568/RK3588:

RK3588: https://gist.github.com/Kwiboo/1c020d37e3adbc9d0d79dc003d921977
RK3568: https://gist.github.com/Kwiboo/6d983693c79365b43c330eb3191cbace

  count = number of banks
  bank[x] = start addr
  bank[x + count] = size

> 
>> +	 */
>> +	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?

Based on dumps of multiple different versions and models, the banks have
always been reported in correct order.

Regards,
Jonas

> 
> Cheers,
> Quentin