diff mbox series

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

Message ID 1528183254-9302-1-git-send-email-siva.durga.paladugu@xilinx.com
State Superseded
Delegated to: Michal Simek
Headers show
Series [U-Boot,1/4] lib: fdtdec: Fill initial ram top with DDR start value from dt | expand

Commit Message

Siva Durga Prasad Paladugu June 5, 2018, 7:20 a.m. UTC 
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(+)

--
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.

Comments

Michal Simek June 7, 2018, 2:18 p.m. UTC | #1 
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
Simon Glass June 8, 2018, 9:59 p.m. UTC | #2 
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
Michal Simek June 11, 2018, 6:17 a.m. UTC | #3 
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
Siva Durga Prasad Paladugu June 14, 2018, 6:53 a.m. UTC | #4 
Hi Simon,

> -----Original Message-----
> From: sjg@google.com [mailto:sjg@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
Simon Glass June 14, 2018, 12:58 p.m. UTC | #5 
Hi Siva,

On 14 June 2018 at 00:53, Siva Durga Prasad Paladugu <sivadur@xilinx.com> wrote:
> Hi Simon,
>
>> -----Original Message-----
>> From: sjg@google.com [mailto:sjg@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
diff mbox series

Patch

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);