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

[Michal Simek]

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.

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