Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-05-03 Thread Simon Goldschmidt

Hi Lokesh,

On 03.05.19 16:24, Lokesh Vutla wrote:

Simon,

On 22/04/19 8:15 PM, Tom Rini wrote:

On Thu, Apr 18, 2019 at 08:23:45AM +0200, Simon Goldschmidt wrote:

On Thu, Apr 18, 2019 at 8:12 AM Lokesh Vutla  wrote:




On 18/04/19 12:46 AM, Simon Goldschmidt wrote:

[This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]


Right.



Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:

SPL while copying u-boot and dtb it does the following:
- Copy u-boot
- Copy right dtb.
- mark dtb location as reserved in dtb.


Hmm, why does it do that? Reserving space when U-Boot starts Linux *might* make
sense, but why should SPL add this reservation when starting U-Boot? These steps
are for U-Boot in a fit-image, right? Because I can't see this for U-Boot as
uImage.

Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The name of
that function doesn't suggest that to me. Would it make more sense to let this
funtion only *change* the reservation, i.e. only add it if it is removed at the
beginning of said function? Would that fix your issue?

Something like this:

diff --git a/common/fdt_support.c b/common/fdt_support.c
index ab08a0114f..4e7cf6ebe9 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 uint64_t addr, size;
 int total, ret;
 uint actualsize;
+   int fdt_memrsv = 0;

 if (!blob)
 return 0;
@@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 fdt_get_mem_rsv(blob, i, , );
 if (addr == (uintptr_t)blob) {
 fdt_del_mem_rsv(blob, i);
+   fdt_memrsv = 1;
 break;
 }
 }
@@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 /* Change the fdt header to reflect the correct size */
 fdt_set_totalsize(blob, actualsize);

-   /* Add the new reservation */
-   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
-   if (ret < 0)
-   return ret;
+   if (fdt_memrsv) {
+   /* Add the new reservation */
+   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
+   if (ret < 0)
+   return ret;
+   }


Agreed. This works and more appropriate place to fix it. Will you post a
separate patch or do you want me to post it on your behalf?


While it's good to know this fixes your issue, I'm not sure this doesn't break
anything else.

Tom, Simon, can you add anything here? Why is the dtb memory added as
reserved to itself? It seems to me this is redundant, as the code using the
dtb should well know the pointer to it...

Lokesh, I can send a patch once this is discussed.


OK, so the history of that call is really what's seen in:
commit 41c5eaa7253ed82bbae1eda5667755872c615164
Author: Andy Fleming 
Date:   Mon Jun 16 13:58:56 2008 -0500

 Resize device tree to allow space for board changes and the chosen node
 
 Current code requires that a compiled device tree have space added to the end to

 leave room for extra nodes added by board code (and the chosen node).  This
 requires that device tree creators anticipate how much space U-Boot will 
add to
 the tree, which is absurd.  Ideally, the code would resize and/or relocate 
the
 tree when it needed more space, but this would require a systemic change 
to the
 fdt code, which is non-trivial.  Instead, we resize the tree inside
 boot_relocate_fdt, reserving either the remainder of the bootmap (in the 
case
 where the fdt is inside the bootmap), or adding CFG_FDT_PAD bytes to the 
size.
 
 Signed-off-by: Andy Fleming 


Which is all about preparing things for passing the device tree we
loaded specifically for Linux.  In:
commit 3082d2348c8e13342f5fdd10e9b3f7408062dbf9
Author: Kumar Gala 
Date:   Fri Aug 15 08:24:42 2008 -0500

 fdt: refactor fdt resize code
 
 Move the fdt resizing code out of ppc specific boot code and into

 common fdt support code.
 
 Signed-off-by: Kumar Gala 


The code was moved to a more common area.  So the intent on "reserve"
was to make sure that we had enough space set aside for the dtb to be
able to be updated at runtime, by U-Boot, for various needs and that we
wouldn't give that memory away to something else / allow it to be
clobbered.



Any update on this? Any chance you will be posting your changes?


No, sorry, not update. I'll send the patch I suggested above and we'll 
see how the discussion continues.


Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-05-03 Thread Lokesh Vutla
Simon,

On 22/04/19 8:15 PM, Tom Rini wrote:
> On Thu, Apr 18, 2019 at 08:23:45AM +0200, Simon Goldschmidt wrote:
>> On Thu, Apr 18, 2019 at 8:12 AM Lokesh Vutla  wrote:
>>>
>>>
>>>
>>> On 18/04/19 12:46 AM, Simon Goldschmidt wrote:
 [This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]
>>>
>>> Right.
>>>

 Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:
> SPL while copying u-boot and dtb it does the following:
> - Copy u-boot
> - Copy right dtb.
> - mark dtb location as reserved in dtb.

 Hmm, why does it do that? Reserving space when U-Boot starts Linux *might* 
 make
 sense, but why should SPL add this reservation when starting U-Boot? These 
 steps
 are for U-Boot in a fit-image, right? Because I can't see this for U-Boot 
 as
 uImage.

 Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The 
 name of
 that function doesn't suggest that to me. Would it make more sense to let 
 this
 funtion only *change* the reservation, i.e. only add it if it is removed 
 at the
 beginning of said function? Would that fix your issue?

 Something like this:

 diff --git a/common/fdt_support.c b/common/fdt_support.c
 index ab08a0114f..4e7cf6ebe9 100644
 --- a/common/fdt_support.c
 +++ b/common/fdt_support.c
 @@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 uint64_t addr, size;
 int total, ret;
 uint actualsize;
 +   int fdt_memrsv = 0;

 if (!blob)
 return 0;
 @@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 fdt_get_mem_rsv(blob, i, , );
 if (addr == (uintptr_t)blob) {
 fdt_del_mem_rsv(blob, i);
 +   fdt_memrsv = 1;
 break;
 }
 }
 @@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 /* Change the fdt header to reflect the correct size */
 fdt_set_totalsize(blob, actualsize);

 -   /* Add the new reservation */
 -   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
 -   if (ret < 0)
 -   return ret;
 +   if (fdt_memrsv) {
 +   /* Add the new reservation */
 +   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), 
 actualsize);
 +   if (ret < 0)
 +   return ret;
 +   }
>>>
>>> Agreed. This works and more appropriate place to fix it. Will you post a
>>> separate patch or do you want me to post it on your behalf?
>>
>> While it's good to know this fixes your issue, I'm not sure this doesn't 
>> break
>> anything else.
>>
>> Tom, Simon, can you add anything here? Why is the dtb memory added as
>> reserved to itself? It seems to me this is redundant, as the code using the
>> dtb should well know the pointer to it...
>>
>> Lokesh, I can send a patch once this is discussed.
> 
> OK, so the history of that call is really what's seen in:
> commit 41c5eaa7253ed82bbae1eda5667755872c615164
> Author: Andy Fleming 
> Date:   Mon Jun 16 13:58:56 2008 -0500
> 
> Resize device tree to allow space for board changes and the chosen node
> 
> Current code requires that a compiled device tree have space added to the 
> end to
> leave room for extra nodes added by board code (and the chosen node).  
> This
> requires that device tree creators anticipate how much space U-Boot will 
> add to
> the tree, which is absurd.  Ideally, the code would resize and/or 
> relocate the
> tree when it needed more space, but this would require a systemic change 
> to the
> fdt code, which is non-trivial.  Instead, we resize the tree inside
> boot_relocate_fdt, reserving either the remainder of the bootmap (in the 
> case
> where the fdt is inside the bootmap), or adding CFG_FDT_PAD bytes to the 
> size.
> 
> Signed-off-by: Andy Fleming 
> 
> Which is all about preparing things for passing the device tree we
> loaded specifically for Linux.  In:
> commit 3082d2348c8e13342f5fdd10e9b3f7408062dbf9
> Author: Kumar Gala 
> Date:   Fri Aug 15 08:24:42 2008 -0500
> 
> fdt: refactor fdt resize code
> 
> Move the fdt resizing code out of ppc specific boot code and into
> common fdt support code.
> 
> Signed-off-by: Kumar Gala 
> 
> The code was moved to a more common area.  So the intent on "reserve"
> was to make sure that we had enough space set aside for the dtb to be
> able to be updated at runtime, by U-Boot, for various needs and that we
> wouldn't give that memory away to something else / allow it to be
> clobbered.
> 

Any update on this? Any chance you will be posting your changes?

Thanks and regards,
Lokesh

___

Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-04-22 Thread Tom Rini
On Thu, Apr 18, 2019 at 08:23:45AM +0200, Simon Goldschmidt wrote:
> On Thu, Apr 18, 2019 at 8:12 AM Lokesh Vutla  wrote:
> >
> >
> >
> > On 18/04/19 12:46 AM, Simon Goldschmidt wrote:
> > > [This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ 
> > > right?]
> >
> > Right.
> >
> > >
> > > Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:
> > >> SPL while copying u-boot and dtb it does the following:
> > >> - Copy u-boot
> > >> - Copy right dtb.
> > >> - mark dtb location as reserved in dtb.
> > >
> > > Hmm, why does it do that? Reserving space when U-Boot starts Linux 
> > > *might* make
> > > sense, but why should SPL add this reservation when starting U-Boot? 
> > > These steps
> > > are for U-Boot in a fit-image, right? Because I can't see this for U-Boot 
> > > as
> > > uImage.
> > >
> > > Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The 
> > > name of
> > > that function doesn't suggest that to me. Would it make more sense to let 
> > > this
> > > funtion only *change* the reservation, i.e. only add it if it is removed 
> > > at the
> > > beginning of said function? Would that fix your issue?
> > >
> > > Something like this:
> > >
> > > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > > index ab08a0114f..4e7cf6ebe9 100644
> > > --- a/common/fdt_support.c
> > > +++ b/common/fdt_support.c
> > > @@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > > uint64_t addr, size;
> > > int total, ret;
> > > uint actualsize;
> > > +   int fdt_memrsv = 0;
> > >
> > > if (!blob)
> > > return 0;
> > > @@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > > fdt_get_mem_rsv(blob, i, , );
> > > if (addr == (uintptr_t)blob) {
> > > fdt_del_mem_rsv(blob, i);
> > > +   fdt_memrsv = 1;
> > > break;
> > > }
> > > }
> > > @@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint 
> > > extrasize)
> > > /* Change the fdt header to reflect the correct size */
> > > fdt_set_totalsize(blob, actualsize);
> > >
> > > -   /* Add the new reservation */
> > > -   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> > > -   if (ret < 0)
> > > -   return ret;
> > > +   if (fdt_memrsv) {
> > > +   /* Add the new reservation */
> > > +   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), 
> > > actualsize);
> > > +   if (ret < 0)
> > > +   return ret;
> > > +   }
> >
> > Agreed. This works and more appropriate place to fix it. Will you post a
> > separate patch or do you want me to post it on your behalf?
> 
> While it's good to know this fixes your issue, I'm not sure this doesn't break
> anything else.
> 
> Tom, Simon, can you add anything here? Why is the dtb memory added as
> reserved to itself? It seems to me this is redundant, as the code using the
> dtb should well know the pointer to it...
> 
> Lokesh, I can send a patch once this is discussed.

OK, so the history of that call is really what's seen in:
commit 41c5eaa7253ed82bbae1eda5667755872c615164
Author: Andy Fleming 
Date:   Mon Jun 16 13:58:56 2008 -0500

Resize device tree to allow space for board changes and the chosen node

Current code requires that a compiled device tree have space added to the 
end to
leave room for extra nodes added by board code (and the chosen node).  This
requires that device tree creators anticipate how much space U-Boot will 
add to
the tree, which is absurd.  Ideally, the code would resize and/or relocate 
the
tree when it needed more space, but this would require a systemic change to 
the
fdt code, which is non-trivial.  Instead, we resize the tree inside
boot_relocate_fdt, reserving either the remainder of the bootmap (in the 
case
where the fdt is inside the bootmap), or adding CFG_FDT_PAD bytes to the 
size.

Signed-off-by: Andy Fleming 

Which is all about preparing things for passing the device tree we
loaded specifically for Linux.  In:
commit 3082d2348c8e13342f5fdd10e9b3f7408062dbf9
Author: Kumar Gala 
Date:   Fri Aug 15 08:24:42 2008 -0500

fdt: refactor fdt resize code

Move the fdt resizing code out of ppc specific boot code and into
common fdt support code.

Signed-off-by: Kumar Gala 

The code was moved to a more common area.  So the intent on "reserve"
was to make sure that we had enough space set aside for the dtb to be
able to be updated at runtime, by U-Boot, for various needs and that we
wouldn't give that memory away to something else / allow it to be
clobbered.

-- 
Tom


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-04-18 Thread Simon Goldschmidt
On Thu, Apr 18, 2019 at 8:12 AM Lokesh Vutla  wrote:
>
>
>
> On 18/04/19 12:46 AM, Simon Goldschmidt wrote:
> > [This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]
>
> Right.
>
> >
> > Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:
> >> SPL while copying u-boot and dtb it does the following:
> >> - Copy u-boot
> >> - Copy right dtb.
> >> - mark dtb location as reserved in dtb.
> >
> > Hmm, why does it do that? Reserving space when U-Boot starts Linux *might* 
> > make
> > sense, but why should SPL add this reservation when starting U-Boot? These 
> > steps
> > are for U-Boot in a fit-image, right? Because I can't see this for U-Boot as
> > uImage.
> >
> > Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The name 
> > of
> > that function doesn't suggest that to me. Would it make more sense to let 
> > this
> > funtion only *change* the reservation, i.e. only add it if it is removed at 
> > the
> > beginning of said function? Would that fix your issue?
> >
> > Something like this:
> >
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index ab08a0114f..4e7cf6ebe9 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > uint64_t addr, size;
> > int total, ret;
> > uint actualsize;
> > +   int fdt_memrsv = 0;
> >
> > if (!blob)
> > return 0;
> > @@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > fdt_get_mem_rsv(blob, i, , );
> > if (addr == (uintptr_t)blob) {
> > fdt_del_mem_rsv(blob, i);
> > +   fdt_memrsv = 1;
> > break;
> > }
> > }
> > @@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > /* Change the fdt header to reflect the correct size */
> > fdt_set_totalsize(blob, actualsize);
> >
> > -   /* Add the new reservation */
> > -   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> > -   if (ret < 0)
> > -   return ret;
> > +   if (fdt_memrsv) {
> > +   /* Add the new reservation */
> > +   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), 
> > actualsize);
> > +   if (ret < 0)
> > +   return ret;
> > +   }
>
> Agreed. This works and more appropriate place to fix it. Will you post a
> separate patch or do you want me to post it on your behalf?

While it's good to know this fixes your issue, I'm not sure this doesn't break
anything else.

Tom, Simon, can you add anything here? Why is the dtb memory added as
reserved to itself? It seems to me this is redundant, as the code using the
dtb should well know the pointer to it...

Lokesh, I can send a patch once this is discussed.

Regards,
Simon

>
> Thanks and regards,
> Lokesh
>
> >
> > return actualsize;
> >  }
> >
> >
> >>
> >> U-Boot when copying images at U-Boot prompt, is not able to copy to the
> >> above dtb location as it sees the region as reserved. But at this stage
> >> dtb is re located to end of DDR. And the above dtb region is not reserved
> >> anymore. So delete this reserved region when re locating dtb.
> >>
> >> Reported-by: Keerthy 
> >> Signed-off-by: Lokesh Vutla 
> >> ---
> >>   common/board_f.c | 13 +
> >>   1 file changed, 13 insertions(+)
> >>
> >> diff --git a/common/board_f.c b/common/board_f.c
> >> index 149a7229e8..e4383ae3fa 100644
> >> --- a/common/board_f.c
> >> +++ b/common/board_f.c
> >> @@ -651,10 +651,23 @@ static int init_post(void)
> >>   static int reloc_fdt(void)
> >>   {
> >>   #ifndef CONFIG_OF_EMBED
> >> +uint64_t addr, size;
> >> +int i, cnt, err;
> >> +
> >>   if (gd->flags & GD_FLG_SKIP_RELOC)
> >>   return 0;
> >>   if (gd->new_fdt) {
> >>   memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> >> +
> >> +/* Deleting the previously marked FDT reserved region */
> >> +cnt = fdt_num_mem_rsv(gd->new_fdt);
> >> +for (i = 0; i < cnt ; i++) {
> >> +err = fdt_get_mem_rsv(gd->new_fdt, i, , );
> >> +if (!err && addr == (uintptr_t)gd->fdt_blob) {
> >> +fdt_del_mem_rsv(gd->new_fdt, i);
> >> +break;
> >> +}
> >> +}
> >
> > That code should work, but it's a bit unexpected in this location
> >
> >>   gd->fdt_blob = gd->new_fdt;
> >>   }
> >>   #endif
> >>
> >
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-04-18 Thread Lokesh Vutla


On 18/04/19 12:46 AM, Simon Goldschmidt wrote:
> [This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]

Right.

> 
> Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:
>> SPL while copying u-boot and dtb it does the following:
>> - Copy u-boot
>> - Copy right dtb.
>> - mark dtb location as reserved in dtb.
> 
> Hmm, why does it do that? Reserving space when U-Boot starts Linux *might* 
> make
> sense, but why should SPL add this reservation when starting U-Boot? These 
> steps
> are for U-Boot in a fit-image, right? Because I can't see this for U-Boot as
> uImage.
> 
> Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The name of
> that function doesn't suggest that to me. Would it make more sense to let this
> funtion only *change* the reservation, i.e. only add it if it is removed at 
> the
> beginning of said function? Would that fix your issue?
> 
> Something like this:
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index ab08a0114f..4e7cf6ebe9 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>     uint64_t addr, size;
>     int total, ret;
>     uint actualsize;
> +   int fdt_memrsv = 0;
> 
>     if (!blob)
>     return 0;
> @@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>     fdt_get_mem_rsv(blob, i, , );
>     if (addr == (uintptr_t)blob) {
>     fdt_del_mem_rsv(blob, i);
> +   fdt_memrsv = 1;
>     break;
>     }
>     }
> @@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>     /* Change the fdt header to reflect the correct size */
>     fdt_set_totalsize(blob, actualsize);
> 
> -   /* Add the new reservation */
> -   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> -   if (ret < 0)
> -   return ret;
> +   if (fdt_memrsv) {
> +   /* Add the new reservation */
> +   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> +   if (ret < 0)
> +   return ret;
> +   }

Agreed. This works and more appropriate place to fix it. Will you post a
separate patch or do you want me to post it on your behalf?

Thanks and regards,
Lokesh

> 
>     return actualsize;
>  }
> 
> 
>>
>> U-Boot when copying images at U-Boot prompt, is not able to copy to the
>> above dtb location as it sees the region as reserved. But at this stage
>> dtb is re located to end of DDR. And the above dtb region is not reserved
>> anymore. So delete this reserved region when re locating dtb.
>>
>> Reported-by: Keerthy 
>> Signed-off-by: Lokesh Vutla 
>> ---
>>   common/board_f.c | 13 +
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index 149a7229e8..e4383ae3fa 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -651,10 +651,23 @@ static int init_post(void)
>>   static int reloc_fdt(void)
>>   {
>>   #ifndef CONFIG_OF_EMBED
>> +    uint64_t addr, size;
>> +    int i, cnt, err;
>> +
>>   if (gd->flags & GD_FLG_SKIP_RELOC)
>>   return 0;
>>   if (gd->new_fdt) {
>>   memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>> +
>> +    /* Deleting the previously marked FDT reserved region */
>> +    cnt = fdt_num_mem_rsv(gd->new_fdt);
>> +    for (i = 0; i < cnt ; i++) {
>> +    err = fdt_get_mem_rsv(gd->new_fdt, i, , );
>> +    if (!err && addr == (uintptr_t)gd->fdt_blob) {
>> +    fdt_del_mem_rsv(gd->new_fdt, i);
>> +    break;
>> +    }
>> +    }
> 
> That code should work, but it's a bit unexpected in this location
> 
>>   gd->fdt_blob = gd->new_fdt;
>>   }
>>   #endif
>>
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

2019-04-17 Thread Simon Goldschmidt

[This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]

Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:

SPL while copying u-boot and dtb it does the following:
- Copy u-boot
- Copy right dtb.
- mark dtb location as reserved in dtb.


Hmm, why does it do that? Reserving space when U-Boot starts Linux 
*might* make sense, but why should SPL add this reservation when 
starting U-Boot? These steps are for U-Boot in a fit-image, right? 
Because I can't see this for U-Boot as uImage.


Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The 
name of that function doesn't suggest that to me. Would it make more 
sense to let this funtion only *change* the reservation, i.e. only add 
it if it is removed at the beginning of said function? Would that fix 
your issue?


Something like this:

diff --git a/common/fdt_support.c b/common/fdt_support.c
index ab08a0114f..4e7cf6ebe9 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
uint64_t addr, size;
int total, ret;
uint actualsize;
+   int fdt_memrsv = 0;

if (!blob)
return 0;
@@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
fdt_get_mem_rsv(blob, i, , );
if (addr == (uintptr_t)blob) {
fdt_del_mem_rsv(blob, i);
+   fdt_memrsv = 1;
break;
}
}
@@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
/* Change the fdt header to reflect the correct size */
fdt_set_totalsize(blob, actualsize);

-   /* Add the new reservation */
-   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
-   if (ret < 0)
-   return ret;
+   if (fdt_memrsv) {
+   /* Add the new reservation */
+   ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), 
actualsize);

+   if (ret < 0)
+   return ret;
+   }

return actualsize;
 }




U-Boot when copying images at U-Boot prompt, is not able to copy to the
above dtb location as it sees the region as reserved. But at this stage
dtb is re located to end of DDR. And the above dtb region is not reserved
anymore. So delete this reserved region when re locating dtb.

Reported-by: Keerthy 
Signed-off-by: Lokesh Vutla 
---
  common/board_f.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c
index 149a7229e8..e4383ae3fa 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -651,10 +651,23 @@ static int init_post(void)
  static int reloc_fdt(void)
  {
  #ifndef CONFIG_OF_EMBED
+   uint64_t addr, size;
+   int i, cnt, err;
+
if (gd->flags & GD_FLG_SKIP_RELOC)
return 0;
if (gd->new_fdt) {
memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
+
+   /* Deleting the previously marked FDT reserved region */
+   cnt = fdt_num_mem_rsv(gd->new_fdt);
+   for (i = 0; i < cnt ; i++) {
+   err = fdt_get_mem_rsv(gd->new_fdt, i, , );
+   if (!err && addr == (uintptr_t)gd->fdt_blob) {
+   fdt_del_mem_rsv(gd->new_fdt, i);
+   break;
+   }
+   }


That code should work, but it's a bit unexpected in this location


gd->fdt_blob = gd->new_fdt;
}
  #endif



___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot