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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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@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://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 <weijie.gao@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.

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

> +
> +	case STATUS_ECC_LIMIT_BITFLIPS:
> +		return nand->eccreq.strength;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}

Thanks,
Miquèl