[U-Boot] [PATCH PATCH v4 04/15] spl: fit: allocate a temporary buffer to load the overlays

[Jean-Jacques Hiblot]

Hi Simon,

On 13/08/2019 11:33, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Mon, 5 Aug 2019 at 03:44, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
>> If the node describing an overlay does not specify a load address, it will
>> be loaded at the address previously used.
>> Fixing it by allocating a temporary 64kB region that will be used as a
>> default load address.
> How did you come to decide on 64KB? Might this be too large or too small?

It seemed big enough to me to accommodate a DTB overlay.

There is no easy way to know how big the buffer must be without reading 
it first because it can be compressed.

>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>>
>> ---
>>
>> Changes in v4:
>> - make sure that the temp buffer is freed in all cases
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   common/spl/spl_fit.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index ab47da5094..977074cd99 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -8,8 +8,9 @@
>>   #include <errno.h>
>>   #include <fpga.h>
>>   #include <image.h>
>> -#include <linux/libfdt.h>
>> +#include <malloc.h>
>>   #include <spl.h>
>> +#include <linux/libfdt.h>
>>
>>   #ifndef CONFIG_SYS_BOOTM_LEN
>>   #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
>> @@ -302,33 +303,50 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>>          spl_image->fdt_addr = (void *)image_info.load_addr;
>>   #if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
>>          if (CONFIG_IS_ENABLED(LOAD_FIT_APPLY_OVERLAY)) {
>> +               void *tmpbuffer;
>> +               /*
>> +                * allocate 64KB of memory. This will be used to store the DT
>> +                * overlay before it is applied. It may not be used depending on
>> +                * how the overlay is stored, so don't fail yet if the
>> +                * allocation failed.
>> +                */
>> +               tmpbuffer = malloc(64 * 1024);
>> +               if (!tmpbuffer)
>> +                       debug("%s: unable to allocate space for overlays\n",
>> +                             __func__);
> Can you adjust this to only allocate buf when you find it is needed?
could be done.
>
>> +
>>                  for (; ; index++) {
>>                          node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP,
>>                                                        index);
>>                          if (node < 0) {
>>                                  debug("%s: No additional FDT node\n", __func__);
>> -                               return 0;
>> +                               break;
>>                          }
>>
>> +                       image_info.load_addr = (ulong)tmpbuffer;
> map_to_sysmem() or this won't work on sandbox.
ok
>
>>                          ret = spl_load_fit_image(info, sector, fit, base_offset,
>>                                                   node, &image_info);
>>                          if (ret < 0)
>> -                               return ret;
>> +                               break;
>>
>>                          /* Make room in FDT for changes from the overlay */
>>                          ret = fdt_increase_size(spl_image->fdt_addr,
>>                                                  image_info.size);
>>                          if (ret < 0)
>> -                               return ret;
>> +                               break;
>>
>>                          ret = fdt_overlay_apply_verbose(spl_image->fdt_addr,
>>                                                          (void *)image_info.load_addr);
> map_sysmem()
>
>>                          if (ret)
>> -                               return ret;
>> +                               break;
>>
>>                          debug("%s: DT overlay %s applied\n", __func__,
>>                                fit_get_name(fit, node, NULL));
>>                  }
>> +               if (tmpbuffer)
>> +                       free(tmpbuffer);
>> +               if (ret)
>> +                       return ret;
>>          }
>>          /* Try to make space, so we can inject details on the loadables */
>>          ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192);
>> --
>> 2.17.1
>>
> Regads,
> Simon
>