No subject
Kyeongrho.Kim
kr.kim at skyhighmemory.com
Thu Mar 28 21:41:29 PDT 2024
(I send again this mail with plain text not HTML.)
Dear Miquel,
Please see my reply in below email.
And please comment if you have any others.
Thanks,
KR
-----Original Message-----
From: Miquel Raynal <mailto:miquel.raynal at bootlin.com>
Sent: Thursday, March 7, 2024 5:01 PM
To: Kyeongrho.Kim <mailto:kr.kim at skyhighmemory.com>
Cc: mailto:richard at nod.at; mailto:vigneshr at ti.com; mailto:mmkurbanov at salutedevices.com; mailto:ddrokosov at sberdevices.ru; mailto:gch981213 at gmail.com; mailto:michael at walle.cc; mailto:broonie at kernel.org; mailto:mika.westerberg at linux.intel.com; mailto:acelan.kao at canonical.com; mailto:linux-kernel at vger.kernel.org; mailto:linux-mtd at lists.infradead.org; Mohamed Sardi <mailto:moh.sardi at skyhighmemory.com>; Changsub.Shim <mailto:changsub.shim at skyhighmemory.com>
Subject: Re:
Hi,
mailto:kr.kim at skyhighmemory.com wrote on Thu, 7 Mar 2024 15:07:29 +0900:
> Feat: Add SkyHigh Memory Patch code
>
> Add SPI Nand Patch code of SkyHigh Memory
> - Add company dependent code with 'skyhigh.c'
> - Insert into 'core.c' so that 'always ECC on'
Patch formatting is still messed up.
> commit 6061b97a830af8cb5fd0917e833e779451f9046a (HEAD -> master)
> Author: KR Kim <mailto:kr.kim at skyhighmemory.com>
> Date: Thu Mar 7 13:24:11 2024 +0900
>
> SPI Nand Patch code of SkyHigh Momory
>
> Signed-off-by: KR Kim <mailto:kr.kim at skyhighmemory.com>
>
> From 6061b97a830af8cb5fd0917e833e779451f9046a Mon Sep 17 00:00:00 2001
> From: KR Kim <mailto:kr.kim at skyhighmemory.com>
> Date: Thu, 7 Mar 2024 13:24:11 +0900
> Subject: [PATCH] SPI Nand Patch code of SkyHigh Memory
>
> ---
> drivers/mtd/nand/spi/Makefile | 2 +-
> drivers/mtd/nand/spi/core.c | 7 +-
> drivers/mtd/nand/spi/skyhigh.c | 155 +++++++++++++++++++++++++++++++++
> include/linux/mtd/spinand.h | 3 +
> 4 files changed, 165 insertions(+), 2 deletions(-) mode change
> 100644 => 100755 drivers/mtd/nand/spi/Makefile mode change 100644 =>
> 100755 drivers/mtd/nand/spi/core.c create mode 100644
> drivers/mtd/nand/spi/skyhigh.c mode change 100644 => 100755
> include/linux/mtd/spinand.h
>
> diff --git a/drivers/mtd/nand/spi/Makefile
> b/drivers/mtd/nand/spi/Makefile old mode 100644 new mode 100755 index
> 19cc77288ebb..1e61ab21893a
> --- 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 += micron.o paragon.o skyhigh.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 old mode
> 100644 new mode 100755 index e0b6715e5dfe..e3f0a7544ba4
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -34,7 +34,7 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> return 0;
> }
>
> -static int spinand_write_reg_op(struct spinand_device *spinand, u8
> reg, u8 val)
> +int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8
> +val)
Please do this in a separate commit.
[SHM] May I know why we need to do a separate commit?
Please elaborate for the reason.
> {
> struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,
> spinand->scratchbuf);
> @@ -196,6 +196,10 @@ static int spinand_init_quad_enable(struct
> spinand_device *spinand) static int spinand_ecc_enable(struct spinand_device *spinand,
> bool enable)
> {
> + /* SHM : always ECC enable */
> + if (spinand->flags & SPINAND_ON_DIE_ECC_MANDATORY)
> + return 0;
Silently always enabling ECC is not possible. If you cannot disable the on-die engine, then:
- you should prevent any other engine type to be used
- you should error out if a raw access is requested
- these chips are broken, IMO
[SHM] I understand that you are concern.
We have already reviewed 'Always ECC on' to see if there was any problem in many aspects and confirmed that there was no problem.
> +
> return spinand_upd_cfg(spinand, CFG_ECC_ENABLE,
> enable ? CFG_ECC_ENABLE : 0); } @@ -945,6 +949,7 @@ static
> const struct spinand_manufacturer *spinand_manufacturers[] = {
> ¯onix_spinand_manufacturer,
> µn_spinand_manufacturer,
> ¶gon_spinand_manufacturer,
> + &skyhigh_spinand_manufacturer,
> &toshiba_spinand_manufacturer,
> &winbond_spinand_manufacturer,
> &xtx_spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/skyhigh.c
> b/drivers/mtd/nand/spi/skyhigh.c new file mode 100644 index
> 000000000000..92e7572094ff
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/skyhigh.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 SkyHigh Memory Limited
> + *
> + * Author: Takahiro Kuwano <mailto:takahiro.kuwano at infineon.com> */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/spinand.h>
> +
> +#define SPINAND_MFR_SKYHIGH 0x01
> +
> +#define SKYHIGH_STATUS_ECC_1TO2_BITFLIPS (1 << 4)
> +#define SKYHIGH_STATUS_ECC_3TO6_BITFLIPS (2 << 4)
> +#define SKYHIGH_STATUS_ECC_UNCOR_ERROR (3 << 4)
> +
> +#define SKYHIGH_CONFIG_PROTECT_EN BIT(1)
> +
> +static SPINAND_OP_VARIANTS(read_cache_variants,
> + SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 4, NULL, 0),
> + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> + SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 2, 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 skyhigh_spinand_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + if (section)
> + return -ERANGE;
> +
> + /* SkyHigh's ecc parity is stored in the internal hidden area and is
> +not needed for them. */
ECC an
"needed" is wrong here. Just stop after "area"
> + region->length = 0;
> + region->offset = mtd->oobsize;
> +
> + return 0;
> +}
> +
> +static int skyhigh_spinand_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region) {
> + if (section)
> + return -ERANGE;
> +
> + region->length = mtd->oobsize - 2;
> + region->offset = 2;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops skyhigh_spinand_ooblayout = {
> + .ecc = skyhigh_spinand_ooblayout_ecc,
> + .free = skyhigh_spinand_ooblayout_free, };
> +
> +static int skyhigh_spinand_ecc_get_status(struct spinand_device *spinand,
> + u8 status)
> +{
> + /* SHM
> + * 00 : No bit-flip
> + * 01 : 1-2 errors corrected
> + * 10 : 3-6 errors corrected
> + * 11 : uncorrectable
> + */
Thanks for the comment but the switch case looks rather straightforward, it is self-sufficient in this case.
> +
> + switch (status & STATUS_ECC_MASK) {
> + case STATUS_ECC_NO_BITFLIPS:
> + return 0;
> +
> + case SKYHIGH_STATUS_ECC_1TO2_BITFLIPS:
> + return 2;
> +
> + case SKYHIGH_STATUS_ECC_3TO6_BITFLIPS:
> + return 6;
> +
> + case SKYHIGH_STATUS_ECC_UNCOR_ERROR:
> + return -EBADMSG;;
> +
> + default:
> + break;
I guess you can directly call return -EINVAL here?
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct spinand_info skyhigh_spinand_table[] = {
> + SPINAND_INFO("S35ML01G301",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x15),
> + NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
> + NAND_ECCREQ(6, 32),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + SPINAND_ON_DIE_ECC_MANDATORY,
> + SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
> + skyhigh_spinand_ecc_get_status)),
> + SPINAND_INFO("S35ML01G300",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14),
> + NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
> + NAND_ECCREQ(6, 32),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + SPINAND_ON_DIE_ECC_MANDATORY,
> + SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
> + skyhigh_spinand_ecc_get_status)),
> + SPINAND_INFO("S35ML02G300",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),
> + NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
> + NAND_ECCREQ(6, 32),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + SPINAND_ON_DIE_ECC_MANDATORY,
> + SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
> + skyhigh_spinand_ecc_get_status)),
> + SPINAND_INFO("S35ML04G300",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35),
> + NAND_MEMORG(1, 2048, 128, 64, 4096, 80, 2, 1, 1),
> + NAND_ECCREQ(6, 32),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + SPINAND_ON_DIE_ECC_MANDATORY,
> + SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
> + skyhigh_spinand_ecc_get_status)), };
> +
> +static int skyhigh_spinand_init(struct spinand_device *spinand) {
> + return spinand_write_reg_op(spinand, REG_BLOCK_LOCK,
> + SKYHIGH_CONFIG_PROTECT_EN);
Is this really relevant? Isn't there an API for the block lock mechanism?
[SHM] SHM device should be done ‘Config Protect Enable’ first for unlock.
I changed to use the 'spinand_lock_block' function instead of the 'spinand_write_reg_op' function.
> +}
> +
> +static const struct spinand_manufacturer_ops skyhigh_spinand_manuf_ops = {
> + .init = skyhigh_spinand_init,
> + };
> +
> +const struct spinand_manufacturer skyhigh_spinand_manufacturer = {
> + .id = SPINAND_MFR_SKYHIGH,
> + .name = "SkyHigh",
> + .chips = skyhigh_spinand_table,
> + .nchips = ARRAY_SIZE(skyhigh_spinand_table),
> + .ops = &skyhigh_spinand_manuf_ops,
> +};
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> old mode 100644 new mode 100755 index badb4c1ac079..0e135076df24
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -268,6 +268,7 @@ extern const struct spinand_manufacturer
> gigadevice_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;
> +extern const struct spinand_manufacturer
> +skyhigh_spinand_manufacturer;
> extern const struct spinand_manufacturer
> toshiba_spinand_manufacturer; extern const struct
> spinand_manufacturer winbond_spinand_manufacturer; extern const
> struct spinand_manufacturer xtx_spinand_manufacturer; @@ -312,6 +313,7
> @@ struct spinand_ecc_info {
>
> #define SPINAND_HAS_QE_BIT BIT(0)
> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> +#define SPINAND_ON_DIE_ECC_MANDATORY BIT(2) /* SHM */
If we go this route, then "mandatory" is not relevant here, we shall convey the fact that the on-die ECC engine cannot be disabled and as mentioned above, there are other impacts.
[SHM] Please elaborate in more specific what I should do.
>
> /**
> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine
> structure @@ -518,5 +520,6 @@ int spinand_match_and_init(struct
> spinand_device *spinand,
>
> int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
> int spinand_select_target(struct spinand_device *spinand, unsigned int
> target);
> +int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8
> +val);
>
> #endif /* __LINUX_MTD_SPINAND_H */
Thanks,
Miquèl
More information about the linux-mtd
mailing list