[PATCH v1 11/11] [RFC] rockchip: rk356x: attempt to fix ram detection
Simon Glass
sjg at chromium.org
Sat Mar 12 06:02:47 CET 2022
Hi Peter,
On Fri, 11 Mar 2022 at 20:54, Peter Geis <pgwipeout at gmail.com> wrote:
>
> 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.
+Tom Rini
Well it is (I believe) OK to have #defines in files like asm/system.h
that are not CONFIG options. So you could move them there is there is
a separate system.h for each SoC. But if not, then Kconfig is better.
>
> >
> >
> > > + 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.
It could perhaps be a property in the dram node, like
u-boot,sdram-range = /bits 64/ <0xf0000000 0x100000000>
>
> >
> > > +
> > > #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.
I'm not sure about that one.
[..]
Regards,
Simon
More information about the U-Boot
mailing list