[PATCH 0/2] mtd: spinand: Add support for SkyHigh S35ML-3 family

[PATCH 0/2] mtd: spinand: Add support for SkyHigh S35ML-3 family

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

This series is successor of the patch submitted by KR Kim:
https://patchwork.ozlabs.org/project/linux-mtd/patch/20240820064547.5035-1-kr.kim@skyhighmemory.com/

Takahiro Kuwano (2):
  mtd: spinand: Introduce a way to avoid raw access
  mtd: spinand: Add support for SkyHigh S35ML-3 family

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

-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

SkyHigh spinand device has ECC enable bit in configuration register but
it must be always enabled. If ECC is disabled, read and write ops
results in undetermined state. For such devices, a way to avoid raw
access is needed.

Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
support raw access. Read and write page in raw mode for the device
returns error.

Checking and marking BBM need to be performed with ECC enabled to read
and write the BBM correctly.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/nand/spi/core.c | 12 ++++++++++++
 include/linux/mtd/spinand.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 4d76f9f71a0e..de04f6e7ce1e 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -608,6 +608,9 @@ static int spinand_read_page(struct spinand_device *spinand,
 	u8 status;
 	int ret;
 
+	if (req->mode == MTD_OPS_RAW && spinand->flags & SPINAND_NO_RAW_ACCESS)
+		return -ENOTSUPP;
+
 	ret = nand_ecc_prepare_io_req(nand, (struct nand_page_io_req *)req);
 	if (ret)
 		return ret;
@@ -639,6 +642,9 @@ static int spinand_write_page(struct spinand_device *spinand,
 	u8 status;
 	int ret;
 
+	if (req->mode == MTD_OPS_RAW && spinand->flags & SPINAND_NO_RAW_ACCESS)
+		return -ENOTSUPP;
+
 	ret = nand_ecc_prepare_io_req(nand, (struct nand_page_io_req *)req);
 	if (ret)
 		return ret;
@@ -902,6 +908,9 @@ static bool spinand_isbad(struct nand_device *nand, const struct nand_pos *pos)
 		.mode = MTD_OPS_RAW,
 	};
 
+	if (spinand->flags & SPINAND_NO_RAW_ACCESS)
+		req.mode = MTD_OPS_PLACE_OOB;
+
 	spinand_select_target(spinand, pos->target);
 	spinand_read_page(spinand, &req);
 	if (marker[0] != 0xff || marker[1] != 0xff)
@@ -938,6 +947,9 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
 	};
 	int ret;
 
+	if (spinand->flags & SPINAND_NO_RAW_ACCESS)
+		req.mode = MTD_OPS_PLACE_OOB;
+
 	ret = spinand_select_target(spinand, pos->target);
 	if (ret)
 		return ret;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 702e5fb13dae..5cf11005b41a 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -314,6 +314,7 @@ struct spinand_ecc_info {
 #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
 #define SPINAND_HAS_PROG_PLANE_SELECT_BIT		BIT(2)
 #define SPINAND_HAS_READ_PLANE_SELECT_BIT		BIT(3)
+#define SPINAND_NO_RAW_ACCESS				BIT(4)
 
 /**
  * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: spinand: Add support for SkyHigh S35ML-3 family

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

SkyHigh S35ML01G300, S35ML01G301, S35ML02G300, and S35ML04G300 are 1Gb,
2Gb, and 4Gb SLC SPI NAND flash family. This family of devices has
on-die ECC which parity bits are stored to hidden area. In this family
the on-die ECC cannot be disabled so raw access needs to be prevented.

Link: https://www.skyhighmemory.com/download/SPI_S35ML01_04G3_002_19205.pdf?v=P
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Signed-off-by: KR Kim <kr.kim@skyhighmemory.com>
---
 drivers/mtd/nand/spi/Makefile  |   2 +-
 drivers/mtd/nand/spi/core.c    |   1 +
 drivers/mtd/nand/spi/skyhigh.c | 143 +++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h    |   1 +
 4 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/spi/skyhigh.c

diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index 19cc77288ebb..1e61ab21893a 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 += 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
index de04f6e7ce1e..53a859a130ed 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1129,6 +1129,7 @@ static const struct spinand_manufacturer *spinand_manufacturers[] = {
 	&macronix_spinand_manufacturer,
 	&micron_spinand_manufacturer,
 	&paragon_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..209218cf85f4
--- /dev/null
+++ b/drivers/mtd/nand/spi/skyhigh.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 SkyHigh Memory Limited
+ *
+ * Author: Takahiro Kuwano <takahiro.kuwano@infineon.com>
+ * Co-Author: KR Kim <kr.kim@skyhighmemory.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)
+{
+	/* ECC bytes are stored in hidden area. */
+	return -ERANGE;
+}
+
+static int skyhigh_spinand_ooblayout_free(struct mtd_info *mtd, int section,
+					  struct mtd_oob_region *region)
+{
+	if (section)
+		return -ERANGE;
+
+	/* ECC bytes are stored in hidden area. Reserve 2 bytes for the BBM. */
+	region->offset = 2;
+	region->length = mtd->oobsize - 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)
+{
+	switch (status & STATUS_ECC_MASK) {
+	case STATUS_ECC_NO_BITFLIPS:
+		return 0;
+
+	case SKYHIGH_STATUS_ECC_UNCOR_ERROR:
+		return -EBADMSG;
+
+	case SKYHIGH_STATUS_ECC_1TO2_BITFLIPS:
+		return 2;
+
+	case SKYHIGH_STATUS_ECC_3TO6_BITFLIPS:
+		return 6;
+
+	default:
+		break;
+	}
+
+	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_NO_RAW_ACCESS,
+		     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_NO_RAW_ACCESS,
+		     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_NO_RAW_ACCESS,
+		     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_NO_RAW_ACCESS,
+		     SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
+				     skyhigh_spinand_ecc_get_status)),
+};
+
+static int skyhigh_spinand_init(struct spinand_device *spinand)
+{
+	/* A0[1] must be enabled before block unlock */
+	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK,
+				    SKYHIGH_CONFIG_PROTECT_EN);
+}
+
+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
index 5cf11005b41a..cbbcd44ac225 100644
--- 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;
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/2] mtd: spinand: Add support for SkyHigh S35ML-3 family

[Miquel Raynal]

On 31/10/2024 at 11:21:53 +09, tkuw584924@gmail.com wrote:

> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> This series is successor of the patch submitted by KR Kim:
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20240820064547.5035-1-kr.kim@skyhighmemory.com/

Please append a changelog next time.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spinand: Add support for SkyHigh S35ML-3 family

[Miquel Raynal]

On 31/10/2024 at 11:21:55 +09, tkuw584924@gmail.com wrote:

> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> SkyHigh S35ML01G300, S35ML01G301, S35ML02G300, and S35ML04G300 are 1Gb,
> 2Gb, and 4Gb SLC SPI NAND flash family. This family of devices has
> on-die ECC which parity bits are stored to hidden area. In this family
> the on-die ECC cannot be disabled so raw access needs to be prevented.
>
> Link: https://www.skyhighmemory.com/download/SPI_S35ML01_04G3_002_19205.pdf?v=P
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Signed-off-by: KR Kim <kr.kim@skyhighmemory.com>

Please have a look to the documentation regarding how to document
authorship, because currently the tags are wrong.

...

> +static int skyhigh_spinand_init(struct spinand_device *spinand)
> +{
> +	/* A0[1] must be enabled before block unlock */

What is A0[1] ? It does not mean anything here, please be more explicit.

> +	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK,
> +				    SKYHIGH_CONFIG_PROTECT_EN);
> +}
> +
> +static const struct spinand_manufacturer_ops skyhigh_spinand_manuf_ops = {
> +	.init = skyhigh_spinand_init,
> +};

Otherwise this looks goot do me.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Miquel Raynal]

Hi,

On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote:

> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> SkyHigh spinand device has ECC enable bit in configuration register but
> it must be always enabled. If ECC is disabled, read and write ops
> results in undetermined state. For such devices, a way to avoid raw
> access is needed.
>
> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
> support raw access. Read and write page in raw mode for the device
> returns error.
>
> Checking and marking BBM need to be performed with ECC enabled to read
> and write the BBM correctly.

I see your point but I'm a bit puzzled by how it's being done.

First, you disregard completely the isbad() and markbad()
situations. Please have look into that because these functions are
broken with your devices.

Second, what about adding this detail to the ondie ECC engine? You could
simply return an error from there, so basically a single (or maybe two)
changes overall.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Takahiro Kuwano]

Hi,

On 11/12/2024 3:54 AM, Miquel Raynal wrote:
> Hi,
> 
> On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote:
> 
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> SkyHigh spinand device has ECC enable bit in configuration register but
>> it must be always enabled. If ECC is disabled, read and write ops
>> results in undetermined state. For such devices, a way to avoid raw
>> access is needed.
>>
>> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
>> support raw access. Read and write page in raw mode for the device
>> returns error.
>>
>> Checking and marking BBM need to be performed with ECC enabled to read
>> and write the BBM correctly.
> 
> I see your point but I'm a bit puzzled by how it's being done.
> 
> First, you disregard completely the isbad() and markbad()
> situations. Please have look into that because these functions are
> broken with your devices.
> 
Yes... now I understand how they are broken.

> Second, what about adding this detail to the ondie ECC engine? You could
> simply return an error from there, so basically a single (or maybe two)
> changes overall.
> 
Thank you for the suggestion.
How about change like below in prepare_finish_io_req()?

static int spinand_ondie_ecc_prepare_io_req(struct nand_device *nand,
					    struct nand_page_io_req *req)
{
	struct spinand_device *spinand = nand_to_spinand(nand);
	bool enable = (req->mode != MTD_OPS_RAW);

+	/*
+	 * For the devices that prohibit raw access (on-die ECC must be always
+	 * enabled), return error in case of raw data access. Accessing to OOB
+	 * needs to be allowed with on-die ECC enabled to support BBM checking
+	 * and marking.
+	 */
+	if (!enable && spinand->flags & SPINAND_NO_RAW_ACCESS) {
+		if (req->datalen)
+			return -ENOTSUPP;
+
+		enable = true;
+	}
...

> Thanks,
> Miquèl

Thanks!
Takahiro

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Miquel Raynal]

On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924@gmail.com> wrote:

> Hi,
>
> On 11/12/2024 3:54 AM, Miquel Raynal wrote:
>> Hi,
>> 
>> On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote:
>> 
>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>
>>> SkyHigh spinand device has ECC enable bit in configuration register but
>>> it must be always enabled. If ECC is disabled, read and write ops
>>> results in undetermined state. For such devices, a way to avoid raw
>>> access is needed.
>>>
>>> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
>>> support raw access. Read and write page in raw mode for the device
>>> returns error.
>>>
>>> Checking and marking BBM need to be performed with ECC enabled to read
>>> and write the BBM correctly.
>> 
>> I see your point but I'm a bit puzzled by how it's being done.
>> 
>> First, you disregard completely the isbad() and markbad()
>> situations. Please have look into that because these functions are
>> broken with your devices.
>> 
> Yes... now I understand how they are broken.
>
>> Second, what about adding this detail to the ondie ECC engine? You could
>> simply return an error from there, so basically a single (or maybe two)
>> changes overall.
>> 
> Thank you for the suggestion.
> How about change like below in prepare_finish_io_req()?
>
> static int spinand_ondie_ecc_prepare_io_req(struct nand_device *nand,
> 					    struct nand_page_io_req *req)
> {
> 	struct spinand_device *spinand = nand_to_spinand(nand);
> 	bool enable = (req->mode != MTD_OPS_RAW);
>
> +	/*
> +	 * For the devices that prohibit raw access (on-die ECC must be always
> +	 * enabled), return error in case of raw data access. Accessing to OOB
> +	 * needs to be allowed with on-die ECC enabled to support BBM checking
> +	 * and marking.
> +	 */
> +	if (!enable && spinand->flags & SPINAND_NO_RAW_ACCESS) {
> +		if (req->datalen)
> +			return -ENOTSUPP;

EOPNOTSUPP

> +
> +		enable = true;
> +	}

No, this is lying, I don't like that.

What about just returnint an error here if you cannot do what the upper
layer has asked you to do. And in the upper layer, you can add two
specific conditions to is_bad/mark_bad() allowing to fallback to a "with
ECC" operation in the EOPNOTSUPP case (with a short comment).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Takahiro Kuwano]

On 11/13/2024 6:11 PM, Miquel Raynal wrote:
> On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924@gmail.com> wrote:
> 
>> Hi,
>>
>> On 11/12/2024 3:54 AM, Miquel Raynal wrote:
>>> Hi,
>>>
>>> On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote:
>>>
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>
>>>> SkyHigh spinand device has ECC enable bit in configuration register but
>>>> it must be always enabled. If ECC is disabled, read and write ops
>>>> results in undetermined state. For such devices, a way to avoid raw
>>>> access is needed.
>>>>
>>>> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
>>>> support raw access. Read and write page in raw mode for the device
>>>> returns error.
>>>>
>>>> Checking and marking BBM need to be performed with ECC enabled to read
>>>> and write the BBM correctly.
>>>
>>> I see your point but I'm a bit puzzled by how it's being done.
>>>
>>> First, you disregard completely the isbad() and markbad()
>>> situations. Please have look into that because these functions are
>>> broken with your devices.
>>>
>> Yes... now I understand how they are broken.
>>
>>> Second, what about adding this detail to the ondie ECC engine? You could
>>> simply return an error from there, so basically a single (or maybe two)
>>> changes overall.
>>>
>> Thank you for the suggestion.
>> How about change like below in prepare_finish_io_req()?
>>
>> static int spinand_ondie_ecc_prepare_io_req(struct nand_device *nand,
>> 					    struct nand_page_io_req *req)
>> {
>> 	struct spinand_device *spinand = nand_to_spinand(nand);
>> 	bool enable = (req->mode != MTD_OPS_RAW);
>>
>> +	/*
>> +	 * For the devices that prohibit raw access (on-die ECC must be always
>> +	 * enabled), return error in case of raw data access. Accessing to OOB
>> +	 * needs to be allowed with on-die ECC enabled to support BBM checking
>> +	 * and marking.
>> +	 */
>> +	if (!enable && spinand->flags & SPINAND_NO_RAW_ACCESS) {
>> +		if (req->datalen)
>> +			return -ENOTSUPP;
> 
> EOPNOTSUPP
> 
>> +
>> +		enable = true;
>> +	}
> 
> No, this is lying, I don't like that.
> 
> What about just returnint an error here if you cannot do what the upper
> layer has asked you to do. And in the upper layer, you can add two
> specific conditions to is_bad/mark_bad() allowing to fallback to a "with
> ECC" operation in the EOPNOTSUPP case (with a short comment).
> 
OK, I will do it in next revision.
Thanks again.

BTW, spinand_write_enable_op() in spinand_markbad() looks redundant as it is
called in spinand_write_page(). Let me remove it before adding fallback.

> Thanks,
> Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/