| Message ID | 20220404040153.1509966-1-gch981213@gmail.com |
|---|---|
| Headers | show |
| Series | spi: add support for Mediatek SPI-NAND controller | expand |
Hi Chuanhong, gch981213@gmail.com 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@gmail.com> > --- > > 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
Hi! On Mon, Apr 4, 2022 at 3:59 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Chuanhong, > > gch981213@gmail.com 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@gmail.com> > > --- > > > > 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
Hi! On Mon, Apr 4, 2022 at 5:13 PM Chuanhong Guo <gch981213@gmail.com> wrote: > [...] > 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 I mean If there is no ecc.user_conf in the 2nd case :) > Is this OK?
Hi Chuanhong, gch981213@gmail.com wrote on Mon, 4 Apr 2022 17:13:55 +0800: > Hi! > > On Mon, Apr 4, 2022 at 3:59 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Chuanhong, > > > > gch981213@gmail.com 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@gmail.com> > > > --- > > > > > > 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
Hi! On Mon, Apr 4, 2022 at 6:28 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > [...] > > > > + > > > > + // 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. The core provided ecc strength may be smaller than the calculated one. e.g. A nand chip may only have a requirement of 8/512bits ECC. But if it has a 2k+128 pagesize, this controller can do 12/512bits ECC and the bootrom expects the latter. > You should take the user requirements first. If there are no > user inputs, you should in theory look at the device's requirements. I'll take the user requirements if there is one. If there isn't, I'll follow the calculated strength instead of the device requirement so that user doesn't have to specify a custom strength in dt. > [...] > > > > +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. I did. the setup_pagefmt is called once with the minimal page+oob size in probe. > In > any way, you need to free that memory at some point (when removing the > driver). I was using the devm api for this allocation so kernel should take care of that. I'll change the DMA to use streamed API in the next version to avoid an extra memory copy in reading, and the allocated buffer will be freed in remove().
Mediatek has an extended version of their NAND Flash Interface which has a SPI-NAND mode. In this mode, the controller can perform 1-bit spi-mem ops for up-to 0xa0 bytes and typical SPI-NAND single, dual and quad IO page cache ops with 2-byte address. Additionally, the page cache ops can be performed with ECC and auto data formatting using the ECC engine of the controller. This patchset implements support of this mode as a separated SPI-MEM driver with piplined ECC engine. Changes since v1: add a blank line between properties in dt binding doc rename ecc-engine to nand-ecc-engine for the generic properties fix warnings/errors from the CI Chuanhong Guo (5): mtd: nand: make mtk_ecc.c a separated module spi: add driver for MTK SPI NAND Flash Interface mtd: nand: mtk-ecc: also parse nand-ecc-engine if available dt-bindings: spi: add binding doc for spi-mtk-snfi arm64: dts: mediatek: add mtk-snfi for mt7622 .../bindings/spi/mediatek,spi-mtk-snfi.yaml | 88 ++ arch/arm64/boot/dts/mediatek/mt7622.dtsi | 12 + drivers/mtd/nand/Kconfig | 7 + drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/{raw/mtk_ecc.c => ecc-mtk.c} | 8 +- drivers/mtd/nand/raw/Kconfig | 1 + drivers/mtd/nand/raw/Makefile | 2 +- drivers/mtd/nand/raw/mtk_nand.c | 2 +- drivers/spi/Kconfig | 10 + drivers/spi/Makefile | 1 + drivers/spi/spi-mtk-snfi.c | 1351 +++++++++++++++++ .../linux/mtd/nand-ecc-mtk.h | 0 12 files changed, 1478 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/spi/mediatek,spi-mtk-snfi.yaml rename drivers/mtd/nand/{raw/mtk_ecc.c => ecc-mtk.c} (98%) create mode 100644 drivers/spi/spi-mtk-snfi.c rename drivers/mtd/nand/raw/mtk_ecc.h => include/linux/mtd/nand-ecc-mtk.h (100%)