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 <macromor...@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 <macromor...@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
