[RFC 0/2] Update RAM Bank Logic for RK3588

[RFC 0/2] Update RAM Bank Logic for RK3588

[Chris Morgan]

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(+)

-- 
2.34.1

[RFC 1/2] rockchip: sdram: Allow board/soc specific RAM bank logic

[Chris Morgan]

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

Re: [RFC 1/2] rockchip: sdram: Allow board/soc specific RAM bank logic

[Jonas Karlman]

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;

Re: [RFC 1/2] rockchip: sdram: Allow board/soc specific RAM bank logic

[Chris Morgan]

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

[RFC 2/2] rockchip: rk3588: Add SoC specific RAM bank logic

[Chris Morgan]

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)
 {
-- 
2.34.1

Re: [RFC 2/2] rockchip: rk3588: Add SoC specific RAM bank logic

[Jonas Karlman]

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(+)
> 

Re: [RFC 2/2] rockchip: rk3588: Add SoC specific RAM bank logic

[Chris Morgan]

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(+)
> > 
>