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

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

[Siva Durga Prasad Paladugu]

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@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@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);

--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

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

[Michal Simek]

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@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@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?

Thanks,
Michal

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

[Simon Glass]

Hi,


On 7 June 2018 at 06:18, Michal Simek <michal.simek@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@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@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 */

Regards,
Simon

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

[Michal Simek]

On 8.6.2018 23:59, Simon Glass wrote:
> Hi,
> 
> 
> On 7 June 2018 at 06:18, Michal Simek <michal.simek@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@xilinx.com>
>>> Signed-off-by: Michal Simek <michal.simek@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 */

I am aware about this but in common/ but I have incorrectly read this
code in setup_dest_addr()

DP: Can you please check this again why you have created this patch?

Thanks,
Michal

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

[Siva Durga Prasad Paladugu]

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@xilinx.com>
> Cc: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; U-Boot Mailing List
> <u-boot@lists.denx.de>; Tom Rini <trini@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@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@xilinx.com>
> >> Signed-off-by: Michal Simek <michal.simek@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,
Siva
> 
> Regards,
> Simon

[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@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@xilinx.com>
>> Cc: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; U-Boot Mailing List
>> <u-boot@lists.denx.de>; Tom Rini <trini@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@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@xilinx.com>
>> >> Signed-off-by: Michal Simek <michal.simek@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