On Tue, Jan 19, 2021 at 5:56 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> Date: Tue Jan 19 12:27:07 2021 +0100
>
> wip: fix omap
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>
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@68000000/gpmc@6e000000/nand@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
