On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao weijie.gao@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@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?
Actually nothing will happen.
So can you drop 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.
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.
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?
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?
/** 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.
Regards, Simon
Regards, Simon
return 0;}
2.17.1