[PATCH] mtd: spinand: add support for Etron SPI-NAND flashes

[Weijie Gao]

On Wed, 2026-05-27 at 10:26 +0200, Miquel Raynal wrote:
> 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 <weijie.gao at mediatek.com>
> 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://urldefense.com/v3/__https://etron.com/wp-content/uploads/2024/08/EM73C044VCF-SPI-NAND-Flash_Rev-1.03.pdf__;!!CTRNKA9wMg0ARbw!gspAb_ATP-dOd61dvrUFgW4djua1MqYvOWrCMpr08fy1cd0aXgBHc0DpLK1QjsvUpnqO8M17049YgIPRAD7TX0rDOTY$
> >  
> > 
https://urldefense.com/v3/__https://etron.com/wp-content/uploads/2022/04/EM73D044VCOR-SPI-NAND-Flash_Rev-1.00.pdf__;!!CTRNKA9wMg0ARbw!gspAb_ATP-dOd61dvrUFgW4djua1MqYvOWrCMpr08fy1cd0aXgBHc0DpLK1QjsvUpnqO8M17049YgIPRAD7TDU5Zmz4$
> >  
> > 
https://urldefense.com/v3/__https://etron.com/wp-content/uploads/2022/04/EM78DE044VC-SPI-NAND-Flash_Rev-1.01.pdf__;!!CTRNKA9wMg0ARbw!gspAb_ATP-dOd61dvrUFgW4djua1MqYvOWrCMpr08fy1cd0aXgBHc0DpLK1QjsvUpnqO8M17049YgIPRAD7TC0zknDQ$
> >  
> > 
https://urldefense.com/v3/__https://etron.com/wp-content/uploads/2024/08/EM73E044VCEG-SPI-NAND-Flash_Rev-1.00.pdf__;!!CTRNKA9wMg0ARbw!gspAb_ATP-dOd61dvrUFgW4djua1MqYvOWrCMpr08fy1cd0aXgBHc0DpLK1QjsvUpnqO8M17049YgIPRAD7TN5lTwzg$
> >  
> > 
https://urldefense.com/v3/__https://etron.com/wp-content/uploads/2022/04/EM738F044VCA-SPI-NAND-Flash_Rev-1.02.pdf__;!!CTRNKA9wMg0ARbw!gspAb_ATP-dOd61dvrUFgW4djua1MqYvOWrCMpr08fy1cd0aXgBHc0DpLK1QjsvUpnqO8M17049YgIPRAD7T0csUZhs$
> >  
> 
> I haven't checked all the chips in detail, I checked just a couple of
> them.
> 
> > Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> > ---
> 
> [...]
> 
> > +#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.

I'll use STATUS_ECC_MASK instead, as what the linux kernel does.

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

OK.

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

I didn't see and mistake here.
This file can be compiled without error.

> 
> > +};
> > +
> > +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/20210908201624.237634-1-bert@biot.com/
>  
> 

OK. I'll follow the linux kernel.

> > +
> > +	case STATUS_ECC_LIMIT_BITFLIPS:
> > +		return nand->eccreq.strength;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> 
> Thanks,
> Miquèl