[PATCH v2 0/4] mtd: spi-nor: Fix software reset; add mx66lm1g45g

[PATCH v2 0/4] mtd: spi-nor: Fix software reset; add mx66lm1g45g

[Tudor Ambarus]

Add support for mx66lm1g45g:
- Introduce SPI_NOR_SOFT_RESET flash_info flag -> the flash does not
define the SFDP tables and can't determine its reset sequence on its
own.
- Fix the opcode extension for the software reset sequence -> it
was hardcoded.

Changes in v2: revert soft reset on boot.

Tudor Ambarus (4):
  mtd: spi-nor-core: Introduce SPI_NOR_SOFT_RESET flash_info flag
  mtd: spi-nor-core: macronix: Add support for mx66lm1g45g
  Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"
  mtd: spi-nor-core: Fix the opcode extension for the software reset
    sequence

 drivers/mtd/spi/Kconfig        |  17 +++--
 drivers/mtd/spi/sf_internal.h  |   5 ++
 drivers/mtd/spi/spi-nor-core.c | 116 +++++++++++++++++++++++----------
 drivers/mtd/spi/spi-nor-ids.c  |   6 ++
 include/linux/mtd/spi-nor.h    |   8 +++
 5 files changed, 107 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] mtd: spi-nor-core: Introduce SPI_NOR_SOFT_RESET flash_info flag

[Tudor Ambarus]

Some flashes can determine if Soft Reset is supported by parsing
BFPT_DWORD(16). There are however flashes that do not define any of
the SFDP tables, so they can't discover this capability at SFDP
parsing time. For such cases introduce the SPI_NOR_SOFT_RESET
flash_info flag to be able to specify statically this support.
This flag must be used together with the SPI_NOR_SKIP_SFDP flag.
For flashes that support Soft Reset and define BFPT, but have
the BFPT_DWORD(16) missing or with a wrong value, a post_sfdp()
fixup hook should be used instead, where SNOR_F_SOFT_RESET should
be set.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi/sf_internal.h  | 5 +++++
 drivers/mtd/spi/spi-nor-core.c | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index d3ef69ec74..9a16e07bc4 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -69,6 +69,11 @@ struct flash_info {
 #define SPI_NOR_HAS_SST26LOCK	BIT(15)	/* Flash supports lock/unlock via BPR */
 #define SPI_NOR_OCTAL_READ	BIT(16)	/* Flash supports Octal Read */
 #define SPI_NOR_OCTAL_DTR_READ	BIT(17)	/* Flash supports Octal DTR Read */
+#define SPI_NOR_SOFT_RESET	BIT(18)	/*
+					 * Used by flashes that do not define
+					 * any SFDP tables, i.e. in conjunction
+					 * with SPI_NOR_SKIP_SFDP.
+					 */
 };
 
 extern const struct flash_info spi_nor_ids[];
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 4388a08a90..ce1ecae899 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3813,6 +3813,9 @@ int spi_nor_scan(struct spi_nor *nor)
 	if (info->flags & SPI_NOR_NO_ERASE)
 		mtd->flags |= MTD_NO_ERASE;
 
+	if (info->flags & SPI_NOR_SOFT_RESET)
+		nor->flags |= SNOR_F_SOFT_RESET;
+
 	nor->page_size = params.page_size;
 	mtd->writebufsize = nor->page_size;
 
-- 
2.25.1

Re: [PATCH v2 1/4] mtd: spi-nor-core: Introduce SPI_NOR_SOFT_RESET flash_info flag

[Pratyush Yadav]

On 04/11/21 01:49AM, Tudor Ambarus wrote:
> Some flashes can determine if Soft Reset is supported by parsing
> BFPT_DWORD(16). There are however flashes that do not define any of
> the SFDP tables, so they can't discover this capability at SFDP
> parsing time. For such cases introduce the SPI_NOR_SOFT_RESET
> flash_info flag to be able to specify statically this support.
> This flag must be used together with the SPI_NOR_SKIP_SFDP flag.
> For flashes that support Soft Reset and define BFPT, but have
> the BFPT_DWORD(16) missing or with a wrong value, a post_sfdp()
> fixup hook should be used instead, where SNOR_F_SOFT_RESET should
> be set.

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 2/4] mtd: spi-nor-core: macronix: Add support for mx66lm1g45g

[Tudor Ambarus]

mx66lm1g45g supports just 1-1-1, 8-8-8 and 8d-8d-8d modes.
Tested in 1-1-1 and 8d-8d-8d modes using microchip's Octal SPI IP.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi/Kconfig        |  8 ++++
 drivers/mtd/spi/spi-nor-core.c | 74 ++++++++++++++++++++++++++++++++++
 drivers/mtd/spi/spi-nor-ids.c  |  6 +++
 include/linux/mtd/spi-nor.h    |  8 ++++
 4 files changed, 96 insertions(+)

diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
index 408a53f861..2d2b71c52d 100644
--- a/drivers/mtd/spi/Kconfig
+++ b/drivers/mtd/spi/Kconfig
@@ -160,6 +160,14 @@ config SPI_FLASH_MACRONIX
 	help
 	  Add support for various Macronix SPI flash chips (MX25Lxxx)
 
+config SPI_FLASH_MX66LM1G45G
+	bool "Macronix MX66LM1G45G chip support"
+	depends on SPI_FLASH_MACRONIX
+	help
+	  Add support for the Macronix mx66lm1g45g chip. This is a separate
+	  config because the fixup hooks for this flash add extra size overhead.
+	  Boards that don't use the flash can disable this to save space.
+
 config SPI_FLASH_SPANSION
 	bool "Spansion SPI flash support"
 	help
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index ce1ecae899..df66a73495 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3526,6 +3526,75 @@ static struct spi_nor_fixups mt35xu512aba_fixups = {
 };
 #endif /* CONFIG_SPI_FLASH_MT35XU */
 
+#ifdef CONFIG_SPI_FLASH_MX66LM1G45G
+static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 buf;
+	int ret;
+
+	/* Use 20 dummy cycles for memory array reads. */
+	buf = SPINOR_REG_CR2_DUMMY_20;
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRITE_CR2, 1),
+			   SPI_MEM_OP_ADDR(4, SPINOR_REG_CR2_DUMMY_ADDR,
+					   1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, &buf, 1));
+
+	ret = write_enable(nor);
+	if (ret)
+		return ret;
+
+	ret = spi_mem_exec_op(nor->spi, &op);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	buf = SPINOR_REG_CR2_DTR_OPI_ENABLE;
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRITE_CR2, 1),
+			   SPI_MEM_OP_ADDR(4, SPINOR_REG_CR2_MODE_ADDR, 1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, &buf, 1));
+
+	ret = write_enable(nor);
+	if (ret)
+		return ret;
+
+	return spi_mem_exec_op(nor->spi, &op);
+}
+
+static void mx66lm1g45g_default_init(struct spi_nor *nor)
+{
+	nor->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
+}
+
+static void mx66lm1g45g_post_sfdp(struct spi_nor *nor,
+				  struct spi_nor_flash_parameter *params)
+{
+	/* Set the Fast Read settings. */
+	params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, 20, SPINOR_OP_MX_DTR_RD,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
+
+	nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+	params->rdsr_dummy = 4;
+	params->rdsr_addr_nbytes = 4;
+}
+
+static struct spi_nor_fixups mx66lm1g45g_fixups = {
+	.default_init = mx66lm1g45g_default_init,
+	.post_sfdp = mx66lm1g45g_post_sfdp,
+};
+#endif /* CONFIG_SPI_FLASH_MX66LM1G45G */
+
 /** spi_nor_octal_dtr_enable() - enable Octal DTR I/O if needed
  * @nor:                 pointer to a 'struct spi_nor'
  *
@@ -3696,6 +3765,11 @@ void spi_nor_set_fixups(struct spi_nor *nor)
 	if (!strcmp(nor->info->name, "mt35xu512aba"))
 		nor->fixups = &mt35xu512aba_fixups;
 #endif
+
+#ifdef CONFIG_SPI_FLASH_MX66LM1G45G
+	if (!strcmp(nor->info->name, "mx66lm1g45g"))
+		nor->fixups = &mx66lm1g45g_fixups;
+#endif
 }
 
 int spi_nor_scan(struct spi_nor *nor)
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index 3ae7bb1ed7..d5bc106e68 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -179,6 +179,12 @@ const struct flash_info spi_nor_ids[] = {
 	{ INFO("mx25l1633e", 0xc22415, 0, 64 * 1024,   32, SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | SECT_4K) },
 	{ INFO("mx25r6435f", 0xc22817, 0, 64 * 1024,   128,  SECT_4K) },
 	{ INFO("mx66uw2g345g", 0xc2943c, 0, 64 * 1024, 4096, SECT_4K | SPI_NOR_OCTAL_READ | SPI_NOR_4B_OPCODES) },
+#ifdef CONFIG_SPI_FLASH_MX66LM1G45G
+	{ INFO("mx66lm1g45g", 0xc2853b, 0, 64 * 1024, 2048,
+	       SPI_NOR_SKIP_SFDP | SPI_NOR_SOFT_RESET |
+	       SECT_4K | SPI_NOR_OCTAL_DTR_READ | SPI_NOR_4B_OPCODES)
+	},
+#endif
 #endif
 
 #ifdef CONFIG_SPI_FLASH_STMICRO		/* STMICRO */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4ceeae623d..c8b8a242ba 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -119,6 +119,14 @@
 /* Used for Macronix and Winbond flashes. */
 #define SPINOR_OP_EN4B		0xb7	/* Enter 4-byte mode */
 #define SPINOR_OP_EX4B		0xe9	/* Exit 4-byte mode */
+#define SPINOR_OP_READ_CR2		0x71
+#define SPINOR_OP_WRITE_CR2		0x72
+#define SPINOR_OP_MX_DTR_RD		0xee
+#define SPINOR_REG_CR2_MODE_ADDR	0
+#define SPINOR_REG_CR2_DTR_OPI_ENABLE	BIT(1)
+#define SPINOR_REG_CR2_SPI		0
+#define SPINOR_REG_CR2_DUMMY_ADDR	0x300
+#define SPINOR_REG_CR2_DUMMY_20		0
 
 /* Used for Spansion flashes only. */
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
-- 
2.25.1

Re: [PATCH v2 2/4] mtd: spi-nor-core: macronix: Add support for mx66lm1g45g

[Pratyush Yadav]

On 04/11/21 01:49AM, Tudor Ambarus wrote:
> mx66lm1g45g supports just 1-1-1, 8-8-8 and 8d-8d-8d modes.
> Tested in 1-1-1 and 8d-8d-8d modes using microchip's Octal SPI IP.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi/Kconfig        |  8 ++++
>  drivers/mtd/spi/spi-nor-core.c | 74 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi/spi-nor-ids.c  |  6 +++
>  include/linux/mtd/spi-nor.h    |  8 ++++
>  4 files changed, 96 insertions(+)
> 
> @@ -3696,6 +3765,11 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>  	if (!strcmp(nor->info->name, "mt35xu512aba"))
>  		nor->fixups = &mt35xu512aba_fixups;
>  #endif
> +
> +#ifdef CONFIG_SPI_FLASH_MX66LM1G45G
> +	if (!strcmp(nor->info->name, "mx66lm1g45g"))
> +		nor->fixups = &mx66lm1g45g_fixups;
> +#endif

*sigh* this is very ugly. Let's make it a bit better.

Takahiro, this shouldn't break the other Cypress flashes either, but I 
don't have them so I can't test them. A Tested-by would be nice.

-- 8< --
Subject: [PATCH] mtd: spi-nor-core: clean up how fixups are set

Currently fixups for flashes are set via the function
spi_nor_set_fixups(). This was done since tiny SPI NOR does not support
fixups so fixups could not be set directly in the flash_info entry.

The function identifies flashes by either strcmp()ing their name or
checking their ID bytes. Neither approach is not very elegant and does
not scale very well when there are multiple flashes specifying the
fixups.

Add a macro that conditionally sets the fixups based on if tiny SPI NOR
is enabled or not. Also, move the fixups from struct spi_nor to
flash_info since they make more sense there. The fixups are defined in
spi-nor-core.c so they need to be exported, so they are no longer
static.

Tested with Cypress S28HS512T and Micron MT35XU512ABA.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi/sf_internal.h  |  4 +++
 drivers/mtd/spi/spi-nor-core.c | 57 ++++++++--------------------------
 drivers/mtd/spi/spi-nor-ids.c  | 42 +++++++++++++++++++------
 include/linux/mtd/spi-nor.h    |  1 -
 4 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index d3ef69ec74..2e251c1d63 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -69,6 +69,10 @@ struct flash_info {
 #define SPI_NOR_HAS_SST26LOCK	BIT(15)	/* Flash supports lock/unlock via BPR */
 #define SPI_NOR_OCTAL_READ	BIT(16)	/* Flash supports Octal Read */
 #define SPI_NOR_OCTAL_DTR_READ	BIT(17)	/* Flash supports Octal DTR Read */
+
+#if !CONFIG_IS_ENABLED(SPI_FLASH_TINY)
+	const struct spi_nor_fixups	*fixups;
+#endif
 };
 
 extern const struct flash_info spi_nor_ids[];
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 4388a08a90..dd3d9ea4cf 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -2139,8 +2139,9 @@ spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_bfpt *bfpt,
 			 struct spi_nor_flash_parameter *params)
 {
-	if (nor->fixups && nor->fixups->post_bfpt)
-		return nor->fixups->post_bfpt(nor, bfpt_header, bfpt, params);
+	if (nor->info->fixups && nor->info->fixups->post_bfpt)
+		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
+						    params);
 
 	return 0;
 }
@@ -2605,14 +2606,14 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 static void spi_nor_post_sfdp_fixups(struct spi_nor *nor,
 				     struct spi_nor_flash_parameter *params)
 {
-	if (nor->fixups && nor->fixups->post_sfdp)
-		nor->fixups->post_sfdp(nor, params);
+	if (nor->info->fixups && nor->info->fixups->post_sfdp)
+		nor->info->fixups->post_sfdp(nor, params);
 }
 
 static void spi_nor_default_init_fixups(struct spi_nor *nor)
 {
-	if (nor->fixups && nor->fixups->default_init)
-		nor->fixups->default_init(nor);
+	if (nor->info->fixups && nor->info->fixups->default_init)
+		nor->info->fixups->default_init(nor);
 }
 
 static int spi_nor_init_params(struct spi_nor *nor,
@@ -3239,7 +3240,7 @@ static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
 	params->quad_enable = s25hx_t_quad_enable;
 }
 
-static struct spi_nor_fixups s25hx_t_fixups = {
+struct spi_nor_fixups s25hx_t_fixups = {
 	.default_init = s25hx_t_default_init,
 	.post_bfpt = s25hx_t_post_bfpt_fixup,
 	.post_sfdp = s25hx_t_post_sfdp_fixup,
@@ -3253,10 +3254,11 @@ static int s25fl256l_setup(struct spi_nor *nor, const struct flash_info *info,
 
 static void s25fl256l_default_init(struct spi_nor *nor)
 {
-	nor->setup = s25fl256l_setup;
+	if (CONFIG_IS_ENABLED(SPI_FLASH_BAR))
+		nor->setup = s25fl256l_setup;
 }
 
-static struct spi_nor_fixups s25fl256l_fixups = {
+struct spi_nor_fixups s25fl256l_fixups = {
 	.default_init = s25fl256l_default_init,
 };
 #endif
@@ -3437,7 +3439,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
 	return 0;
 }
 
-static struct spi_nor_fixups s28hs512t_fixups = {
+struct spi_nor_fixups s28hs512t_fixups = {
 	.default_init = s28hs512t_default_init,
 	.post_sfdp = s28hs512t_post_sfdp_fixup,
 	.post_bfpt = s28hs512t_post_bfpt_fixup,
@@ -3520,7 +3522,7 @@ static void mt35xu512aba_post_sfdp_fixup(struct spi_nor *nor,
 	params->quad_enable = NULL;
 }
 
-static struct spi_nor_fixups mt35xu512aba_fixups = {
+struct spi_nor_fixups mt35xu512aba_fixups = {
 	.default_init = mt35xu512aba_default_init,
 	.post_sfdp = mt35xu512aba_post_sfdp_fixup,
 };
@@ -3667,37 +3669,6 @@ int spi_nor_remove(struct spi_nor *nor)
 	return 0;
 }
 
-void spi_nor_set_fixups(struct spi_nor *nor)
-{
-#ifdef CONFIG_SPI_FLASH_SPANSION
-	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
-		switch (nor->info->id[1]) {
-		case 0x2a: /* S25HL (QSPI, 3.3V) */
-		case 0x2b: /* S25HS (QSPI, 1.8V) */
-			nor->fixups = &s25hx_t_fixups;
-			break;
-
-		default:
-			break;
-		}
-	}
-
-	if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) &&
-	    !strcmp(nor->info->name, "s25fl256l"))
-		nor->fixups = &s25fl256l_fixups;
-#endif
-
-#ifdef CONFIG_SPI_FLASH_S28HS512T
-	if (!strcmp(nor->info->name, "s28hs512t"))
-		nor->fixups = &s28hs512t_fixups;
-#endif
-
-#ifdef CONFIG_SPI_FLASH_MT35XU
-	if (!strcmp(nor->info->name, "mt35xu512aba"))
-		nor->fixups = &mt35xu512aba_fixups;
-#endif
-}
-
 int spi_nor_scan(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter params;
@@ -3754,8 +3725,6 @@ int spi_nor_scan(struct spi_nor *nor)
 		return -ENOENT;
 	nor->info = info;
 
-	spi_nor_set_fixups(nor);
-
 	/* Parse the Serial Flash Discoverable Parameters table. */
 	ret = spi_nor_init_params(nor, info, &params);
 	if (ret)
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index 3ae7bb1ed7..f063209440 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -19,6 +19,25 @@
 #define INFO_NAME(_name)
 #endif
 
+/* Fixups are not implemented by tiny SPI NOR. Exclude them. */
+#if !CONFIG_IS_ENABLED(SPI_FLASH_TINY)
+#define FIXUPS(_fixups) .fixups = _fixups,
+#else
+#define FIXUPS(_fixups)
+#endif
+
+#ifdef CONFIG_SPI_FLASH_SPANSION
+extern struct spi_nor_fixups s25hx_t_fixups, s25fl256l_fixups;
+#endif
+
+#ifdef CONFIG_SPI_FLASH_S28HS512T
+extern struct spi_nor_fixups s28hs512t_fixups;
+#endif
+
+#ifdef CONFIG_SPI_FLASH_MT35XU
+extern struct spi_nor_fixups mt35xu512aba_fixups;
+#endif
+
 /* Used when the "_ext_id" is two bytes at most */
 #define INFO(_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
 		INFO_NAME(_name)					\
@@ -206,7 +225,8 @@ const struct flash_info spi_nor_ids[] = {
 	{ INFO("mt25qu02g",   0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
 	{ INFO("mt25ql02g",   0x20ba22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE | SPI_NOR_4B_OPCODES) },
 #ifdef CONFIG_SPI_FLASH_MT35XU
-	{ INFO("mt35xu512aba", 0x2c5b1a, 0,  128 * 1024,  512, USE_FSR | SPI_NOR_OCTAL_READ | SPI_NOR_4B_OPCODES | SPI_NOR_OCTAL_DTR_READ) },
+	{ INFO("mt35xu512aba", 0x2c5b1a, 0,  128 * 1024,  512, USE_FSR | SPI_NOR_OCTAL_READ | SPI_NOR_4B_OPCODES | SPI_NOR_OCTAL_DTR_READ)
+	  FIXUPS(&mt35xu512aba_fixups) },
 #endif /* CONFIG_SPI_FLASH_MT35XU */
 	{ INFO("mt35xu02g",  0x2c5b1c, 0, 128 * 1024,  2048, USE_FSR | SPI_NOR_OCTAL_READ | SPI_NOR_4B_OPCODES) },
 #endif
@@ -237,25 +257,29 @@ const struct flash_info spi_nor_ids[] = {
 	{ INFO("s25fl208k",  0x014014,      0,  64 * 1024,  16, SECT_4K | SPI_NOR_DUAL_READ) },
 	{ INFO("s25fl064l",  0x016017,      0,  64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ INFO("s25fl128l",  0x016018,      0,  64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
-	{ INFO("s25fl256l",  0x016019,      0,  64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+	{ INFO("s25fl256l",  0x016019,      0,  64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) FIXUPS(&s25fl256l_fixups) },
 	{ INFO6("s25hl512t",  0x342a1a, 0x0f0390, 256 * 1024, 256,
 		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
-		USE_CLSR) },
+		USE_CLSR) FIXUPS(&s25hx_t_fixups) },
 	{ INFO6("s25hl01gt",  0x342a1b, 0x0f0390, 256 * 1024, 512,
 		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
-		USE_CLSR) },
+		USE_CLSR) FIXUPS(&s25hx_t_fixups) },
 	{ INFO6("s25hl02gt",  0x342a1c, 0x0f0090, 256 * 1024, 1024,
-		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
+	  FIXUPS(&s25hx_t_fixups) },
 	{ INFO6("s25hs512t",  0x342b1a, 0x0f0390, 256 * 1024, 256,
 		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
-		USE_CLSR) },
+		USE_CLSR)
+	  FIXUPS(&s25hx_t_fixups) },
 	{ INFO6("s25hs01gt",  0x342b1b, 0x0f0390, 256 * 1024, 512,
 		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
-		USE_CLSR) },
+		USE_CLSR)
+	  FIXUPS(&s25hx_t_fixups) },
 	{ INFO6("s25hs02gt",  0x342b1c, 0x0f0090, 256 * 1024, 1024,
-		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
+	  FIXUPS(&s25hx_t_fixups) },
 #ifdef CONFIG_SPI_FLASH_S28HS512T
-	{ INFO("s28hs512t",  0x345b1a,      0, 256 * 1024, 256, SPI_NOR_OCTAL_DTR_READ) },
+	{ INFO("s28hs512t",  0x345b1a,      0, 256 * 1024, 256, SPI_NOR_OCTAL_DTR_READ) FIXUPS(&s28hs512t_fixups) },
 #endif
 #endif
 #ifdef CONFIG_SPI_FLASH_SST		/* SST */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4ceeae623d..7afb8396d8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -539,7 +539,6 @@ struct spi_nor {
 	u32			flags;
 	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
 	enum spi_nor_cmd_ext	cmd_ext_type;
-	struct spi_nor_fixups	*fixups;
 
 	int (*setup)(struct spi_nor *nor, const struct flash_info *info,
 		     const struct spi_nor_flash_parameter *params);
-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 2/4] mtd: spi-nor-core: macronix: Add support for mx66lm1g45g

[Jagan Teki]

On Thu, Nov 4, 2021 at 5:20 AM Tudor Ambarus
<tudor.ambarus@microchip.com> wrote:
>
> mx66lm1g45g supports just 1-1-1, 8-8-8 and 8d-8d-8d modes.
> Tested in 1-1-1 and 8d-8d-8d modes using microchip's Octal SPI IP.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi/Kconfig        |  8 ++++
>  drivers/mtd/spi/spi-nor-core.c | 74 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi/spi-nor-ids.c  |  6 +++
>  include/linux/mtd/spi-nor.h    |  8 ++++
>  4 files changed, 96 insertions(+)
>
> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> index 408a53f861..2d2b71c52d 100644
> --- a/drivers/mtd/spi/Kconfig
> +++ b/drivers/mtd/spi/Kconfig
> @@ -160,6 +160,14 @@ config SPI_FLASH_MACRONIX
>         help
>           Add support for various Macronix SPI flash chips (MX25Lxxx)
>
> +config SPI_FLASH_MX66LM1G45G
> +       bool "Macronix MX66LM1G45G chip support"

Apart from actual code, I don't like to have specific part configs,
look like list is increasing like anything. Better use existing
macronix.

Jagan.

Re: [PATCH v2 2/4] mtd: spi-nor-core: macronix: Add support for mx66lm1g45g

[Tudor.Ambarus]

On 11/13/21 3:48 PM, Jagan Teki wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Nov 4, 2021 at 5:20 AM Tudor Ambarus
> <tudor.ambarus@microchip.com> wrote:
>>
>> mx66lm1g45g supports just 1-1-1, 8-8-8 and 8d-8d-8d modes.
>> Tested in 1-1-1 and 8d-8d-8d modes using microchip's Octal SPI IP.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi/Kconfig        |  8 ++++
>>  drivers/mtd/spi/spi-nor-core.c | 74 ++++++++++++++++++++++++++++++++++
>>  drivers/mtd/spi/spi-nor-ids.c  |  6 +++
>>  include/linux/mtd/spi-nor.h    |  8 ++++
>>  4 files changed, 96 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
>> index 408a53f861..2d2b71c52d 100644
>> --- a/drivers/mtd/spi/Kconfig
>> +++ b/drivers/mtd/spi/Kconfig
>> @@ -160,6 +160,14 @@ config SPI_FLASH_MACRONIX
>>         help
>>           Add support for various Macronix SPI flash chips (MX25Lxxx)
>>
>> +config SPI_FLASH_MX66LM1G45G
>> +       bool "Macronix MX66LM1G45G chip support"
> 
> Apart from actual code, I don't like to have specific part configs,
> look like list is increasing like anything. Better use existing
> macronix.
> 

I don't like it either, I just followed the u-boot code. Will come up
with something.

Cheers,
ta

[PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Tudor Ambarus]

This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.

There are multiple reset commands and it was used one at guess,
see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
commands to a flash. Since there's no config in mainline that actually
uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
support is added. One should instead determine the mode in which the
flash device is configured, then to parse SFDP to determine the
cmd_ext_type and then to issue a READID command if flash identification
is really a must. JESD216D-01 proposes an algorithm to try to read the
SFDP signature to determine the mode in which the flash is configured:
'''
try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
these typically support SFDP read operation in both 1S-1S-1S mode and
8D-8D-8D mode. If the host controller does not know exactly which
protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
directly in 8D-8D-8D mode, the host controller may read from address 0,
and count the number of dummy clocks required before the SFDP signature
is received.)
'''
If the flash does not support SFDP at all, one should introduce dedicated
configs for each reset type and issue just the needed reset command.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi/Kconfig        |  9 ---------
 drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
 2 files changed, 36 deletions(-)

diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
index 2d2b71c52d..14800b5e93 100644
--- a/drivers/mtd/spi/Kconfig
+++ b/drivers/mtd/spi/Kconfig
@@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
 	 Enable support for xSPI Software Reset. It will be used to switch from
 	 Octal DTR mode to legacy mode on shutdown and boot (if enabled).
 
-config SPI_FLASH_SOFT_RESET_ON_BOOT
-	bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
-	depends on SPI_FLASH_SOFT_RESET
-	help
-	 Perform a Software Reset on boot to allow detecting flashes that are
-	 handed to us in Octal DTR mode. Do not enable this config on flashes
-	 that are not supposed to be handed to U-Boot in Octal DTR mode, even
-	 if they _do_ support the Soft Reset sequence.
-
 config SPI_FLASH_BAR
 	bool "SPI flash Bank/Extended address register support"
 	help
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index df66a73495..caf764720c 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
 
 	nor->setup = spi_nor_default_setup;
 
-#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
-	/*
-	 * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
-	 * is difficult to detect the mode the flash is in. One option is to
-	 * read SFDP in all modes and see which one gives the correct "SFDP"
-	 * signature, but not all flashes support SFDP in 8D-8D-8D mode.
-	 *
-	 * Further, even if you detect the mode of the flash via SFDP, you
-	 * still have the problem of actually reading the ID. The Read ID
-	 * command is not standardized across flash vendors. Flashes can have
-	 * different dummy cycles needed for reading the ID. Some flashes even
-	 * expect a 4-byte dummy address with the Read ID command. All this
-	 * information cannot be obtained from the SFDP table.
-	 *
-	 * So, perform a Software Reset sequence before reading the ID and
-	 * initializing the flash. A Soft Reset will bring back the flash in
-	 * its default protocol mode assuming no non-volatile configuration was
-	 * set. This will let us detect the flash even if ROM hands it to us in
-	 * Octal DTR mode.
-	 *
-	 * To accommodate cases where there is more than one flash on a board,
-	 * and only one of them needs a soft reset, failure to reset is not
-	 * made fatal, and we still try to read ID if possible.
-	 */
-	spi_nor_soft_reset(nor);
-#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
-
 	info = spi_nor_read_id(nor);
 	if (IS_ERR_OR_NULL(info))
 		return -ENOENT;
-- 
2.25.1

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Pratyush Yadav]

Hi Tudor,

On 04/11/21 01:49AM, Tudor Ambarus wrote:
> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
> 
> There are multiple reset commands and it was used one at guess,

Correct.

> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
> commands to a flash. Since there's no config in mainline that actually
> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper

I have been lagging behind on that front. I have some defconfig patches 
for TI platforms but I did not get around to cleaning them up and 
posting them upstream. Still, this feature is very much needed on TI 
platforms.

> support is added. One should instead determine the mode in which the
> flash device is configured, then to parse SFDP to determine the
> cmd_ext_type and then to issue a READID command if flash identification
> is really a must. JESD216D-01 proposes an algorithm to try to read the

Firstly, Read ID command is not standardized in 8D-8D-8D mode. For 
example, Cypress S28 flash family expects 4 address bytes for Read ID in 
8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy 
cycles also tends to vary. There is no way to determine these parameters 
from SFDP.

Also, you would have to run the detection algorithm for _every_ flash 
that SPI NOR supports since we don't really know what we are dealing 
with at this point. This would include flashes that do not support the 
Read SFDP command at all. If your goal is to not send unsupported 
commands to a flash then you won't get very far. On top of that Read 
SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is 
no guarantee that the detection algorithm would even work. It _should_ 
work for most flashes but we will eventually have to deal with the 
corner cases.

In other words, if you drop this then you would have to run the 
detection algorithm for every flash and see what mode it is in. If an 
8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you 
would have to read SFDP, determine the extension and reset type, and 
then perform the reset. If it does not support the Read SFDP command 
then you are left with a non-working flash. You would still probably end 
up issuing unsupported commands.

I can implement such an algorithm but is it really worth the hassle?

> SFDP signature to determine the mode in which the flash is configured:
> '''
> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
> these typically support SFDP read operation in both 1S-1S-1S mode and
> 8D-8D-8D mode. If the host controller does not know exactly which
> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
> directly in 8D-8D-8D mode, the host controller may read from address 0,
> and count the number of dummy clocks required before the SFDP signature
> is received.)
> '''
> If the flash does not support SFDP at all, one should introduce dedicated
> configs for each reset type and issue just the needed reset command.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi/Kconfig        |  9 ---------
>  drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
>  2 files changed, 36 deletions(-)
> 
> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> index 2d2b71c52d..14800b5e93 100644
> --- a/drivers/mtd/spi/Kconfig
> +++ b/drivers/mtd/spi/Kconfig
> @@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
>  	 Enable support for xSPI Software Reset. It will be used to switch from
>  	 Octal DTR mode to legacy mode on shutdown and boot (if enabled).
>  
> -config SPI_FLASH_SOFT_RESET_ON_BOOT
> -	bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
> -	depends on SPI_FLASH_SOFT_RESET
> -	help
> -	 Perform a Software Reset on boot to allow detecting flashes that are
> -	 handed to us in Octal DTR mode. Do not enable this config on flashes
> -	 that are not supposed to be handed to U-Boot in Octal DTR mode, even
> -	 if they _do_ support the Soft Reset sequence.
> -
>  config SPI_FLASH_BAR
>  	bool "SPI flash Bank/Extended address register support"
>  	help
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index df66a73495..caf764720c 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
>  
>  	nor->setup = spi_nor_default_setup;
>  
> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
> -	/*
> -	 * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
> -	 * is difficult to detect the mode the flash is in. One option is to
> -	 * read SFDP in all modes and see which one gives the correct "SFDP"
> -	 * signature, but not all flashes support SFDP in 8D-8D-8D mode.
> -	 *
> -	 * Further, even if you detect the mode of the flash via SFDP, you
> -	 * still have the problem of actually reading the ID. The Read ID
> -	 * command is not standardized across flash vendors. Flashes can have
> -	 * different dummy cycles needed for reading the ID. Some flashes even
> -	 * expect a 4-byte dummy address with the Read ID command. All this
> -	 * information cannot be obtained from the SFDP table.
> -	 *
> -	 * So, perform a Software Reset sequence before reading the ID and
> -	 * initializing the flash. A Soft Reset will bring back the flash in
> -	 * its default protocol mode assuming no non-volatile configuration was
> -	 * set. This will let us detect the flash even if ROM hands it to us in
> -	 * Octal DTR mode.
> -	 *
> -	 * To accommodate cases where there is more than one flash on a board,
> -	 * and only one of them needs a soft reset, failure to reset is not
> -	 * made fatal, and we still try to read ID if possible.
> -	 */
> -	spi_nor_soft_reset(nor);
> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
> -
>  	info = spi_nor_read_id(nor);
>  	if (IS_ERR_OR_NULL(info))
>  		return -ENOENT;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Tudor.Ambarus]

On 11/9/21 9:26 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,

Hi, Pratyush,

Thanks for reviewing the series.

> 
> On 04/11/21 01:49AM, Tudor Ambarus wrote:
>> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
>>
>> There are multiple reset commands and it was used one at guess,
> 
> Correct.
> 
>> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
>> commands to a flash. Since there's no config in mainline that actually
>> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
> 
> I have been lagging behind on that front. I have some defconfig patches
> for TI platforms but I did not get around to cleaning them up and
> posting them upstream. Still, this feature is very much needed on TI
> platforms.
> 
>> support is added. One should instead determine the mode in which the
>> flash device is configured, then to parse SFDP to determine the
>> cmd_ext_type and then to issue a READID command if flash identification
>> is really a must. JESD216D-01 proposes an algorithm to try to read the
> 
> Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
> example, Cypress S28 flash family expects 4 address bytes for Read ID in
> 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
> cycles also tends to vary. There is no way to determine these parameters
> from SFDP.

If flash supports SFDP, the read ID becomes of minor importance. We can as
well not issue the read ID at all.

> 
> Also, you would have to run the detection algorithm for _every_ flash
> that SPI NOR supports since we don't really know what we are dealing
> with at this point. This would include flashes that do not support the
> Read SFDP command at all. If your goal is to not send unsupported

but we can have a dt property or a config option to indicate when to issue
readSFDP.

> commands to a flash then you won't get very far. On top of that Read
> SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
> no guarantee that the detection algorithm would even work. It _should_
> work for most flashes but we will eventually have to deal with the
> corner cases.

in case we can't determine the mode in which the flash is configured,
we can adopt other approach, see below.
> 
> In other words, if you drop this then you would have to run the
> detection algorithm for every flash and see what mode it is in. If an

not for every flash, just the ones that we marked as SFDP compliant.

> 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
> would have to read SFDP, determine the extension and reset type, and
> then perform the reset. If it does not support the Read SFDP command

right.

> then you are left with a non-working flash. You would still probably end
> up issuing unsupported commands.

not necessarily.

> 
> I can implement such an algorithm but is it really worth the hassle?

I find having such an alg would be of benefit, yes.
 
> 
>> SFDP signature to determine the mode in which the flash is configured:
>> '''
>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
>> these typically support SFDP read operation in both 1S-1S-1S mode and
>> 8D-8D-8D mode. If the host controller does not know exactly which
>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
>> directly in 8D-8D-8D mode, the host controller may read from address 0,
>> and count the number of dummy clocks required before the SFDP signature
>> is received.)
>> '''

Below is the approach for the flashes that are not SFDP compliant:

>> If the flash does not support SFDP at all, one should introduce dedicated
>> configs for each reset type and issue just the needed reset command.

Apart of the cmd_ext type, you need to also differentiate between the reset
types. So a combination of reset and cmd extension type configs.

I prefer discovering the reset sequence dynamically whenever possible.
You can choose to use the configs approach directly, if you don't want
to spend time on the dynamic discovery approach, the two approaches
will be mutual exclusive anyway.

So yes, I think we should revert this and at least have a better config
approach.

Cheers,
ta
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi/Kconfig        |  9 ---------
>>  drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
>>  2 files changed, 36 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
>> index 2d2b71c52d..14800b5e93 100644
>> --- a/drivers/mtd/spi/Kconfig
>> +++ b/drivers/mtd/spi/Kconfig
>> @@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
>>        Enable support for xSPI Software Reset. It will be used to switch from
>>        Octal DTR mode to legacy mode on shutdown and boot (if enabled).
>>
>> -config SPI_FLASH_SOFT_RESET_ON_BOOT
>> -     bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
>> -     depends on SPI_FLASH_SOFT_RESET
>> -     help
>> -      Perform a Software Reset on boot to allow detecting flashes that are
>> -      handed to us in Octal DTR mode. Do not enable this config on flashes
>> -      that are not supposed to be handed to U-Boot in Octal DTR mode, even
>> -      if they _do_ support the Soft Reset sequence.
>> -
>>  config SPI_FLASH_BAR
>>       bool "SPI flash Bank/Extended address register support"
>>       help
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index df66a73495..caf764720c 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
>>
>>       nor->setup = spi_nor_default_setup;
>>
>> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
>> -     /*
>> -      * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
>> -      * is difficult to detect the mode the flash is in. One option is to
>> -      * read SFDP in all modes and see which one gives the correct "SFDP"
>> -      * signature, but not all flashes support SFDP in 8D-8D-8D mode.
>> -      *
>> -      * Further, even if you detect the mode of the flash via SFDP, you
>> -      * still have the problem of actually reading the ID. The Read ID
>> -      * command is not standardized across flash vendors. Flashes can have
>> -      * different dummy cycles needed for reading the ID. Some flashes even
>> -      * expect a 4-byte dummy address with the Read ID command. All this
>> -      * information cannot be obtained from the SFDP table.
>> -      *
>> -      * So, perform a Software Reset sequence before reading the ID and
>> -      * initializing the flash. A Soft Reset will bring back the flash in
>> -      * its default protocol mode assuming no non-volatile configuration was
>> -      * set. This will let us detect the flash even if ROM hands it to us in
>> -      * Octal DTR mode.
>> -      *
>> -      * To accommodate cases where there is more than one flash on a board,
>> -      * and only one of them needs a soft reset, failure to reset is not
>> -      * made fatal, and we still try to read ID if possible.
>> -      */
>> -     spi_nor_soft_reset(nor);
>> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
>> -
>>       info = spi_nor_read_id(nor);
>>       if (IS_ERR_OR_NULL(info))
>>               return -ENOENT;
>> --
>> 2.25.1
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
> 

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Pratyush Yadav]

On 10/11/21 08:44AM, Tudor.Ambarus@microchip.com wrote:
> On 11/9/21 9:26 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hi Tudor,
> 
> Hi, Pratyush,
> 
> Thanks for reviewing the series.
> 
> > 
> > On 04/11/21 01:49AM, Tudor Ambarus wrote:
> >> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
> >>
> >> There are multiple reset commands and it was used one at guess,
> > 
> > Correct.
> > 
> >> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
> >> commands to a flash. Since there's no config in mainline that actually
> >> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
> > 
> > I have been lagging behind on that front. I have some defconfig patches
> > for TI platforms but I did not get around to cleaning them up and
> > posting them upstream. Still, this feature is very much needed on TI
> > platforms.
> > 
> >> support is added. One should instead determine the mode in which the
> >> flash device is configured, then to parse SFDP to determine the
> >> cmd_ext_type and then to issue a READID command if flash identification
> >> is really a must. JESD216D-01 proposes an algorithm to try to read the
> > 
> > Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
> > example, Cypress S28 flash family expects 4 address bytes for Read ID in
> > 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
> > cycles also tends to vary. There is no way to determine these parameters
> > from SFDP.
> 
> If flash supports SFDP, the read ID becomes of minor importance. We can as
> well not issue the read ID at all.

We need to read the ID so we can apply fixups. One option is to specify 
Read ID type in device tree.

> 
> > 
> > Also, you would have to run the detection algorithm for _every_ flash
> > that SPI NOR supports since we don't really know what we are dealing
> > with at this point. This would include flashes that do not support the
> > Read SFDP command at all. If your goal is to not send unsupported
> 
> but we can have a dt property or a config option to indicate when to issue
> readSFDP.

Okay.

> 
> > commands to a flash then you won't get very far. On top of that Read
> > SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
> > no guarantee that the detection algorithm would even work. It _should_
> > work for most flashes but we will eventually have to deal with the
> > corner cases.
> 
> in case we can't determine the mode in which the flash is configured,
> we can adopt other approach, see below.
> > 
> > In other words, if you drop this then you would have to run the
> > detection algorithm for every flash and see what mode it is in. If an
> 
> not for every flash, just the ones that we marked as SFDP compliant.
> 
> > 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
> > would have to read SFDP, determine the extension and reset type, and
> > then perform the reset. If it does not support the Read SFDP command
> 
> right.
> 
> > then you are left with a non-working flash. You would still probably end
> > up issuing unsupported commands.
> 
> not necessarily.
> 
> > 
> > I can implement such an algorithm but is it really worth the hassle?
> 
> I find having such an alg would be of benefit, yes.
>  
> > 
> >> SFDP signature to determine the mode in which the flash is configured:
> >> '''
> >> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
> >> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
> >> these typically support SFDP read operation in both 1S-1S-1S mode and
> >> 8D-8D-8D mode. If the host controller does not know exactly which
> >> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
> >> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
> >> directly in 8D-8D-8D mode, the host controller may read from address 0,
> >> and count the number of dummy clocks required before the SFDP signature
> >> is received.)
> >> '''
> 
> Below is the approach for the flashes that are not SFDP compliant:
> 
> >> If the flash does not support SFDP at all, one should introduce dedicated
> >> configs for each reset type and issue just the needed reset command.

There are some problems with this approach. What if we have two flashes 
on the board and both use different reset types? How do we figure out 
which reset to apply? This applies to the current implementation as 
well. If there are two flashes then it will issue the reset to both even 
if one of them does not support/need it.

> 
> Apart of the cmd_ext type, you need to also differentiate between the reset
> types. So a combination of reset and cmd extension type configs.
> 
> I prefer discovering the reset sequence dynamically whenever possible.
> You can choose to use the configs approach directly, if you don't want
> to spend time on the dynamic discovery approach, the two approaches
> will be mutual exclusive anyway.
> 
> So yes, I think we should revert this and at least have a better config
> approach.
> 
> Cheers,
> ta
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >>  drivers/mtd/spi/Kconfig        |  9 ---------
> >>  drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
> >>  2 files changed, 36 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> >> index 2d2b71c52d..14800b5e93 100644
> >> --- a/drivers/mtd/spi/Kconfig
> >> +++ b/drivers/mtd/spi/Kconfig
> >> @@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
> >>        Enable support for xSPI Software Reset. It will be used to switch from
> >>        Octal DTR mode to legacy mode on shutdown and boot (if enabled).
> >>
> >> -config SPI_FLASH_SOFT_RESET_ON_BOOT
> >> -     bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
> >> -     depends on SPI_FLASH_SOFT_RESET
> >> -     help
> >> -      Perform a Software Reset on boot to allow detecting flashes that are
> >> -      handed to us in Octal DTR mode. Do not enable this config on flashes
> >> -      that are not supposed to be handed to U-Boot in Octal DTR mode, even
> >> -      if they _do_ support the Soft Reset sequence.
> >> -
> >>  config SPI_FLASH_BAR
> >>       bool "SPI flash Bank/Extended address register support"
> >>       help
> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> >> index df66a73495..caf764720c 100644
> >> --- a/drivers/mtd/spi/spi-nor-core.c
> >> +++ b/drivers/mtd/spi/spi-nor-core.c
> >> @@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
> >>
> >>       nor->setup = spi_nor_default_setup;
> >>
> >> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
> >> -     /*
> >> -      * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
> >> -      * is difficult to detect the mode the flash is in. One option is to
> >> -      * read SFDP in all modes and see which one gives the correct "SFDP"
> >> -      * signature, but not all flashes support SFDP in 8D-8D-8D mode.
> >> -      *
> >> -      * Further, even if you detect the mode of the flash via SFDP, you
> >> -      * still have the problem of actually reading the ID. The Read ID
> >> -      * command is not standardized across flash vendors. Flashes can have
> >> -      * different dummy cycles needed for reading the ID. Some flashes even
> >> -      * expect a 4-byte dummy address with the Read ID command. All this
> >> -      * information cannot be obtained from the SFDP table.
> >> -      *
> >> -      * So, perform a Software Reset sequence before reading the ID and
> >> -      * initializing the flash. A Soft Reset will bring back the flash in
> >> -      * its default protocol mode assuming no non-volatile configuration was
> >> -      * set. This will let us detect the flash even if ROM hands it to us in
> >> -      * Octal DTR mode.
> >> -      *
> >> -      * To accommodate cases where there is more than one flash on a board,
> >> -      * and only one of them needs a soft reset, failure to reset is not
> >> -      * made fatal, and we still try to read ID if possible.
> >> -      */
> >> -     spi_nor_soft_reset(nor);
> >> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
> >> -
> >>       info = spi_nor_read_id(nor);
> >>       if (IS_ERR_OR_NULL(info))
> >>               return -ENOENT;
> >> --
> >> 2.25.1
> >>
> > 
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
> > 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Tudor.Ambarus]

On 11/12/21 3:13 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10/11/21 08:44AM, Tudor.Ambarus@microchip.com wrote:
>> On 11/9/21 9:26 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Tudor,
>>
>> Hi, Pratyush,
>>
>> Thanks for reviewing the series.
>>
>>>
>>> On 04/11/21 01:49AM, Tudor Ambarus wrote:
>>>> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
>>>>
>>>> There are multiple reset commands and it was used one at guess,
>>>
>>> Correct.
>>>
>>>> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
>>>> commands to a flash. Since there's no config in mainline that actually
>>>> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
>>>
>>> I have been lagging behind on that front. I have some defconfig patches
>>> for TI platforms but I did not get around to cleaning them up and
>>> posting them upstream. Still, this feature is very much needed on TI
>>> platforms.
>>>
>>>> support is added. One should instead determine the mode in which the
>>>> flash device is configured, then to parse SFDP to determine the
>>>> cmd_ext_type and then to issue a READID command if flash identification
>>>> is really a must. JESD216D-01 proposes an algorithm to try to read the
>>>
>>> Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
>>> example, Cypress S28 flash family expects 4 address bytes for Read ID in
>>> 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
>>> cycles also tends to vary. There is no way to determine these parameters
>>> from SFDP.
>>
>> If flash supports SFDP, the read ID becomes of minor importance. We can as
>> well not issue the read ID at all.
> 
> We need to read the ID so we can apply fixups. One option is to specify

Fixup hooks are there just because the manufacturers can't get their SFDP
tables right. Read ID determines the static flags and parameters, which should
be used just as a fallback.

> Read ID type in device tree.

we may consider this when really needed. Until then we can use the recommended
sequence I guess.

> 
>>
>>>
>>> Also, you would have to run the detection algorithm for _every_ flash
>>> that SPI NOR supports since we don't really know what we are dealing
>>> with at this point. This would include flashes that do not support the
>>> Read SFDP command at all. If your goal is to not send unsupported
>>
>> but we can have a dt property or a config option to indicate when to issue
>> readSFDP.
> 
> Okay.
> 
>>
>>> commands to a flash then you won't get very far. On top of that Read
>>> SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
>>> no guarantee that the detection algorithm would even work. It _should_
>>> work for most flashes but we will eventually have to deal with the
>>> corner cases.
>>
>> in case we can't determine the mode in which the flash is configured,
>> we can adopt other approach, see below.
>>>
>>> In other words, if you drop this then you would have to run the
>>> detection algorithm for every flash and see what mode it is in. If an
>>
>> not for every flash, just the ones that we marked as SFDP compliant.
>>
>>> 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
>>> would have to read SFDP, determine the extension and reset type, and
>>> then perform the reset. If it does not support the Read SFDP command
>>
>> right.
>>
>>> then you are left with a non-working flash. You would still probably end
>>> up issuing unsupported commands.
>>
>> not necessarily.
>>
>>>
>>> I can implement such an algorithm but is it really worth the hassle?
>>
>> I find having such an alg would be of benefit, yes.
>>
>>>
>>>> SFDP signature to determine the mode in which the flash is configured:
>>>> '''
>>>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
>>>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
>>>> these typically support SFDP read operation in both 1S-1S-1S mode and
>>>> 8D-8D-8D mode. If the host controller does not know exactly which
>>>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
>>>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
>>>> directly in 8D-8D-8D mode, the host controller may read from address 0,
>>>> and count the number of dummy clocks required before the SFDP signature
>>>> is received.)
>>>> '''
>>
>> Below is the approach for the flashes that are not SFDP compliant:
>>
>>>> If the flash does not support SFDP at all, one should introduce dedicated
>>>> configs for each reset type and issue just the needed reset command.
> 
> There are some problems with this approach. What if we have two flashes
> on the board and both use different reset types? How do we figure out
> which reset to apply? This applies to the current implementation as
> well. If there are two flashes then it will issue the reset to both even
> if one of them does not support/need it.

One would have to choose the NOR manufacturer with care next time. If we'll
have to statically define the reset type for both the flashes, there's nothing
much we can do. Maybe if you have a gpio reset line connected to the flash you
can toggle that instead.

Cheers,
ta
> 
>>
>> Apart of the cmd_ext type, you need to also differentiate between the reset
>> types. So a combination of reset and cmd extension type configs.
>>
>> I prefer discovering the reset sequence dynamically whenever possible.
>> You can choose to use the configs approach directly, if you don't want
>> to spend time on the dynamic discovery approach, the two approaches
>> will be mutual exclusive anyway.
>>
>> So yes, I think we should revert this and at least have a better config
>> approach.
>>
>> Cheers,
>> ta
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>> ---
>>>>  drivers/mtd/spi/Kconfig        |  9 ---------
>>>>  drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
>>>>  2 files changed, 36 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
>>>> index 2d2b71c52d..14800b5e93 100644
>>>> --- a/drivers/mtd/spi/Kconfig
>>>> +++ b/drivers/mtd/spi/Kconfig
>>>> @@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
>>>>        Enable support for xSPI Software Reset. It will be used to switch from
>>>>        Octal DTR mode to legacy mode on shutdown and boot (if enabled).
>>>>
>>>> -config SPI_FLASH_SOFT_RESET_ON_BOOT
>>>> -     bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
>>>> -     depends on SPI_FLASH_SOFT_RESET
>>>> -     help
>>>> -      Perform a Software Reset on boot to allow detecting flashes that are
>>>> -      handed to us in Octal DTR mode. Do not enable this config on flashes
>>>> -      that are not supposed to be handed to U-Boot in Octal DTR mode, even
>>>> -      if they _do_ support the Soft Reset sequence.
>>>> -
>>>>  config SPI_FLASH_BAR
>>>>       bool "SPI flash Bank/Extended address register support"
>>>>       help
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index df66a73495..caf764720c 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>> @@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
>>>>
>>>>       nor->setup = spi_nor_default_setup;
>>>>
>>>> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
>>>> -     /*
>>>> -      * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
>>>> -      * is difficult to detect the mode the flash is in. One option is to
>>>> -      * read SFDP in all modes and see which one gives the correct "SFDP"
>>>> -      * signature, but not all flashes support SFDP in 8D-8D-8D mode.
>>>> -      *
>>>> -      * Further, even if you detect the mode of the flash via SFDP, you
>>>> -      * still have the problem of actually reading the ID. The Read ID
>>>> -      * command is not standardized across flash vendors. Flashes can have
>>>> -      * different dummy cycles needed for reading the ID. Some flashes even
>>>> -      * expect a 4-byte dummy address with the Read ID command. All this
>>>> -      * information cannot be obtained from the SFDP table.
>>>> -      *
>>>> -      * So, perform a Software Reset sequence before reading the ID and
>>>> -      * initializing the flash. A Soft Reset will bring back the flash in
>>>> -      * its default protocol mode assuming no non-volatile configuration was
>>>> -      * set. This will let us detect the flash even if ROM hands it to us in
>>>> -      * Octal DTR mode.
>>>> -      *
>>>> -      * To accommodate cases where there is more than one flash on a board,
>>>> -      * and only one of them needs a soft reset, failure to reset is not
>>>> -      * made fatal, and we still try to read ID if possible.
>>>> -      */
>>>> -     spi_nor_soft_reset(nor);
>>>> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
>>>> -
>>>>       info = spi_nor_read_id(nor);
>>>>       if (IS_ERR_OR_NULL(info))
>>>>               return -ENOENT;
>>>> --
>>>> 2.25.1
>>>>
>>>
>>> --
>>> Regards,
>>> Pratyush Yadav
>>> Texas Instruments Inc.
>>>
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
> 

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Pratyush Yadav]

Hi Tudor,

I am not sure if you have sent a re-roll of this series. I am catching 
back up on my email backlog.

On 15/11/21 05:44AM, Tudor.Ambarus@microchip.com wrote:
> On 11/12/21 3:13 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 10/11/21 08:44AM, Tudor.Ambarus@microchip.com wrote:
> >> On 11/9/21 9:26 PM, Pratyush Yadav wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> Hi Tudor,
> >>
> >> Hi, Pratyush,
> >>
> >> Thanks for reviewing the series.
> >>
> >>>
> >>> On 04/11/21 01:49AM, Tudor Ambarus wrote:
> >>>> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
> >>>>
> >>>> There are multiple reset commands and it was used one at guess,
> >>>
> >>> Correct.
> >>>
> >>>> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
> >>>> commands to a flash. Since there's no config in mainline that actually
> >>>> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
> >>>
> >>> I have been lagging behind on that front. I have some defconfig patches
> >>> for TI platforms but I did not get around to cleaning them up and
> >>> posting them upstream. Still, this feature is very much needed on TI
> >>> platforms.
> >>>
> >>>> support is added. One should instead determine the mode in which the
> >>>> flash device is configured, then to parse SFDP to determine the
> >>>> cmd_ext_type and then to issue a READID command if flash identification
> >>>> is really a must. JESD216D-01 proposes an algorithm to try to read the
> >>>
> >>> Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
> >>> example, Cypress S28 flash family expects 4 address bytes for Read ID in
> >>> 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
> >>> cycles also tends to vary. There is no way to determine these parameters
> >>> from SFDP.
> >>
> >> If flash supports SFDP, the read ID becomes of minor importance. We can as
> >> well not issue the read ID at all.
> > 
> > We need to read the ID so we can apply fixups. One option is to specify
> 
> Fixup hooks are there just because the manufacturers can't get their SFDP
> tables right. Read ID determines the static flags and parameters, which should
> be used just as a fallback.
> 
> > Read ID type in device tree.
> 
> we may consider this when really needed. Until then we can use the recommended
> sequence I guess.

Right. And I have two flashes which both need fixups and both have 
different Read ID commands in 8D-8D-8D mode: Micron MT35X and Cypress 
S28H. Both of those fortunately have the correct data about how to reset 
them so we should be good for now.

> 
> > 
> >>
> >>>
> >>> Also, you would have to run the detection algorithm for _every_ flash
> >>> that SPI NOR supports since we don't really know what we are dealing
> >>> with at this point. This would include flashes that do not support the
> >>> Read SFDP command at all. If your goal is to not send unsupported
> >>
> >> but we can have a dt property or a config option to indicate when to issue
> >> readSFDP.
> > 
> > Okay.
> > 
> >>
> >>> commands to a flash then you won't get very far. On top of that Read
> >>> SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
> >>> no guarantee that the detection algorithm would even work. It _should_
> >>> work for most flashes but we will eventually have to deal with the
> >>> corner cases.
> >>
> >> in case we can't determine the mode in which the flash is configured,
> >> we can adopt other approach, see below.
> >>>
> >>> In other words, if you drop this then you would have to run the
> >>> detection algorithm for every flash and see what mode it is in. If an
> >>
> >> not for every flash, just the ones that we marked as SFDP compliant.
> >>
> >>> 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
> >>> would have to read SFDP, determine the extension and reset type, and
> >>> then perform the reset. If it does not support the Read SFDP command
> >>
> >> right.
> >>
> >>> then you are left with a non-working flash. You would still probably end
> >>> up issuing unsupported commands.
> >>
> >> not necessarily.
> >>
> >>>
> >>> I can implement such an algorithm but is it really worth the hassle?
> >>
> >> I find having such an alg would be of benefit, yes.
> >>
> >>>
> >>>> SFDP signature to determine the mode in which the flash is configured:
> >>>> '''
> >>>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
> >>>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
> >>>> these typically support SFDP read operation in both 1S-1S-1S mode and
> >>>> 8D-8D-8D mode. If the host controller does not know exactly which
> >>>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
> >>>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
> >>>> directly in 8D-8D-8D mode, the host controller may read from address 0,
> >>>> and count the number of dummy clocks required before the SFDP signature
> >>>> is received.)
> >>>> '''
> >>
> >> Below is the approach for the flashes that are not SFDP compliant:
> >>
> >>>> If the flash does not support SFDP at all, one should introduce dedicated
> >>>> configs for each reset type and issue just the needed reset command.
> > 
> > There are some problems with this approach. What if we have two flashes
> > on the board and both use different reset types? How do we figure out
> > which reset to apply? This applies to the current implementation as
> > well. If there are two flashes then it will issue the reset to both even
> > if one of them does not support/need it.
> 
> One would have to choose the NOR manufacturer with care next time. If we'll
> have to statically define the reset type for both the flashes, there's nothing
> much we can do. Maybe if you have a gpio reset line connected to the flash you
> can toggle that instead.

Or we can specify the reset type in the device tree? That should neatly 
solve the problem I believe.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Tudor.Ambarus]

On 12/16/21 8:45 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> I am not sure if you have sent a re-roll of this series. I am catching
> back up on my email backlog.

I haven't, I was busy with other things, but I will.

> 
> On 15/11/21 05:44AM, Tudor.Ambarus@microchip.com wrote:
>> On 11/12/21 3:13 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/11/21 08:44AM, Tudor.Ambarus@microchip.com wrote:
>>>> On 11/9/21 9:26 PM, Pratyush Yadav wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Hi Tudor,
>>>>
>>>> Hi, Pratyush,
>>>>
>>>> Thanks for reviewing the series.
>>>>
>>>>>
>>>>> On 04/11/21 01:49AM, Tudor Ambarus wrote:
>>>>>> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
>>>>>>
>>>>>> There are multiple reset commands and it was used one at guess,
>>>>>
>>>>> Correct.
>>>>>
>>>>>> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
>>>>>> commands to a flash. Since there's no config in mainline that actually
>>>>>> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
>>>>>
>>>>> I have been lagging behind on that front. I have some defconfig patches
>>>>> for TI platforms but I did not get around to cleaning them up and
>>>>> posting them upstream. Still, this feature is very much needed on TI
>>>>> platforms.
>>>>>
>>>>>> support is added. One should instead determine the mode in which the
>>>>>> flash device is configured, then to parse SFDP to determine the
>>>>>> cmd_ext_type and then to issue a READID command if flash identification
>>>>>> is really a must. JESD216D-01 proposes an algorithm to try to read the
>>>>>
>>>>> Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
>>>>> example, Cypress S28 flash family expects 4 address bytes for Read ID in
>>>>> 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
>>>>> cycles also tends to vary. There is no way to determine these parameters
>>>>> from SFDP.
>>>>
>>>> If flash supports SFDP, the read ID becomes of minor importance. We can as
>>>> well not issue the read ID at all.
>>>
>>> We need to read the ID so we can apply fixups. One option is to specify
>>
>> Fixup hooks are there just because the manufacturers can't get their SFDP
>> tables right. Read ID determines the static flags and parameters, which should
>> be used just as a fallback.
>>
>>> Read ID type in device tree.
>>
>> we may consider this when really needed. Until then we can use the recommended
>> sequence I guess.
> 
> Right. And I have two flashes which both need fixups and both have
> different Read ID commands in 8D-8D-8D mode: Micron MT35X and Cypress
> S28H. Both of those fortunately have the correct data about how to reset
> them so we should be good for now.
> 
>>
>>>
>>>>
>>>>>
>>>>> Also, you would have to run the detection algorithm for _every_ flash
>>>>> that SPI NOR supports since we don't really know what we are dealing
>>>>> with at this point. This would include flashes that do not support the
>>>>> Read SFDP command at all. If your goal is to not send unsupported
>>>>
>>>> but we can have a dt property or a config option to indicate when to issue
>>>> readSFDP.
>>>
>>> Okay.
>>>
>>>>
>>>>> commands to a flash then you won't get very far. On top of that Read
>>>>> SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
>>>>> no guarantee that the detection algorithm would even work. It _should_
>>>>> work for most flashes but we will eventually have to deal with the
>>>>> corner cases.
>>>>
>>>> in case we can't determine the mode in which the flash is configured,
>>>> we can adopt other approach, see below.
>>>>>
>>>>> In other words, if you drop this then you would have to run the
>>>>> detection algorithm for every flash and see what mode it is in. If an
>>>>
>>>> not for every flash, just the ones that we marked as SFDP compliant.
>>>>
>>>>> 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
>>>>> would have to read SFDP, determine the extension and reset type, and
>>>>> then perform the reset. If it does not support the Read SFDP command
>>>>
>>>> right.
>>>>
>>>>> then you are left with a non-working flash. You would still probably end
>>>>> up issuing unsupported commands.
>>>>
>>>> not necessarily.
>>>>
>>>>>
>>>>> I can implement such an algorithm but is it really worth the hassle?
>>>>
>>>> I find having such an alg would be of benefit, yes.
>>>>
>>>>>
>>>>>> SFDP signature to determine the mode in which the flash is configured:
>>>>>> '''
>>>>>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
>>>>>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
>>>>>> these typically support SFDP read operation in both 1S-1S-1S mode and
>>>>>> 8D-8D-8D mode. If the host controller does not know exactly which
>>>>>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
>>>>>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
>>>>>> directly in 8D-8D-8D mode, the host controller may read from address 0,
>>>>>> and count the number of dummy clocks required before the SFDP signature
>>>>>> is received.)
>>>>>> '''
>>>>
>>>> Below is the approach for the flashes that are not SFDP compliant:
>>>>
>>>>>> If the flash does not support SFDP at all, one should introduce dedicated
>>>>>> configs for each reset type and issue just the needed reset command.
>>>
>>> There are some problems with this approach. What if we have two flashes
>>> on the board and both use different reset types? How do we figure out
>>> which reset to apply? This applies to the current implementation as
>>> well. If there are two flashes then it will issue the reset to both even
>>> if one of them does not support/need it.
>>
>> One would have to choose the NOR manufacturer with care next time. If we'll
>> have to statically define the reset type for both the flashes, there's nothing
>> much we can do. Maybe if you have a gpio reset line connected to the flash you
>> can toggle that instead.
> 
> Or we can specify the reset type in the device tree? That should neatly
> solve the problem I believe.
> 

It is surely an approach to consider. The downside with it is that people might
abuse it, and use it regardless if the reset type is defined in SFDP or not.
Do we care?

ta

Re: [PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Pratyush Yadav]

On 17/12/21 06:27AM, Tudor.Ambarus@microchip.com wrote:
> On 12/16/21 8:45 PM, Pratyush Yadav wrote:
> >>>>
> >>>>>
> >>>>>> SFDP signature to determine the mode in which the flash is configured:
> >>>>>> '''
> >>>>>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
> >>>>>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
> >>>>>> these typically support SFDP read operation in both 1S-1S-1S mode and
> >>>>>> 8D-8D-8D mode. If the host controller does not know exactly which
> >>>>>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
> >>>>>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
> >>>>>> directly in 8D-8D-8D mode, the host controller may read from address 0,
> >>>>>> and count the number of dummy clocks required before the SFDP signature
> >>>>>> is received.)
> >>>>>> '''
> >>>>
> >>>> Below is the approach for the flashes that are not SFDP compliant:
> >>>>
> >>>>>> If the flash does not support SFDP at all, one should introduce dedicated
> >>>>>> configs for each reset type and issue just the needed reset command.
> >>>
> >>> There are some problems with this approach. What if we have two flashes
> >>> on the board and both use different reset types? How do we figure out
> >>> which reset to apply? This applies to the current implementation as
> >>> well. If there are two flashes then it will issue the reset to both even
> >>> if one of them does not support/need it.
> >>
> >> One would have to choose the NOR manufacturer with care next time. If we'll
> >> have to statically define the reset type for both the flashes, there's nothing
> >> much we can do. Maybe if you have a gpio reset line connected to the flash you
> >> can toggle that instead.
> > 
> > Or we can specify the reset type in the device tree? That should neatly
> > solve the problem I believe.
> > 
> 
> It is surely an approach to consider. The downside with it is that people might
> abuse it, and use it regardless if the reset type is defined in SFDP or not.
> Do we care?

I am not sure to be honest. Let's think through that once the problem 
actually comes up I suppose.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 4/4] mtd: spi-nor-core: Fix the opcode extension for the software reset sequence

[Tudor Ambarus]

It was always hardcoded to SPI_NOR_EXT_REPEAT, while there are
flashes that may use an SPI_NOR_EXT_INVERT opcode extension
(mx66lm1g45g). Remove the hardcoded value and let flashes use
their per flash opcode extension type.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi/spi-nor-core.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index caf764720c..cbdad335b3 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3685,10 +3685,6 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 {
 	struct spi_mem_op op;
 	int ret;
-	enum spi_nor_cmd_ext ext;
-
-	ext = nor->cmd_ext_type;
-	nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
 
 	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
 			SPI_MEM_OP_NO_DUMMY,
@@ -3698,7 +3694,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 0),
@@ -3709,7 +3705,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -3719,9 +3715,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	 */
 	udelay(SPI_NOR_SRST_SLEEP_LEN);
 
-out:
-	nor->cmd_ext_type = ext;
-	return ret;
+	return 0;
 }
 #endif /* CONFIG_SPI_FLASH_SOFT_RESET */
 
-- 
2.25.1

Re: [PATCH v2 4/4] mtd: spi-nor-core: Fix the opcode extension for the software reset sequence

[Pratyush Yadav]

On 04/11/21 01:49AM, Tudor Ambarus wrote:
> It was always hardcoded to SPI_NOR_EXT_REPEAT, while there are
> flashes that may use an SPI_NOR_EXT_INVERT opcode extension
> (mx66lm1g45g). Remove the hardcoded value and let flashes use
> their per flash opcode extension type.

I suggested this exact patch, along with invert extension support for 
soft reset on boot in [0]. Once we decide on patch 3/4 this should be 
good to go.

[0] https://lore.kernel.org/u-boot/20211025194216.mpocldhlu3pdy63h@ti.com/

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index caf764720c..cbdad335b3 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3685,10 +3685,6 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  {
>  	struct spi_mem_op op;
>  	int ret;
> -	enum spi_nor_cmd_ext ext;
> -
> -	ext = nor->cmd_ext_type;
> -	nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
>  
>  	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
>  			SPI_MEM_OP_NO_DUMMY,
> @@ -3698,7 +3694,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  	ret = spi_mem_exec_op(nor->spi, &op);
>  	if (ret) {
>  		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 0),
> @@ -3709,7 +3705,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  	ret = spi_mem_exec_op(nor->spi, &op);
>  	if (ret) {
>  		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  	/*
> @@ -3719,9 +3715,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  	 */
>  	udelay(SPI_NOR_SRST_SLEEP_LEN);
>  
> -out:
> -	nor->cmd_ext_type = ext;
> -	return ret;
> +	return 0;
>  }
>  #endif /* CONFIG_SPI_FLASH_SOFT_RESET */
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.