[V1 PATCH 1/2] rockchip: sdram: Support getting banks from TPL for rk3568 and rk3588

[Quentin Schulz]

Hi Chris,

On 4/1/24 20:14, Chris Morgan wrote:
> From: Chris Morgan <macromorgan at hotmail.com>
> 
> Allow RK3568 and RK3588 based boards to get the RAM bank configuration
> from the ROCKCHIP_TPL stage instead of the current logic. This fixes
> both an issue where 256MB of RAM is blocked for devices with >= 4GB
> of RAM and where memory holes need to be defined for devices with
>> = 16GB of RAM. In the event that neither SOC is used and the
> ROCKCHIP_TPL stage is not used, fall back to existing logic.
> 
> Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> ---
>   arch/arm/mach-rockchip/sdram.c | 100 +++++++++++++++++++++++++++++++++
>   1 file changed, 100 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c
> index 0d9a0aef6f..e02fb03c5f 100644
> --- a/arch/arm/mach-rockchip/sdram.c
> +++ b/arch/arm/mach-rockchip/sdram.c
> @@ -12,6 +12,7 @@
>   #include <asm/io.h>
>   #include <asm/arch-rockchip/sdram.h>
>   #include <dm/uclass-internal.h>
> +#include <linux/errno.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -35,11 +36,110 @@ struct tos_parameter_t {
>   	s64 reserve[8];
>   };
>   
> +/* Tag magic */
> +#define ATAGS_CORE_MAGIC	0x54410001
> +#define ATAGS_DDR_MEM_MAGIC	0x54410052
> +
> +/* Tag size and offset */
> +#define ATAGS_SIZE		SZ_8K
> +#define ATAGS_OFFSET		(SZ_2M - ATAGS_SIZE)
> +#define ATAGS_PHYS_BASE		(CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
> +
> +/* ATAGS memory structure. */
> +struct tag_ddr_mem {
> +	u32 count;
> +	u32 version;
> +	u64 bank[20];
> +	u32 flags;
> +	u32 data[2];
> +	u32 hash;
> +} __packed;
> +
> +/**
> + * rockchip_dram_init_banksize() - Get RAM banks from Rockchip TPL
> + *
> + * Iterate through the defined ATAGS memory location to first find a
> + * valid core header, then find a valid ddr_info header. Sanity check
> + * the number of banks found. Then, iterate through the data to add
> + * each individual memory bank. Perform fixups on memory banks that
> + * overlap with a reserved space. If an error condition is received,
> + * it is expected that memory bank setup will fall back on existing
> + * logic. If ROCKCHIP_EXTERNAL_TPL is false then immediately return,
> + * and if neither ROCKCHIP_RK3588 or ROCKCHIP_RK3568 is enabled
> + * immediately return.
> + *
> + * Return number of banks found on success or negative on error.
> + */
> +__weak int rockchip_dram_init_banksize(void)
> +{
> +	struct tag_ddr_mem *ddr_info;
> +	size_t val;
> +	size_t addr = ATAGS_PHYS_BASE;

I think this should be phys_addr_t instead of size_t?

size_t is an unsigned long on aarch64 and phys_addr_t is an unsigned 
long long so 4B vs 8B.

This however would likely prevent us from reusing this code on aarch32 
machines, but maybe it's a problem for the people who'll look into 
supporting this :) (also, aarch32 and >= 3.75GiB may be a bit optimistic 
:) ).

> +	int i;
> +

u8 is plenty enough here :)

> +	if (!IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL))
> +		return 0;
> +	if (!IS_ENABLED(CONFIG_ROCKCHIP_RK3588) &&
> +	    !IS_ENABLED(CONFIG_ROCKCHIP_RK3568))
> +		return 0;
> +
> +	if (!IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL))
> +		return -EPERM;
> +

I think testing once is enough :)

Also, you probably want to use -ENOTSUPP instead?

> +	while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) {
> +		val = readl(addr);
> +		if (val == ATAGS_CORE_MAGIC)

Save a variable by not saving the result of readl and just check against 
ATAGS_CORE_MAGIC.

> +			break;
> +		addr += 4;

This is an incorrect step size, addr is 4B, so this will result in 16B 
increments, which may be too much. Additionally, we shouldn't read every 
4B as the tag is only ever guaranteed to be 4B aligned, not that we 
would have a tag every 4B. This also means that it's possible somehow 
the content of a tag at a 4B-aligned offset has the CORE_MAGIC for some 
reason, but we shouldn't match on it.

I recommend to follow what Rockchip does downstream:

"""
struct tag_header {
     u32 size; /* size in units of 4B */
     u32 magic;
};
"""

if magic != CORE_MAGIC, then we should increment addr by size * 4B and 
check again.

> +	}
> +	if (addr >= (ATAGS_PHYS_BASE + ATAGS_SIZE))
> +		return -ENODATA;

I think it'd be nice to have debug messages here and there :)

> +
> +	while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) {
> +		val = readl(addr);
> +		if (val == ATAGS_DDR_MEM_MAGIC)
> +			break;
> +		addr += 4;

Same remarks here.

> +	}
> +	if (addr >= (ATAGS_PHYS_BASE + ATAGS_SIZE))
> +		return -ENODATA;
> +
> +	ddr_info = (void *)addr + 4;

It seems that arithmetic operations on void pointers is illegal in the C 
standard. This also quite obfuscate what you want to do here.

Considering that in this patch you're iterating for each 4B until you 
get the MAGIC, the next 4B are data for that header of that magic.

If you go for the tag_header I suggested above, I would recommend to do 
the following instead:

"""
ddr_info = (u8*)addr + sizeof(addr);
"""

This is a bit more explicit wrt to what's expected, we want to have the 
address of the data right after the tag_header we point to currently. 
(and in-code comments also do not hurt :) ).


> +	if (!ddr_info->count || ddr_info->count > CONFIG_NR_DRAM_BANKS)
> +		return -ENODATA;
> +
> +	for (i = 0; i < (ddr_info->count); i++) {
> +		size_t start_addr = ddr_info->bank[i];

This should be phys_addr_t since it represents an address in physical 
memory?

> +		size_t size = ddr_info->bank[(i + ddr_info->count)];
> +		size_t tmp;
> +

Please add a comment here to explain everything about those first 2M :) 
(reserved for TF-A but sometimes the ATAGS don't have it reserved)

> +		if (start_addr < SZ_2M) {
> +			tmp = SZ_2M - start_addr;
> +			start_addr = SZ_2M;
> +			size = size - tmp;
> +		}
> +

Same here, a small comment to explain the check below would be nice.

> +		if (start_addr >= SDRAM_MAX_SIZE && start_addr < SZ_4G)
> +			start_addr = SZ_4G;
> +

Why are we not changing the size here as well?

Same here, a small comment to explain the check below would be nice.

> +		tmp = start_addr + size;
> +		if (tmp > SDRAM_MAX_SIZE && tmp < SZ_4G)
> +			size = SDRAM_MAX_SIZE - start_addr;
> + > +		gd->bd->bi_dram[i].start = start_addr;
> +		gd->bd->bi_dram[i].size = size;
> +	}
> +
> +	return i;
> +}
> +

Cheers,
Quentin