Re: [PATCH] mtd: spinand: Add support for HeYangTek HYF1GQ4UDACAE

[Maxim Anisimov <maxim.anisimov.ua@gmail.com>]

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