On Thu, Feb 13, 2020 at 3:53 AM Weijie Gao <weijie....@mediatek.com> wrote: > > On Wed, 2020-02-12 at 11:18 +0100, Simon Goldschmidt wrote: > > On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao <weijie....@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....@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? > > > > > > Actually nothing will happen. > > > > So can you drop it? > > Already dropped in v5.
Which v5? Oh, I see you sent a v5 just about 2 hours after v4? That's way too fast to have a discussion about a version. > > > > > > > > > > > > > > > +#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). Same value, different code. Still ugly and hard to maintain to have this code copied (*and* not copied literally, even if the resulting value is the same). > > > > > > > > > > > > > > > + > > > > > 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. Ok, what I meant is that even when CONFIG_SPL_LZMA is *disabled*, adding the switch and default case as well as adding the call to flush_cache might increase the SPL binary. For no benefit, that might be bad for some size constrained platforms which just happen to fit before that patch? > > > > > > > > > > > > > > > + 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. I do think this belongs into a different file. But even if you keep this here, I think you'd need to mention it in the commit message. Better still would be to move it into a separate patch, since this has *nothing* to do with adding LZMA support. > > > > > > > > > > > > > > > + > > > > > + /* > > > > > + * 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. To me, "that is more work" is not a valid argument :-) Regards, Simon > > > > > Regards, > > Simon > > > > > > > > > > > > > Regards, > > > > Simon > > > > > > > > > > > > > > return 0; > > > > > } > > > > > -- > > > > > 2.17.1 > > > >