| Message ID | 20240330050515.470025-2-macroalpha82@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Update RAM Bank Logic for RK3588 | expand |
Hi Chris, On 2024-03-30 06:05, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > Allow individual boards or SoCs to alter the RAM bank addition logic > by defining a __weak function that these boards can then override > if needed. In the event this function fails, fallback to the default > detection logic. > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > arch/arm/mach-rockchip/sdram.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c > index 0d9a0aef6f..53aa19feca 100644 > --- a/arch/arm/mach-rockchip/sdram.c > +++ b/arch/arm/mach-rockchip/sdram.c > @@ -35,11 +35,18 @@ struct tos_parameter_t { > s64 reserve[8]; > }; > > +__weak int rk_get_ram_banks(void) I would call this rockchip_dram_init_banksize() > +{ > + return -EINVAL; and return 0 in default implementation, > +} > + > 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 (!rk_get_ram_banks()) > + return 0; and something like: ret = rockchip_dram_init_banksize(); if (ret) return ret; is probably a better pattern when allowing board specific weak implementations. Regards, Jonas > #ifdef CONFIG_ARM64 > /* Reserve 0x200000 for ATF bl31 */ > gd->bd->bi_dram[0].start = 0x200000;
On Sat, Mar 30, 2024 at 12:00:46PM +0100, Jonas Karlman wrote: > Hi Chris, > > On 2024-03-30 06:05, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > Allow individual boards or SoCs to alter the RAM bank addition logic > > by defining a __weak function that these boards can then override > > if needed. In the event this function fails, fallback to the default > > detection logic. > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > --- > > arch/arm/mach-rockchip/sdram.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c > > index 0d9a0aef6f..53aa19feca 100644 > > --- a/arch/arm/mach-rockchip/sdram.c > > +++ b/arch/arm/mach-rockchip/sdram.c > > @@ -35,11 +35,18 @@ struct tos_parameter_t { > > s64 reserve[8]; > > }; > > > > +__weak int rk_get_ram_banks(void) > > I would call this rockchip_dram_init_banksize() > > > +{ > > + return -EINVAL; > > and return 0 in default implementation, > > > +} > > + > > 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 (!rk_get_ram_banks()) > > + return 0; > > and something like: > > ret = rockchip_dram_init_banksize(); > if (ret) > return ret; > > is probably a better pattern when allowing board specific weak > implementations. Thank you for the input, I'll find a way to refactor it where we return 0 on success, return < 0 on failure, and return > 0 on success. Then we can just check for values > 0 to skip the fallback code. Chris > > Regards, > Jonas > > > #ifdef CONFIG_ARM64 > > /* Reserve 0x200000 for ATF bl31 */ > > gd->bd->bi_dram[0].start = 0x200000; >
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..53aa19feca 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -35,11 +35,18 @@ struct tos_parameter_t { s64 reserve[8]; }; +__weak int rk_get_ram_banks(void) +{ + return -EINVAL; +} + 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 (!rk_get_ram_banks()) + return 0; #ifdef CONFIG_ARM64 /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000;