[PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure

[Adam Ford]

On Tue, Jan 19, 2021 at 5:56 AM Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> Hi Adam,
>
> Thank you very much for troubleshooting this, here is my proposal.
>
> > > > I appear to have the NAND flash working with the following patch:
> > > >
> > > > @@ -247,11 +253,21 @@ int nand_ecc_sw_bch_init_ctx(struct nand_device *nand)
> > > >         nand->ecc.ctx.priv = engine_conf;
> > > >         nand->ecc.ctx.total = nsteps * code_size;
> > > >
> > > > +       struct nand_chip *chip = mtd_to_nand(mtd);
> > > > +       chip->ecc.steps = nsteps;
> > > > +       chip->ecc.size =  conf->step_size;
>
> I was fearing that many boards would be affected by this issue but it
> appears that the problem will only show up here because the OMAP driver
> makes a strange use of the BCH library: it initializes it itself
> because it only needs it for a single operation while usually, the core
> is in charge of doing that. During the initialization, the OOB layout
> is verified. Usually, the BCH driver is used with one of the generic OOB
> layouts, while here the OMAP driver uses its own, which reads raw NAND
> chip entries.
>
> I recently moved the BCH driver to only use "generic" NAND bits, which
> produced the bug because the entries derived by the layout helpers
> have not been updated yet.
>
> So using raw NAND bits in the BCH driver is not an option here.
> Instead, I think the best way to address this is to export the
> declaration of the BCH internal configuration structure to the OMAP
> driver and use the right values, recently derived by the driver:
>
> ---8<---
>
> Author: Miquel Raynal <miquel.raynal at bootlin.com>
> Date:   Tue Jan 19 12:27:07 2021 +0100
>
>     wip: fix omap
>
>     Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
>
Thanks for fixing this.

I tested your patch, and I no longer get a Panic and the MTD device
appears to appear correctly:

mtdoops: mtd device (mtddev=name/number) must be supplied
omap2-nand 30000000.nand: GPIO lookup for consumer rb
omap2-nand 30000000.nand: using device tree for GPIO lookup
of_get_named_gpiod_flags: parsed 'rb-gpios' property of node
'/ocp at 68000000/gpmc at 6e000000/nand at 0,0[0]' - status (0)
gpio gpiochip6: Persistence not supported for GPIO 0
nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xbc
nand: Micron MT29F4G16ABBDA3W
nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
6 cmdlinepart partitions found on MTD device omap2-nand.0
Creating 6 MTD partitions on "omap2-nand.0":
...

When you submit a formal patch, CC me on the patch, and I'll respond
with a 'tested-by'

adam

> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> index fbb9955f2467..2c3e65cb68f3 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -15,6 +15,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/sched.h>
>  #include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand-ecc-sw-bch.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
>  #include <linux/omap-dma.h>
> @@ -1866,18 +1867,19 @@ static const struct mtd_ooblayout_ops omap_ooblayout_ops = {
>  static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
>                                  struct mtd_oob_region *oobregion)
>  {
> -       struct nand_chip *chip = mtd_to_nand(mtd);
> +       struct nand_device *nand = mtd_to_nanddev(mtd);
> +       const struct nand_ecc_sw_bch_conf *engine_conf = nand->ecc.ctx.priv;
>         int off = BADBLOCK_MARKER_LENGTH;
>
> -       if (section >= chip->ecc.steps)
> +       if (section >= engine_conf->nsteps)
>                 return -ERANGE;
>
>         /*
>          * When SW correction is employed, one OMAP specific marker byte is
>          * reserved after each ECC step.
>          */
> -       oobregion->offset = off + (section * (chip->ecc.bytes + 1));
> -       oobregion->length = chip->ecc.bytes;
> +       oobregion->offset = off + (section * (engine_conf->code_size + 1));
> +       oobregion->length = engine_conf->code_size;
>
>         return 0;
>  }
> @@ -1885,7 +1887,8 @@ static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
>  static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
>                                   struct mtd_oob_region *oobregion)
>  {
> -       struct nand_chip *chip = mtd_to_nand(mtd);
> +       struct nand_device *nand = mtd_to_nanddev(mtd);
> +       const struct nand_ecc_sw_bch_conf *engine_conf = nand->ecc.ctx.priv;
>         int off = BADBLOCK_MARKER_LENGTH;
>
>         if (section)
> @@ -1895,7 +1898,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
>          * When SW correction is employed, one OMAP specific marker byte is
>          * reserved after each ECC step.
>          */
> -       off += ((chip->ecc.bytes + 1) * chip->ecc.steps);
> +       off += ((engine_conf->code_size + 1) * engine_conf->nsteps);
>         if (off >= mtd->oobsize)
>                 return -ERANGE;
>
> --->8---
>
> Can you please try this patch and compare the values between your hack
> and mine of:
> - chip->ecc.bytes vs. engine_conf->code_size
> - chip->ecc.steps vs. engine_conf->nsteps
> The values should be the same, but I prefer to be sure.
>
> Thanks,
> Miquèl