[v1] spl: nand: allow partial nand page reads during nand_spl_load_image

[Dario Binacchi]

Hi Colinn

On Fri, Nov 18, 2022 at 1:08 PM Dario Binacchi
<dario.binacchi at amarulasolutions.com> wrote:
>
> Hi Colin,
>
> On Tue, Nov 15, 2022 at 5:35 PM Colin Foster
> <colin.foster at 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 at 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 at 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 at amarulasolutions.com
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> info at amarulasolutions.com
>
> www.amarulasolutions.com



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi at amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info at amarulasolutions.com

www.amarulasolutions.com