| Message ID | 20240326204944.1572667-3-macroalpha82@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Use ATAGs for RK3588 For RAM Info | expand |
Hi Chris, On 3/26/24 21:49, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > Add support for defining the usable RAM from ATAGs provided by the > Rockchip binary TPL loader. This allows us to automatically account > for necessary memory holes on RK3588 devices with 16GB of RAM or > more, as well as ensure we can use the full amount of RAM available. > > In the event we can't cleanly read the ATAG values from RAM or are > not running an RK3588 board, simply fall back to the old method of > detecting the RAM. > > Tested on Indiedroid Nova with 4GB and 16GB of RAM. > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c > index 0d9a0aef6f..58b78466b0 100644 > --- a/arch/arm/mach-rockchip/sdram.c > +++ b/arch/arm/mach-rockchip/sdram.c > @@ -10,6 +10,7 @@ > #include <ram.h> > #include <asm/global_data.h> > #include <asm/io.h> > +#include <asm/arch-rockchip/atags.h> > #include <asm/arch-rockchip/sdram.h> > #include <dm/uclass-internal.h> > > @@ -35,12 +36,69 @@ struct tos_parameter_t { > s64 reserve[8]; > }; > > +/* > + * Read the ATAGs to identify all the memory banks. If we can't do it > + * cleanly return 1 to note an unsuccessful attempt, otherwise return > + * 0 for a successful attempt. Return a valid error code instead? Negative if possible? > + */ > +int rockchip_atag_ram_banks(void) > +{ > + struct tag *t; > + int bank_cnt; > + size_t tmp; > + > + if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588)) > + return 1; > + > + t = atags_get_tag(ATAG_DDR_MEM); I believe this will not compile for non RK3588 because this function is not defined. There are a few ways to handle this: - always compile atags.c but have ifdefs there with inline functions returning -ENOTSUPP or something like that. > + if (!t) > + return 1; > + -ENOENT? -ENOXIO? > + bank_cnt = t->u.ddr_mem.count; > + > + /* > + * Check to make sure the first bank ends at 0xf0000000, if it Explain what 0xf0000000 represents here. > + * does not fall back to the other methods of RAM bank > + * detection. Do we really need the first bank to ends exactly at 0xf0000000? Or should we rather make sure that it doesn't go into that space? (so anything below that would be fine?). I assume this is the result of some experiments, what did you learn from those boards? > + */ > + if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000) > + return 1; > + -EINVAL? > + /* > + * Iterate over the RAM banks. If the start address of bank 0 > + * is less than or equal to 0x200000, set it to 0x200000 to > + * reserve room for A-TF. Make sure the size of bank 0 doesn't > + * bleed into the address space for hardware (starting at > + * 0xf0000000). Banks 1 and on can be defined as-is. > + */ > + for (int i = 0; i < (t->u.ddr_mem.count); i++) { This may result in out-of-bounds access. Indeed gd->bd->bi_dram is a build-time allocated array of size CONFIG_NR_DRAM_BANKS. So I would recommend printing an error message if t->u.ddr_mem.count is bigger than CONFIG_NR_DRAM_BANKS. I assume we may want to still boot but with less RAM in that case? If we do, then only loop for the min between t->u.ddr_mem.count and CONFIG_NR_DRAM_BANKS. > + if (i == 0) { > + if (t->u.ddr_mem.bank[i] <= 0x200000) > + gd->bd->bi_dram[i].start = 0x200000; > + else > + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; > + tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)]; This is incorrect, it may be offsetting up to 0x200000. If it's offset, you need to remove it from the address size. """ if (t->u.ddr_mem.bank[i] <= 0x200000) tmp -= 0x200000; """ > + if (tmp > 0xf0000000) > + gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start; Shouldn't we do this check for all declared address spaces, not only the first one? > + else > + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; You may need to remove the 0x200000 offset here as well, e.g. with """ if (tmp > 0xf0000000) tmp = 0xf0000000; gd->bd->bi_dram[i].size = tmp - gd->bd->bi_dram[i].start; """ Renaming tmp into end would probably also help figure out what it's supposed to contain. > + } else { > + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; > + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; > + } > + }; > + You can make this for-loop logic a bit more readable (well, to me :) ) with: """ /* Iterate over the RAM banks. */ /* If the start address of bank 0 * is less than or equal to 0x200000, set it to 0x200000 to * reserve room for A-TF. Make sure the size of bank 0 doesn't * bleed into the address space for hardware (starting at * 0xf0000000). */ if (t->u.ddr_mem.bank[0] <= 0x200000) gd->bd->bi_dram[0].start = 0x200000; else gd->bd->bi_dram[0].start = t->u.ddr_mem.bank[0]; tmp = gd->bd->bi_dram[0].start + t->u.ddr_mem.bank[bank_cnt]; if (tmp > 0xf0000000) gd->bd->bi_dram[0].size = 0xf0000000 - gd->bd->bi_dram[0].start; else gd->bd->bi_dram[0].size = t->u.ddr_mem.bank[bank_cnt]; for (int i = 1; i < (t->u.ddr_mem.count); i++) { gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; } """ Side question, do gd->bd->bi_dram have to store banks in a specific order (e.g. from lowest (0) to highest (0xfffffff) location in memory) or can it be random? If the former, is Rockchip guaranteeing us that the ATAGS will always be ordered properly or should we order them at runtime too? Cheers, Quentin
Hi Chris and Quentin, On 2024-03-27 11:51, Quentin Schulz wrote: > Hi Chris, > > On 3/26/24 21:49, Chris Morgan wrote: >> From: Chris Morgan <macromorgan@hotmail.com> >> >> Add support for defining the usable RAM from ATAGs provided by the >> Rockchip binary TPL loader. This allows us to automatically account >> for necessary memory holes on RK3588 devices with 16GB of RAM or >> more, as well as ensure we can use the full amount of RAM available. >> >> In the event we can't cleanly read the ATAG values from RAM or are >> not running an RK3588 board, simply fall back to the old method of >> detecting the RAM. >> >> Tested on Indiedroid Nova with 4GB and 16GB of RAM. >> >> Signed-off-by: Chris Morgan <macromorgan@hotmail.com> >> --- >> arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 58 insertions(+) >> >> diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c >> index 0d9a0aef6f..58b78466b0 100644 >> --- a/arch/arm/mach-rockchip/sdram.c >> +++ b/arch/arm/mach-rockchip/sdram.c >> @@ -10,6 +10,7 @@ >> #include <ram.h> >> #include <asm/global_data.h> >> #include <asm/io.h> >> +#include <asm/arch-rockchip/atags.h> >> #include <asm/arch-rockchip/sdram.h> >> #include <dm/uclass-internal.h> >> >> @@ -35,12 +36,69 @@ struct tos_parameter_t { >> s64 reserve[8]; >> }; >> >> +/* >> + * Read the ATAGs to identify all the memory banks. If we can't do it >> + * cleanly return 1 to note an unsuccessful attempt, otherwise return >> + * 0 for a successful attempt. > > Return a valid error code instead? Negative if possible? > >> + */ >> +int rockchip_atag_ram_banks(void) >> +{ >> + struct tag *t; >> + int bank_cnt; >> + size_t tmp; >> + >> + if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588)) CONFIG_IS_ENABLED should only be used if there is an xPL variant of the config symbol, IS_ENABLED should probably be used here. Also I would possible check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) instead. >> + return 1; >> + >> + t = atags_get_tag(ATAG_DDR_MEM); > > I believe this will not compile for non RK3588 because this function is > not defined. > > There are a few ways to handle this: > - always compile atags.c but have ifdefs there with inline functions > returning -ENOTSUPP or something like that. > >> + if (!t) >> + return 1; >> + > > -ENOENT? -ENOXIO? > >> + bank_cnt = t->u.ddr_mem.count; >> + >> + /* >> + * Check to make sure the first bank ends at 0xf0000000, if it > > Explain what 0xf0000000 represents here. You should use SDRAM_MAX_SIZE instead of 0xf0000000. > >> + * does not fall back to the other methods of RAM bank >> + * detection. > > Do we really need the first bank to ends exactly at 0xf0000000? Or > should we rather make sure that it doesn't go into that space? (so > anything below that would be fine?). I assume this is the result of some > experiments, what did you learn from those boards? The Rockchip TPL will report differently depending on model/build. Some report regions that can be used as-is, other report actual memory. So we must handle the case where first bank is reported for 0 - SDRAM_MAX_SIZE and the other case when full memory, 0 - 8 GiB, is reported and skips the SDRAM_MAX_SIZE - 4GiB hw regs space. Here are dumps of ddr_mem atag on RK3568/RK3588: RK3588: https://gist.github.com/Kwiboo/1c020d37e3adbc9d0d79dc003d921977 RK3568: https://gist.github.com/Kwiboo/6d983693c79365b43c330eb3191cbace count = number of banks bank[x] = start addr bank[x + count] = size > >> + */ >> + if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000) >> + return 1; >> + > > -EINVAL? > >> + /* >> + * Iterate over the RAM banks. If the start address of bank 0 >> + * is less than or equal to 0x200000, set it to 0x200000 to >> + * reserve room for A-TF. Make sure the size of bank 0 doesn't >> + * bleed into the address space for hardware (starting at >> + * 0xf0000000). Banks 1 and on can be defined as-is. >> + */ >> + for (int i = 0; i < (t->u.ddr_mem.count); i++) { > > This may result in out-of-bounds access. Indeed gd->bd->bi_dram is a > build-time allocated array of size CONFIG_NR_DRAM_BANKS. > > So I would recommend printing an error message if t->u.ddr_mem.count is > bigger than CONFIG_NR_DRAM_BANKS. I assume we may want to still boot but > with less RAM in that case? If we do, then only loop for the min between > t->u.ddr_mem.count and CONFIG_NR_DRAM_BANKS. > >> + if (i == 0) { >> + if (t->u.ddr_mem.bank[i] <= 0x200000) >> + gd->bd->bi_dram[i].start = 0x200000; >> + else >> + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; >> + tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)]; > > This is incorrect, it may be offsetting up to 0x200000. If it's offset, > you need to remove it from the address size. > > """ > if (t->u.ddr_mem.bank[i] <= 0x200000) > tmp -= 0x200000; > """ > >> + if (tmp > 0xf0000000) >> + gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start; > > Shouldn't we do this check for all declared address spaces, not only the > first one? > >> + else >> + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; > > You may need to remove the 0x200000 offset here as well, e.g. with > > """ > > if (tmp > 0xf0000000) > tmp = 0xf0000000; > > gd->bd->bi_dram[i].size = tmp - gd->bd->bi_dram[i].start; > """ > > Renaming tmp into end would probably also help figure out what it's > supposed to contain. > >> + } else { >> + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; >> + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; >> + } >> + }; >> + > > You can make this for-loop logic a bit more readable (well, to me :) ) with: > > """ > /* Iterate over the RAM banks. */ > /* If the start address of bank 0 > * is less than or equal to 0x200000, set it to 0x200000 to > * reserve room for A-TF. Make sure the size of bank 0 doesn't > * bleed into the address space for hardware (starting at > * 0xf0000000). > */ > if (t->u.ddr_mem.bank[0] <= 0x200000) > gd->bd->bi_dram[0].start = 0x200000; > else > gd->bd->bi_dram[0].start = t->u.ddr_mem.bank[0]; > > tmp = gd->bd->bi_dram[0].start + t->u.ddr_mem.bank[bank_cnt]; > > if (tmp > 0xf0000000) > gd->bd->bi_dram[0].size = 0xf0000000 - gd->bd->bi_dram[0].start; > else > gd->bd->bi_dram[0].size = t->u.ddr_mem.bank[bank_cnt]; > > for (int i = 1; i < (t->u.ddr_mem.count); i++) { > gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; > gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; > } > """ > > Side question, do gd->bd->bi_dram have to store banks in a specific > order (e.g. from lowest (0) to highest (0xfffffff) location in memory) > or can it be random? If the former, is Rockchip guaranteeing us that the > ATAGS will always be ordered properly or should we order them at runtime > too? Based on dumps of multiple different versions and models, the banks have always been reported in correct order. Regards, Jonas > > Cheers, > Quentin
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..58b78466b0 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -10,6 +10,7 @@ #include <ram.h> #include <asm/global_data.h> #include <asm/io.h> +#include <asm/arch-rockchip/atags.h> #include <asm/arch-rockchip/sdram.h> #include <dm/uclass-internal.h> @@ -35,12 +36,69 @@ struct tos_parameter_t { s64 reserve[8]; }; +/* + * Read the ATAGs to identify all the memory banks. If we can't do it + * cleanly return 1 to note an unsuccessful attempt, otherwise return + * 0 for a successful attempt. + */ +int rockchip_atag_ram_banks(void) +{ + struct tag *t; + int bank_cnt; + size_t tmp; + + if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588)) + return 1; + + t = atags_get_tag(ATAG_DDR_MEM); + if (!t) + return 1; + + bank_cnt = t->u.ddr_mem.count; + + /* + * Check to make sure the first bank ends at 0xf0000000, if it + * does not fall back to the other methods of RAM bank + * detection. + */ + if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000) + return 1; + + /* + * Iterate over the RAM banks. If the start address of bank 0 + * is less than or equal to 0x200000, set it to 0x200000 to + * reserve room for A-TF. Make sure the size of bank 0 doesn't + * bleed into the address space for hardware (starting at + * 0xf0000000). Banks 1 and on can be defined as-is. + */ + for (int i = 0; i < (t->u.ddr_mem.count); i++) { + if (i == 0) { + if (t->u.ddr_mem.bank[i] <= 0x200000) + gd->bd->bi_dram[i].start = 0x200000; + else + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; + tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)]; + if (tmp > 0xf0000000) + gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start; + else + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; + } else { + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; + } + }; + + return 0; +} + 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)); #ifdef CONFIG_ARM64 + if (!rockchip_atag_ram_banks()) + return 0; /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000; gd->bd->bi_dram[0].size = top - gd->bd->bi_dram[0].start;