[RFC 0/2] Update RAM Bank Logic for RK3588
From: Chris Morgan macromorgan@hotmail.com
Use the ATAG info provided by the Rockchip binary TPL to identify RAM banks on the RK3588 when using the ROCKCHIP_TPL binary.
This is needed because there are specific addresses that should not be written to for all RK3588 based devices with >=16GB of RAM, writing to these addresses immediately results in a crash.
I intended this to be an RFC the first time I submitted it, so I'm correcting that mistake now. Additionally I have reduced a lot of the ATAGS code to only focus on the few bits we need in this case.
Chris Morgan (2): rockchip: sdram: Allow board/soc specific RAM bank logic rockchip: rk3588: Add SoC specific RAM bank logic
arch/arm/mach-rockchip/rk3588/rk3588.c | 93 ++++++++++++++++++++++++++ arch/arm/mach-rockchip/sdram.c | 7 ++ 2 files changed, 100 insertions(+)
From: Chris Morgan macromorgan@hotmail.com
Allow individual boards or SoCs to alter the RAM bank addition logic by defining a __weak function that these boards can then override if needed. In the event this function fails, fallback to the default detection logic.
Signed-off-by: Chris Morgan macromorgan@hotmail.com --- arch/arm/mach-rockchip/sdram.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..53aa19feca 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -35,11 +35,18 @@ struct tos_parameter_t { s64 reserve[8]; };
+__weak int rk_get_ram_banks(void) +{ + return -EINVAL; +} + int dram_init_banksize(void) { size_t ram_top = (unsigned long)(gd->ram_size + CFG_SYS_SDRAM_BASE); size_t top = min((unsigned long)ram_top, (unsigned long)(gd->ram_top));
+ if (!rk_get_ram_banks()) + return 0; #ifdef CONFIG_ARM64 /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000;
Hi Chris,
On 2024-03-30 06:05, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Allow individual boards or SoCs to alter the RAM bank addition logic by defining a __weak function that these boards can then override if needed. In the event this function fails, fallback to the default detection logic.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/sdram.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..53aa19feca 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -35,11 +35,18 @@ struct tos_parameter_t { s64 reserve[8]; };
+__weak int rk_get_ram_banks(void)
I would call this rockchip_dram_init_banksize()
+{
- return -EINVAL;
and return 0 in default implementation,
+}
int dram_init_banksize(void) { size_t ram_top = (unsigned long)(gd->ram_size + CFG_SYS_SDRAM_BASE); size_t top = min((unsigned long)ram_top, (unsigned long)(gd->ram_top));
- if (!rk_get_ram_banks())
return 0;
and something like:
ret = rockchip_dram_init_banksize(); if (ret) return ret;
is probably a better pattern when allowing board specific weak implementations.
Regards, Jonas
#ifdef CONFIG_ARM64 /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000;
On Sat, Mar 30, 2024 at 12:00:46PM +0100, Jonas Karlman wrote:
Hi Chris,
On 2024-03-30 06:05, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Allow individual boards or SoCs to alter the RAM bank addition logic by defining a __weak function that these boards can then override if needed. In the event this function fails, fallback to the default detection logic.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/sdram.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..53aa19feca 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -35,11 +35,18 @@ struct tos_parameter_t { s64 reserve[8]; };
+__weak int rk_get_ram_banks(void)
I would call this rockchip_dram_init_banksize()
+{
- return -EINVAL;
and return 0 in default implementation,
+}
int dram_init_banksize(void) { size_t ram_top = (unsigned long)(gd->ram_size + CFG_SYS_SDRAM_BASE); size_t top = min((unsigned long)ram_top, (unsigned long)(gd->ram_top));
- if (!rk_get_ram_banks())
return 0;and something like:
ret = rockchip_dram_init_banksize(); if (ret) return ret;
is probably a better pattern when allowing board specific weak implementations.
Thank you for the input, I'll find a way to refactor it where we return 0 on success, return < 0 on failure, and return > 0 on success. Then we can just check for values > 0 to skip the fallback code.
Chris
Regards, Jonas
#ifdef CONFIG_ARM64 /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000;
From: Chris Morgan macromorgan@hotmail.com
Add SoC specific RAM bank logic for the rk3588 boards. This logic works by reading the ATAGS created by the ROCKCHIP_TPL stage and applies fixups on those to ensure we aren't stepping on any reserved memory addresses.
The existing logic requires us to define memory holes to allow devices with 16GB or more RAM to function properly, as well as blocking up to 256MB of otherwise accessible RAM.
Signed-off-by: Chris Morgan macromorgan@hotmail.com --- arch/arm/mach-rockchip/rk3588/rk3588.c | 93 ++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)
diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c index 38e95a6e2b..73ff742ffc 100644 --- a/arch/arm/mach-rockchip/rk3588/rk3588.c +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c @@ -7,10 +7,14 @@ #include <common.h> #include <spl.h> #include <asm/armv8/mmu.h> +#include <asm/global_data.h> #include <asm/io.h> #include <asm/arch-rockchip/bootrom.h> #include <asm/arch-rockchip/hardware.h> #include <asm/arch-rockchip/ioc_rk3588.h> +#include <linux/errno.h> + +DECLARE_GLOBAL_DATA_PTR;
#define FIREWALL_DDR_BASE 0xfe030000 #define FW_DDR_MST5_REG 0x54 @@ -35,6 +39,15 @@ #define BUS_IOC_GPIO2D_IOMUX_SEL_H 0x5c #define BUS_IOC_GPIO3A_IOMUX_SEL_L 0x60
+/* Tag magic */ +#define ATAG_CORE_MAGIC 0x54410001 +#define ATAG_DDR_MEM_MAGIC 0x54410052 + +/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (SZ_2M - ATAGS_SIZE) +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET) + /** * Boot-device identifiers used by the BROM on RK3588 when device is booted * from SPI flash. IOMUX used for SPI flash affect the value used by the BROM @@ -83,6 +96,16 @@ static struct mm_region rk3588_mem_map[] = {
struct mm_region *mem_map = rk3588_mem_map;
+/* ATAGS memory structure. */ +struct tag_ddr_mem { + u32 count; + u32 version; + u64 bank[20]; + u32 flags; + u32 data[2]; + u32 hash; +} __packed; + /* GPIO0B_IOMUX_SEL_H */ enum { GPIO0B5_SHIFT = 4, @@ -130,6 +153,76 @@ void rockchip_stimer_init(void) } #endif
+/** + * rk_get_ram_banks() - Get RAM banks from Rockchip TPL stage + * + * 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 CONFIG_IS_ENABLED(ROCKCHIP_EXTERNAL_TPL) is false then + * immediately return. + * + * Return 0 on success or negative on error. + */ +int rk_get_ram_banks(void) +{ + struct tag_ddr_mem *ddr_info; + size_t val; + size_t addr = ATAGS_PHYS_BASE; + + if (!IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL)) + return -EPERM; + + while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) { + val = readl(addr); + if (val == ATAG_CORE_MAGIC) + break; + addr += 4; + } + if (addr >= (ATAGS_PHYS_BASE + ATAGS_SIZE)) + return -ENODATA; + + while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) { + val = readl(addr); + if (val == ATAG_DDR_MEM_MAGIC) + break; + addr += 4; + } + if (addr >= (ATAGS_PHYS_BASE + ATAGS_SIZE)) + return -ENODATA; + + ddr_info = (void *)addr + 4; + if (!ddr_info->count || ddr_info->count > CONFIG_NR_DRAM_BANKS) + return -ENODATA; + + for (int i = 0; i < (ddr_info->count); i++) { + size_t start_addr = ddr_info->bank[i]; + size_t size = ddr_info->bank[(i + ddr_info->count)]; + size_t tmp; + + if (start_addr < SZ_2M) { + tmp = SZ_2M - start_addr; + start_addr = SZ_2M; + size = size - tmp; + } + + if (start_addr >= SDRAM_MAX_SIZE && start_addr < SZ_4G) + start_addr = SZ_4G; + + 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 0; +} + #ifndef CONFIG_TPL_BUILD int arch_cpu_init(void) {
Hi Chris,
On 2024-03-30 06:05, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add SoC specific RAM bank logic for the rk3588 boards. This logic works by reading the ATAGS created by the ROCKCHIP_TPL stage and applies fixups on those to ensure we aren't stepping on any reserved memory addresses.
The existing logic requires us to define memory holes to allow devices with 16GB or more RAM to function properly, as well as blocking up to 256MB of otherwise accessible RAM.
Looks good at first glance, will runtime test later.
Please move this out from being RK3588 specific, my prior request to depend on !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) was so that this also gets used on RK356x. On 4 GiB RAM RK356x boards the same 256 MiB could be reclaimed with this.
For final patch submission please also remove all duplicated RK3588 board implementations of ft_board_setup() that adds reserved memory nodes to DT, they are no longer needed after this.
Regards, Jonas
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/rk3588/rk3588.c | 93 ++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)
On Sat, Mar 30, 2024 at 11:53:38AM +0100, Jonas Karlman wrote:
Hi Chris,
On 2024-03-30 06:05, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add SoC specific RAM bank logic for the rk3588 boards. This logic works by reading the ATAGS created by the ROCKCHIP_TPL stage and applies fixups on those to ensure we aren't stepping on any reserved memory addresses.
The existing logic requires us to define memory holes to allow devices with 16GB or more RAM to function properly, as well as blocking up to 256MB of otherwise accessible RAM.
Looks good at first glance, will runtime test later.
Thank you, I look forward to the results. I had mainline running some stress-ng memory tests last night and all appeared well for me on my 16GB board. Touching the RAM code for 1 (now 2) entire SoCs is just something I wanted a bit more eyes on though.
Please move this out from being RK3588 specific, my prior request to depend on !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) was so that this also gets used on RK356x. On 4 GiB RAM RK356x boards the same 256 MiB could be reclaimed with this.
Will do, I'll also test this on my 3566 boards. I don't think I lost any memory on those though, but then again I never had more than 2GB to work with...
For final patch submission please also remove all duplicated RK3588 board implementations of ft_board_setup() that adds reserved memory nodes to DT, they are no longer needed after this.
Will do, thank you.
Chris
Regards, Jonas
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/rk3588/rk3588.c | 93 ++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)
participants (3)
-
Chris Morgan -
Chris Morgan -
Jonas Karlman