Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On 12.02.20 09:57, Weijie Gao wrote: 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
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Thu, Feb 13, 2020 at 8:53 AM Stefan 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 wrote: > >> > >> On 12.02.20 09:57, Weijie Gao wrote: > >> > >> > >> > 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
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
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 wrote: On 12.02.20 09:57, Weijie Gao wrote: 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
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Thu, Feb 13, 2020 at 3:53 AM Weijie Gao wrote:
>
> On Wed, 2020-02-12 at 11:18 +0100, Simon Goldschmidt wrote:
> > On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao wrote:
> > >
> > > On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> > > > On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao
> > > > 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
> > > > > Signed-off-by: Weijie Gao
> > > > > ---
> > > > > 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
> > > > > +#include
> > > > > #include
> > > > >
> > > > > +#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
> > > > > +#include
> > > > > +#include
> > > > > +#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, 0x80 = (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
plat
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
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 wrote: > > On 12.02.20 09:57, Weijie Gao wrote: > > > > >> 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
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Wed, 2020-02-12 at 11:18 +0100, Simon Goldschmidt wrote:
> On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao wrote:
> >
> > On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> > > On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao
> > > 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
> > > > Signed-off-by: Weijie Gao
> > > > ---
> > > > 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
> > > > +#include
> > > > #include
> > > >
> > > > +#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
> > > > +#include
> > > > +#include
> > > > +#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, 0x80 = (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 = l
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Wed, Feb 12, 2020 at 9:57 AM Weijie Gao wrote:
>
> On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> > On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao 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
> > > Signed-off-by: Weijie Gao
> > > ---
> > > 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
> > > +#include
> > > #include
> > >
> > > +#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
> > > +#include
> > > +#include
> > > +#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 th
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao 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
> > Signed-off-by: Weijie Gao
> > ---
> > 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
> > +#include
> > #include
> >
> > +#if IS_ENABLED(CONFIG_SPL_LZMA)
>
> Is this guard really necessary? What happens without it?
Actually nothing will happen.
>
> > +#include
> > +#include
> > +#include
> > +#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
> wo
Re: [PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image
On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao 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
> Signed-off-by: Weijie Gao
> ---
> 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
> +#include
> #include
>
> +#if IS_ENABLED(CONFIG_SPL_LZMA)
Is this guard really necessary? What happens without it?
> +#include
> +#include
> +#include
> +#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
