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

[Kever Yang]

On 04/02/2018 05:50 AM, Philipp Tomsich wrote:
>
>
> 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.

Will add it.
>
>> +
>> +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.

Yes, you are right, but ATF in ARM64 is not optional, it's mandatory, so
I don't think we need
a #if/#else here.
>
>> +#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.
>

Will update and use new MACRO, I leave it there because you have such a
sloooow response and then a looong time discussion.

Thanks,
- Kever
> 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);
>>
>