On Thu, Feb 13, 2020 at 3:53 AM Weijie Gao weijie.gao@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.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?
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