| Message ID | 1522142971-20739-17-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | rockchip: clean up board file for rockchip SoCs | expand |
> Trust OS decode this info like this: > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/common/drivers/parameter/ddr_parameter.c#L19 > We have to set a available value, or else we get error info from > Trust OS like this: > "ERROR: over or zero region, nr=3145987, max=10" > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/include/asm/arch-rockchip/sdram_common.h | 4 ++++ > arch/arm/mach-rockchip/sdram_common.c | 21 +++++++++++++++++++++ > 2 files changed, 25 insertions(+) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
On Tue, 27 Mar 2018, Kever Yang wrote: > Trust OS decode this info like this: > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/common/drivers/parameter/ddr_parameter.c#L19 > We have to set a available value, or else we get error info from > Trust OS like this: > "ERROR: over or zero region, nr=3145987, max=10" Please describe what changes the patch makes. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> See below for requested changes. > --- > > arch/arm/include/asm/arch-rockchip/sdram_common.h | 4 ++++ > arch/arm/mach-rockchip/sdram_common.c | 21 +++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/arch/arm/include/asm/arch-rockchip/sdram_common.h b/arch/arm/include/asm/arch-rockchip/sdram_common.h > index fec8586..55c6b81 100644 > --- a/arch/arm/include/asm/arch-rockchip/sdram_common.h > +++ b/arch/arm/include/asm/arch-rockchip/sdram_common.h > @@ -55,4 +55,8 @@ size_t rockchip_sdram_size(phys_addr_t reg); > > /* Called by U-Boot board_init_r for Rockchip SoCs */ > int dram_init(void); > + > +/* Write ddr param to a known place for trustos */ > +int rockchip_setup_ddr_param(struct ram_info *info); > + > #endif > diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c > index 76dbdc8..3a71f09 100644 > --- a/arch/arm/mach-rockchip/sdram_common.c > +++ b/arch/arm/mach-rockchip/sdram_common.c > @@ -12,6 +12,15 @@ > #include <dm/uclass-internal.h> > > DECLARE_GLOBAL_DATA_PTR; > +struct ddr_param { > + u32 count; > + u32 reserved; > + u64 bank_addr; > + u64 bank_size; > +}; > + > +#define PARAM_DRAM_INFO_OFFSET 0x2000000 Having this a fixed offset feels a bit hackish... especially as this is now implemented for all SOCs. > + > size_t rockchip_sdram_size(phys_addr_t reg) > { > u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4; > @@ -81,3 +90,15 @@ ulong board_get_usable_ram_top(ulong total_size) > > return (gd->ram_top > top) ? top : gd->ram_top; > } > + > +int rockchip_setup_ddr_param(struct ram_info *info) > +{ > + struct ddr_param *dinfo = (struct ddr_param *)CONFIG_SYS_SDRAM_BASE + > + PARAM_DRAM_INFO_OFFSET; > + > + dinfo->count = 1; > + dinfo->bank_addr = info->base; > + dinfo->bank_size = info->size; > + > + return 0; > +} This setup should only be performed when preparing the system to enter the ATF. So you will need to hook it into path where the ATF is prepared for being jumped into. I would further prefer (but this is a personal preference) to eventually move to using the FDT we inject into the ATF to convey this information. >
On 04/02/2018 05:43 AM, Philipp Tomsich wrote: > > > On Tue, 27 Mar 2018, Kever Yang wrote: > >> Trust OS decode this info like this: >> https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/common/drivers/parameter/ddr_parameter.c#L19 >> >> We have to set a available value, or else we get error info from >> Trust OS like this: >> "ERROR: over or zero region, nr=3145987, max=10" > > Please describe what changes the patch makes. > >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > > See below for requested changes. > >> --- >> >> arch/arm/include/asm/arch-rockchip/sdram_common.h | 4 ++++ >> arch/arm/mach-rockchip/sdram_common.c | 21 >> +++++++++++++++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/arch/arm/include/asm/arch-rockchip/sdram_common.h >> b/arch/arm/include/asm/arch-rockchip/sdram_common.h >> index fec8586..55c6b81 100644 >> --- a/arch/arm/include/asm/arch-rockchip/sdram_common.h >> +++ b/arch/arm/include/asm/arch-rockchip/sdram_common.h >> @@ -55,4 +55,8 @@ size_t rockchip_sdram_size(phys_addr_t reg); >> >> /* Called by U-Boot board_init_r for Rockchip SoCs */ >> int dram_init(void); >> + >> +/* Write ddr param to a known place for trustos */ >> +int rockchip_setup_ddr_param(struct ram_info *info); >> + >> #endif >> diff --git a/arch/arm/mach-rockchip/sdram_common.c >> b/arch/arm/mach-rockchip/sdram_common.c >> index 76dbdc8..3a71f09 100644 >> --- a/arch/arm/mach-rockchip/sdram_common.c >> +++ b/arch/arm/mach-rockchip/sdram_common.c >> @@ -12,6 +12,15 @@ >> #include <dm/uclass-internal.h> >> >> DECLARE_GLOBAL_DATA_PTR; >> +struct ddr_param { >> + u32 count; >> + u32 reserved; >> + u64 bank_addr; >> + u64 bank_size; >> +}; >> + >> +#define PARAM_DRAM_INFO_OFFSET 0x2000000 > > Having this a fixed offset feels a bit hackish... especially as this > is now implemented for all SOCs. Yes, but it's using in all rockchip binaries now. I would like to use fdt or ATAGS instead, but this still the most simple way to implement. Many modules with different owner in different team need to update if we use fdt/atags: - Rockchip ddr.bin to provide dram info, one os_reg may not enough for those DRAM have more than one bank, - Rockchip miniloader.bin to decode the DRAM info and pass it to Trust(ATF/OPTEE); - ATF/OPTEE get the DRAM info and update what memory it used, and then passed to U-Boot, - U-Boot need to decode the DRAM info from previous stage like ddr and Trust. - U-Boot TPL equal to rockchip ddr.bin, and U-Boot SPL equal to rockchip miniloader.bin 7 modules for different SoCs need to update: ddr.bin, miniloader.bin, atf bl31.bin, optee.bin, tpl, spl, U-Boot. And here is the problem we may met: - ddr.bin/TPL only have limited available internal sram, not able to support fdt modify in most case, so ATAGS or private structure may be used; - miniloader.bin/U-Boot SPL does not support parse input parameter, miniloader event not support fdt now; - U-Boot does not support parse input parameter; So we have to use what already use in product before we make new solution work in all modules. Thanks, - Kever > >> + >> size_t rockchip_sdram_size(phys_addr_t reg) >> { >> u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4; >> @@ -81,3 +90,15 @@ ulong board_get_usable_ram_top(ulong total_size) >> >> return (gd->ram_top > top) ? top : gd->ram_top; >> } >> + >> +int rockchip_setup_ddr_param(struct ram_info *info) >> +{ >> + struct ddr_param *dinfo = (struct ddr_param >> *)CONFIG_SYS_SDRAM_BASE + >> + PARAM_DRAM_INFO_OFFSET; >> + >> + dinfo->count = 1; >> + dinfo->bank_addr = info->base; >> + dinfo->bank_size = info->size; >> + >> + return 0; >> +} > > This setup should only be performed when preparing the system to enter > the ATF. So you will need to hook it into path where the ATF is > prepared for being jumped into. > > I would further prefer (but this is a personal preference) to > eventually move to using the FDT we inject into the ATF to convey this > information. > >> >
diff --git a/arch/arm/include/asm/arch-rockchip/sdram_common.h b/arch/arm/include/asm/arch-rockchip/sdram_common.h index fec8586..55c6b81 100644 --- a/arch/arm/include/asm/arch-rockchip/sdram_common.h +++ b/arch/arm/include/asm/arch-rockchip/sdram_common.h @@ -55,4 +55,8 @@ size_t rockchip_sdram_size(phys_addr_t reg); /* Called by U-Boot board_init_r for Rockchip SoCs */ int dram_init(void); + +/* Write ddr param to a known place for trustos */ +int rockchip_setup_ddr_param(struct ram_info *info); + #endif diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c index 76dbdc8..3a71f09 100644 --- a/arch/arm/mach-rockchip/sdram_common.c +++ b/arch/arm/mach-rockchip/sdram_common.c @@ -12,6 +12,15 @@ #include <dm/uclass-internal.h> DECLARE_GLOBAL_DATA_PTR; +struct ddr_param { + u32 count; + u32 reserved; + u64 bank_addr; + u64 bank_size; +}; + +#define PARAM_DRAM_INFO_OFFSET 0x2000000 + size_t rockchip_sdram_size(phys_addr_t reg) { u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4; @@ -81,3 +90,15 @@ ulong board_get_usable_ram_top(ulong total_size) return (gd->ram_top > top) ? top : gd->ram_top; } + +int rockchip_setup_ddr_param(struct ram_info *info) +{ + struct ddr_param *dinfo = (struct ddr_param *)CONFIG_SYS_SDRAM_BASE + + PARAM_DRAM_INFO_OFFSET; + + dinfo->count = 1; + dinfo->bank_addr = info->base; + dinfo->bank_size = info->size; + + return 0; +}
Trust OS decode this info like this: https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/common/drivers/parameter/ddr_parameter.c#L19 We have to set a available value, or else we get error info from Trust OS like this: "ERROR: over or zero region, nr=3145987, max=10" Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- arch/arm/include/asm/arch-rockchip/sdram_common.h | 4 ++++ arch/arm/mach-rockchip/sdram_common.c | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+)