* [v1] spl: nand: allow partial nand page reads during nand_spl_load_image
@ 2022-11-15 16:35 Colin Foster
2022-11-18 12:08 ` Dario Binacchi
0 siblings, 1 reply; 4+ messages in thread
From: Colin Foster @ 2022-11-15 16:35 UTC (permalink / raw)
To: u-boot; +Cc: Michael Trimarchi, Dario Binacchi
The nand_spl_load_image function was guaranteed to read an entire block
into RAM, regardless of how many bytes were to be read. This is
particularly problematic when spl_load_legacy_image is called, as this
function attempts to load a struct image_header but gets surprised with
a full flash sector.
Allow partial reads to restore functionality to the code path
spl_nand_load_element() -> spl_load_legacy_img() ->
spl_nand_legacy_read(load, header, sizeof(hdr), header);
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
drivers/mtd/nand/raw/nand_spl_loaders.c | 36 ++++++++++++++++---------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/nand/raw/nand_spl_loaders.c b/drivers/mtd/nand/raw/nand_spl_loaders.c
index 4befc75c04..84ac482c06 100644
--- a/drivers/mtd/nand/raw/nand_spl_loaders.c
+++ b/drivers/mtd/nand/raw/nand_spl_loaders.c
@@ -1,9 +1,18 @@
+/*
+ * Temporary storage for non NAND page aligned and non NAND page sized
+ * reads. Note: This does not support runtime detected FLASH yet, but
+ * that should be reasonably easy to fix by making the buffer large
+ * enough :)
+ */
+static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
+
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
{
+ unsigned int read_this_time = CONFIG_SYS_NAND_PAGE_SIZE;
+ unsigned int size_remaining = size;
unsigned int block, lastblock;
unsigned int page, page_offset;
- /* offs has to be aligned to a page address! */
block = offs / CONFIG_SYS_NAND_BLOCK_SIZE;
lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE;
page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE;
@@ -12,8 +21,18 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
while (block <= lastblock) {
if (!nand_is_bad_block(block)) {
/* Skip bad blocks */
- while (page < CONFIG_SYS_NAND_PAGE_COUNT) {
- nand_read_page(block, page, dst);
+ while (page < CONFIG_SYS_NAND_PAGE_COUNT &&
+ size_remaining > 0) {
+ if (size_remaining < CONFIG_SYS_NAND_PAGE_SIZE)
+ {
+ nand_read_page(block, page,
+ scratch_buf);
+ memcpy(dst, scratch_buf,
+ size_remaining);
+ read_this_time = size_remaining;
+ } else {
+ nand_read_page(block, page, dst);
+ }
/*
* When offs is not aligned to page address the
* extra offset is copied to dst as well. Copy
@@ -26,7 +45,8 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
dst = (void *)((int)dst - page_offset);
page_offset = 0;
}
- dst += CONFIG_SYS_NAND_PAGE_SIZE;
+ dst += read_this_time;
+ size_remaining -= read_this_time;
page++;
}
@@ -70,14 +90,6 @@ u32 nand_spl_adjust_offset(u32 sector, u32 offs)
}
#ifdef CONFIG_SPL_UBI
-/*
- * Temporary storage for non NAND page aligned and non NAND page sized
- * reads. Note: This does not support runtime detected FLASH yet, but
- * that should be reasonably easy to fix by making the buffer large
- * enough :)
- */
-static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
-
/**
* nand_spl_read_block - Read data from physical eraseblock into a buffer
* @block: Number of the physical eraseblock
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [v1] spl: nand: allow partial nand page reads during nand_spl_load_image
2022-11-15 16:35 [v1] spl: nand: allow partial nand page reads during nand_spl_load_image Colin Foster
@ 2022-11-18 12:08 ` Dario Binacchi
2022-11-29 9:00 ` Dario Binacchi
0 siblings, 1 reply; 4+ messages in thread
From: Dario Binacchi @ 2022-11-18 12:08 UTC (permalink / raw)
To: Colin Foster; +Cc: u-boot, Michael Trimarchi
Hi Colin,
On Tue, Nov 15, 2022 at 5:35 PM Colin Foster
<colin.foster@in-advantage.com> wrote:
>
> The nand_spl_load_image function was guaranteed to read an entire block
> into RAM, regardless of how many bytes were to be read. This is
> particularly problematic when spl_load_legacy_image is called, as this
> function attempts to load a struct image_header but gets surprised with
> a full flash sector.
>
> Allow partial reads to restore functionality to the code path
> spl_nand_load_element() -> spl_load_legacy_img() ->
> spl_nand_legacy_read(load, header, sizeof(hdr), header);
>
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> drivers/mtd/nand/raw/nand_spl_loaders.c | 36 ++++++++++++++++---------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_spl_loaders.c b/drivers/mtd/nand/raw/nand_spl_loaders.c
> index 4befc75c04..84ac482c06 100644
> --- a/drivers/mtd/nand/raw/nand_spl_loaders.c
> +++ b/drivers/mtd/nand/raw/nand_spl_loaders.c
> @@ -1,9 +1,18 @@
> +/*
> + * Temporary storage for non NAND page aligned and non NAND page sized
> + * reads. Note: This does not support runtime detected FLASH yet, but
> + * that should be reasonably easy to fix by making the buffer large
> + * enough :)
> + */
> +static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> +
> int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> {
> + unsigned int read_this_time = CONFIG_SYS_NAND_PAGE_SIZE;
> + unsigned int size_remaining = size;
> unsigned int block, lastblock;
> unsigned int page, page_offset;
>
> - /* offs has to be aligned to a page address! */
> block = offs / CONFIG_SYS_NAND_BLOCK_SIZE;
> lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE;
> page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE;
> @@ -12,8 +21,18 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> while (block <= lastblock) {
> if (!nand_is_bad_block(block)) {
> /* Skip bad blocks */
> - while (page < CONFIG_SYS_NAND_PAGE_COUNT) {
> - nand_read_page(block, page, dst);
> + while (page < CONFIG_SYS_NAND_PAGE_COUNT &&
> + size_remaining > 0) {
> + if (size_remaining < CONFIG_SYS_NAND_PAGE_SIZE)
> + {
> + nand_read_page(block, page,
> + scratch_buf);
> + memcpy(dst, scratch_buf,
> + size_remaining);
> + read_this_time = size_remaining;
> + } else {
> + nand_read_page(block, page, dst);
> + }
> /*
> * When offs is not aligned to page address the
> * extra offset is copied to dst as well. Copy
> @@ -26,7 +45,8 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> dst = (void *)((int)dst - page_offset);
> page_offset = 0;
> }
> - dst += CONFIG_SYS_NAND_PAGE_SIZE;
> + dst += read_this_time;
> + size_remaining -= read_this_time;
> page++;
> }
>
> @@ -70,14 +90,6 @@ u32 nand_spl_adjust_offset(u32 sector, u32 offs)
> }
>
> #ifdef CONFIG_SPL_UBI
> -/*
> - * Temporary storage for non NAND page aligned and non NAND page sized
> - * reads. Note: This does not support runtime detected FLASH yet, but
> - * that should be reasonably easy to fix by making the buffer large
> - * enough :)
> - */
> -static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> -
> /**
> * nand_spl_read_block - Read data from physical eraseblock into a buffer
> * @block: Number of the physical eraseblock
> --
> 2.25.1
>
Reviewed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Thanks and regards,
Dario
--
Dario Binacchi
Embedded Linux Developer
dario.binacchi@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v1] spl: nand: allow partial nand page reads during nand_spl_load_image
2022-11-18 12:08 ` Dario Binacchi
@ 2022-11-29 9:00 ` Dario Binacchi
2022-12-06 2:08 ` Colin Foster
0 siblings, 1 reply; 4+ messages in thread
From: Dario Binacchi @ 2022-11-29 9:00 UTC (permalink / raw)
To: Colin Foster; +Cc: u-boot, Michael Trimarchi
Hi Colinn
On Fri, Nov 18, 2022 at 1:08 PM Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
>
> Hi Colin,
>
> On Tue, Nov 15, 2022 at 5:35 PM Colin Foster
> <colin.foster@in-advantage.com> wrote:
> >
> > The nand_spl_load_image function was guaranteed to read an entire block
> > into RAM, regardless of how many bytes were to be read. This is
> > particularly problematic when spl_load_legacy_image is called, as this
> > function attempts to load a struct image_header but gets surprised with
> > a full flash sector.
> >
> > Allow partial reads to restore functionality to the code path
> > spl_nand_load_element() -> spl_load_legacy_img() ->
> > spl_nand_legacy_read(load, header, sizeof(hdr), header);
> >
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> > drivers/mtd/nand/raw/nand_spl_loaders.c | 36 ++++++++++++++++---------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_spl_loaders.c b/drivers/mtd/nand/raw/nand_spl_loaders.c
> > index 4befc75c04..84ac482c06 100644
> > --- a/drivers/mtd/nand/raw/nand_spl_loaders.c
> > +++ b/drivers/mtd/nand/raw/nand_spl_loaders.c
> > @@ -1,9 +1,18 @@
> > +/*
> > + * Temporary storage for non NAND page aligned and non NAND page sized
> > + * reads. Note: This does not support runtime detected FLASH yet, but
> > + * that should be reasonably easy to fix by making the buffer large
> > + * enough :)
> > + */
> > +static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> > +
> > int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > {
> > + unsigned int read_this_time = CONFIG_SYS_NAND_PAGE_SIZE;
> > + unsigned int size_remaining = size;
> > unsigned int block, lastblock;
> > unsigned int page, page_offset;
> >
> > - /* offs has to be aligned to a page address! */
> > block = offs / CONFIG_SYS_NAND_BLOCK_SIZE;
> > lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE;
> > page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE;
> > @@ -12,8 +21,18 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > while (block <= lastblock) {
> > if (!nand_is_bad_block(block)) {
> > /* Skip bad blocks */
> > - while (page < CONFIG_SYS_NAND_PAGE_COUNT) {
> > - nand_read_page(block, page, dst);
> > + while (page < CONFIG_SYS_NAND_PAGE_COUNT &&
> > + size_remaining > 0) {
> > + if (size_remaining < CONFIG_SYS_NAND_PAGE_SIZE)
> > + {
> > + nand_read_page(block, page,
> > + scratch_buf);
> > + memcpy(dst, scratch_buf,
> > + size_remaining);
> > + read_this_time = size_remaining;
> > + } else {
> > + nand_read_page(block, page, dst);
> > + }
> > /*
> > * When offs is not aligned to page address the
> > * extra offset is copied to dst as well. Copy
> > @@ -26,7 +45,8 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > dst = (void *)((int)dst - page_offset);
> > page_offset = 0;
> > }
> > - dst += CONFIG_SYS_NAND_PAGE_SIZE;
> > + dst += read_this_time;
> > + size_remaining -= read_this_time;
> > page++;
> > }
> >
> > @@ -70,14 +90,6 @@ u32 nand_spl_adjust_offset(u32 sector, u32 offs)
> > }
> >
> > #ifdef CONFIG_SPL_UBI
> > -/*
> > - * Temporary storage for non NAND page aligned and non NAND page sized
> > - * reads. Note: This does not support runtime detected FLASH yet, but
> > - * that should be reasonably easy to fix by making the buffer large
> > - * enough :)
> > - */
> > -static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> > -
> > /**
> > * nand_spl_read_block - Read data from physical eraseblock into a buffer
> > * @block: Number of the physical eraseblock
> > --
> > 2.25.1
> >
>
> Reviewed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> Thanks and regards,
> Dario
>
This is the error reported by the CI/CD pipeline:
arm: + corvus
241+arm-linux-gnueabi-ld.bfd: u-boot-spl section `.bss' will not fit
in region `.sdram'
242+arm-linux-gnueabi-ld.bfd: SPL image BSS too big
243+arm-linux-gnueabi-ld.bfd: region `.sdram' overflowed by 1388 bytes
244+make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
245+make[1]: *** [Makefile:2074: spl/u-boot-spl] Error 2
246+make: *** [Makefile:177: sub-make] Error 2
I think the problem is:
+static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
.+
So, please try with a malloc() and test the patch again.
You can take as example the file drivers/mtd/nand/raw/mt7621_nand_spl.c.
Thanks and regards,
Dario
> --
>
> Dario Binacchi
>
> Embedded Linux Developer
>
> dario.binacchi@amarulasolutions.com
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> info@amarulasolutions.com
>
> www.amarulasolutions.com
--
Dario Binacchi
Embedded Linux Developer
dario.binacchi@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v1] spl: nand: allow partial nand page reads during nand_spl_load_image
2022-11-29 9:00 ` Dario Binacchi
@ 2022-12-06 2:08 ` Colin Foster
0 siblings, 0 replies; 4+ messages in thread
From: Colin Foster @ 2022-12-06 2:08 UTC (permalink / raw)
To: Dario Binacchi; +Cc: u-boot, Michael Trimarchi
On Tue, Nov 29, 2022 at 10:00:19AM +0100, Dario Binacchi wrote:
> Hi Colinn
>
> On Fri, Nov 18, 2022 at 1:08 PM Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
> >
> > Hi Colin,
> >
> > On Tue, Nov 15, 2022 at 5:35 PM Colin Foster
> > <colin.foster@in-advantage.com> wrote:
> > >
> > > The nand_spl_load_image function was guaranteed to read an entire block
> > > into RAM, regardless of how many bytes were to be read. This is
> > > particularly problematic when spl_load_legacy_image is called, as this
> > > function attempts to load a struct image_header but gets surprised with
> > > a full flash sector.
> > >
> > > Allow partial reads to restore functionality to the code path
> > > spl_nand_load_element() -> spl_load_legacy_img() ->
> > > spl_nand_legacy_read(load, header, sizeof(hdr), header);
> > >
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> > > drivers/mtd/nand/raw/nand_spl_loaders.c | 36 ++++++++++++++++---------
> > > 1 file changed, 24 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/nand_spl_loaders.c b/drivers/mtd/nand/raw/nand_spl_loaders.c
> > > index 4befc75c04..84ac482c06 100644
> > > --- a/drivers/mtd/nand/raw/nand_spl_loaders.c
> > > +++ b/drivers/mtd/nand/raw/nand_spl_loaders.c
> > > @@ -1,9 +1,18 @@
> > > +/*
> > > + * Temporary storage for non NAND page aligned and non NAND page sized
> > > + * reads. Note: This does not support runtime detected FLASH yet, but
> > > + * that should be reasonably easy to fix by making the buffer large
> > > + * enough :)
> > > + */
> > > +static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> > > +
> > > int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > {
> > > + unsigned int read_this_time = CONFIG_SYS_NAND_PAGE_SIZE;
> > > + unsigned int size_remaining = size;
> > > unsigned int block, lastblock;
> > > unsigned int page, page_offset;
> > >
> > > - /* offs has to be aligned to a page address! */
> > > block = offs / CONFIG_SYS_NAND_BLOCK_SIZE;
> > > lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE;
> > > page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE;
> > > @@ -12,8 +21,18 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > while (block <= lastblock) {
> > > if (!nand_is_bad_block(block)) {
> > > /* Skip bad blocks */
> > > - while (page < CONFIG_SYS_NAND_PAGE_COUNT) {
> > > - nand_read_page(block, page, dst);
> > > + while (page < CONFIG_SYS_NAND_PAGE_COUNT &&
> > > + size_remaining > 0) {
> > > + if (size_remaining < CONFIG_SYS_NAND_PAGE_SIZE)
> > > + {
> > > + nand_read_page(block, page,
> > > + scratch_buf);
> > > + memcpy(dst, scratch_buf,
> > > + size_remaining);
> > > + read_this_time = size_remaining;
> > > + } else {
> > > + nand_read_page(block, page, dst);
> > > + }
> > > /*
> > > * When offs is not aligned to page address the
> > > * extra offset is copied to dst as well. Copy
> > > @@ -26,7 +45,8 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > dst = (void *)((int)dst - page_offset);
> > > page_offset = 0;
> > > }
> > > - dst += CONFIG_SYS_NAND_PAGE_SIZE;
> > > + dst += read_this_time;
> > > + size_remaining -= read_this_time;
> > > page++;
> > > }
> > >
> > > @@ -70,14 +90,6 @@ u32 nand_spl_adjust_offset(u32 sector, u32 offs)
> > > }
> > >
> > > #ifdef CONFIG_SPL_UBI
> > > -/*
> > > - * Temporary storage for non NAND page aligned and non NAND page sized
> > > - * reads. Note: This does not support runtime detected FLASH yet, but
> > > - * that should be reasonably easy to fix by making the buffer large
> > > - * enough :)
> > > - */
> > > -static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> > > -
> > > /**
> > > * nand_spl_read_block - Read data from physical eraseblock into a buffer
> > > * @block: Number of the physical eraseblock
> > > --
> > > 2.25.1
> > >
> >
> > Reviewed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > Thanks and regards,
> > Dario
> >
>
> This is the error reported by the CI/CD pipeline:
>
> arm: + corvus
> 241+arm-linux-gnueabi-ld.bfd: u-boot-spl section `.bss' will not fit
> in region `.sdram'
> 242+arm-linux-gnueabi-ld.bfd: SPL image BSS too big
> 243+arm-linux-gnueabi-ld.bfd: region `.sdram' overflowed by 1388 bytes
> 244+make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> 245+make[1]: *** [Makefile:2074: spl/u-boot-spl] Error 2
> 246+make: *** [Makefile:177: sub-make] Error 2
>
> I think the problem is:
>
> +static u8 scratch_buf[CONFIG_SYS_NAND_PAGE_SIZE];
> .+
>
> So, please try with a malloc() and test the patch again.
> You can take as example the file drivers/mtd/nand/raw/mt7621_nand_spl.c.
Hi Dario,
Thanks for all the info. I'll make this change and resubmit once I have
time to test it.
>
> Thanks and regards,
> Dario
>
> > --
> >
> > Dario Binacchi
> >
> > Embedded Linux Developer
> >
> > dario.binacchi@amarulasolutions.com
> >
> > __________________________________
> >
> >
> > Amarula Solutions SRL
> >
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> >
> > T. +39 042 243 5310
> > info@amarulasolutions.com
> >
> > www.amarulasolutions.com
>
>
>
> --
>
> Dario Binacchi
>
> Embedded Linux Developer
>
> dario.binacchi@amarulasolutions.com
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> info@amarulasolutions.com
>
> www.amarulasolutions.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-06 1:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15 16:35 [v1] spl: nand: allow partial nand page reads during nand_spl_load_image Colin Foster
2022-11-18 12:08 ` Dario Binacchi
2022-11-29 9:00 ` Dario Binacchi
2022-12-06 2:08 ` Colin Foster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.