[U-Boot] [U-Boot, 17/36] rockchip: sdram_common: add common dram_init_banksize

[Philipp Tomsich]

On Tue, 27 Mar 2018, Kever Yang wrote:

> dram_init_banksize() can be common used by all SoCs, move it into
> sdram_common.c
>
> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> ---
>
> arch/arm/mach-rockchip/sdram_common.c | 63 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c
> index 3a71f09..ff86096 100644
> --- a/arch/arm/mach-rockchip/sdram_common.c
> +++ b/arch/arm/mach-rockchip/sdram_common.c
> @@ -21,13 +21,74 @@ struct ddr_param {
>
> #define PARAM_DRAM_INFO_OFFSET 0x2000000
>
> +#define TRUST_PARAMETER_OFFSET    (34 * 1024 * 1024)

This isn't covered by what you commit message states as content for this 
patch.

> +
> +struct tos_parameter_t {
> +	u32 version;
> +	u32 checksum;
> +	struct {
> +		char name[8];
> +		s64 phy_addr;
> +		u32 size;
> +		u32 flags;
> +	} tee_mem;
> +	struct {
> +		char name[8];
> +		s64 phy_addr;
> +		u32 size;
> +		u32 flags;
> +	} drm_mem;
> +	s64 reserve[8];
> +};
> +
> +int dram_init_banksize(void)
> +{
> +	size_t top = min((unsigned long)(gd->ram_size + CONFIG_SYS_SDRAM_BASE),
> +			 gd->ram_top);
> +
> +#ifdef CONFIG_ARM64
> +	/* 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;

This should only be done when preparing to start a
 	IH_OS_ARM_TRUSTED_FIRMWARE.

> +#else
> +#ifdef CONFIG_SPL_OPTEE

I don't think that this CONFIG_SPL_OPTEE was what the comments in the 
OPTEE thread have arrived at... please check again.

Just as an unreleated comment/reminder: you still need to revise the other 
series as per the comments and final decision on how to implement it.

> +	struct tos_parameter_t *tos_parameter;
> +
> +	tos_parameter = (struct tos_parameter_t *)(CONFIG_SYS_SDRAM_BASE +
> +			TRUST_PARAMETER_OFFSET);
> +
> +	if (tos_parameter->tee_mem.flags == 1) {

Please describe what this 'flags' member means and how it's encoded.

> +		gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;
> +		gd->bd->bi_dram[0].size = tos_parameter->tee_mem.phy_addr
> +					- 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;
> +	} 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;
> +	}

This should again only be done, when the appropriate IH_OS_* is entered.

> +#else
> +	gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;
> +	gd->bd->bi_dram[0].size = top - gd->bd->bi_dram[0].start;

This should be the default (so you can remove the #if paths) once you make 
sure that the other paths are actually called for entering the particular 
IH_OS_* types.

> +#endif
> +#endif
> +
> +	return 0;
> +}
> +
> size_t rockchip_sdram_size(phys_addr_t reg)
> {
> 	u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4;
> 	size_t chipsize_mb = 0;
> 	size_t size_mb = 0;
> 	u32 ch;
> -

Ok, but not really needed (unless you want to do a separate 'cosmetic' or 
'whitespace' patch.  Touching an unrelated function is usually a bad idea.

> 	u32 sys_reg = readl(reg);
> 	u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> 		       & SYS_REG_NUM_CH_MASK);
>