[U-Boot] [PATCH 1/4] lib: fdtdec: Fill initial ram top with DDR start value from dt

[Simon Glass]

Hi Siva,

On 14 June 2018 at 00:53, Siva Durga Prasad Paladugu <sivadur at xilinx.com> wrote:
> Hi Simon,
>
>> -----Original Message-----
>> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
>> Sent: Saturday, June 09, 2018 3:29 AM
>> To: Michal Simek <michal.simek at xilinx.com>
>> Cc: Siva Durga Prasad Paladugu <sivadur at xilinx.com>; U-Boot Mailing List
>> <u-boot at lists.denx.de>; Tom Rini <trini at konsulko.com>
>> Subject: Re: [PATCH 1/4] lib: fdtdec: Fill initial ram top with DDR start value
>> from dt
>>
>> Hi,
>>
>>
>> On 7 June 2018 at 06:18, Michal Simek <michal.simek at xilinx.com> wrote:
>> > Hi,
>> >
>> > On 5.6.2018 09:20, Siva Durga Prasad Paladugu wrote:
>> >> Fill initial ram top with DDR base addr value from DT as not filling
>> >> it here always assumes it as zero while calculating relocation offset
>> >> and hence lead to failures in somecases. This will assumed as zero if
>> >> CONFIG_SYS_SDRAM_BASE is not defined.
>> >>
>> >> Signed-off-by: Siva Durga Prasad Paladugu
>> >> <siva.durga.paladugu at xilinx.com>
>> >> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>> >> ---
>> >>  lib/fdtdec.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c index f4e8dbf..34ef9b8
>> >> 100644
>> >> --- a/lib/fdtdec.c
>> >> +++ b/lib/fdtdec.c
>> >> @@ -1172,6 +1172,7 @@ int fdtdec_setup_memory_size(void)
>> >>       }
>> >>
>> >>       gd->ram_size = (phys_size_t)(res.end - res.start + 1);
>> >> +     gd->ram_top = (unsigned long)res.start;
>> >>       debug("%s: Initial DRAM size %llx\n", __func__,
>> >>             (unsigned long long)gd->ram_size);
>> >>
>> >>
>> >
>> > I am curious about ram_top meaning. It is used more as ram_base.
>> >
>> > I expect we can workaround it by board_get_usable_ram_top() where we
>> > decode it exactly the same as patched fdtdec_setup_memory_size() but I
>> > don't think it is better solution than this one.
>> >
>> > Simon/Tom: any comment?
>>
>> I wonder why it is not set to res.end in this patch?
>>
>> Comments from global_data.h:
>>
>> unsigned long ram_top; /* Top address of RAM used by U-Boot */
>> phys_size_t ram_size; /* RAM size */
>
> Yes, it holds the ram high address but, initially it should contain start address then in routine setup_dest_addr() (file common_board_f.c), it will be updated by getting the
> total mem size as below.
> gd->ram_top += get_effective_memsize();
>
> Lets consider if start address is non zero then it results in wrong ram_top without this patch. So, this patch fixes it by initializing it to ram start address and later in setup_dest_addr(), it will be updated to actual ram high address.
> Otherway of fixing it is, we should add new variable to gd_t to hold ram_start and then while calculating ram_top in setup_dest_addr(), we should use it as gd->ram_top = gd->ram_start + get_effective_memsize() as
> gd->bd->bi_dram[0].start didn’t get filled by this time during init.

Thanks for the explanation. I think adding that new variable would be
better and less confusing. But then fdtdec_setup_memory_size() needs
to be renamed.

Also I think it is confusing to set gd->ram_size to
CONFIG_SYS_SDRAM_BASE and then increase it, so you should be able to
change that to ram_start also.

Regards,
Simon