[U-Boot] [RFC PATCH v1 7/9] spl: fit: Add support for applying DT overlay

[Jean-Jacques Hiblot]

Hi Michal,

On 25/03/2019 09:18, Michal Simek wrote:
> On 22. 03. 19 15:39, Jean-Jacques Hiblot wrote:
>> From: Michal Simek <michal.simek at xilinx.com>
>>
>> doc/uImage.FIT/overlay-fdt-boot.txt is describing how to create FIT
>> image with DT overlays in it.
>> Add support for this feature to SPL.
>>
>> The whole feature depends on OF_LIBFDT_OVERLAY
>>
>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> First of all will be better if you based this on the top of my patch and
> not change the origin one. I have it in my queue to send it when merge
> window is open.

Thanks for the review.

Do you have a branch with this work publicly available ? I could rebase 
this series on top of it.

JJ

>> ---
>>
>>   common/spl/spl_fit.c | 60 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 90bf458ee8..ebce93ec1f 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -8,6 +8,7 @@
>>   #include <errno.h>
>>   #include <fpga.h>
>>   #include <image.h>
>> +#include <malloc.h>
>>   #include <linux/libfdt.h>
>>   #include <spl.h>
>>   
>> @@ -289,6 +290,48 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>>   	return 0;
>>   }
>>   
>> +#if defined(CONFIG_OF_LIBFDT_OVERLAY)
>> +static int load_and_apply_overlay(void *fdt_addr, struct spl_load_info *info,
>> +				  ulong sector, void *fit, ulong base_offset,
>> +				  int node)
>> +{
>> +	int ret;
>> +	struct spl_image_info img_info;
>> +
>> +	/* get the size of the image */
>> +	ret = spl_load_fit_image(info, sector, fit, base_offset, node,
>> +				 &img_info, true);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* allocate space to load the image */
>> +	img_info.load_addr = (ulong)malloc(img_info.size);
>> +	if (!img_info.load_addr)
>> +		return -ENOMEM;
>> +
> I am not quite sure about this. You are allocating memory for overlay
> and when load address for overlay is specified this place will be
> completely unused. And then you are freeing it later.
>
> And allocating size is just for image inside FIT not uncompressed one.
>
> I understand that you don't want to put load address to FIT not to deal
> with memory layouts which is not bad idea. But right now you should be
> re data out of malloc area.
>
> I am really not quite sure if this is the right way how to handle images
> without load address specified. I didn't try to solve this in my patch
> but we should really solve it to handle all these cases.
>
>
>> +	/* load the image */
>> +	ret = spl_load_fit_image(info, sector, fit, base_offset, node,
>> +				 &img_info, false);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Increase the size of the fdt before applying the dtbo */
> Good to be align with comment style and start with "increase"
>
>> +	fdt_shrink_to_minimum(fdt_addr, img_info.size);
>> +
>> +	/* apply the DTB overlay */
>> +	ret = fdt_overlay_apply_verbose(fdt_addr, (void *)img_info.load_addr);
>> +	free((void *)img_info.load_addr);
>> +	if (ret) {
>> +		pr_err("Could not apply overlay %s\n",
>> +		       fit_get_name(fit, node, NULL));
>> +		return ret;
>> +	}
>> +	debug("DT overlay %s applied\n", fit_get_name(fit, node, NULL));
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>>   static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>>   			      struct spl_load_info *info, ulong sector,
>>   			      void *fit, int images, ulong base_offset)
>> @@ -316,11 +359,26 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>>   
>>   	/* Make the load-address of the FDT available for the SPL framework */
>>   	spl_image->fdt_addr = (void *)image_info.load_addr;
>> +#if defined(CONFIG_OF_LIBFDT_OVERLAY)
>> +	int index;
>> +
>> +	/* Apply overlays located in the "fdt" property (after the DTB) */
>> +	for (index = 1; ; index++) {
>> +		node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index);
>> +		if (node < 0) {
>> +			debug("%s: No additional FDT node\n", __func__);
>> +			break;
>> +		}
>> +		ret = load_and_apply_overlay(spl_image->fdt_addr, info, sector,
>> +					     fit, base_offset, node);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +#endif
>>   #if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
>>   	/* Try to make space, so we can inject details on the loadables */
>>   	ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192);
>>   #endif
>> -
> unrelated.
>
>>   	return ret;
>>   }
>>   
>>
> M
>