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

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

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

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

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