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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Chuanhong,

gch981213@xxxxxxxxx wrote on Mon, 4 Apr 2022 17:13:55 +0800:

> 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.

Ok, fine.

> 
> >  
> > > +     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.

That's fine.

> 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,

That is okay, but that does not mean you can only support this one.

> because we can't just tell end-users to customize
> their dts by taking apart their device and figure out which flash
> is used.

They don't have to do so. In theory they should not request anything,
the core would take care of all of that. But they can request specific
values by using the DT and you must follow them in the driver.

On his side the core is responsible of telling you which strength
should be used otherwise and the driver is expected to use it.

> I can implement the following:

You should take the user requirements first. If there are no
user inputs, you should in theory look at the device's requirements.

> 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.

Yes, you can give the mtd-utils test suite a try. Almost all the tools
have been migrated there. There even is a Buildroot package.

> > > +}
> > > +
> > > +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.

AFAIR you initialize the buffer in the ECC part, so if it must be used
without ECC you should probably allocate it for the SPI controller. In
any way, you need to free that memory at some point (when removing the
driver).

> 
> >  
> > > +};
> > > +
> > > +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


Thanks,
Miquèl