| Message ID | 20240401181435.553351-2-macroalpha82@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Update RAM Bank Logic for RK3568/RK3588 | expand |
Hi Chris, On 4/1/24 20:14, Chris Morgan wrote: > From: Chris Morgan <macromorgan@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@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
On Tue, Apr 02, 2024 at 06:38:59PM +0200, Quentin Schulz wrote: > Hi Chris, > > On 4/1/24 20:14, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@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@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 :) > ). Could I just specify a size and not worry about it? A u32 should be more than enough to hold the maximum RAM address of an RK3588 board (32GB). That would allow this to work on both 32 and 64 right? Otherwise I could further restrict this code to the AARCH64 ifdef. > > > + int i; > > + > > u8 is plenty enough here :) I use it as a return value where there are negative numbers (though obviously this should never be negative since it's an increment counter). Does that matter? > > > + 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? Acknowledged. > > > + 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. Acknowledged. > > > + 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'm not quite sure I follow. Are you saying I need to increment every 4 * the value of size in the tag_header? The value I show is 0x5 in my header meaning increment every 0x14? > 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 :) > Will do. > > + > > + 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 :) ). > Should I use a (u8*) though? While that works for now, if the ATAGS get put at a higher offset wouldn't something like a u32 make more sense? > > > + 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? Most likely, yes. > > > + 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) > Will do. My original code had comments inline, but I replaced it with a function description at the top. I think I'll try to do a simple description at the top with inline comments in the next version. > > + 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? Mistake on my part, will correct. Thank you. > > 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 Thank you for your feedback. I'll work on all this and resubmit within a few days. Chris
Hi Chris, On 4/4/24 23:33, Chris Morgan wrote: > On Tue, Apr 02, 2024 at 06:38:59PM +0200, Quentin Schulz wrote: >> Hi Chris, >> >> On 4/1/24 20:14, Chris Morgan wrote: >>> From: Chris Morgan <macromorgan@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@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 :) >> ). > > Could I just specify a size and not worry about it? A u32 should be more > than enough to hold the maximum RAM address of an RK3588 board (32GB). > That would allow this to work on both 32 and 64 right? Otherwise I could > further restrict this code to the AARCH64 ifdef. > There's even a bigger issue here as phys_t is an 8B structure (on Aarch64) while the atags are actually 4B aligned, so that would unnecessarily increase the complexity for arithmetic on those addresses. So u32 would probably be fine then. >> >>> + int i; >>> + >> >> u8 is plenty enough here :) > > I use it as a return value where there are negative numbers (though > obviously this should never be negative since it's an increment > counter). Does that matter? > You could return i still and let the compiler do the conversion. No, it's not a blocker, but that may unnecessarily increase the size of the TPL. [...] >>> + 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'm not quite sure I follow. Are you saying I need to increment every > 4 * the value of size in the tag_header? The value I show is 0x5 in > my header meaning increment every 0x14? > What we have in memory is (each 4B) (correct me if I misread Rockchip's code) tag1.size = 6 tag1.magic data1[0] data1[1] data1[2] data1[3] tag2.size = 4 tag2.magic data2[0] data2[1] ... You start with addr = &tag1.size when you do addr +4, you now have addr= &data1[2] By casting data1[2] into a tag struct, you would have tagX.size = data1[2] tagX.magic = data1[3] If somehow data1[3] matches the magic, you'll detect tag data as a tag header, and that's no good. Also, you may be missing a tag by checking every 16B, which isn't guaranteed by Rockchip's ATAGS (only guaranteed to be 4B aligned) tag1.size is the size of the tag1 in multiples of 4B (u32), so the header + data. That's what I got from """ #define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) """ because of pointer arithmetic there. [...] >>> + } >>> + 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 :) ). >> > > Should I use a (u8*) though? While that works for now, if the ATAGS get > put at a higher offset wouldn't something like a u32 make more sense? > That's a good point, but because of pointer arithmetic you cannot simply have """ ddr_info = (u32*)addr + sizeof(addr); """ Because that would make ddr_info 4* 4B (sizeof(addr) * u32 pointer arithmetic). What you could do is: """ bool core_found = false; u32* addr = (void *)ATAGS_PHYS_BASE; struct tag_header *tag = addr; while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) { val = tag->magic; if (!core_found) { if (val == ATAGS_CORE_MAGIC) core_found = true; } else if (val == ATAGS_DDR_MEM_MAGIC) { break; } addr += tag->size; tag = addr; } if (addr >= (ATAGS_PHYS_BASE + ATAGS_SIZE)) return -ENODATA; ddr_info = addr + sizeof(tag) / sizeof(u32); if (!ddr_info->count || ddr_info->count > CONFIG_NR_DRAM_BANKS) return -ENODATA; """ (NOT COMPILED/TESTED) Cheers, Quentin
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; + int i; + + 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; + + while (addr < (ATAGS_PHYS_BASE + ATAGS_SIZE)) { + val = readl(addr); + if (val == ATAGS_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 == ATAGS_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 (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 i; +} + 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 (rockchip_dram_init_banksize() > 0) + return 0; #ifdef CONFIG_ARM64 /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000;