mtd: spinand: Add support for HeYangTek HYF1GQ4UDACAE

Add Support HeYangTek HYF1GQ4UDACAE SPI NAND.

Datasheet Link:
- https://www.heyangtek.cn/previewfile.jsp?file=ABUIABA9GAAgwsvRnwYo-eDpsgc

Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
---
 drivers/mtd/nand/spi/Makefile    |   4 +-
 drivers/mtd/nand/spi/core.c      |   1 +
 drivers/mtd/nand/spi/heyangtek.c | 112 +++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h      |   1 +
 4 files changed, 116 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mtd/nand/spi/heyangtek.c

Hi Maxim,

maxim.anisimov.ua@gmail.com wrote on Mon, 24 Jun 2024 09:12:17 +0300:

> Add Support HeYangTek HYF1GQ4UDACAE SPI NAND.
> 
> Datasheet Link:
> - https://www.heyangtek.cn/previewfile.jsp?file=ABUIABA9GAAgwsvRnwYo-eDpsgc

Thanks for the patch! Few comments below.

> Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
> ---
>  drivers/mtd/nand/spi/Makefile    |   4 +-
>  drivers/mtd/nand/spi/core.c      |   1 +
>  drivers/mtd/nand/spi/heyangtek.c | 112 +++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h      |   1 +
>  4 files changed, 116 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mtd/nand/spi/heyangtek.c
> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 19cc77288ebb..69d95fbdd0ce 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
> -spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
> -spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
> +spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o heyangtek.o
> +spinand-objs += macronix.o micron.o paragon.o toshiba.o winbond.o xtx.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 e0b6715e5dfe..45795e5f1e49 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -942,6 +942,7 @@ static const struct spinand_manufacturer *spinand_manufacturers[] = {
>  	&esmt_c8_spinand_manufacturer,
>  	&foresee_spinand_manufacturer,
>  	&gigadevice_spinand_manufacturer,
> +	&heyangtek_spinand_manufacturer,
>  	&macronix_spinand_manufacturer,
>  	&micron_spinand_manufacturer,
>  	&paragon_spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/heyangtek.c b/drivers/mtd/nand/spi/heyangtek.c
> new file mode 100644
> index 000000000000..d4a5dbca40fb
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/heyangtek.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Author:
> + *      Andrey Zolotarev <andrey.zolotarev@keenetic.com> - the main driver logic
> + *      Maxim Anisimov <maxim.anisimov.ua@gmail.com> - adaptation to mainline linux kernel
> + *
> + * Based on:
> + *      https://github.com/keenetic/kernel-49/commit/bacade569fb12bc0ad31ba09bca9b890118fbca7
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/spinand.h>
> +
> +#define SPINAND_MFR_HEYANGTEK		0xC9
> +
> +#define STATUS_ECC_LIMIT_BITFLIPS	(3 << 4)
> +
> +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 hyfxgq4uda_ooblayout_ecc(struct mtd_info *mtd, int section,
> +				   struct mtd_oob_region *region)
> +{
> +	if (section > 3)
> +		return -ERANGE;
> +
> +	region->offset = section * 16 + 8;
> +	region->length = 8;

This is: 8-15, 24-31, 40-47, 56-62

> +
> +	return 0;
> +}
> +
> +static int hyfxgq4uda_ooblayout_free(struct mtd_info *mtd, int section,
> +				   struct mtd_oob_region *region)
> +{
> +	if (section > 3)
> +		return -ERANGE;
> +
> +	/* ECC-protected user meta-data */
> +	region->offset = section * 16 + 4;
> +	region->length = 4;

This is: 4-7, 20-23, 32-35, 48-51

So what about 2-4, 16-19, 36-39, 52-55, 63-64 ?

> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops hyfxgq4uda_ooblayout = {
> +	.ecc = hyfxgq4uda_ooblayout_ecc,
> +	.free = hyfxgq4uda_ooblayout_free,
> +};
> +
> +static int hyfxgq4uda_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 nanddev_get_ecc_conf(nand)->strength >> 1;

Maybe an explanation of this line is needed. Is this just guessing or
is this defined in the datasheet?

Also please do not use shifts when you want to divide. Just use / 2
which is easier to understand. Compilers know how to optimize that.

> +
> +	case STATUS_ECC_LIMIT_BITFLIPS:
> +		return nanddev_get_ecc_conf(nand)->strength;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct spinand_info heyangtek_spinand_table[] = {
> +	SPINAND_INFO("HYF1GQ4UDACAE",
> +		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x21),
> +		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
> +		     NAND_ECCREQ(4, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     SPINAND_HAS_QE_BIT,
> +		     SPINAND_ECCINFO(&hyfxgq4uda_ooblayout,
> +				     hyfxgq4uda_ecc_get_status)),
> +};
> +
> +static const struct spinand_manufacturer_ops heyangtek_spinand_manuf_ops = {
> +};
> +
> +const struct spinand_manufacturer heyangtek_spinand_manufacturer = {
> +	.id = SPINAND_MFR_HEYANGTEK,
> +	.name = "HeYangTek",
> +	.chips = heyangtek_spinand_table,
> +	.nchips = ARRAY_SIZE(heyangtek_spinand_table),
> +	.ops = &heyangtek_spinand_manuf_ops,
> +};
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 5c19ead60499..06ee35a27e3b 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -265,6 +265,7 @@ extern const struct spinand_manufacturer ato_spinand_manufacturer;
>  extern const struct spinand_manufacturer esmt_c8_spinand_manufacturer;
>  extern const struct spinand_manufacturer foresee_spinand_manufacturer;
>  extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
> +extern const struct spinand_manufacturer heyangtek_spinand_manufacturer;
>  extern const struct spinand_manufacturer macronix_spinand_manufacturer;
>  extern const struct spinand_manufacturer micron_spinand_manufacturer;
>  extern const struct spinand_manufacturer paragon_spinand_manufacturer;


Thanks,
Miquèl

Hi Miquel,

Sorry for late answer. I was on vocation. To be honest, I don't 
understand very well how this works. One of the OpenWrt developers asked 
me to send this patch to Linux upstream. Related OpenWrt pull request is 
here https://github.com/openwrt/openwrt/pull/15551 . Originally this 
patch code was taken from the device manufacturer's repository 
https://github.com/keenetic/kernel-49/commit/bacade569fb12bc0ad31ba09bca9b890118fbca7 
. I pointed to this in the source code. From my side I only adapted the 
code for successful compilation on the Linux upstream. It's difficult 
for me to answer your questions because I don't understand how the spi 
nand framework works. I can make corrections in the patch but with 
outside help. Thanks!

> Hi Maxim,
>
> maxim.anisimov.ua@gmail.com wrote on Mon, 24 Jun 2024 09:12:17 +0300:
>
>> Add Support HeYangTek HYF1GQ4UDACAE SPI NAND.
>>
>> Datasheet Link:
>> - https://www.heyangtek.cn/previewfile.jsp?file=ABUIABA9GAAgwsvRnwYo-eDpsgc
> Thanks for the patch! Few comments below.
>
>> Signed-off-by: Maxim Anisimov <maxim.anisimov.ua@gmail.com>
>> ---
>>   drivers/mtd/nand/spi/Makefile    |   4 +-
>>   drivers/mtd/nand/spi/core.c      |   1 +
>>   drivers/mtd/nand/spi/heyangtek.c | 112 +++++++++++++++++++++++++++++++
>>   include/linux/mtd/spinand.h      |   1 +
>>   4 files changed, 116 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/mtd/nand/spi/heyangtek.c
>>
>> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
>> index 19cc77288ebb..69d95fbdd0ce 100644
>> --- a/drivers/mtd/nand/spi/Makefile
>> +++ b/drivers/mtd/nand/spi/Makefile
>> @@ -1,4 +1,4 @@
>>   # SPDX-License-Identifier: GPL-2.0
>> -spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
>> -spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
>> +spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o heyangtek.o
>> +spinand-objs += macronix.o micron.o paragon.o toshiba.o winbond.o xtx.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 e0b6715e5dfe..45795e5f1e49 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -942,6 +942,7 @@ static const struct spinand_manufacturer *spinand_manufacturers[] = {
>>   	&esmt_c8_spinand_manufacturer,
>>   	&foresee_spinand_manufacturer,
>>   	&gigadevice_spinand_manufacturer,
>> +	&heyangtek_spinand_manufacturer,
>>   	&macronix_spinand_manufacturer,
>>   	&micron_spinand_manufacturer,
>>   	&paragon_spinand_manufacturer,
>> diff --git a/drivers/mtd/nand/spi/heyangtek.c b/drivers/mtd/nand/spi/heyangtek.c
>> new file mode 100644
>> index 000000000000..d4a5dbca40fb
>> --- /dev/null
>> +++ b/drivers/mtd/nand/spi/heyangtek.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Author:
>> + *      Andrey Zolotarev <andrey.zolotarev@keenetic.com> - the main driver logic
>> + *      Maxim Anisimov <maxim.anisimov.ua@gmail.com> - adaptation to mainline linux kernel
>> + *
>> + * Based on:
>> + *      https://github.com/keenetic/kernel-49/commit/bacade569fb12bc0ad31ba09bca9b890118fbca7
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mtd/spinand.h>
>> +
>> +#define SPINAND_MFR_HEYANGTEK		0xC9
>> +
>> +#define STATUS_ECC_LIMIT_BITFLIPS	(3 << 4)
>> +
>> +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 hyfxgq4uda_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +				   struct mtd_oob_region *region)
>> +{
>> +	if (section > 3)
>> +		return -ERANGE;
>> +
>> +	region->offset = section * 16 + 8;
>> +	region->length = 8;
> This is: 8-15, 24-31, 40-47, 56-62
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int hyfxgq4uda_ooblayout_free(struct mtd_info *mtd, int section,
>> +				   struct mtd_oob_region *region)
>> +{
>> +	if (section > 3)
>> +		return -ERANGE;
>> +
>> +	/* ECC-protected user meta-data */
>> +	region->offset = section * 16 + 4;
>> +	region->length = 4;
> This is: 4-7, 20-23, 32-35, 48-51
>
> So what about 2-4, 16-19, 36-39, 52-55, 63-64 ?
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops hyfxgq4uda_ooblayout = {
>> +	.ecc = hyfxgq4uda_ooblayout_ecc,
>> +	.free = hyfxgq4uda_ooblayout_free,
>> +};
>> +
>> +static int hyfxgq4uda_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 nanddev_get_ecc_conf(nand)->strength >> 1;
> Maybe an explanation of this line is needed. Is this just guessing or
> is this defined in the datasheet?
>
> Also please do not use shifts when you want to divide. Just use / 2
> which is easier to understand. Compilers know how to optimize that.
>
>> +
>> +	case STATUS_ECC_LIMIT_BITFLIPS:
>> +		return nanddev_get_ecc_conf(nand)->strength;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct spinand_info heyangtek_spinand_table[] = {
>> +	SPINAND_INFO("HYF1GQ4UDACAE",
>> +		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x21),
>> +		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
>> +		     NAND_ECCREQ(4, 512),
>> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> +					      &write_cache_variants,
>> +					      &update_cache_variants),
>> +		     SPINAND_HAS_QE_BIT,
>> +		     SPINAND_ECCINFO(&hyfxgq4uda_ooblayout,
>> +				     hyfxgq4uda_ecc_get_status)),
>> +};
>> +
>> +static const struct spinand_manufacturer_ops heyangtek_spinand_manuf_ops = {
>> +};
>> +
>> +const struct spinand_manufacturer heyangtek_spinand_manufacturer = {
>> +	.id = SPINAND_MFR_HEYANGTEK,
>> +	.name = "HeYangTek",
>> +	.chips = heyangtek_spinand_table,
>> +	.nchips = ARRAY_SIZE(heyangtek_spinand_table),
>> +	.ops = &heyangtek_spinand_manuf_ops,
>> +};
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index 5c19ead60499..06ee35a27e3b 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -265,6 +265,7 @@ extern const struct spinand_manufacturer ato_spinand_manufacturer;
>>   extern const struct spinand_manufacturer esmt_c8_spinand_manufacturer;
>>   extern const struct spinand_manufacturer foresee_spinand_manufacturer;
>>   extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
>> +extern const struct spinand_manufacturer heyangtek_spinand_manufacturer;
>>   extern const struct spinand_manufacturer macronix_spinand_manufacturer;
>>   extern const struct spinand_manufacturer micron_spinand_manufacturer;
>>   extern const struct spinand_manufacturer paragon_spinand_manufacturer;
>
> Thanks,
> Miquèl

Thanks,

Maxim

Hi Maxim,

maxim.anisimov.ua@gmail.com wrote on Mon, 22 Jul 2024 09:54:55 +0300:

> Hi Miquel,
> 
> Sorry for late answer. I was on vocation. To be honest, I don't understand very well how this works. One of the OpenWrt developers asked me to send this patch to Linux upstream. Related OpenWrt pull request is here https://github.com/openwrt/openwrt/pull/15551 . Originally this patch code was taken from the device manufacturer's repository https://github.com/keenetic/kernel-49/commit/bacade569fb12bc0ad31ba09bca9b890118fbca7 . I pointed to this in the source code. From my side I only adapted the code for successful compilation on the Linux upstream. It's difficult for me to answer your questions because I don't understand how the spi nand framework works. I can make corrections in the patch but with outside help. Thanks!

I'm sorry I cannot write the changes for you, but I can explain my
point below.

> >> +static int hyfxgq4uda_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +				   struct mtd_oob_region *region)
> >> +{
> >> +	if (section > 3)
> >> +		return -ERANGE;
> >> +
> >> +	region->offset = section * 16 + 8;
> >> +	region->length = 8;  
> > This is: 8-15, 24-31, 40-47, 56-62
> >  
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int hyfxgq4uda_ooblayout_free(struct mtd_info *mtd, int section,
> >> +				   struct mtd_oob_region *region)
> >> +{
> >> +	if (section > 3)
> >> +		return -ERANGE;
> >> +
> >> +	/* ECC-protected user meta-data */
> >> +	region->offset = section * 16 + 4;
> >> +	region->length = 4;  
> > This is: 4-7, 20-23, 32-35, 48-51
> >
> > So what about 2-4, 16-19, 36-39, 52-55, 63-64 ?

The OOB area is a specific area where we store:
- the bad block marker: it is at offset 0 and 1 and must be excluded
  from both functions.
- ECC bytes, so data for the correction engine: the amount depend on
  the strength of your correction and the position depends on the
  hardware. If this is not told in the datasheet explicitly you can
  play with flash_erase/nandwrite/nanddump: you can write a known
  pattern to an OOB area with the ECC engine enabled and you'll see
  what bytes have been smashed by the ECC engine.
- the rest is user data (typically jffs2 metadata).

In the current implementation, some bytes are just undefined, which is
not expected.

> >  
> >> +
> >> +	return 0;
> >> +}

...

> >> +static int hyfxgq4uda_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 nanddev_get_ecc_conf(nand)->strength >> 1;  
> > Maybe an explanation of this line is needed. Is this just guessing or
> > is this defined in the datasheet?

The datasheet should tell you what this bit means. If it means some
bits have been corrected up to half of the strength capability, then
this is fine. But please make sure this is right by using nandbiterrs
-i from the mtd-utils test suite.

> > Also please do not use shifts when you want to divide. Just use / 2
> > which is easier to understand. Compilers know how to optimize that.

This is self explanatory.

Thanks,
Miquèl