| Message ID | 1581493743-8992-1-git-send-email-weijie.gao@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor the architecture parts of mt7628 | expand |
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? > +#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
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. > > > +#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. > > > + > > 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. > > > + 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. > > > + > > + /* > > + * 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. > > Regards, > Simon > > > > > return 0; > > } > > -- > > 2.17.1
On 12.02.20 09:57, Weijie Gao wrote: <snip> >> 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. I agree with Simon, that it would make sense to move this code into a even more generic location, so that other "loaders" can also use this feature. I know, that I suggested to add it here and I think we can make this move into the common SPL interface at a later point, after this patch set has been integrated. > 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. Yes and no. As you pointed out, it might be faster to load and decompress a smaller U-Boot proper image than just loading a bigger image. So other platforms might very well take advantage of this feature. And size increase is always a big issue in modern U-Boot. So a smaller image is always welcome. ;) Thanks, Stefan
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? > > > > > > +#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 >
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 > >
Sorry if it seems I ignored this mail yesterday, I just found it now :) On Wed, Feb 12, 2020 at 10:05 AM Stefan Roese <mail at roese.nl> wrote: > > On 12.02.20 09:57, Weijie Gao wrote: > > <snip> > > >> 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. > > I agree with Simon, that it would make sense to move this code into a > even more generic location, so that other "loaders" can also use this > feature. I know, that I suggested to add it here and I think we can > make this move into the common SPL interface at a later point, after > this patch set has been integrated. My concern is that we add poor code now and that cleanup at a "later point" just gets forgotten. To me, this patch looks like another "get the stuff I need in fast" thing, without thinking about overall code quality. Yes it might be more work to do it properly, but in my opinion, the result will be code that is easier to maintain in the long run. Regards, Simon > > > 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. > > Yes and no. As you pointed out, it might be faster to load and > decompress a smaller U-Boot proper image than just loading a bigger > image. So other platforms might very well take advantage of this > feature. And size increase is always a big issue in modern U-Boot. So a > smaller image is always welcome. ;) > > Thanks, > Stefan
On Thu, Feb 13, 2020 at 3:53 AM Weijie Gao <weijie.gao at 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 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. 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 > > > >
Hi Simon, On 13.02.20 08:40, Simon Goldschmidt wrote: > Sorry if it seems I ignored this mail yesterday, I just found it now :) > > On Wed, Feb 12, 2020 at 10:05 AM Stefan Roese <mail at roese.nl> wrote: >> >> On 12.02.20 09:57, Weijie Gao wrote: >> >> <snip> >> >>>> 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. >> >> I agree with Simon, that it would make sense to move this code into a >> even more generic location, so that other "loaders" can also use this >> feature. I know, that I suggested to add it here and I think we can >> make this move into the common SPL interface at a later point, after >> this patch set has been integrated. > > My concern is that we add poor code now and that cleanup at a "later point" > just gets forgotten. I understand. > To me, this patch looks like another "get the stuff I need in fast" thing, > without thinking about overall code quality. Yes it might be more work to > do it properly, but in my opinion, the result will be code that is easier to > maintain in the long run. I fully agree. But I already pushed Weijie to make many enhancements to the code and I fear that this work gets just too complex (time consuming) right now. As this type of generalization is not restricted on this new lzma implementation, but will very likely touch other features as well. So again, I'm still okay with adding this feature for spi_nor only right now. But if Weijie volunteers to move it to a even more generic location, that would be very welcome of course. ;) Thanks, Stefan
On Thu, Feb 13, 2020 at 8:53 AM Stefan <sr at denx.de> wrote: > > Hi Simon, > > On 13.02.20 08:40, Simon Goldschmidt wrote: > > Sorry if it seems I ignored this mail yesterday, I just found it now :) > > > > On Wed, Feb 12, 2020 at 10:05 AM Stefan Roese <mail at roese.nl> wrote: > >> > >> On 12.02.20 09:57, Weijie Gao wrote: > >> > >> <snip> > >> > >>>> 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. > >> > >> I agree with Simon, that it would make sense to move this code into a > >> even more generic location, so that other "loaders" can also use this > >> feature. I know, that I suggested to add it here and I think we can > >> make this move into the common SPL interface at a later point, after > >> this patch set has been integrated. > > > > My concern is that we add poor code now and that cleanup at a "later point" > > just gets forgotten. > > I understand. > > > To me, this patch looks like another "get the stuff I need in fast" thing, > > without thinking about overall code quality. Yes it might be more work to > > do it properly, but in my opinion, the result will be code that is easier to > > maintain in the long run. > > I fully agree. But I already pushed Weijie to make many enhancements to > the code and I fear that this work gets just too complex (time > consuming) right now. As this type of generalization is not restricted > on this new lzma implementation, but will very likely touch other > features as well. > > So again, I'm still okay with adding this feature for spi_nor only right > now. But if Weijie volunteers to move it to a even more generic > location, that would be very welcome of course. ;) Ok, I'm not the one in charge to decide if it's ok to merge this code. Regards, Simon > > Thanks, > Stefan
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) +#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 + 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: + 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); + + /* + * 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; return 0; }