[V2] mtd: spinand: Add support for Etron EM73D044VCx

This adds a new vendor Etron, and support for a 2Gb chip.

The datasheet is available at
https://www.etron.com/cn/products/EM73%5B8%5DC%5BD_E_F%5DVC%20SPI%20NAND%20Flash_Promotion_Rev%201_00A.pdf

Signed-off-by: Bert Vermeulen <bert@biot.com>
---
v2: Fixed a small style issue, and now sending to linux-mtd@ as well.

 drivers/mtd/nand/spi/Makefile |  2 +-
 drivers/mtd/nand/spi/core.c   |  1 +
 drivers/mtd/nand/spi/etron.c  | 98 +++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h   |  1 +
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/spi/etron.c

Hi Bert,

Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:

> This adds a new vendor Etron, and support for a 2Gb chip.

Interesting name :)

> The datasheet is available at
> https://www.etron.com/cn/products/EM73%5B8%5DC%5BD_E_F%5DVC%20SPI%20NAND%20Flash_Promotion_Rev%201_00A.pdf
> 
> Signed-off-by: Bert Vermeulen <bert@biot.com>
> ---
> v2: Fixed a small style issue, and now sending to linux-mtd@ as well.
> 
>  drivers/mtd/nand/spi/Makefile |  2 +-
>  drivers/mtd/nand/spi/core.c   |  1 +
>  drivers/mtd/nand/spi/etron.c  | 98 +++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h   |  1 +
>  4 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/spi/etron.c
> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 9662b9c1d5a9..cc3c4e046ea9 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,3 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
> -spinand-objs := core.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o
> +spinand-objs := core.o etron.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o
>  obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 446ba8d43fbc..2a806ffa7310 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -895,6 +895,7 @@ static const struct nand_ops spinand_ops = {
>  
>  static const struct spinand_manufacturer *spinand_manufacturers[] = {
>  	&gigadevice_spinand_manufacturer,
> +	&etron_spinand_manufacturer,

Should be placed above :)

>  	&macronix_spinand_manufacturer,
>  	&micron_spinand_manufacturer,
>  	&paragon_spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/etron.c b/drivers/mtd/nand/spi/etron.c
> new file mode 100644
> index 000000000000..653092be5938
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/etron.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/spinand.h>
> +
> +#define SPINAND_MFR_ETRON			0xd5
> +
> +

No double \n please

> +static SPINAND_OP_VARIANTS(read_cache_variants,
> +		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(write_cache_variants,
> +		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
> +		SPINAND_PROG_LOAD(true, 0, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(update_cache_variants,
> +		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> +		SPINAND_PROG_LOAD(false, 0, NULL, 0));
> +
> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
> +					struct mtd_oob_region *oobregion)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = 72;
> +	oobregion->length = 56;

This looks highly dependent on a single chip, could you make these
values (at least the offset) dependent on the MTD device?

> +
> +	return 0;
> +}
> +
> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
> +			   struct mtd_oob_region *oobregion)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = 1;
> +	oobregion->length = 71;

Same here.

> +
> +	return 0;
> +}
> +
> +static int etron_ecc_get_status(struct spinand_device *spinand, u8 status)
> +{
> +	switch (status & STATUS_ECC_MASK) {
> +	case STATUS_ECC_NO_BITFLIPS:
> +		return 0;
> +
> +	case STATUS_ECC_HAS_BITFLIPS:
> +		/* Between 1-7 bitflips were corrected */
> +		return 7;
> +
> +	case STATUS_ECC_MASK:
> +		/* Maximum bitflips were corrected */
> +		return 8;
> +
> +	case STATUS_ECC_UNCOR_ERROR:
> +		return -EBADMSG;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct mtd_ooblayout_ops etron_ooblayout = {
> +	.ecc = etron_ooblayout_ecc,
> +	.free = etron_ooblayout_free,
> +};

Could be above _get_status()

> +
> +static const struct spinand_info etron_spinand_table[] = {
> +	SPINAND_INFO("EM73D044VCx",
> +		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x1f),
> +		     // bpc, pagesize, oobsize, pagesperblock, bperlun, maxbadplun, ppl, lpt, #t

Please drop this comment

> +		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     SPINAND_HAS_QE_BIT,
> +		     SPINAND_ECCINFO(&etron_ooblayout, etron_ecc_get_status)),
> +};
> +
> +static const struct spinand_manufacturer_ops etron_spinand_manuf_ops = {
> +};
> +
> +const struct spinand_manufacturer etron_spinand_manufacturer = {
> +	.id = SPINAND_MFR_ETRON,
> +	.name = "Etron",
> +	.chips = etron_spinand_table,
> +	.nchips = ARRAY_SIZE(etron_spinand_table),
> +	.ops = &etron_spinand_manuf_ops,
> +};
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 6988956b8492..d7c0a0439652 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -260,6 +260,7 @@ struct spinand_manufacturer {
>  };
>  
>  /* SPI NAND manufacturers */
> +extern const struct spinand_manufacturer etron_spinand_manufacturer;
>  extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
>  extern const struct spinand_manufacturer macronix_spinand_manufacturer;
>  extern const struct spinand_manufacturer micron_spinand_manufacturer;

Thanks,
Miquèl

On 8/6/21 9:34 PM, Miquel Raynal wrote:

Hi Miquel,

Thanks for reviewing. I'll send in a v2 with fixes. However...

> Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:
>> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +					struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 72;
>> +	oobregion->length = 56;
> 
> This looks highly dependent on a single chip, could you make these
> values (at least the offset) dependent on the MTD device?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
>> +			   struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 1;
>> +	oobregion->length = 71;
> 
> Same here.

I was rather hoping for some guidance on the _ecc and _free
offset/length numbers. There's not much info around on what's best
practice here, and I haven't played with NAND chips enough to actually
test the in-kernel ECC stuff.

The _free offset of 1 is for the bad block byte, which I saw in another
NAND chip driver, but I don't remember where I got the other numbers
from, and really need somebody to double-check that.

thanks,

Hi Bert,

Bert Vermeulen <bert@biot.com> wrote on Sat, 7 Aug 2021 09:31:55 +0200:

> On 8/6/21 9:34 PM, Miquel Raynal wrote:
> 
> Hi Miquel,
> 
> Thanks for reviewing. I'll send in a v2 with fixes. However...
> 
> > Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:  
> >> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +					struct mtd_oob_region *oobregion)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	oobregion->offset = 72;
> >> +	oobregion->length = 56;  
> > 
> > This looks highly dependent on a single chip, could you make these
> > values (at least the offset) dependent on the MTD device?
> >   
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
> >> +			   struct mtd_oob_region *oobregion)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	oobregion->offset = 1;
> >> +	oobregion->length = 71;  
> > 
> > Same here.  
> 
> I was rather hoping for some guidance on the _ecc and _free
> offset/length numbers. There's not much info around on what's best
> practice here, and I haven't played with NAND chips enough to actually
> test the in-kernel ECC stuff.
> 
> The _free offset of 1 is for the bad block byte, which I saw in another
> NAND chip driver, but I don't remember where I got the other numbers
> from, and really need somebody to double-check that.

These values should be in the datasheet somehow.

Depending on the algorithm (ex: BCH) and the strength (ex: 4b/512B) you
will need a certain number of ECC bytes per chunk. Then you multiply
that number by the number of chunks and you get the total number of ECC
bytes. Assuming you know an offset from the start or the end of the OOB
section, you might write something like that:

	length = mtd->writesize / chunk_size * nb_ecc_bytes/chunk;
	offset = mtd->oobsize - length;

Also please reserve two bytes for OOB (even if so far we encountered
only 1B markers).

Thanks,
Miquèl