[U-Boot] Getting rid of board_mtdparts_default()

[U-Boot] Getting rid of board_mtdparts_default()

[Boris Brezillon]

Hello Enric,

Miquel and I recently reworked drivers/mtd/mtdpart.c to get MTD
partitions exposed as MTD devices (as is done in Linux) instead of
having yet another abstraction to handle them (see what's currently
done in cmd/{mtdparts,nand,sf,...}.c).

This lead us to duplicate the mtdparts/mtdids parsing logic we found in
cmd/mtdparts.c, and while doing that we noticed that one function is
only implemented by igep boards: board_mtdparts_default().

We'd like to get rid of this function if possible in order to simplify
the mtdparts/mtdids retrieval logic, and there might be an easy
solution to do that: use the CONFIG_{MTDPARTS,MTDIDS}_DEFAULT config
options

CONFIG_MTDPARTS_DEFAULT="omap2-nand:512k(SPL),-(UBI);omap2-onenand:512k(SPL),-(UBI)"
CONFIG_MTDIDS_DEFAULT="nand0=omap2-nand,onenand0=omap2-onenand"

It seems those defaults were built at runtime, and the only thing I'm
not sure about is whether 512k for the SPL is always good. Looks like
4 eraseblocks are reserved for the SPL [1], but is the eraseblock size
always 128k? If you don't know about the eraseblock size, maybe you
know which OneNAND/NAND parts are used on these boards?

Regards,

Boris

[1]https://elixir.bootlin.com/u-boot/latest/source/board/isee/igep00x0/igep00x0.c#L254

-- 
Boris Brezillon, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[U-Boot] Getting rid of board_mtdparts_default()

[Enric Balletbo Serra]

Hello Boris,

Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
15 de nov. 2018 a les 16:58:
>
> Hello Enric,
>
> Miquel and I recently reworked drivers/mtd/mtdpart.c to get MTD
> partitions exposed as MTD devices (as is done in Linux) instead of
> having yet another abstraction to handle them (see what's currently
> done in cmd/{mtdparts,nand,sf,...}.c).
>
> This lead us to duplicate the mtdparts/mtdids parsing logic we found in
> cmd/mtdparts.c, and while doing that we noticed that one function is
> only implemented by igep boards: board_mtdparts_default().
>
> We'd like to get rid of this function if possible in order to simplify
> the mtdparts/mtdids retrieval logic, and there might be an easy
> solution to do that: use the CONFIG_{MTDPARTS,MTDIDS}_DEFAULT config
> options
>
> CONFIG_MTDPARTS_DEFAULT="omap2-nand:512k(SPL),-(UBI);omap2-onenand:512k(SPL),-(UBI)"
> CONFIG_MTDIDS_DEFAULT="nand0=omap2-nand,onenand0=omap2-onenand"
>
> It seems those defaults were built at runtime, and the only thing I'm
> not sure about is whether 512k for the SPL is always good. Looks like
> 4 eraseblocks are reserved for the SPL [1], but is the eraseblock size
> always 128k?

No, there are boards in the field that doesn't have and eraseblock
size of 128k, as far as I can remember there are at least boards with
an eraseblock of 64k, I don't remember though, boards with and
eraseblock size greater than 128k. About the 4 eraseblocks, there is a
reason for this., the blocks are reserved for the SPL because you can
flash 4 times (once per block) the SPL, the ROM code looks for an
image in the first four blocks and it detects the validity status of
these blocks. So, if the first one is corrupted, boots from the
second, if the second is corrupted looks for the third, etc.

Said this, I think that the main question is if it's fine to get rid
of this code. Ideally, we should cover all the cases, so assuming that
there aren't devices with an eraseblock size greater than 128k,
reserve 512k seems a good number. There is, though, a corner case
where this number is not fine. There are some boards that came with a
OneNand(DDP), the ROM code is only capable to read the odd blocks (1,
3, 5, 7...). So, in this case, you need to reserve 896k (128k NA 128k
NA 128k NA 128k ). Fwiw that was also not covered with current code.

To be honest, I wouldn't worry about this latest case, and in my
opinion, a fixed size of 512k is enough. Also, it's common now that
the first few blocks of NAND flash are guaranteed to be safe for the
first N erase cycles. So, I'm fine with the proposed fixed 512k for
SPL. Btw, good job.

Best regards,
  Enric

> If you don't know about the eraseblock size, maybe you
> know which OneNAND/NAND parts are used on these boards?
>
> Regards,
>
> Boris
>
> [1]https://elixir.bootlin.com/u-boot/latest/source/board/isee/igep00x0/igep00x0.c#L254
>
> --
> Boris Brezillon, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[U-Boot] Getting rid of board_mtdparts_default()

[Boris Brezillon]

On Thu, 15 Nov 2018 23:18:46 +0100
Enric Balletbo Serra <eballetbo@gmail.com> wrote:

> Hello Boris,
> 
> Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
> 15 de nov. 2018 a les 16:58:
> >
> > Hello Enric,
> >
> > Miquel and I recently reworked drivers/mtd/mtdpart.c to get MTD
> > partitions exposed as MTD devices (as is done in Linux) instead of
> > having yet another abstraction to handle them (see what's currently
> > done in cmd/{mtdparts,nand,sf,...}.c).
> >
> > This lead us to duplicate the mtdparts/mtdids parsing logic we found in
> > cmd/mtdparts.c, and while doing that we noticed that one function is
> > only implemented by igep boards: board_mtdparts_default().
> >
> > We'd like to get rid of this function if possible in order to simplify
> > the mtdparts/mtdids retrieval logic, and there might be an easy
> > solution to do that: use the CONFIG_{MTDPARTS,MTDIDS}_DEFAULT config
> > options
> >
> > CONFIG_MTDPARTS_DEFAULT="omap2-nand:512k(SPL),-(UBI);omap2-onenand:512k(SPL),-(UBI)"
> > CONFIG_MTDIDS_DEFAULT="nand0=omap2-nand,onenand0=omap2-onenand"
> >
> > It seems those defaults were built at runtime, and the only thing I'm
> > not sure about is whether 512k for the SPL is always good. Looks like
> > 4 eraseblocks are reserved for the SPL [1], but is the eraseblock size
> > always 128k?  
> 
> No, there are boards in the field that doesn't have and eraseblock
> size of 128k, as far as I can remember there are at least boards with
> an eraseblock of 64k, I don't remember though, boards with and
> eraseblock size greater than 128k. About the 4 eraseblocks, there is a
> reason for this., the blocks are reserved for the SPL because you can
> flash 4 times (once per block) the SPL, the ROM code looks for an
> image in the first four blocks and it detects the validity status of
> these blocks. So, if the first one is corrupted, boots from the
> second, if the second is corrupted looks for the third, etc.
> 
> Said this, I think that the main question is if it's fine to get rid
> of this code. Ideally, we should cover all the cases, so assuming that
> there aren't devices with an eraseblock size greater than 128k,
> reserve 512k seems a good number. There is, though, a corner case
> where this number is not fine. There are some boards that came with a
> OneNand(DDP), the ROM code is only capable to read the odd blocks (1,
> 3, 5, 7...). So, in this case, you need to reserve 896k (128k NA 128k
> NA 128k NA 128k ). Fwiw that was also not covered with current code.
> 
> To be honest, I wouldn't worry about this latest case, and in my
> opinion, a fixed size of 512k is enough. Also, it's common now that
> the first few blocks of NAND flash are guaranteed to be safe for the
> first N erase cycles. So, I'm fine with the proposed fixed 512k for
> SPL. Btw, good job.

Okay, so next question is, does anyone rely on the default
partitioning? If that's the case we're screwed because using something
different will break things (UBI will be smaller than expected and
complain that blocks are missing).

[U-Boot] Getting rid of board_mtdparts_default()

[Enric Balletbo Serra]

Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
15 de nov. 2018 a les 23:40:
>
> On Thu, 15 Nov 2018 23:18:46 +0100
> Enric Balletbo Serra <eballetbo@gmail.com> wrote:
>
> > Hello Boris,
> >
> > Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
> > 15 de nov. 2018 a les 16:58:
> > >
> > > Hello Enric,
> > >
> > > Miquel and I recently reworked drivers/mtd/mtdpart.c to get MTD
> > > partitions exposed as MTD devices (as is done in Linux) instead of
> > > having yet another abstraction to handle them (see what's currently
> > > done in cmd/{mtdparts,nand,sf,...}.c).
> > >
> > > This lead us to duplicate the mtdparts/mtdids parsing logic we found in
> > > cmd/mtdparts.c, and while doing that we noticed that one function is
> > > only implemented by igep boards: board_mtdparts_default().
> > >
> > > We'd like to get rid of this function if possible in order to simplify
> > > the mtdparts/mtdids retrieval logic, and there might be an easy
> > > solution to do that: use the CONFIG_{MTDPARTS,MTDIDS}_DEFAULT config
> > > options
> > >
> > > CONFIG_MTDPARTS_DEFAULT="omap2-nand:512k(SPL),-(UBI);omap2-onenand:512k(SPL),-(UBI)"
> > > CONFIG_MTDIDS_DEFAULT="nand0=omap2-nand,onenand0=omap2-onenand"
> > >
> > > It seems those defaults were built at runtime, and the only thing I'm
> > > not sure about is whether 512k for the SPL is always good. Looks like
> > > 4 eraseblocks are reserved for the SPL [1], but is the eraseblock size
> > > always 128k?
> >
> > No, there are boards in the field that doesn't have and eraseblock
> > size of 128k, as far as I can remember there are at least boards with
> > an eraseblock of 64k, I don't remember though, boards with and
> > eraseblock size greater than 128k. About the 4 eraseblocks, there is a
> > reason for this., the blocks are reserved for the SPL because you can
> > flash 4 times (once per block) the SPL, the ROM code looks for an
> > image in the first four blocks and it detects the validity status of
> > these blocks. So, if the first one is corrupted, boots from the
> > second, if the second is corrupted looks for the third, etc.
> >
> > Said this, I think that the main question is if it's fine to get rid
> > of this code. Ideally, we should cover all the cases, so assuming that
> > there aren't devices with an eraseblock size greater than 128k,
> > reserve 512k seems a good number. There is, though, a corner case
> > where this number is not fine. There are some boards that came with a
> > OneNand(DDP), the ROM code is only capable to read the odd blocks (1,
> > 3, 5, 7...). So, in this case, you need to reserve 896k (128k NA 128k
> > NA 128k NA 128k ). Fwiw that was also not covered with current code.
> >
> > To be honest, I wouldn't worry about this latest case, and in my
> > opinion, a fixed size of 512k is enough. Also, it's common now that
> > the first few blocks of NAND flash are guaranteed to be safe for the
> > first N erase cycles. So, I'm fine with the proposed fixed 512k for
> > SPL. Btw, good job.
>
> Okay, so next question is, does anyone rely on the default
> partitioning? If that's the case we're screwed because using something
> different will break things (UBI will be smaller than expected and
> complain that blocks are missing).

I don't rely on the default partitioning and the few people I know
also don't rely on it. I can't talk for everyone as is more than 3
year I left from the company that makes the IGEP, although I am pretty
sure nobody relies on it. That partioning is the most common, so my
suggestion is to go ahead and let's see if somebody complains.

[U-Boot] Getting rid of board_mtdparts_default()

[Boris Brezillon]

On Fri, 16 Nov 2018 23:30:41 +0100
Enric Balletbo Serra <eballetbo@gmail.com> wrote:

> Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
> 15 de nov. 2018 a les 23:40:
> >
> > On Thu, 15 Nov 2018 23:18:46 +0100
> > Enric Balletbo Serra <eballetbo@gmail.com> wrote:
> >  
> > > Hello Boris,
> > >
> > > Missatge de Boris Brezillon <boris.brezillon@bootlin.com> del dia dj.,
> > > 15 de nov. 2018 a les 16:58:  
> > > >
> > > > Hello Enric,
> > > >
> > > > Miquel and I recently reworked drivers/mtd/mtdpart.c to get MTD
> > > > partitions exposed as MTD devices (as is done in Linux) instead of
> > > > having yet another abstraction to handle them (see what's currently
> > > > done in cmd/{mtdparts,nand,sf,...}.c).
> > > >
> > > > This lead us to duplicate the mtdparts/mtdids parsing logic we found in
> > > > cmd/mtdparts.c, and while doing that we noticed that one function is
> > > > only implemented by igep boards: board_mtdparts_default().
> > > >
> > > > We'd like to get rid of this function if possible in order to simplify
> > > > the mtdparts/mtdids retrieval logic, and there might be an easy
> > > > solution to do that: use the CONFIG_{MTDPARTS,MTDIDS}_DEFAULT config
> > > > options
> > > >
> > > > CONFIG_MTDPARTS_DEFAULT="omap2-nand:512k(SPL),-(UBI);omap2-onenand:512k(SPL),-(UBI)"
> > > > CONFIG_MTDIDS_DEFAULT="nand0=omap2-nand,onenand0=omap2-onenand"
> > > >
> > > > It seems those defaults were built at runtime, and the only thing I'm
> > > > not sure about is whether 512k for the SPL is always good. Looks like
> > > > 4 eraseblocks are reserved for the SPL [1], but is the eraseblock size
> > > > always 128k?  
> > >
> > > No, there are boards in the field that doesn't have and eraseblock
> > > size of 128k, as far as I can remember there are at least boards with
> > > an eraseblock of 64k, I don't remember though, boards with and
> > > eraseblock size greater than 128k. About the 4 eraseblocks, there is a
> > > reason for this., the blocks are reserved for the SPL because you can
> > > flash 4 times (once per block) the SPL, the ROM code looks for an
> > > image in the first four blocks and it detects the validity status of
> > > these blocks. So, if the first one is corrupted, boots from the
> > > second, if the second is corrupted looks for the third, etc.
> > >
> > > Said this, I think that the main question is if it's fine to get rid
> > > of this code. Ideally, we should cover all the cases, so assuming that
> > > there aren't devices with an eraseblock size greater than 128k,
> > > reserve 512k seems a good number. There is, though, a corner case
> > > where this number is not fine. There are some boards that came with a
> > > OneNand(DDP), the ROM code is only capable to read the odd blocks (1,
> > > 3, 5, 7...). So, in this case, you need to reserve 896k (128k NA 128k
> > > NA 128k NA 128k ). Fwiw that was also not covered with current code.
> > >
> > > To be honest, I wouldn't worry about this latest case, and in my
> > > opinion, a fixed size of 512k is enough. Also, it's common now that
> > > the first few blocks of NAND flash are guaranteed to be safe for the
> > > first N erase cycles. So, I'm fine with the proposed fixed 512k for
> > > SPL. Btw, good job.  
> >
> > Okay, so next question is, does anyone rely on the default
> > partitioning? If that's the case we're screwed because using something
> > different will break things (UBI will be smaller than expected and
> > complain that blocks are missing).  
> 
> I don't rely on the default partitioning and the few people I know
> also don't rely on it. I can't talk for everyone as is more than 3
> year I left from the company that makes the IGEP, although I am pretty
> sure nobody relies on it. That partioning is the most common, so my
> suggestion is to go ahead and let's see if somebody complains.

Okay, I'll send a patch soon.

Thanks,

Boris