| Message ID | 20200331172534.9080-1-justin.swartz@risingedge.co.za |
|---|---|
| State | Rejected |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: sdram: fix DRAM bank declaration around OP-TEE | expand |
Hi Justin, On 2020/4/1 上午1:25, Justin Swartz wrote: > If OP-TEE is configured, it makes sense to use CONFIG_OPTEE_TZDRAM_BASE > and CONFIG_OPTEE_TZDRAM_SIZE to declare the boundaries of the TrustZone > memory reserved for OP-TEE instead of assuming that a 32MB reservation is > always in place. > > In this case, the following calculations may be used to determine the > boundaries of DRAM bank 0 and 1 which surround the TrustZone reservation: > > [DRAM bank 0] > base = CONFIG_SYS_DRAM_BASE > size = CONFIG_OPTEE_TZDRAM_BASE - CONFIG_SYS_SDRAM_BASE > > [DRAM bank 1] > base = CONFIG_OPTEE_TZDRAM_BASE + CONFIG_OPTEE_TZDRAM_SIZE > size = top of memory - base of DRAM bank 1 We do not use CONFIG_OPTEE_TZDRAM_BASE and code in lib/optee/ for rockchip platform now, and this patch update to use this macro without adapt other code will break the boards already run with it. Thanks, - Kever > > Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> > --- > arch/arm/mach-rockchip/sdram.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c > index 530644c043..def2c23294 100644 > --- a/arch/arm/mach-rockchip/sdram.c > +++ b/arch/arm/mach-rockchip/sdram.c > @@ -55,16 +55,14 @@ int dram_init_banksize(void) > - CONFIG_SYS_SDRAM_BASE; > gd->bd->bi_dram[1].start = tos_parameter->tee_mem.phy_addr + > tos_parameter->tee_mem.size; > - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start > - + top - gd->bd->bi_dram[1].start; > + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; > } else { > gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; > - gd->bd->bi_dram[0].size = 0x8400000; > - /* Reserve 32M for OPTEE with TA */ > - gd->bd->bi_dram[1].start = CONFIG_SYS_SDRAM_BASE > - + gd->bd->bi_dram[0].size + 0x2000000; > - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start > - + top - gd->bd->bi_dram[1].start; > + gd->bd->bi_dram[0].size = CONFIG_OPTEE_TZDRAM_BASE > + - CONFIG_SYS_SDRAM_BASE; > + gd->bd->bi_dram[1].start = CONFIG_OPTEE_TZDRAM_BASE > + + CONFIG_OPTEE_TZDRAM_SIZE; > + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; > } > #else > gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;
Hi Kever, On 2020-04-17 08:48, Kever Yang wrote: > Hi Justin, > > On 2020/4/1 上午1:25, Justin Swartz wrote: > >> If OP-TEE is configured, it makes sense to use >> CONFIG_OPTEE_TZDRAM_BASE >> and CONFIG_OPTEE_TZDRAM_SIZE to declare the boundaries of the >> TrustZone >> memory reserved for OP-TEE instead of assuming that a 32MB reservation >> is >> always in place. >> >> In this case, the following calculations may be used to determine the >> boundaries of DRAM bank 0 and 1 which surround the TrustZone >> reservation: >> >> [DRAM bank 0] >> base = CONFIG_SYS_DRAM_BASE >> size = CONFIG_OPTEE_TZDRAM_BASE - CONFIG_SYS_SDRAM_BASE >> >> [DRAM bank 1] >> base = CONFIG_OPTEE_TZDRAM_BASE + CONFIG_OPTEE_TZDRAM_SIZE >> size = top of memory - base of DRAM bank 1 > > We do not use CONFIG_OPTEE_TZDRAM_BASE and code in lib/optee/ for > rockchip platform now, > and this patch update to use this macro without adapt other code will > break the boards already > run with it. Considering that this block of code is guarded by "#ifdef CONFIG_SPL_OPTEE", why wouldn't it make sense to use the pair of CONFIG_OPTEE_TZDRAM_BASE and CONFIG_OPTEE_TZDRAM_SIZE pair to define the boundaries of the TrustZone reservation? Surely not every configuration requires a 32MB reservation at a fixed offset? I am curious to know which boards will break with these changes applied considering that the current calculation of the DRAM bank 1 size appears to be incorrect. From an RK3229 based device, with 1024MB of RAM, here is an excerpt from the output of the bdinfo command prior to applying the patch: => bdinfo arch_number = 0x00000000 boot_params = 0x00000000 DRAM bank = 0x00000000 -> start = 0x60000000 -> size = 0x08400000 DRAM bank = 0x00000001 -> start = 0x6a400000 -> size = 0x95c00000 baudrate = 1500000 bps TLB addr = 0x9fff0000 relocaddr = 0x9ff7c000 reloc off = 0x3ef7c000 irq_sp = 0x9df6d040 sp start = 0x9df6d030 Early malloc usage: 6c4 / 800 fdt_blob = 0x9df6d058 And after applying the patch: => bdinfo arch_number = 0x00000000 boot_params = 0x00000000 DRAM bank = 0x00000000 -> start = 0x60000000 -> size = 0x08400000 DRAM bank = 0x00000001 -> start = 0x68600000 -> size = 0x37a00000 baudrate = 1500000 bps TLB addr = 0x9fff0000 relocaddr = 0x9ff81000 reloc off = 0x3ef81000 irq_sp = 0x9df71580 sp start = 0x9df71570 Early malloc usage: 6ac / 800 fdt_blob = 0x9df71598 Notice the difference in reported DRAM bank 1 sizes, and the lower DRAM bank 1 starting offset after patching. As much as I might wish that this device has 0x95c00000 bytes (+-2396MB) to spare beyond OP-TEE, it really doesn't. :) Kind Regards, Justin > Thanks, > > - Kever > >> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> >> --- >> arch/arm/mach-rockchip/sdram.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/mach-rockchip/sdram.c >> b/arch/arm/mach-rockchip/sdram.c >> index 530644c043..def2c23294 100644 >> --- a/arch/arm/mach-rockchip/sdram.c >> +++ b/arch/arm/mach-rockchip/sdram.c >> @@ -55,16 +55,14 @@ int dram_init_banksize(void) >> - CONFIG_SYS_SDRAM_BASE; >> gd->bd->bi_dram[1].start = tos_parameter->tee_mem.phy_addr + >> tos_parameter->tee_mem.size; >> - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start >> - + top - gd->bd->bi_dram[1].start; >> + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; >> } else { >> gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; >> - gd->bd->bi_dram[0].size = 0x8400000; >> - /* Reserve 32M for OPTEE with TA */ >> - gd->bd->bi_dram[1].start = CONFIG_SYS_SDRAM_BASE >> - + gd->bd->bi_dram[0].size + 0x2000000; >> - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start >> - + top - gd->bd->bi_dram[1].start; >> + gd->bd->bi_dram[0].size = CONFIG_OPTEE_TZDRAM_BASE >> + - CONFIG_SYS_SDRAM_BASE; >> + gd->bd->bi_dram[1].start = CONFIG_OPTEE_TZDRAM_BASE >> + + CONFIG_OPTEE_TZDRAM_SIZE; >> + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; >> } >> #else >> gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 530644c043..def2c23294 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -55,16 +55,14 @@ int dram_init_banksize(void) - CONFIG_SYS_SDRAM_BASE; gd->bd->bi_dram[1].start = tos_parameter->tee_mem.phy_addr + tos_parameter->tee_mem.size; - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start - + top - gd->bd->bi_dram[1].start; + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; } else { gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; - gd->bd->bi_dram[0].size = 0x8400000; - /* Reserve 32M for OPTEE with TA */ - gd->bd->bi_dram[1].start = CONFIG_SYS_SDRAM_BASE - + gd->bd->bi_dram[0].size + 0x2000000; - gd->bd->bi_dram[1].size = gd->bd->bi_dram[0].start - + top - gd->bd->bi_dram[1].start; + gd->bd->bi_dram[0].size = CONFIG_OPTEE_TZDRAM_BASE + - CONFIG_SYS_SDRAM_BASE; + gd->bd->bi_dram[1].start = CONFIG_OPTEE_TZDRAM_BASE + + CONFIG_OPTEE_TZDRAM_SIZE; + gd->bd->bi_dram[1].size = top - gd->bd->bi_dram[1].start; } #else gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;
If OP-TEE is configured, it makes sense to use CONFIG_OPTEE_TZDRAM_BASE and CONFIG_OPTEE_TZDRAM_SIZE to declare the boundaries of the TrustZone memory reserved for OP-TEE instead of assuming that a 32MB reservation is always in place. In this case, the following calculations may be used to determine the boundaries of DRAM bank 0 and 1 which surround the TrustZone reservation: [DRAM bank 0] base = CONFIG_SYS_DRAM_BASE size = CONFIG_OPTEE_TZDRAM_BASE - CONFIG_SYS_SDRAM_BASE [DRAM bank 1] base = CONFIG_OPTEE_TZDRAM_BASE + CONFIG_OPTEE_TZDRAM_SIZE size = top of memory - base of DRAM bank 1 Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> --- arch/arm/mach-rockchip/sdram.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)