On Wed, Feb 12, 2020 at 10:46 AM Weijie Gao weijie.gao@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@denx.de Signed-off-by: Weijie Gao weijie.gao@mediatek.com

---

Changes since v3: removed unused code. add description for cache flushing

This is v5. What has changed since v4?

---

common/spl/spl_nor.c | 51 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c index b1e79b9ded..b8e133e7b6 100644 --- a/common/spl/spl_nor.c +++ b/common/spl/spl_nor.c @@ -4,8 +4,17 @@ */

#include <common.h> +#include <cpu_func.h> #include <spl.h>

+#include <lzma/LzmaTypes.h> +#include <lzma/LzmaDec.h> +#include <lzma/LzmaTools.h>

+#ifndef CONFIG_SYS_BOOTM_LEN +#define CONFIG_SYS_BOOTM_LEN (8 << 20) +#endif

static ulong spl_nor_load_read(struct spl_load_info *load, ulong sector, ulong count, void *buf) { @@ -27,6 +36,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 +119,43 @@ 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:
  

•           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);
  

Is using CONFIG_SYS_BOOTM_LEN correct here at all? The name says it is used for bootm but now it is used for SPL loading U-Boot as well. How do we know this is the available memory size for both use cases? (And no, you're not adding this, it is being used in spl_fit.c already. Still, I'm not sure this is correct.)

•           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 instruction cache */
  

Is this "add description for cache flushing" mentioned in the log above? I can see from the function name that you're flushing cache. Only I still don't see why this is necessary here (but not in the rest of the code where SPL starts a U-Boot image).

Regards, Simon

•   flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
  
    return 0;
  

}

2.17.1