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

[Weijie Gao]

On Wed, 2020-02-12 at 11:18 +0100, Simon Goldschmidt wrote:
> On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao <weijie.gao at mediatek.com> wrote:
> >
> > On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> > > 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?
> >
> > Actually nothing will happen.
> 
> So can you drop it?

Already dropped in v5.

> 
> >
> > >
> > > > +#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.
> >
> > This is copied from common/bootm.c. It does exist in two files:
> > common/bootm.c and common/spl/spl_fit.c.
> 
> Exactly. It is copied. Yet another duplication, which is bad.
> And it is not even copied 1:1, as those two files define a different default
> value and you define yet another different default value.

Actually the same value. from common/bootm.c, 0x800000 = (8 << 20).

> 
> >
> > >
> > > > +
> > > >  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?
> >
> > Yes. about 8KiB on mips32r2 platform.
> 
> 8KiB more in SPL even without LZMA enabled? That sounds a bit too high?

no. 8kib more only when CONFIG_SPL_LZMA is enabled.

> 
> >
> > >
> > > > +               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.
> >
> > This is used for flushing the instruction cache. Without this the
> > payload may not be able to boot. For this patch series, this will happen
> > if the payload has a small size.
> 
> But how does this depend on adding LZMA? And why is this specific to
> loading from spi nor? And if it's not, can we add this in a central place,
> not here?

This has no relation with LZMA. It's more likely to be a issue observed
on mips platforms. Small data written into cached memory will not cause
the instruction cache to be flushed automatically. So an explicit cache
flushing is needed.

flush_cache is also used by bootm, in bootm_load_os. Perhaps we can put
this into common/spl.c, in jump_to_image_no_args.

> 
> >
> > >
> > > > +
> > > > +       /*
> > > > +        * 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.
> >
> > I've checked this is no longer needed. This will be removed.
> >
> > >
> > > 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.
> >
> > This feature is originally designed for the case that u-boot is stored
> > in a small capacity storage device, mostly NOR flashes, and the space
> > reserved for u-boot is very small. Most loaders (MMC, NAND, SATA, ...)
> > do not need this at all.
> 
> What about ymodem? I don't think we should be too smart here. And for booting
> U-Boot from FPGA, I could use LZMA compression for RAM, too.

I think it's better to implement this in a future patch series. I
believe there will be lots of work to do.

> 
> Regards,
> Simon
> 
> >
> > >
> > > Regards,
> > > Simon
> > >
> > > >
> > > >         return 0;
> > > >  }
> > > > --
> > > > 2.17.1
> >