[PATCH v1 11/11] [RFC] rockchip: rk356x: attempt to fix ram detection
Peter Geis
pgwipeout at gmail.com
Sat Mar 12 04:54:16 CET 2022
On Fri, Mar 11, 2022 at 9:25 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Peter,
>
> On Mon, 21 Feb 2022 at 18:31, Peter Geis <pgwipeout at gmail.com> wrote:
> >
> > This patch attempts to fix ram detection on rk3568.
> > Prior to this, the rk3568 incorrectly detected 8gb ram as 2gb.
> > On top of this, the board panics when u-boot accesses ram above 4gb.
> >
> > Fix this by correcting ram detection in hopefully a backwards compatable
> > way, and extend board_f.c to enforce an upper limit on the ram u-boot
> > uses.
> > This allows us to limit the ram u-boot accesses, while passing the
> > correctly detected size to follow on software (eg linux).
> >
> > This has been tested on rk3566 2gb, 4gb, and 8gb configurations, as well
> > as rk3399 4gb configurations.
> >
> > I do not have other configurations available, and I do not have the
> > insights into rockchip ram handling to tell if this is the correct way
> > to go about this.
> >
> > Signed-off-by: Peter Geis <pgwipeout at gmail.com>
> > ---
> > arch/arm/mach-rockchip/Kconfig | 1 +
> > arch/arm/mach-rockchip/rk3568/rk3568.c | 29 ++++++++++++++++++++++++++
> > arch/arm/mach-rockchip/sdram.c | 19 +++++++++++------
> > common/board_f.c | 7 +++++++
> > include/configs/rk3568_common.h | 5 +++++
> > 5 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> > index 92f35309e4a6..58393cc623f8 100644
> > --- a/arch/arm/mach-rockchip/Kconfig
> > +++ b/arch/arm/mach-rockchip/Kconfig
> > @@ -264,6 +264,7 @@ config ROCKCHIP_RK3568
> > select SYSCON
> > select BOARD_LATE_INIT
> > imply ROCKCHIP_COMMON_BOARD
> > + imply OF_SYSTEM_SETUP
> > help
> > The Rockchip RK3568 is a ARM-based SoC with quad-core Cortex-A55,
> > including NEON and GPU, 512K L3 cache, Mali-G52 based graphics,
> > diff --git a/arch/arm/mach-rockchip/rk3568/rk3568.c b/arch/arm/mach-rockchip/rk3568/rk3568.c
> > index ef6bc67a88b0..8d2a59bc649d 100644
> > --- a/arch/arm/mach-rockchip/rk3568/rk3568.c
> > +++ b/arch/arm/mach-rockchip/rk3568/rk3568.c
> > @@ -5,6 +5,7 @@
> >
> > #include <common.h>
> > #include <dm.h>
> > +#include <fdt_support.h>
> > #include <asm/armv8/mmu.h>
> > #include <asm/io.h>
> > #include <asm/arch-rockchip/bootrom.h>
> > @@ -187,3 +188,31 @@ int board_usb_init(int index, enum usb_init_type init)
> > #endif /* CONFIG_USB_DWC3_GADGET */
> >
> > #endif /* CONFIG_USB_GADGET */
> > +
> > +#ifdef CONFIG_OF_SYSTEM_SETUP
> > +int ft_system_setup(void *blob, struct bd_info *bd)
> > +{
> > + int ret;
> > + int areas = 1;
> > + u64 start[2], size[2];
> > +
> > + /* Reserve the io address space. */
> > + if (gd->ram_top > SDRAM_UPPER_ADDR_MIN) {
>
> Can you put SDRAM_UPPER_ADDR_MIN and SDRAM_LOWER_ADDR_MAX into the
> device tree, perhaps?
This is the part of the patch series that gets really cludgy and I'm
not proud of it.
Essentially this is a cleaner implementation of what downstream does.
The issue is u-boot panics if it tries to access ram above 4G on this chip.
Combined with the MMIO address space below 4G, the available memory
map gets to be quite a mess.
I did try to keep it backwards compatible with the previous chips however.
The rk356x has 2G, 4G, and 8G boards in the wild.
I'll play a bit with a device tree method, to see if I can make it
make sense though.
>
>
> > + start[0] = gd->bd->bi_dram[0].start;
> > + size[0] = SDRAM_LOWER_ADDR_MAX - gd->bd->bi_dram[0].start;
> > +
> > + /* Add the upper 4GB address space */
> > + start[1] = SDRAM_UPPER_ADDR_MIN;
> > + size[1] = gd->ram_top - SDRAM_UPPER_ADDR_MIN;
> > + areas = 2;
> > +
> > + ret = fdt_set_usable_memory(blob, start, size, areas);
> > + if (ret) {
> > + printf("Cannot set usable memory\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +};
> > +#endif
> > diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c
> > index 705ec7ba6450..52974e6dc333 100644
> > --- a/arch/arm/mach-rockchip/sdram.c
> > +++ b/arch/arm/mach-rockchip/sdram.c
> > @@ -3,6 +3,8 @@
> > * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > */
> >
> > +#define DEBUG
> > +
> > #include <common.h>
> > #include <dm.h>
> > #include <init.h>
> > @@ -98,8 +100,7 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > SYS_REG_COL_MASK);
> > cs1_col = cs0_col;
> > bk = 3 - ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> > - if ((sys_reg3 >> SYS_REG_VERSION_SHIFT &
> > - SYS_REG_VERSION_MASK) == 0x2) {
> > + if ((sys_reg3 >> SYS_REG_VERSION_SHIFT & SYS_REG_VERSION_MASK) >= 0x2) {
> > cs1_col = 9 + (sys_reg3 >> SYS_REG_CS1_COL_SHIFT(ch) &
> > SYS_REG_CS1_COL_MASK);
> > if (((sys_reg3 >> SYS_REG_EXTEND_CS0_ROW_SHIFT(ch) &
> > @@ -136,7 +137,7 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > SYS_REG_BW_MASK));
> > row_3_4 = sys_reg2 >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > SYS_REG_ROW_3_4_MASK;
> > - if (dram_type == DDR4) {
> > + if ((dram_type == DDR4) && (sys_reg3 >> SYS_REG_VERSION_SHIFT & SYS_REG_VERSION_MASK) != 0x3){
>
> Space before {
>
> Also can you please add a comment as to what this is doing?
Thanks!
Indeed, I can. For reference here, it's retaining backwards
compatibility with older versions of the ram controller, but the new
ram controller ends up doubling the available ram if this is enabled.
>
> > dbw = (sys_reg2 >> SYS_REG_DBW_SHIFT(ch)) &
> > SYS_REG_DBW_MASK;
> > bg = (dbw == 2) ? 2 : 1;
> > @@ -176,9 +177,11 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > * 2. update board_get_usable_ram_top() and dram_init_banksize()
> > * to reserve memory for peripheral space after previous update.
> > */
> > +
> > +#ifndef __aarch64__
>
> if (!IS_ENABLED(CONFIG_ARM64))
Thanks!
>
> > if (size_mb > (SDRAM_MAX_SIZE >> 20))
> > size_mb = (SDRAM_MAX_SIZE >> 20);
> > -
> > +#endif
> > return (size_t)size_mb << 20;
> > }
> >
> > @@ -208,6 +211,10 @@ int dram_init(void)
> > ulong board_get_usable_ram_top(ulong total_size)
> > {
> > unsigned long top = CONFIG_SYS_SDRAM_BASE + SDRAM_MAX_SIZE;
> > -
> > - return (gd->ram_top > top) ? top : gd->ram_top;
> > +#ifdef SDRAM_UPPER_ADDR_MIN
>
> eek, what is going on here? Can you use C instead of the preprocessor?
I was trying to keep to current convention in u-boot (which I admit is
uncomfortable coming from linux).
This is clamping the max available u-boot ram to SDRAM_UPPER_ADDR_MIN,
if it is set.
This is necessary because of the above panic issue, and is very much a hack.
Essentially I attempted to make this affect nothing if it isn't set,
thus if it isn't set boards that have 8GB will detect as 2GB.
The way rockchip currently clamps the ram to less than the MMIO space
feels really slimy too, but I didn't want to try and touch it too much
and risk breaking things.
>
>
> > + if (gd->ram_top > SDRAM_UPPER_ADDR_MIN)
> > + return gd->ram_top;
> > + else
> > +#endif
> > + return (gd->ram_top > top) ? top : gd->ram_top;
> > }
> > diff --git a/common/board_f.c b/common/board_f.c
> > index a68760092ac1..933ba7aedac0 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -344,7 +344,14 @@ static int setup_dest_addr(void)
> > #endif
> > gd->ram_top = gd->ram_base + get_effective_memsize();
> > gd->ram_top = board_get_usable_ram_top(gd->mon_len);
> > +#ifdef SDRAM_LOWER_ADDR_MAX
> > + if (gd->ram_top > SDRAM_LOWER_ADDR_MAX)
> > + gd->relocaddr = SDRAM_LOWER_ADDR_MAX;
> > + else
> > + gd->relocaddr = gd->ram_top;
> > +#else
> > gd->relocaddr = gd->ram_top;
> > +#endif
> > debug("Ram top: %08lX\n", (ulong)gd->ram_top);
> > #if defined(CONFIG_MP) && (defined(CONFIG_MPC86xx) || defined(CONFIG_E500))
> > /*
> > diff --git a/include/configs/rk3568_common.h b/include/configs/rk3568_common.h
> > index 25d7c5cc8fff..8dd1b033017b 100644
> > --- a/include/configs/rk3568_common.h
> > +++ b/include/configs/rk3568_common.h
> > @@ -27,6 +27,11 @@
> > #define CONFIG_SYS_SDRAM_BASE 0
> > #define SDRAM_MAX_SIZE 0xf0000000
> >
> > +#ifdef CONFIG_OF_SYSTEM_SETUP
> > +#define SDRAM_LOWER_ADDR_MAX 0xf0000000
> > +#define SDRAM_UPPER_ADDR_MIN 0x100000000
> > +#endif
>
> These config files are going away, so either use Kconfig or device tree.
Noted, I'm not sure how this could fit in a sane way to a device tree,
so Kconfig is probably the better option.
>
> > +
> > #ifndef CONFIG_SPL_BUILD
> > #define ENV_MEM_LAYOUT_SETTINGS \
> > "scriptaddr=0x00c00000\0" \
> > --
> > 2.25.1
> >
>
This series was meant to expose the various flaws I'm working around
with the rk356x series right now.
This patch in particular was to expose the current issues with
rockchip ram detection.
It extended the already hacky method of handling this, mostly because
I don't have the internal knowledge of how Rockchip ram controllers
work.
On top of that I really don't like that u-boot can't touch above 4G
without a panic, but I definitely don't have enough knowledge of
u-boot's inner workings to fix it.
> Regards,
> Simon
Thanks for the review!
Peter
More information about the U-Boot
mailing list