On Wed, Feb 12, 2020 at 8:49 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: 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