Hello Weijie, Sorry for the delay, I have a couple of comments, see below.
On 29/04/2026 at 16:13:13 +08, Weijie Gao <[email protected]> wrote: > This patch adds some Etron SPI-NAND flashes from 1Gb to 8Gb with > 4-bits/8-bits On-die ECC support. > > EM73C044VCF/EM73D044VCO/EM73E044VCE/EM73F044VCA are tested on MediaTek > filogic MT7988 reference board. > > Datasheets: > https://etron.com/wp-content/uploads/2024/08/EM73C044VCF-SPI-NAND-Flash_Rev-1.03.pdf > https://etron.com/wp-content/uploads/2022/04/EM73D044VCOR-SPI-NAND-Flash_Rev-1.00.pdf > https://etron.com/wp-content/uploads/2022/04/EM78DE044VC-SPI-NAND-Flash_Rev-1.01.pdf > https://etron.com/wp-content/uploads/2024/08/EM73E044VCEG-SPI-NAND-Flash_Rev-1.00.pdf > https://etron.com/wp-content/uploads/2022/04/EM738F044VCA-SPI-NAND-Flash_Rev-1.02.pdf I haven't checked all the chips in detail, I checked just a couple of them. > Signed-off-by: Weijie Gao <[email protected]> > --- [...] > +#ifndef __UBOOT__ > +#include <linux/device.h> > +#include <linux/kernel.h> > +#endif Please drop this #if block > +#include <linux/mtd/spinand.h> > + > +#define SPINAND_MFR_ETRON 0xD5 > + > +#define STATUS_ECC_LIMIT_BITFLIPS (3 << 4) Please add a prefix aligned with Etron's namespace. > +static SPINAND_OP_VARIANTS(read_cache_variants, > + SPINAND_PAGE_READ_FROM_CACHE_1S_4S_4S_OP(0, 1, NULL, 0, 0), > + SPINAND_PAGE_READ_FROM_CACHE_1S_1S_4S_OP(0, 1, NULL, 0, 0), > + SPINAND_PAGE_READ_FROM_CACHE_1S_2S_2S_OP(0, 1, NULL, 0, 0), > + SPINAND_PAGE_READ_FROM_CACHE_1S_1S_2S_OP(0, 1, NULL, 0, 0), > + SPINAND_PAGE_READ_FROM_CACHE_FAST_1S_1S_1S_OP(0, 1, NULL, 0, 0), > + SPINAND_PAGE_READ_FROM_CACHE_1S_1S_1S_OP(0, 1, NULL, 0, 0)); > + > +static SPINAND_OP_VARIANTS(write_cache_variants, > + SPINAND_PROG_LOAD_1S_1S_4S_OP(true, 0, NULL, 0), > + SPINAND_PROG_LOAD_1S_1S_1S_OP(true, 0, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(update_cache_variants, > + SPINAND_PROG_LOAD_1S_1S_4S_OP(false, 0, NULL, 0), > + SPINAND_PROG_LOAD_1S_1S_1S_OP(false, 0, NULL, 0)); > + > +static int etron_ecc4_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + struct spinand_device *spinand = mtd_to_spinand(mtd); > + > + if (section >= spinand->base.memorg.pagesize / mtd->ecc_step_size) > + return -ERANGE; > + > + region->offset = (8 * section) + 32; > + region->length = 8; This should be grouped into one big continuous area. > + > + return 0; > +} > + > +static int etron_ecc4_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + struct spinand_device *spinand = mtd_to_spinand(mtd); > + > + if (section >= spinand->base.memorg.pagesize / mtd->ecc_step_size) > + return -ERANGE; > + > + if (section) { > + region->offset = 8 * section; > + region->length = 8; Ditto. And same below, in the other OOB layouts. > + } else { > + /* section 0 has two bytes reserved for bad block mark */ > + region->offset = 2; > + region->length = 6; > + } > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops etron_ecc4_ooblayout = { > + .ecc = etron_ecc4_ooblayout_ecc, > + .rfree = etron_ecc4_ooblayout_free, > +}; > + > +static int etron_ecc8_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + struct spinand_device *spinand = mtd_to_spinand(mtd); > + > + if (section >= spinand->base.memorg.pagesize / mtd->ecc_step_size) > + return -ERANGE; > + > + region->offset = (14 * section) + 72; > + region->length = 14; > + > + return 0; > +} > + > +static int etron_ecc8_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + struct spinand_device *spinand = mtd_to_spinand(mtd); > + > + if (section >= spinand->base.memorg.pagesize / mtd->ecc_step_size) > + return -ERANGE; > + > + if (section) { > + region->offset = 18 * section; > + region->length = 18; > + } else { > + /* section 0 has two bytes reserved for bad block mark */ > + region->offset = 2; > + region->length = 16; > + } > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops etron_ecc8_ooblayout = { > + .ecc = etron_ecc8_ooblayout_ecc, > + .rfree = etron_ecc8_ooblayout_free, ^ Should not even compile? > +}; > + > +static int etron_ecc_get_status(struct spinand_device *spinand, u8 status) > +{ > + struct nand_device *nand = spinand_to_nand(spinand); > + > + switch (status & STATUS_ECC_MASK) { > + case STATUS_ECC_NO_BITFLIPS: > + return 0; > + > + case STATUS_ECC_UNCOR_ERROR: > + return -EBADMSG; > + > + case STATUS_ECC_HAS_BITFLIPS: > + return nand->eccreq.strength >> 1; This is incorrect, you should probably return strength - 1 if that is what the bit means. We already had a discussion about this in the past: https://lore.kernel.org/linux-mtd/[email protected]/ > + > + case STATUS_ECC_LIMIT_BITFLIPS: > + return nand->eccreq.strength; > + > + default: > + break; > + } > + > + return -EINVAL; > +} Thanks, Miquèl
