Re: [PATCH v2 2/5] spi: add driver for MTK SPI NAND Flash Interface

[Chuanhong Guo]

Hi!

On Mon, Apr 4, 2022 at 3:59 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Chuanhong,
>
> gch981213@xxxxxxxxx wrote on Mon,  4 Apr 2022 12:01:50 +0800:
>
> > This driver implements support for the SPI-NAND mode of MTK NAND Flash
> > Interface as a SPI-MEM controller with piplined ECC capability.
> >
> > Signed-off-by: Chuanhong Guo <gch981213@xxxxxxxxx>
> > ---
> >
> > Change since v1:
> >   fix CI warnings
> >
> >  drivers/spi/Kconfig        |   10 +
> >  drivers/spi/Makefile       |    1 +
> >  drivers/spi/spi-mtk-snfi.c | 1351 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 1362 insertions(+)
> >  create mode 100644 drivers/spi/spi-mtk-snfi.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index d2815eb361c0..739eec7d0c15 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -590,6 +590,16 @@ config SPI_MTK_NOR
> >         SPI interface as well as several SPI NOR specific instructions
> >         via SPI MEM interface.
> >
> > +config SPI_MTK_SNFI
> > +     tristate "MediaTek SPI NAND Flash Interface"
> > +     depends on ARCH_MEDIATEK || COMPILE_TEST
> > +     depends on MTD_NAND_ECC_MEDIATEK
> > +     help
> > +       This enables support for SPI-NAND mode on the MediaTek NAND
> > +       Flash Interface found on MediaTek ARM SoCs. This controller
> > +       is implemented as a SPI-MEM controller with pipelined ECC
> > +       capcability.
> > +
> >  config SPI_NPCM_FIU
> >       tristate "Nuvoton NPCM FLASH Interface Unit"
> >       depends on ARCH_NPCM || COMPILE_TEST
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 3aa28ed3f761..51541ff17e67 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -76,6 +76,7 @@ obj-$(CONFIG_SPI_MPC52xx)           += spi-mpc52xx.o
> >  obj-$(CONFIG_SPI_MT65XX)                += spi-mt65xx.o
> >  obj-$(CONFIG_SPI_MT7621)             += spi-mt7621.o
> >  obj-$(CONFIG_SPI_MTK_NOR)            += spi-mtk-nor.o
> > +obj-$(CONFIG_SPI_MTK_SNFI)           += spi-mtk-snfi.o
> >  obj-$(CONFIG_SPI_MXIC)                       += spi-mxic.o
> >  obj-$(CONFIG_SPI_MXS)                        += spi-mxs.o
> >  obj-$(CONFIG_SPI_NPCM_FIU)           += spi-npcm-fiu.o
> > diff --git a/drivers/spi/spi-mtk-snfi.c b/drivers/spi/spi-mtk-snfi.c
> > new file mode 100644
> > index 000000000000..e8f8f30bd7ee
> > --- /dev/null
> > +++ b/drivers/spi/spi-mtk-snfi.c
>
> [...]
>
> > +static struct mtk_snand *nand_to_mtk_snand(struct nand_device *nand)
> > +{
> > +     struct nand_ecc_engine *eng = nand->ecc.engine;
> > +
> > +     return container_of(eng, struct mtk_snand, ecc_eng);
> > +}
> > +
> > +static inline int snand_prepare_bouncebuf(struct mtk_snand *snf, size_t size)
> > +{
> > +     if (snf->buf_len >= size)
> > +             return 0;
> > +     if (snf->buf)
> > +             dmam_free_coherent(snf->dev, snf->buf_len, snf->buf,
> > +                                snf->buf_dma);
>
> Can't we use a single coherent buffer once for all?

This only reallocates when the page size changes to a larger one,
so there's at most two allocations: one during probe and the other
one in the first call to init_ctx. The other solution is to allocate a
buffer for the maximum supported page size but I think that's a
waste of memory.

>
> > +     snf->buf =
> > +             dmam_alloc_coherent(snf->dev, size, &snf->buf_dma, GFP_KERNEL);
> > +     if (!snf->buf)
> > +             return -ENOMEM;
> > +     snf->buf_len = size;
> > +     memset(snf->buf, 0xff, snf->buf_len);
> > +     return 0;
> > +}
> > +
>
> [...]
>
> > +static int mtk_snand_ecc_init_ctx(struct nand_device *nand)
> > +{
> > +     struct mtk_snand *snf = nand_to_mtk_snand(nand);
> > +     struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
> > +     struct mtd_info *mtd = nanddev_to_mtd(nand);
> > +     int ret;
> > +
> > +     ret = mtk_snand_setup_pagefmt(snf, nand->memorg.pagesize,
> > +                                   nand->memorg.oobsize);
> > +     if (ret)
> > +             return ret;
> > +
> > +     mtd_set_ooblayout(mtd, &mtk_snand_ooblayout);
> > +
> > +     // This driver ignores any ECC capability configured by user or
> > +     // requested by the nand chip because the BootROM and MTK bootloader
> > +     // expects the page format to be the exact one as calculated in
> > +     // setup_pagefmt.
>
> I don't like this :)
>
> I understand that the boot partition might have specific constraints,
> but other partitions (or if we don't use the NAND to boot?) should
> probably be usable with other ECC schemes.

In this controller, the ECC step size is fixed and it can only change
ECC strength. The calculated ECC correction capability is the max
possible one supported by the controller.
I still want the default behavior to match the boot partition
requirement, because we can't just tell end-users to customize
their dts by taking apart their device and figure out which flash
is used.

I can implement the following:
1. select the minimum capability exceeding ecc.user_conf
2. If that doesn't exist, use the calculated one and warn
   if it doesn't meet ecc.requirements
Is this OK?

>
> > +     conf->step_size = snf->caps->sector_size;
> > +     conf->strength = snf->ecc_cfg.strength;
> > +
> > +     return 0;
> > +}
> > +
> > +static int mtk_snand_ecc_prepare_io_req(struct nand_device *nand,
> > +                                     struct nand_page_io_req *req)
> > +{
> > +     struct mtk_snand *snf = nand_to_mtk_snand(nand);
> > +     int ret;
> > +
> > +     ret = mtk_snand_setup_pagefmt(snf, nand->memorg.pagesize,
> > +                                   nand->memorg.oobsize);
> > +     if (ret)
> > +             return ret;
> > +     snf->autofmt = true;
> > +     return 0;
> > +}
> > +
> > +static int mtk_snand_ecc_finish_io_req(struct nand_device *nand,
> > +                                    struct nand_page_io_req *req)
> > +{
> > +     struct mtk_snand *snf = nand_to_mtk_snand(nand);
> > +     struct mtd_info *mtd = nanddev_to_mtd(nand);
> > +
> > +     snf->autofmt = false;
> > +     if ((req->mode == MTD_OPS_RAW) || (req->type != NAND_PAGE_READ))
> > +             return 0;
> > +
> > +     if (snf->ecc_stats.failed)
> > +             mtd->ecc_stats.failed += snf->ecc_stats.failed;
> > +     mtd->ecc_stats.corrected += snf->ecc_stats.corrected;
> > +     return snf->ecc_stats.failed ? -EBADMSG : snf->ecc_stats.bitflips;
>
> Did you verify that nandbiterrs -i succeeds?

I did a insmod mtd_nandbiterrs.ko dev=x and the reported bitflips in
kernel log is correct.

Is there a userspace tool for this? I'd like to use that instead of a
kernel module in the future.

>
> > +}
> > +
> > +static struct nand_ecc_engine_ops mtk_snfi_ecc_engine_ops = {
> > +     .init_ctx = mtk_snand_ecc_init_ctx,
> > +     .prepare_io_req = mtk_snand_ecc_prepare_io_req,
> > +     .finish_io_req = mtk_snand_ecc_finish_io_req,
>
> I believe you need to take care of the bounce buffer in the exit path?

No. The buffer should be left there for non-ecc spi-mem operations.

>
> > +};
> > +
> > +static void mtk_snand_read_fdm(struct mtk_snand *snf, uint8_t *buf)
> > +{
> > +     uint32_t vall, valm;
> > +     uint8_t *oobptr = buf;
> > +     int i, j;
> > +
> > +     for (i = 0; i < snf->nfi_cfg.nsectors; i++) {
> > +             vall = nfi_read32(snf, NFI_FDML(i));
> > +             valm = nfi_read32(snf, NFI_FDMM(i));
> > +
> > +             for (j = 0; j < snf->caps->fdm_size; j++)
> > +                     oobptr[j] = (j >= 4 ? valm : vall) >> ((j % 4) * 8);
> > +
> > +             oobptr += snf->caps->fdm_size;
> > +     }
> > +}
>
> Thanks,
> Miquèl



--
Regards,
Chuanhong Guo