[PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image

[Simon Goldschmidt]

On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao <weijie.gao at mediatek.com> wrote:
>
> This patch adds support for decompressing LZMA compressed u-boot payload in
> legacy uImage format.
>
> Using this patch together with u-boot-lzma.img is useful for NOR flashes as
> they can reduce the size and load time of u-boot payload.
>
> Reviewed-by: Stefan Roese <sr at denx.de>
> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> ---
> Changes since v3: none
> ---
>  common/spl/spl_nor.c | 59 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index b1e79b9ded..7c81fb28f6 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -4,8 +4,19 @@
>   */
>
>  #include <common.h>
> +#include <cpu_func.h>
>  #include <spl.h>
>
> +#if IS_ENABLED(CONFIG_SPL_LZMA)

Is this guard really necessary? What happens without it?

> +#include <lzma/LzmaTypes.h>
> +#include <lzma/LzmaDec.h>
> +#include <lzma/LzmaTools.h>
> +#endif
> +
> +#ifndef CONFIG_SYS_BOOTM_LEN
> +#define CONFIG_SYS_BOOTM_LEN   (8 << 20)
> +#endif

This looks strange. I think we should have a central place where this is defined
to a default value. As it is now, you are adding the 3rd place where this is
defined to a "fallback" value, each with a different value.

> +
>  static ulong spl_nor_load_read(struct spl_load_info *load, ulong sector,
>                                ulong count, void *buf)
>  {
> @@ -27,6 +38,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>         int ret;
>         __maybe_unused const struct image_header *header;
>         __maybe_unused struct spl_load_info load;
> +       __maybe_unused SizeT lzma_len;
> +       struct image_header hdr;
> +       uintptr_t dataptr;
>
>         /*
>          * Loading of the payload to SDRAM is done with skipping of
> @@ -107,14 +121,49 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>                                               spl_nor_get_uboot_base());
>         }
>
> -       ret = spl_parse_image_header(spl_image,
> -                       (const struct image_header *)spl_nor_get_uboot_base());
> +       /* Payload image may not be aligned, so copy it for safety. */
> +       memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
> +       ret = spl_parse_image_header(spl_image, &hdr);
>         if (ret)
>                 return ret;
>
> -       memcpy((void *)(unsigned long)spl_image->load_addr,
> -              (void *)(spl_nor_get_uboot_base() + sizeof(struct image_header)),
> -              spl_image->size);
> +       dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);
> +
> +       switch (image_get_comp(&hdr)) {
> +       case IH_COMP_NONE:

I guess this will increase the size even when LZMA is not enabled, right?
Do you have numbers for that?

> +               memmove((void *)(unsigned long)spl_image->load_addr,
> +                       (void *)dataptr, spl_image->size);
> +               break;
> +#if IS_ENABLED(CONFIG_SPL_LZMA)
> +       case IH_COMP_LZMA:
> +               lzma_len = CONFIG_SYS_BOOTM_LEN;
> +
> +               ret = lzmaBuffToBuffDecompress((void *)spl_image->load_addr,
> +                                              &lzma_len, (void *)dataptr,
> +                                              spl_image->size);
> +
> +               if (ret) {
> +                       printf("LZMA decompression error: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               spl_image->size = lzma_len;
> +               break;
> +#endif
> +       default:
> +               debug("Compression method %s is not supported\n",
> +                     genimg_get_comp_short_name(image_get_comp(&hdr)));
> +               return -EINVAL;
> +       }
> +
> +       flush_cache((unsigned long)spl_image->load_addr, spl_image->size);

Why is this necessary? I can't see this from the commit message.

> +
> +       /*
> +        * If the image did not provide an entry point, assume the entry point
> +        * is the same as its load address.
> +        */
> +       if (!spl_image->entry_point)
> +               spl_image->entry_point = spl_image->load_addr;

And another change that is not described in the commit message.

And more general: why do we need to code this in every loader? I think it would
be preferrable to have the loader load the binary data and do the decompression
(and entry_point assignment) in a central place so that all loaders can benefit
from compression. As it is now, we are duplicating code when implementing LZMA
in the next loader.

Regards,
Simon

>
>         return 0;
>  }
> --
> 2.17.1