On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao <weijie....@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 <s...@denx.de> > Signed-off-by: Weijie Gao <weijie....@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