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

Maxim Anisimov
2024-07-22T06:54:59Z

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