| Message ID | 20200410110431.12256-2-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/3] spl: spl_nor: Move legacy image loading into spl_legacy.c | expand |
Am 10.04.20 um 13:04 schrieb Stefan Roese: > From: Weijie Gao <weijie.gao at mediatek.com> > > This patch adds support for decompressing LZMA compressed u-boot payload > in legacy uImage format. > > Using this patch together with u-boot-lzma.img may be useful for some > platforms as they can reduce the size and load time of u-boot payload. > > Signed-off-by: Weijie Gao <weijie.gao at mediatek.com> > Signed-off-by: Stefan Roese <sr at denx.de> > Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> > --- > common/spl/spl_legacy.c | 50 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 48 insertions(+), 2 deletions(-) Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> nits below > > diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c > index 7f00fc8885..41734c026f 100644 > --- a/common/spl/spl_legacy.c > +++ b/common/spl/spl_legacy.c > @@ -4,8 +4,15 @@ > */ > > #include <common.h> > +#include <malloc.h> > #include <spl.h> > > +#include <lzma/LzmaTypes.h> > +#include <lzma/LzmaDec.h> > +#include <lzma/LzmaTools.h> > + > +#define LZMA_LEN (1 << 20) > + > int spl_parse_legacy_header(struct spl_image_info *spl_image, > const struct image_header *header) > { > @@ -55,7 +62,10 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image, > int spl_load_legacy_img(struct spl_image_info *spl_image, > struct spl_load_info *load, ulong header) > { > + __maybe_unused SizeT lzma_len; > + __maybe_unused void *src; > struct image_header hdr; > + ulong dataptr; > int ret; > > /* Read header into local struct */ > @@ -65,9 +75,45 @@ int spl_load_legacy_img(struct spl_image_info *spl_image, > if (ret) > return ret; > > + dataptr = header + sizeof(hdr); > + > /* Read image */ > - load->read(load, header + sizeof(hdr), spl_image->size, > - (void *)(unsigned long)spl_image->load_addr); > + switch (image_get_comp(&hdr)) { > + case IH_COMP_NONE: > + load->read(load, dataptr, spl_image->size, > + (void *)(unsigned long)spl_image->load_addr); > + break; to avoid the little increase of binary footprint due to the extra check maybe a little wrapper like this could help: static inline int spl_image_get_comp(const struct image_header *hdr) { if (IS_ENABLED(CONFIG_SPL_LZMA) /* || IS_ENABLED(CONFIG_SPL_ANOTHER_FANCY_COMPRESSION) */) return image_get_comp(hdr); return IH_COMP_NONE; } switch (spl_image_get_comp(&hdr)) { ... } then the compiler should optimise the switch/case statement away due to Dead Code Elimination. > + > +#if IS_ENABLED(CONFIG_SPL_LZMA) > + case IH_COMP_LZMA: > + lzma_len = LZMA_LEN; > + > + debug("LZMA: Decompressing %08lx to %08lx\n", > + dataptr, spl_image->load_addr); > + src = malloc(spl_image->size); > + if (!src) { > + printf("Unable to allocate %d bytes for LZMA\n", > + spl_image->size); > + return -ENOMEM; > + } > + > + load->read(load, dataptr, spl_image->size, src); > + ret = lzmaBuffToBuffDecompress((void *)spl_image->load_addr, > + &lzma_len, src, 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; > + } > > return 0; > } >
On 15.04.20 14:52, Daniel Schwierzeck wrote: > > > Am 10.04.20 um 13:04 schrieb Stefan Roese: >> From: Weijie Gao <weijie.gao at mediatek.com> >> >> This patch adds support for decompressing LZMA compressed u-boot payload >> in legacy uImage format. >> >> Using this patch together with u-boot-lzma.img may be useful for some >> platforms as they can reduce the size and load time of u-boot payload. >> >> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com> >> Signed-off-by: Stefan Roese <sr at denx.de> >> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> >> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> >> --- >> common/spl/spl_legacy.c | 50 +++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 48 insertions(+), 2 deletions(-) > > Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> > > nits below > >> >> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c >> index 7f00fc8885..41734c026f 100644 >> --- a/common/spl/spl_legacy.c >> +++ b/common/spl/spl_legacy.c >> @@ -4,8 +4,15 @@ >> */ >> >> #include <common.h> >> +#include <malloc.h> >> #include <spl.h> >> >> +#include <lzma/LzmaTypes.h> >> +#include <lzma/LzmaDec.h> >> +#include <lzma/LzmaTools.h> >> + >> +#define LZMA_LEN (1 << 20) >> + >> int spl_parse_legacy_header(struct spl_image_info *spl_image, >> const struct image_header *header) >> { >> @@ -55,7 +62,10 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image, >> int spl_load_legacy_img(struct spl_image_info *spl_image, >> struct spl_load_info *load, ulong header) >> { >> + __maybe_unused SizeT lzma_len; >> + __maybe_unused void *src; >> struct image_header hdr; >> + ulong dataptr; >> int ret; >> >> /* Read header into local struct */ >> @@ -65,9 +75,45 @@ int spl_load_legacy_img(struct spl_image_info *spl_image, >> if (ret) >> return ret; >> >> + dataptr = header + sizeof(hdr); >> + >> /* Read image */ >> - load->read(load, header + sizeof(hdr), spl_image->size, >> - (void *)(unsigned long)spl_image->load_addr); >> + switch (image_get_comp(&hdr)) { >> + case IH_COMP_NONE: >> + load->read(load, dataptr, spl_image->size, >> + (void *)(unsigned long)spl_image->load_addr); >> + break; > > to avoid the little increase of binary footprint due to the extra check > maybe a little wrapper like this could help: > > static inline int spl_image_get_comp(const struct image_header *hdr) > { > if (IS_ENABLED(CONFIG_SPL_LZMA) /* || > IS_ENABLED(CONFIG_SPL_ANOTHER_FANCY_COMPRESSION) */) > return image_get_comp(hdr); > > return IH_COMP_NONE; > } > > switch (spl_image_get_comp(&hdr)) { > ... > } > > then the compiler should optimise the switch/case statement away due to > Dead Code Elimination. Good idea. I was a bit concerned, even about minimal code size increase in SPL as well. I'll work on this in v7. Thanks, Stefan >> + >> +#if IS_ENABLED(CONFIG_SPL_LZMA) >> + case IH_COMP_LZMA: >> + lzma_len = LZMA_LEN; >> + >> + debug("LZMA: Decompressing %08lx to %08lx\n", >> + dataptr, spl_image->load_addr); >> + src = malloc(spl_image->size); >> + if (!src) { >> + printf("Unable to allocate %d bytes for LZMA\n", >> + spl_image->size); >> + return -ENOMEM; >> + } >> + >> + load->read(load, dataptr, spl_image->size, src); >> + ret = lzmaBuffToBuffDecompress((void *)spl_image->load_addr, >> + &lzma_len, src, 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; >> + } >> >> return 0; >> } >> > Viele Gr??e, Stefan
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 7f00fc8885..41734c026f 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -4,8 +4,15 @@ */ #include <common.h> +#include <malloc.h> #include <spl.h> +#include <lzma/LzmaTypes.h> +#include <lzma/LzmaDec.h> +#include <lzma/LzmaTools.h> + +#define LZMA_LEN (1 << 20) + int spl_parse_legacy_header(struct spl_image_info *spl_image, const struct image_header *header) { @@ -55,7 +62,10 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image, int spl_load_legacy_img(struct spl_image_info *spl_image, struct spl_load_info *load, ulong header) { + __maybe_unused SizeT lzma_len; + __maybe_unused void *src; struct image_header hdr; + ulong dataptr; int ret; /* Read header into local struct */ @@ -65,9 +75,45 @@ int spl_load_legacy_img(struct spl_image_info *spl_image, if (ret) return ret; + dataptr = header + sizeof(hdr); + /* Read image */ - load->read(load, header + sizeof(hdr), spl_image->size, - (void *)(unsigned long)spl_image->load_addr); + switch (image_get_comp(&hdr)) { + case IH_COMP_NONE: + load->read(load, dataptr, spl_image->size, + (void *)(unsigned long)spl_image->load_addr); + break; + +#if IS_ENABLED(CONFIG_SPL_LZMA) + case IH_COMP_LZMA: + lzma_len = LZMA_LEN; + + debug("LZMA: Decompressing %08lx to %08lx\n", + dataptr, spl_image->load_addr); + src = malloc(spl_image->size); + if (!src) { + printf("Unable to allocate %d bytes for LZMA\n", + spl_image->size); + return -ENOMEM; + } + + load->read(load, dataptr, spl_image->size, src); + ret = lzmaBuffToBuffDecompress((void *)spl_image->load_addr, + &lzma_len, src, 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; + } return 0; }