[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Some SPI NOR chips only support 4-byte mode addressing. Here the default
3-byte mode does not work and leads to incorrect accesses. This patch
now reads the 4-byte mode status bit (in this case in the CR register
of the Macronix SPI NOR) and configures the SPI transfers accordingly.

This was noticed on the LinkIt Smart 7688 modul, which is equipped with
an Macronix MX25L25635F device. But this device does *NOT* support
switching to 3-byte mode via the EX4B command.

This should also work when the bootrom configures the SPI flash to
4-byte mode and runs U-Boot after this. U-Boot should dectect this
mode (if the 4-byte mode detection is available for this chip) and
use the correct OPs in this case.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Jagan Teki <jagan@openedev.com>
Tested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---
v3:
- Rebased on latest version (merge conflict because of new patches
  from Simon Glass)
- Added Tested-by tag from Simon Goldschmidt

v2:
- Integrated STMICRO 4-byte detection from Simon

 drivers/mtd/spi/sf_internal.h |   3 +-
 drivers/mtd/spi/spi_flash.c   | 131 ++++++++++++++++++++++++++++------
 include/spi_flash.h           |   5 ++
 3 files changed, 118 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index 4f63cacc64..eb076401d1 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -26,7 +26,8 @@ enum spi_nor_option_flags {
 };
 
 #define SPI_FLASH_3B_ADDR_LEN		3
-#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_3B_ADDR_LEN)
+#define SPI_FLASH_4B_ADDR_LEN		4
+#define SPI_FLASH_CMD_MAX_LEN		(1 + SPI_FLASH_4B_ADDR_LEN)
 #define SPI_FLASH_16MB_BOUN		0x1000000
 
 /* CFI Manufacture ID's */
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 9230060364..b22eea2d1c 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -20,12 +20,19 @@
 
 #include "sf_internal.h"
 
-static void spi_flash_addr(u32 addr, u8 *cmd)
+static void spi_flash_addr(struct spi_flash *flash, u32 addr, u8 *cmd)
 {
 	/* cmd[0] is actual command */
-	cmd[1] = addr >> 16;
-	cmd[2] = addr >> 8;
-	cmd[3] = addr >> 0;
+	if (flash->in_4byte_mode) {
+		cmd[1] = addr >> 24;
+		cmd[2] = addr >> 16;
+		cmd[3] = addr >> 8;
+		cmd[4] = addr >> 0;
+	} else {
+		cmd[1] = addr >> 16;
+		cmd[2] = addr >> 8;
+		cmd[3] = addr >> 0;
+	}
 }
 
 static int read_sr(struct spi_flash *flash, u8 *rs)
@@ -110,6 +117,72 @@ static int write_cr(struct spi_flash *flash, u8 wc)
 }
 #endif
 
+#if defined(CONFIG_SPI_FLASH_MACRONIX)
+static bool flash_in_4byte_mode_macronix(struct spi_flash *flash)
+{
+	int ret;
+	u8 cr;
+	u8 cmd;
+
+	cmd = 0x15;	/* Macronix: read configuration register RDCR */
+	ret = spi_flash_read_common(flash, &cmd, 1, &cr, 1);
+	if (ret < 0) {
+		debug("SF: fail to read config register\n");
+		return false;
+	}
+
+	/* Return true, if 4-byte mode is enabled */
+	if (cr & BIT(5))
+		return true;
+
+	return false;
+}
+#else
+static bool flash_in_4byte_mode_macronix(struct spi_flash *flash)
+{
+	return false;
+}
+#endif
+
+#if defined(CONFIG_SPI_FLASH_STMICRO)
+static bool flash_in_4byte_mode_stmicro(struct spi_flash *flash)
+{
+	int ret;
+	u8 fsr;
+	u8 cmd;
+
+	cmd = 0x70;	/* STMicro/Micron: read flag status register */
+	ret = spi_flash_read_common(flash, &cmd, 1, &fsr, 1);
+	if (ret < 0) {
+		debug("SF: fail to read config register\n");
+		return false;
+	}
+
+	/* Return true, if 4-byte mode is enabled */
+	if (fsr & BIT(0))
+		return true;
+
+	return false;
+}
+#else
+static bool flash_in_4byte_mode_stmicro(struct spi_flash *flash)
+{
+	return false;
+}
+#endif
+
+static bool flash_in_4byte_mode(struct spi_flash *flash,
+				const struct spi_flash_info *info)
+{
+	if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_MACRONIX)
+		return flash_in_4byte_mode_macronix(flash);
+
+	if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_STMICRO)
+		return flash_in_4byte_mode_stmicro(flash);
+
+	return false;
+}
+
 #ifdef CONFIG_SPI_FLASH_BAR
 /*
  * This "clean_bar" is necessary in a situation when one was accessing
@@ -314,7 +387,7 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
 int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 {
 	u32 erase_size, erase_addr;
-	u8 cmd[SPI_FLASH_CMD_LEN];
+	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
 	int ret = -1;
 
 	erase_size = flash->erase_size;
@@ -344,12 +417,13 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 		if (ret < 0)
 			return ret;
 #endif
-		spi_flash_addr(erase_addr, cmd);
+		spi_flash_addr(flash, erase_addr, cmd);
 
 		debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
 		      cmd[2], cmd[3], erase_addr);
 
-		ret = spi_flash_write_common(flash, cmd, sizeof(cmd), NULL, 0);
+		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
+					     NULL, 0);
 		if (ret < 0) {
 			debug("SF: erase failed\n");
 			break;
@@ -373,7 +447,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
 	unsigned long byte_addr, page_size;
 	u32 write_addr;
 	size_t chunk_len, actual;
-	u8 cmd[SPI_FLASH_CMD_LEN];
+	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
 	int ret = -1;
 
 	page_size = flash->page_size;
@@ -404,15 +478,15 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
 
 		if (spi->max_write_size)
 			chunk_len = min(chunk_len,
-					spi->max_write_size - sizeof(cmd));
+					spi->max_write_size - flash->cmdlen);
 
-		spi_flash_addr(write_addr, cmd);
+		spi_flash_addr(flash, write_addr, cmd);
 
 		debug("SF: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",
 		      buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
 
-		ret = spi_flash_write_common(flash, cmd, sizeof(cmd),
-					buf + actual, chunk_len);
+		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
+					     buf + actual, chunk_len);
 		if (ret < 0) {
 			debug("SF: write failed\n");
 			break;
@@ -469,9 +543,11 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 {
 	struct spi_slave *spi = flash->spi;
 	u8 cmdsz;
-	u32 remain_len, read_len, read_addr;
+	u64 remain_len;
+	u32 read_len, read_addr;
 	int bank_sel = 0;
 	int ret = 0;
+	int shift;
 
 	/* Handle memory-mapped SPI */
 	if (flash->memory_map) {
@@ -487,7 +563,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		return 0;
 	}
 
-	cmdsz = SPI_FLASH_CMD_LEN + flash->dummy_byte;
+	cmdsz = flash->cmdlen + flash->dummy_byte;
 	u8 cmd[cmdsz];
 
 	cmd[0] = flash->read_cmd;
@@ -504,8 +580,13 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 			return log_ret(ret);
 		bank_sel = flash->bank_curr;
 #endif
-		remain_len = ((SPI_FLASH_16MB_BOUN << flash->shift) *
-				(bank_sel + 1)) - offset;
+		shift = flash->shift;
+		if (flash->in_4byte_mode)
+			shift += 8;
+
+		remain_len = (((u64)SPI_FLASH_16MB_BOUN << shift) *
+			      (bank_sel + 1)) - offset;
+
 		if (len < remain_len)
 			read_len = len;
 		else
@@ -514,7 +595,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		if (spi->max_read_size)
 			read_len = min(read_len, spi->max_read_size);
 
-		spi_flash_addr(read_addr, cmd);
+		spi_flash_addr(flash, read_addr, cmd);
 
 		ret = spi_flash_read_common(flash, cmd, cmdsz, data, read_len);
 		if (ret < 0) {
@@ -1155,6 +1236,13 @@ int spi_flash_scan(struct spi_flash *flash)
 		write_sr(flash, sr);
 	}
 
+	/* Set default value for cmd length */
+	flash->cmdlen = 1 + SPI_FLASH_3B_ADDR_LEN;
+	if (flash_in_4byte_mode(flash, info)) {
+		flash->in_4byte_mode = true;
+		flash->cmdlen = 1 + SPI_FLASH_4B_ADDR_LEN;
+	}
+
 	flash->name = info->name;
 	flash->memory_map = spi->memory_map;
 
@@ -1306,14 +1394,17 @@ int spi_flash_scan(struct spi_flash *flash)
 	print_size(flash->size, "");
 	if (flash->memory_map)
 		printf(", mapped at %p", flash->memory_map);
+	if (flash->in_4byte_mode)
+		printf(" (4-byte mode)");
 	puts("\n");
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR
-	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
+	if ((((flash->dual_flash == SF_SINGLE_FLASH) &&
+	      (flash->size > SPI_FLASH_16MB_BOUN)) ||
 	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
+	      (flash->size > SPI_FLASH_16MB_BOUN << 1))) &&
+	    !flash->in_4byte_mode) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
 	}
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 0ec98fb55d..b5bc4a85f6 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -36,6 +36,8 @@ struct spi_slave;
  * @dual_flash:		Indicates dual flash memories - dual stacked, parallel
  * @shift:		Flash shift useful in dual parallel
  * @flags:		Indication of spi flash flags
+ * @in_4byte_mode:	True if flash is detected to be in 4-byte mode
+ * @cmdlen:		CMD length (3-byte vs 4-byte mode)
  * @size:		Total flash size
  * @page_size:		Write (page) size
  * @sector_size:	Sector size
@@ -69,6 +71,9 @@ struct spi_flash {
 	u8 shift;
 	u16 flags;
 
+	bool in_4byte_mode;
+	int cmdlen;
+
 	u32 size;
 	u32 page_size;
 	u32 sector_size;
-- 
2.19.1

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[York Sun]

Guys,

Let get back to the original thread. Since Rajat's first reply, the
message id has been changed. All the comments were not captured by
patchwork.

On 10/11/18 07:50, Stefan Roese wrote:
> Some SPI NOR chips only support 4-byte mode addressing. Here the default
> 3-byte mode does not work and leads to incorrect accesses. This patch
> now reads the 4-byte mode status bit (in this case in the CR register
> of the Macronix SPI NOR) and configures the SPI transfers accordingly.
> 
> This was noticed on the LinkIt Smart 7688 modul, which is equipped with
> an Macronix MX25L25635F device. But this device does *NOT* support
> switching to 3-byte mode via the EX4B command.
> 
> This should also work when the bootrom configures the SPI flash to
> 4-byte mode and runs U-Boot after this. U-Boot should dectect this
> mode (if the 4-byte mode detection is available for this chip) and
> use the correct OPs in this case.

From what I read, Rajat's method is to extend the controller driver to
support read SFDP and default to 4-byte mode if supported, or
overwritten by user's flag. Stefan's method is to read 4-byte status bit
and doesn't change controller driver.

Is the default value of this 4-byte status bit valid and correct for all
cases?

Rajat, without your patch set, does Stefan's solution work for your board?

York

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi York,

On 25.10.18 17:49, York Sun wrote:
> Guys,
> 
> Let get back to the original thread. Since Rajat's first reply, the
> message id has been changed. All the comments were not captured by
> patchwork.

I also wondered about this. Seems to have happened at some top-posting
quite at the beginning of this thread.
  
> On 10/11/18 07:50, Stefan Roese wrote:
>> Some SPI NOR chips only support 4-byte mode addressing. Here the default
>> 3-byte mode does not work and leads to incorrect accesses. This patch
>> now reads the 4-byte mode status bit (in this case in the CR register
>> of the Macronix SPI NOR) and configures the SPI transfers accordingly.
>>
>> This was noticed on the LinkIt Smart 7688 modul, which is equipped with
>> an Macronix MX25L25635F device. But this device does *NOT* support
>> switching to 3-byte mode via the EX4B command.
>>
>> This should also work when the bootrom configures the SPI flash to
>> 4-byte mode and runs U-Boot after this. U-Boot should dectect this
>> mode (if the 4-byte mode detection is available for this chip) and
>> use the correct OPs in this case.
> 
>  From what I read, Rajat's method is to extend the controller driver to
> support read SFDP and default to 4-byte mode if supported, or
> overwritten by user's flag. Stefan's method is to read 4-byte status bit
> and doesn't change controller driver.

Yes, the last sentence it correct.

> Is the default value of this 4-byte status bit valid and correct for all
> cases?

This 4-byte address status bit implementation is SPI NOR chip
specific (vendor specific?). With Simon's help we already support
2 vendors now. Extending for other vendors / chips is fairly
easy.

Thanks,
Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[York Sun]

On 10/25/18 08:59, Stefan Roese wrote:
> Hi York,
> 
> On 25.10.18 17:49, York Sun wrote:
>> Guys,
>>
>> Let get back to the original thread. Since Rajat's first reply, the
>> message id has been changed. All the comments were not captured by
>> patchwork.
> 
> I also wondered about this. Seems to have happened at some top-posting
> quite at the beginning of this thread.
>   

I am having trouble recently with office365. It seems breaking all my
email threads if senders use outlook.

>>
>>  From what I read, Rajat's method is to extend the controller driver to
>> support read SFDP and default to 4-byte mode if supported, or
>> overwritten by user's flag. Stefan's method is to read 4-byte status bit
>> and doesn't change controller driver.
> 
> Yes, the last sentence it correct.
> 
>> Is the default value of this 4-byte status bit valid and correct for all
>> cases?
> 
> This 4-byte address status bit implementation is SPI NOR chip
> specific (vendor specific?). With Simon's help we already support
> 2 vendors now. Extending for other vendors / chips is fairly
> easy.

Let's wait for Rajat's comment. If this method doesn't work for his
boards or his controllers, he still needs to extend controller's driver
feature.

York

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

> -----Original Message-----
> From: York Sun
> Sent: Thursday, October 25, 2018 9:20 PM
> To: Stefan Roese <sr@denx.de>
> Cc: u-boot at lists.denx.de; Jagan Teki <jagan@openedev.com>; Rajat
> Srivastava <rajat.srivastava@nxp.com>; simon.k.r.goldschmidt at gmail.com;
> Ashish Kumar <ashish.kumar@nxp.com>
> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> standard 3-byte mode)
> 
> Guys,
> 
> Let get back to the original thread. Since Rajat's first reply, the
> message id has been changed. All the comments were not captured by
> patchwork.
> 
> On 10/11/18 07:50, Stefan Roese wrote:
> > Some SPI NOR chips only support 4-byte mode addressing. Here the
> default
> > 3-byte mode does not work and leads to incorrect accesses. This patch
> > now reads the 4-byte mode status bit (in this case in the CR register
> > of the Macronix SPI NOR) and configures the SPI transfers accordingly.
> >
> > This was noticed on the LinkIt Smart 7688 modul, which is equipped with
> > an Macronix MX25L25635F device. But this device does *NOT* support
> > switching to 3-byte mode via the EX4B command.
> >
> > This should also work when the bootrom configures the SPI flash to
> > 4-byte mode and runs U-Boot after this. U-Boot should dectect this
> > mode (if the 4-byte mode detection is available for this chip) and
> > use the correct OPs in this case.
> 
> From what I read, Rajat's method is to extend the controller driver to
> support read SFDP and default to 4-byte mode if supported, or
> overwritten by user's flag. Stefan's method is to read 4-byte status bit
> and doesn't change controller driver.
> 
> Is the default value of this 4-byte status bit valid and correct for all
> cases?
> 
> Rajat, without your patch set, does Stefan's solution work for your board?
> 
> York

No. Stefan's changes are specific to his boards and is not applicable on ours.
Stefan's patch is to support only certain flash that are factory strapped to work in 4-byte addressing modes only and will default to old method if such a flash is not found. The flashes on our boards (and also other vendor's board) will not work with Stefan's patch.

My patch can handle flashes with address widths of 3-byte, 4-byte or both. It also takes a more generic path (as opposed to supporting only specific flash models) by parsing SFDP standard parameters and then deciding what address width is to be used.

- Rajat

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi Rajat,

On 26.10.18 11:59, Rajat Srivastava wrote:

<snip>

>>  From what I read, Rajat's method is to extend the controller driver to
>> support read SFDP and default to 4-byte mode if supported, or
>> overwritten by user's flag. Stefan's method is to read 4-byte status bit
>> and doesn't change controller driver.
>>
>> Is the default value of this 4-byte status bit valid and correct for all
>> cases?
>>
>> Rajat, without your patch set, does Stefan's solution work for your board?
>>
>> York
> 
> No. Stefan's changes are specific to his boards and is not
> applicable on ours.
> Stefan's patch is to support only certain flash that are factory
> strapped to work in 4-byte addressing modes only and will default
> to old method if such a flash is not found.

That is not 100% correct. The flash does not need to be "factory
strapped" to this 4-byte mode. It needs to be configured to this
4-byte mode. And this is also the case for example, when the ROM
bootloader configures the flash this way or a soft-reboot (without
hard flash chip reset) reboots into U-Boot after the flash was
configured to 4-byte mode in Linux.

And it supports multiple vendors and can easily be extended to
support more. The only thing needed is the detection of the
3-byte vs 4-byte address mode. But please note that it currently
does not actively switch from 3-byte to 4-byte mode. This could
be added though if needed / wanted.

> The flashes on our
> boards (and also other vendor's board) will not work with Stefan's
> patch.
> 
> My patch can handle flashes with address widths of 3-byte, 4-byte
> or both. It also takes a more generic path (as opposed to supporting
> only specific flash models) by parsing SFDP standard parameters and
> then deciding what address width is to be used.

I still don't see why each and every SPI controller driver
needs to be extended to support this SFDP parameter reading. Can't
this be handled by some generic (read common) code part in the
SPI flash infrastructure?

Thanks,
Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

Hi Stefan

> -----Original Message-----
> From: Stefan Roese <sr@denx.de>
> Sent: Friday, October 26, 2018 3:42 PM
> To: Rajat Srivastava <rajat.srivastava@nxp.com>; York Sun
> <york.sun@nxp.com>; Jagan Teki <jagan@openedev.com>
> Cc: u-boot at lists.denx.de; simon.k.r.goldschmidt at gmail.com; Ashish Kumar
> <ashish.kumar@nxp.com>
> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> standard 3-byte mode)
> 
> Hi Rajat,
> 
> On 26.10.18 11:59, Rajat Srivastava wrote:
> 
> <snip>
> 
> >>  From what I read, Rajat's method is to extend the controller driver
> >> to support read SFDP and default to 4-byte mode if supported, or
> >> overwritten by user's flag. Stefan's method is to read 4-byte status
> >> bit and doesn't change controller driver.
> >>
> >> Is the default value of this 4-byte status bit valid and correct for
> >> all cases?
> >>
> >> Rajat, without your patch set, does Stefan's solution work for your board?
> >>
> >> York
> >
> > No. Stefan's changes are specific to his boards and is not applicable
> > on ours.
> > Stefan's patch is to support only certain flash that are factory
> > strapped to work in 4-byte addressing modes only and will default to
> > old method if such a flash is not found.
> 
> That is not 100% correct. The flash does not need to be "factory strapped" to
> this 4-byte mode. It needs to be configured to this 4-byte mode. And this is
> also the case for example, when the ROM bootloader configures the flash
> this way or a soft-reboot (without hard flash chip reset) reboots into U-Boot
> after the flash was configured to 4-byte mode in Linux.
> 
> And it supports multiple vendors and can easily be extended to support
> more. The only thing needed is the detection of the 3-byte vs 4-byte address
> mode. But please note that it currently does not actively switch from 3-byte
> to 4-byte mode. This could be added though if needed / wanted.

Spansion flashes (on our boards) support both 3-byte and 4-byte addressing modes which needs active switching. What could be added to your patch is already supported in SFDP method. SFDP also provides information about page size, flash density, read/write/erase commands, etc, in addition to getting information about address width supported by flash.

> 
> > The flashes on our
> > boards (and also other vendor's board) will not work with Stefan's
> > patch.
> >
> > My patch can handle flashes with address widths of 3-byte, 4-byte or
> > both. It also takes a more generic path (as opposed to supporting only
> > specific flash models) by parsing SFDP standard parameters and then
> > deciding what address width is to be used.
> 
> I still don't see why each and every SPI controller driver needs to be
> extended to support this SFDP parameter reading. Can't this be handled by
> some generic (read common) code part in the SPI flash infrastructure?

The generic (read common) code part in the SPI flash infrastructure also lands on respective SPI drivers which ultimately sends the commands to flash. Currently every SPI driver has support for basic read command (which is called after "generic read common code") but no driver has support for SFDP reading, which is what needs to be added.
Also, only the drivers that want to make use of SFDP needs to extend support for SFDP parameter reading.

I am reiterating that this is how SFDP parsing has been implemented in Linux as well and this is the generic way. Driver support is absolutely required because SPI framework cannot directly send any command to any flash.

Thanks
Rajat

> 
> Thanks,
> Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[York Sun]

On 10/26/18 03:36, Rajat Srivastava wrote:

<snip>

> Spansion flashes (on our boards) support both 3-byte and 4-byte addressing modes which needs active switching. What could be added to your patch is already supported in SFDP method. SFDP also provides information about page size, flash density, read/write/erase commands, etc, in addition to getting information about address width supported by flash.
> 
>>
>>> The flashes on our
>>> boards (and also other vendor's board) will not work with Stefan's
>>> patch.
>>>
>>> My patch can handle flashes with address widths of 3-byte, 4-byte or
>>> both. It also takes a more generic path (as opposed to supporting only
>>> specific flash models) by parsing SFDP standard parameters and then
>>> deciding what address width is to be used.
>>
>> I still don't see why each and every SPI controller driver needs to be
>> extended to support this SFDP parameter reading. Can't this be handled by
>> some generic (read common) code part in the SPI flash infrastructure?
> 
> The generic (read common) code part in the SPI flash infrastructure also lands on respective SPI drivers which ultimately sends the commands to flash. Currently every SPI driver has support for basic read command (which is called after "generic read common code") but no driver has support for SFDP reading, which is what needs to be added.
> Also, only the drivers that want to make use of SFDP needs to extend support for SFDP parameter reading.
> 
> I am reiterating that this is how SFDP parsing has been implemented in Linux as well and this is the generic way. Driver support is absolutely required because SPI framework cannot directly send any command to any flash.
> 

It sounds like your difference is how to detect 4-byte addressing should
be used.

Stefan's method is from the flash chip side, to check status register.
The benefit is to preserve setting before U-Boot. I am not sure if it is
necessary to preserve previous setting though.

Rajat's method is from the controller side, to read SFDP. It can support
all flash chips I presume.

I guess Stefan's method will not determine Rajat's flash as 4-byte
addressing because it indeed supports both 3- and 4-byte. To make
Rajat's method work on Stefan's board, SFDP needs to be extended to the
controller driver Stefan uses, is that right?

I also guess reading SFDP is controller-specific and cannot be done in a
generic way. If the new feature is useful and can be gradually extended
to other controllers, it sounds good to me. My guesses could wrong though.

York

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

Hi Stefan

Sorry for top-posting.

Why can't we read SFDP parameters from flash and auto-detect 3-byte/4-byte addressing mode?
Using address width information we can support both types of flash i.e. flashes supporting 3-byte addressing mode as well as flashes supporting 4-byte addressing mode.

I've floated a similar patch in U-boot that reads and parses SFDP parameters from flash and auto-detects its addressing mode. It send commands according to the address width it detects.
Please find the patch set at:
https://patchwork.ozlabs.org/cover/985326/
https://patchwork.ozlabs.org/patch/985327/
https://patchwork.ozlabs.org/patch/985329/
https://patchwork.ozlabs.org/patch/985328/

Thanks
Rajat

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Stefan Roese
> Sent: Thursday, October 11, 2018 8:20 PM
> To: u-boot at lists.denx.de
> Cc: Jagan Teki <jagan@openedev.com>
> Subject: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs 
> standard 3-byte mode)
> 
> Some SPI NOR chips only support 4-byte mode addressing. Here the 
> default 3- byte mode does not work and leads to incorrect accesses. 
> This patch now reads the 4-byte mode status bit (in this case in the 
> CR register of the Macronix SPI
> NOR) and configures the SPI transfers accordingly.
> 
> This was noticed on the LinkIt Smart 7688 modul, which is equipped 
> with an Macronix MX25L25635F device. But this device does *NOT* 
> support switching to 3-byte mode via the EX4B command.
> 
> This should also work when the bootrom configures the SPI flash to 
> 4-byte mode and runs U-Boot after this. U-Boot should dectect this 
> mode (if the 4-byte mode detection is available for this chip) and use the correct OPs in this case.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Jagan Teki <jagan@openedev.com>
> Tested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> v3:
> - Rebased on latest version (merge conflict because of new patches
>   from Simon Glass)
> - Added Tested-by tag from Simon Goldschmidt
> 
> v2:
> - Integrated STMICRO 4-byte detection from Simon
> 
>  drivers/mtd/spi/sf_internal.h |   3 +-
>  drivers/mtd/spi/spi_flash.c   | 131 ++++++++++++++++++++++++++++------
>  include/spi_flash.h           |   5 ++
>  3 files changed, 118 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/spi/sf_internal.h 
> b/drivers/mtd/spi/sf_internal.h index
> 4f63cacc64..eb076401d1 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -26,7 +26,8 @@ enum spi_nor_option_flags {  };
> 
>  #define SPI_FLASH_3B_ADDR_LEN		3
> -#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_3B_ADDR_LEN)
> +#define SPI_FLASH_4B_ADDR_LEN		4
> +#define SPI_FLASH_CMD_MAX_LEN		(1 +
> SPI_FLASH_4B_ADDR_LEN)
>  #define SPI_FLASH_16MB_BOUN		0x1000000
> 
>  /* CFI Manufacture ID's */
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c 
> index 9230060364..b22eea2d1c 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -20,12 +20,19 @@
> 
>  #include "sf_internal.h"
> 
> -static void spi_flash_addr(u32 addr, u8 *cmd)
> +static void spi_flash_addr(struct spi_flash *flash, u32 addr, u8 
> +*cmd)
>  {
>  	/* cmd[0] is actual command */
> -	cmd[1] = addr >> 16;
> -	cmd[2] = addr >> 8;
> -	cmd[3] = addr >> 0;
> +	if (flash->in_4byte_mode) {
> +		cmd[1] = addr >> 24;
> +		cmd[2] = addr >> 16;
> +		cmd[3] = addr >> 8;
> +		cmd[4] = addr >> 0;
> +	} else {
> +		cmd[1] = addr >> 16;
> +		cmd[2] = addr >> 8;
> +		cmd[3] = addr >> 0;
> +	}
>  }
> 
>  static int read_sr(struct spi_flash *flash, u8 *rs) @@ -110,6 +117,72 
> @@ static int write_cr(struct spi_flash *flash, u8 wc)  }  #endif
> 
> +#if defined(CONFIG_SPI_FLASH_MACRONIX)
> +static bool flash_in_4byte_mode_macronix(struct spi_flash *flash) {
> +	int ret;
> +	u8 cr;
> +	u8 cmd;
> +
> +	cmd = 0x15;	/* Macronix: read configuration register RDCR */
> +	ret = spi_flash_read_common(flash, &cmd, 1, &cr, 1);
> +	if (ret < 0) {
> +		debug("SF: fail to read config register\n");
> +		return false;
> +	}
> +
> +	/* Return true, if 4-byte mode is enabled */
> +	if (cr & BIT(5))
> +		return true;
> +
> +	return false;
> +}
> +#else
> +static bool flash_in_4byte_mode_macronix(struct spi_flash *flash) {
> +	return false;
> +}
> +#endif
> +
> +#if defined(CONFIG_SPI_FLASH_STMICRO) static bool 
> +flash_in_4byte_mode_stmicro(struct spi_flash *flash) {
> +	int ret;
> +	u8 fsr;
> +	u8 cmd;
> +
> +	cmd = 0x70;	/* STMicro/Micron: read flag status register */
> +	ret = spi_flash_read_common(flash, &cmd, 1, &fsr, 1);
> +	if (ret < 0) {
> +		debug("SF: fail to read config register\n");
> +		return false;
> +	}
> +
> +	/* Return true, if 4-byte mode is enabled */
> +	if (fsr & BIT(0))
> +		return true;
> +
> +	return false;
> +}
> +#else
> +static bool flash_in_4byte_mode_stmicro(struct spi_flash *flash) {
> +	return false;
> +}
> +#endif
> +
> +static bool flash_in_4byte_mode(struct spi_flash *flash,
> +				const struct spi_flash_info *info) {
> +	if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_MACRONIX)
> +		return flash_in_4byte_mode_macronix(flash);
> +
> +	if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_STMICRO)
> +		return flash_in_4byte_mode_stmicro(flash);
> +
> +	return false;
> +}
> +
>  #ifdef CONFIG_SPI_FLASH_BAR
>  /*
>   * This "clean_bar" is necessary in a situation when one was 
> accessing @@ -
> 314,7 +387,7 @@ int spi_flash_write_common(struct spi_flash *flash, 
> const u8 *cmd,  int spi_flash_cmd_erase_ops(struct spi_flash *flash, 
> u32 offset, size_t
> len)  {
>  	u32 erase_size, erase_addr;
> -	u8 cmd[SPI_FLASH_CMD_LEN];
> +	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
>  	int ret = -1;
> 
>  	erase_size = flash->erase_size;
> @@ -344,12 +417,13 @@ int spi_flash_cmd_erase_ops(struct spi_flash 
> *flash,
> u32 offset, size_t len)
>  		if (ret < 0)
>  			return ret;
>  #endif
> -		spi_flash_addr(erase_addr, cmd);
> +		spi_flash_addr(flash, erase_addr, cmd);
> 
>  		debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
>  		      cmd[2], cmd[3], erase_addr);
> 
> -		ret = spi_flash_write_common(flash, cmd, sizeof(cmd), NULL,
> 0);
> +		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
> +					     NULL, 0);
>  		if (ret < 0) {
>  			debug("SF: erase failed\n");
>  			break;
> @@ -373,7 +447,7 @@ int spi_flash_cmd_write_ops(struct spi_flash 
> *flash,
> u32 offset,
>  	unsigned long byte_addr, page_size;
>  	u32 write_addr;
>  	size_t chunk_len, actual;
> -	u8 cmd[SPI_FLASH_CMD_LEN];
> +	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
>  	int ret = -1;
> 
>  	page_size = flash->page_size;
> @@ -404,15 +478,15 @@ int spi_flash_cmd_write_ops(struct spi_flash 
> *flash,
> u32 offset,
> 
>  		if (spi->max_write_size)
>  			chunk_len = min(chunk_len,
> -					spi->max_write_size - sizeof(cmd));
> +					spi->max_write_size - flash->cmdlen);
> 
> -		spi_flash_addr(write_addr, cmd);
> +		spi_flash_addr(flash, write_addr, cmd);
> 
>  		debug("SF: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = 
> %zu\n",
>  		      buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
> 
> -		ret = spi_flash_write_common(flash, cmd, sizeof(cmd),
> -					buf + actual, chunk_len);
> +		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
> +					     buf + actual, chunk_len);
>  		if (ret < 0) {
>  			debug("SF: write failed\n");
>  			break;
> @@ -469,9 +543,11 @@ int spi_flash_cmd_read_ops(struct spi_flash 
> *flash,
> u32 offset,  {
>  	struct spi_slave *spi = flash->spi;
>  	u8 cmdsz;
> -	u32 remain_len, read_len, read_addr;
> +	u64 remain_len;
> +	u32 read_len, read_addr;
>  	int bank_sel = 0;
>  	int ret = 0;
> +	int shift;
> 
>  	/* Handle memory-mapped SPI */
>  	if (flash->memory_map) {
> @@ -487,7 +563,7 @@ int spi_flash_cmd_read_ops(struct spi_flash 
> *flash, u32 offset,
>  		return 0;
>  	}
> 
> -	cmdsz = SPI_FLASH_CMD_LEN + flash->dummy_byte;
> +	cmdsz = flash->cmdlen + flash->dummy_byte;
>  	u8 cmd[cmdsz];
> 
>  	cmd[0] = flash->read_cmd;
> @@ -504,8 +580,13 @@ int spi_flash_cmd_read_ops(struct spi_flash 
> *flash,
> u32 offset,
>  			return log_ret(ret);
>  		bank_sel = flash->bank_curr;
>  #endif
> -		remain_len = ((SPI_FLASH_16MB_BOUN << flash->shift) *
> -				(bank_sel + 1)) - offset;
> +		shift = flash->shift;
> +		if (flash->in_4byte_mode)
> +			shift += 8;
> +
> +		remain_len = (((u64)SPI_FLASH_16MB_BOUN << shift) *
> +			      (bank_sel + 1)) - offset;
> +
>  		if (len < remain_len)
>  			read_len = len;
>  		else
> @@ -514,7 +595,7 @@ int spi_flash_cmd_read_ops(struct spi_flash 
> *flash, u32 offset,
>  		if (spi->max_read_size)
>  			read_len = min(read_len, spi->max_read_size);
> 
> -		spi_flash_addr(read_addr, cmd);
> +		spi_flash_addr(flash, read_addr, cmd);
> 
>  		ret = spi_flash_read_common(flash, cmd, cmdsz, data, read_len);
>  		if (ret < 0) {
> @@ -1155,6 +1236,13 @@ int spi_flash_scan(struct spi_flash *flash)
>  		write_sr(flash, sr);
>  	}
> 
> +	/* Set default value for cmd length */
> +	flash->cmdlen = 1 + SPI_FLASH_3B_ADDR_LEN;
> +	if (flash_in_4byte_mode(flash, info)) {
> +		flash->in_4byte_mode = true;
> +		flash->cmdlen = 1 + SPI_FLASH_4B_ADDR_LEN;
> +	}
> +
>  	flash->name = info->name;
>  	flash->memory_map = spi->memory_map;
> 
> @@ -1306,14 +1394,17 @@ int spi_flash_scan(struct spi_flash *flash)
>  	print_size(flash->size, "");
>  	if (flash->memory_map)
>  		printf(", mapped at %p", flash->memory_map);
> +	if (flash->in_4byte_mode)
> +		printf(" (4-byte mode)");
>  	puts("\n");
>  #endif
> 
>  #ifndef CONFIG_SPI_FLASH_BAR
> -	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
> -	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
> +	if ((((flash->dual_flash == SF_SINGLE_FLASH) &&
> +	      (flash->size > SPI_FLASH_16MB_BOUN)) ||
>  	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
> -	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
> +	      (flash->size > SPI_FLASH_16MB_BOUN << 1))) &&
> +	    !flash->in_4byte_mode) {
>  		puts("SF: Warning - Only lower 16MiB accessible,");
>  		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
>  	}
> diff --git a/include/spi_flash.h b/include/spi_flash.h index
> 0ec98fb55d..b5bc4a85f6 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -36,6 +36,8 @@ struct spi_slave;
>   * @dual_flash:		Indicates dual flash memories - dual stacked, parallel
>   * @shift:		Flash shift useful in dual parallel
>   * @flags:		Indication of spi flash flags
> + * @in_4byte_mode:	True if flash is detected to be in 4-byte mode
> + * @cmdlen:		CMD length (3-byte vs 4-byte mode)
>   * @size:		Total flash size
>   * @page_size:		Write (page) size
>   * @sector_size:	Sector size
> @@ -69,6 +71,9 @@ struct spi_flash {
>  	u8 shift;
>  	u16 flags;
> 
> +	bool in_4byte_mode;
> +	int cmdlen;
> +
>  	u32 size;
>  	u32 page_size;
>  	u32 sector_size;
> --
> 2.19.1
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> ts.de
> nx.de%2Flistinfo%2Fu-
> boot&amp;data=02%7C01%7Cprabhakar.kushwaha%40nxp.com%7C532d6ed3
> ad764b27306b08d62f88d688%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636748662097523237&amp;sdata=FpR4s4evDeod9zQPSTMSjigIIAn
> mOj50aFqqH21Ofms%3D&amp;reserved=0

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi Rajat,

On 17.10.18 13:52, Rajat Srivastava wrote:
> Hi Stefan
> 
> Sorry for top-posting.
> 
> Why can't we read SFDP parameters from flash and auto-detect
> 3-byte/4-byte addressing mode?
> Using address width information we can support both types of flash
> i.e. flashes supporting 3-byte addressing mode as well as flashes
> supporting 4-byte addressing mode.

Our flash supports 3- and 4-byte addressing mode. But this special
chip is factory strapped to only support 4-byte mode, even though
its device ID tells us that it should support also 3-byte mode.
This current pretty simple patch enables the use of this flash
with very limited code additions. It also helps others (Simon on
SoCFPGA) with their issues regarding 3-byte vs 4-byte mode -
especially in regard to the bootrom and its setup.
  
> I've floated a similar patch in U-boot that reads and parses SFDP
> parameters from flash and auto-detects its addressing mode. It send
> commands according to the address width it detects.
> Please find the patch set at:
> https://patchwork.ozlabs.org/cover/985326/
> https://patchwork.ozlabs.org/patch/985327/
> https://patchwork.ozlabs.org/patch/985329/
> https://patchwork.ozlabs.org/patch/985328/

I've just applied your 3 patches and have added SFDP support for
our equipped SPI chip (with my patch not applied):

-       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL | WR_QPP) },
+       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL | WR_QPP | SPI_FLASH_USE_SFDP) },

This does not seem to work though:

SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0 Bytes
*** Warning - bad CRC, using default environment

Please note that I'm not opposed to using your SFDP support. But
in our case it does not work - at least not without the
SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
fully detect the chip parameters. So I would prefer to go ahead
with my patch for now.

Thanks,
Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

Hi Stefan

> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de]
> Sent: Monday, October 22, 2018 12:45 PM
> To: Rajat Srivastava <rajat.srivastava@nxp.com>; jagan at openedev.com;
> simon.k.r.goldschmidt at gmail.com
> Cc: Ashish Kumar <ashish.kumar@nxp.com>; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> standard 3-byte mode)
> 
> Hi Rajat,
> 
> On 17.10.18 13:52, Rajat Srivastava wrote:
> > Hi Stefan
> >
> > Sorry for top-posting.
> >
> > Why can't we read SFDP parameters from flash and auto-detect
> > 3-byte/4-byte addressing mode?
> > Using address width information we can support both types of flash
> > i.e. flashes supporting 3-byte addressing mode as well as flashes
> > supporting 4-byte addressing mode.
> 
> Our flash supports 3- and 4-byte addressing mode. But this special
> chip is factory strapped to only support 4-byte mode, even though
> its device ID tells us that it should support also 3-byte mode.
> This current pretty simple patch enables the use of this flash
> with very limited code additions. It also helps others (Simon on
> SoCFPGA) with their issues regarding 3-byte vs 4-byte mode -
> especially in regard to the bootrom and its setup.

If you look into my patch, for the flashes that support both
3-byte and 4-byte addressing modes, the default addressing mode is set to
4-byte. In such case if the user wants to send a command in 3-byte mode
then he has to set a flag. So SFDP path will be able to handle the special
chip that is factory strapped to 4-byte addressing mode.

Code snippet from patch which handles 3-byte/4-byte/both addressing modes:
+ switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
+        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: // flashes with 3-byte addressing
+                flash->addr_width = 3;
+                break;
+        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: // flashes with both 3 or 4 byte
+                printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+                printf("mode on command\n");
+                /*
+                 * By default, 4-byte addressing mode is set.
+                 * To enforce 3-byte addressing mode, set addrwd_3_in_use flag                                                                                             
+                 * in struct spi_flash for every command.
+                 */
+                flash->addr_width = 4;
+                break;
+         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: // flashes with 4-byte addressing
+                flash->addr_width = 4;
+                break;

> 
> > I've floated a similar patch in U-boot that reads and parses SFDP
> > parameters from flash and auto-detects its addressing mode. It send
> > commands according to the address width it detects.
> > Please find the patch set at:
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fcover%2F985326%2F&amp;data=02%7C01%7Crajat.srivast
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=M2aU
> WUxSn9wmlBlYj336%2Bay5rwOddG%2Br7Qn5kH%2Bf1uw%3D&amp;reserved=
> 0
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fpatch%2F985327%2F&amp;data=02%7C01%7Crajat.srivast
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=IIzUJuI
> 9nL5Wn7K5uAqjig9edpW6YIIcSOExNJNB5qE%3D&amp;reserved=0
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fpatch%2F985329%2F&amp;data=02%7C01%7Crajat.srivast
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=N5qQJ
> E1776Wb3siJApPDCkUyY4vn0ZVLjCebn4hi6bk%3D&amp;reserved=0
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fpatch%2F985328%2F&amp;data=02%7C01%7Crajat.srivast
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=tC%2F
> %2FsGVwV%2FrHBPX1gJ5TNYmVnJOL13XpAjgP87w3%2Bx0%3D&amp;reserved
> =0
> 
> I've just applied your 3 patches and have added SFDP support for
> our equipped SPI chip (with my patch not applied):
> 
> -       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> WR_QPP) },
> +       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> WR_QPP | SPI_FLASH_USE_SFDP) },
> 
> This does not seem to work though:

Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP work. You'll
need to add the driver code corresponding to the mtd layer code (in spi_flash.c)
which will send the actual READ SFDP command to flash.

The patch-set I floated adds driver code in fsl_qspi.c (Freescale/NXP QSPI driver).
Please find the patch at https://patchwork.ozlabs.org/patch/985329. 

After the mtd layer calls spi_flash_read_common() function to send any read
command to flash, it lands on driver which ultimately fires the command
(in this case 0x5A command to read SFDP) to flash. 

Since you say the flash is designed to support only 4-byte addressing mode
then it is possible that the READ_SFDP command (0x5A) is also required to be
sent in 4-byte mode (My patch sends 0x5A in 3-byte addressing mode which
is also the SFDP standard that every other flash supports).
Although I looked up the datasheet of mx25l25635f and it says that the
READ_SFDP command will be sent in 3-byte mode (as supported by my patch).

> 
> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0 Bytes
> *** Warning - bad CRC, using default environment
> 
> Please note that I'm not opposed to using your SFDP support. But
> in our case it does not work - at least not without the
> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
> fully detect the chip parameters. So I would prefer to go ahead
> with my patch for now.

My point is that if your case can be handled by adding a generic code,
instead of flash specific code then we should prefer the generic approach, what
do you say?

Thanks
Rajat

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi Rajat,

On 23.10.18 07:17, Rajat Srivastava wrote:
>> -----Original Message-----
>> From: Stefan Roese [mailto:sr at denx.de]
>> Sent: Monday, October 22, 2018 12:45 PM
>> To: Rajat Srivastava <rajat.srivastava@nxp.com>; jagan at openedev.com;
>> simon.k.r.goldschmidt at gmail.com
>> Cc: Ashish Kumar <ashish.kumar@nxp.com>; u-boot at lists.denx.de
>> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
>> standard 3-byte mode)
>>
>> Hi Rajat,
>>
>> On 17.10.18 13:52, Rajat Srivastava wrote:
>>> Hi Stefan
>>>
>>> Sorry for top-posting.
>>>
>>> Why can't we read SFDP parameters from flash and auto-detect
>>> 3-byte/4-byte addressing mode?
>>> Using address width information we can support both types of flash
>>> i.e. flashes supporting 3-byte addressing mode as well as flashes
>>> supporting 4-byte addressing mode.
>>
>> Our flash supports 3- and 4-byte addressing mode. But this special
>> chip is factory strapped to only support 4-byte mode, even though
>> its device ID tells us that it should support also 3-byte mode.
>> This current pretty simple patch enables the use of this flash
>> with very limited code additions. It also helps others (Simon on
>> SoCFPGA) with their issues regarding 3-byte vs 4-byte mode -
>> especially in regard to the bootrom and its setup.
> 
> If you look into my patch, for the flashes that support both
> 3-byte and 4-byte addressing modes, the default addressing mode is set to
> 4-byte. In such case if the user wants to send a command in 3-byte mode
> then he has to set a flag. So SFDP path will be able to handle the special
> chip that is factory strapped to 4-byte addressing mode.
> 
> Code snippet from patch which handles 3-byte/4-byte/both addressing modes:
> + switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> +        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: // flashes with 3-byte addressing
> +                flash->addr_width = 3;
> +                break;
> +        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: // flashes with both 3 or 4 byte
> +                printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
> +                printf("mode on command\n");
> +                /*
> +                 * By default, 4-byte addressing mode is set.
> +                 * To enforce 3-byte addressing mode, set addrwd_3_in_use flag
> +                 * in struct spi_flash for every command.
> +                 */
> +                flash->addr_width = 4;
> +                break;
> +         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: // flashes with 4-byte addressing
> +                flash->addr_width = 4;
> +                break;
> 
>>
>>> I've floated a similar patch in U-boot that reads and parses SFDP
>>> parameters from flash and auto-detects its addressing mode. It send
>>> commands according to the address width it detects.
>>> Please find the patch set at:
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.ozlabs.org%2Fcover%2F985326%2F&amp;data=02%7C01%7Crajat.srivast
>> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
>> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=M2aU
>> WUxSn9wmlBlYj336%2Bay5rwOddG%2Br7Qn5kH%2Bf1uw%3D&amp;reserved=
>> 0
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.ozlabs.org%2Fpatch%2F985327%2F&amp;data=02%7C01%7Crajat.srivast
>> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
>> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=IIzUJuI
>> 9nL5Wn7K5uAqjig9edpW6YIIcSOExNJNB5qE%3D&amp;reserved=0
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.ozlabs.org%2Fpatch%2F985329%2F&amp;data=02%7C01%7Crajat.srivast
>> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
>> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=N5qQJ
>> E1776Wb3siJApPDCkUyY4vn0ZVLjCebn4hi6bk%3D&amp;reserved=0
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.ozlabs.org%2Fpatch%2F985328%2F&amp;data=02%7C01%7Crajat.srivast
>> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
>> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=tC%2F
>> %2FsGVwV%2FrHBPX1gJ5TNYmVnJOL13XpAjgP87w3%2Bx0%3D&amp;reserved
>> =0
>>
>> I've just applied your 3 patches and have added SFDP support for
>> our equipped SPI chip (with my patch not applied):
>>
>> -       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
>> WR_QPP) },
>> +       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
>> WR_QPP | SPI_FLASH_USE_SFDP) },
>>
>> This does not seem to work though:
> 
> Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP work. You'll
> need to add the driver code corresponding to the mtd layer code (in spi_flash.c)
> which will send the actual READ SFDP command to flash.
> 
> The patch-set I floated adds driver code in fsl_qspi.c (Freescale/NXP QSPI driver).
> Please find the patch at https://patchwork.ozlabs.org/patch/985329.
> 
> After the mtd layer calls spi_flash_read_common() function to send any read
> command to flash, it lands on driver which ultimately fires the command
> (in this case 0x5A command to read SFDP) to flash.

So you say, that each SPI driver needs to get some changes to
support the SFDP reading? That does not sound like a generic
approach to me. But maybe I misunderstood this.
  
> Since you say the flash is designed to support only 4-byte addressing mode

No. The flash itself (as you have seen in the datasheet) supports
both. But the special chip version equipped on the LinkIt MT7688
boards is somehow strapped to only support 4-byte mode. This is
not documented anywhere (and did cost me quite some time to figure
it out).

> then it is possible that the READ_SFDP command (0x5A) is also required to be
> sent in 4-byte mode (My patch sends 0x5A in 3-byte addressing mode which
> is also the SFDP standard that every other flash supports).
> Although I looked up the datasheet of mx25l25635f and it says that the
> READ_SFDP command will be sent in 3-byte mode (as supported by my patch).
>
>>
>> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0 Bytes
>> *** Warning - bad CRC, using default environment
>>
>> Please note that I'm not opposed to using your SFDP support. But
>> in our case it does not work - at least not without the
>> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
>> fully detect the chip parameters. So I would prefer to go ahead
>> with my patch for now.
> 
> My point is that if your case can be handled by adding a generic code,
> instead of flash specific code then we should prefer the generic approach, what
> do you say?

Please see above, my questions about SPI driver additions for this
SFDP support. This does not sound very "generic" to me.

But I think both solutions, your SFDP support and my "simple"
pre-configured 3-byte vs 4-byte addressing mode detection and
usage can co-exist.

Thanks,
Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

Hi Stefan

> -----Original Message-----
> From: Stefan Roese <sr@denx.de>
> Sent: Tuesday, October 23, 2018 10:31 PM
> To: Rajat Srivastava <rajat.srivastava@nxp.com>; jagan at openedev.com;
> simon.k.r.goldschmidt at gmail.com
> Cc: Ashish Kumar <ashish.kumar@nxp.com>; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> standard 3-byte mode)
> 
> Hi Rajat,
> 
> On 23.10.18 07:17, Rajat Srivastava wrote:
> >> -----Original Message-----
> >> From: Stefan Roese [mailto:sr at denx.de]
> >> Sent: Monday, October 22, 2018 12:45 PM
> >> To: Rajat Srivastava <mailto:rajat.srivastava@nxp.com>; mailto:jagan at openedev.com;
> >> mailto:simon.k.r.goldschmidt at gmail.com
> >> Cc: Ashish Kumar <mailto:ashish.kumar@nxp.com>; mailto:u-boot at lists.denx.de
> >> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> >> standard 3-byte mode)
> >>
> >> Hi Rajat,
> >>
> >> On 17.10.18 13:52, Rajat Srivastava wrote:
> >>> Hi Stefan
> >>>
> >>> Sorry for top-posting.
> >>>
> >>> Why can't we read SFDP parameters from flash and auto-detect
> >>> 3-byte/4-byte addressing mode?
> >>> Using address width information we can support both types of flash
> >>> i.e. flashes supporting 3-byte addressing mode as well as flashes
> >>> supporting 4-byte addressing mode.
> >>
> >> Our flash supports 3- and 4-byte addressing mode. But this special
> >> chip is factory strapped to only support 4-byte mode, even though
> >> its device ID tells us that it should support also 3-byte mode.
> >> This current pretty simple patch enables the use of this flash
> >> with very limited code additions. It also helps others (Simon on
> >> SoCFPGA) with their issues regarding 3-byte vs 4-byte mode -
> >> especially in regard to the bootrom and its setup.
> >
> > If you look into my patch, for the flashes that support both
> > 3-byte and 4-byte addressing modes, the default addressing mode is set to
> > 4-byte. In such case if the user wants to send a command in 3-byte mode
> > then he has to set a flag. So SFDP path will be able to handle the special
> > chip that is factory strapped to 4-byte addressing mode.
> >
> > Code snippet from patch which handles 3-byte/4-byte/both addressing modes:
> > + switch (bfpt.dwords[BFPT_DWORD(1)] &
> BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> > +        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: // flashes with 3-byte
> addressing
> > +                flash->addr_width = 3;
> > +                break;
> > +        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: // flashes with both 3 or
> 4 byte
> > +                printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
> > +                printf("mode on command\n");
> > +                /*
> > +                 * By default, 4-byte addressing mode is set.
> > +                 * To enforce 3-byte addressing mode, set addrwd_3_in_use flag
> > +                 * in struct spi_flash for every command.
> > +                 */
> > +                flash->addr_width = 4;
> > +                break;
> > +         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: // flashes with 4-byte
> addressing
> > +                flash->addr_width = 4;
> > +                break;
> >
> >>
> >>> I've floated a similar patch in U-boot that reads and parses SFDP
> >>> parameters from flash and auto-detects its addressing mode. It send
> >>> commands according to the address width it detects.
> >>> Please find the patch set at:
> >>>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> >>
> work.ozlabs.org%2Fcover%2F985326%2F&amp;data=02%7C01%7Crajat.srivast
> >>
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> >>
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=M2aU
> >>
> WUxSn9wmlBlYj336%2Bay5rwOddG%2Br7Qn5kH%2Bf1uw%3D&amp;reserved=
> >> 0
> >>>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> >>
> work.ozlabs.org%2Fpatch%2F985327%2F&amp;data=02%7C01%7Crajat.srivast
> >>
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> >>
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=IIzUJuI
> >> 9nL5Wn7K5uAqjig9edpW6YIIcSOExNJNB5qE%3D&amp;reserved=0
> >>>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> >>
> work.ozlabs.org%2Fpatch%2F985329%2F&amp;data=02%7C01%7Crajat.srivast
> >>
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> >>
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=N5qQJ
> >> E1776Wb3siJApPDCkUyY4vn0ZVLjCebn4hi6bk%3D&amp;reserved=0
> >>>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> >>
> work.ozlabs.org%2Fpatch%2F985328%2F&amp;data=02%7C01%7Crajat.srivast
> >>
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b4
> >>
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=tC%2F
> >>
> %2FsGVwV%2FrHBPX1gJ5TNYmVnJOL13XpAjgP87w3%2Bx0%3D&amp;reserved
> >> =0
> >>
> >> I've just applied your 3 patches and have added SFDP support for
> >> our equipped SPI chip (with my patch not applied):
> >>
> >> -       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> >> WR_QPP) },
> >> +       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> >> WR_QPP | SPI_FLASH_USE_SFDP) },
> >>
> >> This does not seem to work though:
> >
> > Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP work.
> You'll
> > need to add the driver code corresponding to the mtd layer code (in
> spi_flash.c)
> > which will send the actual READ SFDP command to flash.
> >
> > The patch-set I floated adds driver code in fsl_qspi.c (Freescale/NXP QSPI
> driver).
> > Please find the patch at
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F985329&amp;data=02%7C01%7Crajat.srivastava%40nxp.com%7Cc967de2c68464c6561b508d639091e21%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636759108670133878&amp;sdata=uZfP5lWvGmpA%2Fxi5AS06c15PKqYwwndgU%2BC3zYu2K3w%3D&amp;reserved=0.
> >
> > After the mtd layer calls spi_flash_read_common() function to send any read
> > command to flash, it lands on driver which ultimately fires the command
> > (in this case 0x5A command to read SFDP) to flash.
> 
> So you say, that each SPI driver needs to get some changes to
> support the SFDP reading? That does not sound like a generic
> approach to me. But maybe I misunderstood this.

To read SFDP parameters, a READ_SFDP command (0x5A) needs to be sent to flash which can be sent only with help of a SPI driver.
Even if one wants to initiate a simple basic 1-byte read operation, there must exist a SPI driver that actually sends the read command to flash.

I think this is a generic approach because that is how mtd framework is designed.

> 
> > Since you say the flash is designed to support only 4-byte addressing mode
> 
> No. The flash itself (as you have seen in the datasheet) supports
> both. But the special chip version equipped on the LinkIt MT7688
> boards is somehow strapped to only support 4-byte mode. This is
> not documented anywhere (and did cost me quite some time to figure
> it out).

Ok I get it. I think this flash does not follow standard SFDP framework. Can you please confirm?

> 
> > then it is possible that the READ_SFDP command (0x5A) is also required to be
> > sent in 4-byte mode (My patch sends 0x5A in 3-byte addressing mode which
> > is also the SFDP standard that every other flash supports).
> > Although I looked up the datasheet of mx25l25635f and it says that the
> > READ_SFDP command will be sent in 3-byte mode (as supported by my patch).
> >
> >>
> >> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0
> Bytes
> >> *** Warning - bad CRC, using default environment
> >>
> >> Please note that I'm not opposed to using your SFDP support. But
> >> in our case it does not work - at least not without the
> >> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
> >> fully detect the chip parameters. So I would prefer to go ahead
> >> with my patch for now.
> >
> > My point is that if your case can be handled by adding a generic code,
> > instead of flash specific code then we should prefer the generic approach,
> what
> > do you say?
> 
> Please see above, my questions about SPI driver additions for this
> SFDP support. This does not sound very "generic" to me.

As mentioned above, this is the generic approach and this is how Linux has also implemented SFDP. By design, the SPI driver should adapt to changes in mtd framework.

> 
> But I think both solutions, your SFDP support and my "simple"
> pre-configured 3-byte vs 4-byte addressing mode detection and
> usage can co-exist.

Ok.
Let us wait for the SPI maintainer to comment on this since for both solutions to co-exist we may have to resolve code conflicts.

> 
> Thanks,
> Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi Rajat,

On 25.10.18 11:28, Rajat Srivastava wrote:

<snip>

>>> Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP work.
>> You'll
>>> need to add the driver code corresponding to the mtd layer code (in
>> spi_flash.c)
>>> which will send the actual READ SFDP command to flash.
>>>
>>> The patch-set I floated adds driver code in fsl_qspi.c (Freescale/NXP QSPI
>> driver).
>>> Please find the patch at
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F985329&amp;data=02%7C01%7Crajat.srivastava%40nxp.com%7Cc967de2c68464c6561b508d639091e21%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636759108670133878&amp;sdata=uZfP5lWvGmpA%2Fxi5AS06c15PKqYwwndgU%2BC3zYu2K3w%3D&amp;reserved=0.
>>>
>>> After the mtd layer calls spi_flash_read_common() function to send any read
>>> command to flash, it lands on driver which ultimately fires the command
>>> (in this case 0x5A command to read SFDP) to flash.
>>
>> So you say, that each SPI driver needs to get some changes to
>> support the SFDP reading? That does not sound like a generic
>> approach to me. But maybe I misunderstood this.
> 
> To read SFDP parameters, a READ_SFDP command (0x5A) needs to be
> sent to flash which can be sent only with help of a SPI driver.
> Even if one wants to initiate a simple basic 1-byte read operation,
> there must exist a SPI driver that actually sends the read command
> to flash.

But does this READ SFDP stuff need to be handled in each and every
SPI driver (platform specific)? Can't this be moved to a common
location in the SPI flash framework so that the platform specific
drivers don't need to be changed here?
  
> I think this is a generic approach because that is how mtd
> framework is designed.
> 
>>
>>> Since you say the flash is designed to support only 4-byte addressing mode
>>
>> No. The flash itself (as you have seen in the datasheet) supports
>> both. But the special chip version equipped on the LinkIt MT7688
>> boards is somehow strapped to only support 4-byte mode. This is
>> not documented anywhere (and did cost me quite some time to figure
>> it out).
> 
> Ok I get it. I think this flash does not follow standard SFDP
> framework. Can you please confirm?

This specific flash chip most likely not. But I'm not 100% sure,
as I've never tried to use it this way yet.
  
>>
>>> then it is possible that the READ_SFDP command (0x5A) is also required to be
>>> sent in 4-byte mode (My patch sends 0x5A in 3-byte addressing mode which
>>> is also the SFDP standard that every other flash supports).
>>> Although I looked up the datasheet of mx25l25635f and it says that the
>>> READ_SFDP command will be sent in 3-byte mode (as supported by my patch).
>>>
>>>>
>>>> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0
>> Bytes
>>>> *** Warning - bad CRC, using default environment
>>>>
>>>> Please note that I'm not opposed to using your SFDP support. But
>>>> in our case it does not work - at least not without the
>>>> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
>>>> fully detect the chip parameters. So I would prefer to go ahead
>>>> with my patch for now.
>>>
>>> My point is that if your case can be handled by adding a generic code,
>>> instead of flash specific code then we should prefer the generic approach,
>> what
>>> do you say?
>>
>> Please see above, my questions about SPI driver additions for this
>> SFDP support. This does not sound very "generic" to me.
> 
> As mentioned above, this is the generic approach and this is how
> Linux has also implemented SFDP. By design, the SPI driver should
> adapt to changes in mtd framework.

Again my question from above, if this SFDP read needs to be
added to all SPI device drivers, or if this can be handled
in a common place?
  
>>
>> But I think both solutions, your SFDP support and my "simple"
>> pre-configured 3-byte vs 4-byte addressing mode detection and
>> usage can co-exist.
> 
> Ok.
> Let us wait for the SPI maintainer to comment on this since for
> both solutions to co-exist we may have to resolve code conflicts.

Sure. Resolving such potential merge conflicts should not be too
hard though.

Thanks,
Stefan

[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Rajat Srivastava]

Hi Jagan

> -----Original Message-----
> From: Rajat Srivastava
> Sent: Thursday, October 25, 2018 2:59 PM
> To: 'Stefan Roese' <sr@denx.de>; jagan at openedev.com;
> simon.k.r.goldschmidt at gmail.com
> Cc: Ashish Kumar <ashish.kumar@nxp.com>; u-boot at lists.denx.de; York Sun
> <york.sun@nxp.com>
> Subject: RE: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs
> standard 3-byte mode)
> 
> Hi Stefan
> 
> > -----Original Message-----
> > From: Stefan Roese <sr@denx.de>
> > Sent: Tuesday, October 23, 2018 10:31 PM
> > To: Rajat Srivastava <rajat.srivastava@nxp.com>; jagan at openedev.com;
> > simon.k.r.goldschmidt at gmail.com
> > Cc: Ashish Kumar <ashish.kumar@nxp.com>; u-boot at lists.denx.de
> > Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode
> > (vs standard 3-byte mode)
> >
> > Hi Rajat,
> >
> > On 23.10.18 07:17, Rajat Srivastava wrote:
> > >> -----Original Message-----
> > >> From: Stefan Roese [mailto:sr at denx.de]
> > >> Sent: Monday, October 22, 2018 12:45 PM
> > >> To: Rajat Srivastava <mailto:rajat.srivastava@nxp.com>;
> > >> mailto:jagan at openedev.com; mailto:simon.k.r.goldschmidt at gmail.com
> > >> Cc: Ashish Kumar <mailto:ashish.kumar@nxp.com>;
> > >> mailto:u-boot at lists.denx.de
> > >> Subject: Re: [U-Boot] [PATCH v3] sf: Add auto detection of 4-byte
> > >> mode (vs standard 3-byte mode)
> > >>
> > >> Hi Rajat,
> > >>
> > >> On 17.10.18 13:52, Rajat Srivastava wrote:
> > >>> Hi Stefan
> > >>>
> > >>> Sorry for top-posting.
> > >>>
> > >>> Why can't we read SFDP parameters from flash and auto-detect
> > >>> 3-byte/4-byte addressing mode?
> > >>> Using address width information we can support both types of flash
> > >>> i.e. flashes supporting 3-byte addressing mode as well as flashes
> > >>> supporting 4-byte addressing mode.
> > >>
> > >> Our flash supports 3- and 4-byte addressing mode. But this special
> > >> chip is factory strapped to only support 4-byte mode, even though
> > >> its device ID tells us that it should support also 3-byte mode.
> > >> This current pretty simple patch enables the use of this flash with
> > >> very limited code additions. It also helps others (Simon on
> > >> SoCFPGA) with their issues regarding 3-byte vs 4-byte mode -
> > >> especially in regard to the bootrom and its setup.
> > >
> > > If you look into my patch, for the flashes that support both 3-byte
> > > and 4-byte addressing modes, the default addressing mode is set to
> > > 4-byte. In such case if the user wants to send a command in 3-byte
> > > mode then he has to set a flag. So SFDP path will be able to handle
> > > the special chip that is factory strapped to 4-byte addressing mode.
> > >
> > > Code snippet from patch which handles 3-byte/4-byte/both addressing
> modes:
> > > + switch (bfpt.dwords[BFPT_DWORD(1)] &
> > BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> > > +        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: // flashes with
> > > + 3-byte
> > addressing
> > > +                flash->addr_width = 3;
> > > +                break;
> > > +        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: // flashes with both
> > > + 3 or
> > 4 byte
> > > +                printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
> > > +                printf("mode on command\n");
> > > +                /*
> > > +                 * By default, 4-byte addressing mode is set.
> > > +                 * To enforce 3-byte addressing mode, set addrwd_3_in_use
> flag
> > > +                 * in struct spi_flash for every command.
> > > +                 */
> > > +                flash->addr_width = 4;
> > > +                break;
> > > +         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: // flashes with
> > > + 4-byte
> > addressing
> > > +                flash->addr_width = 4;
> > > +                break;
> > >
> > >>
> > >>> I've floated a similar patch in U-boot that reads and parses SFDP
> > >>> parameters from flash and auto-detects its addressing mode. It
> > >>> send commands according to the address width it detects.
> > >>> Please find the patch set at:
> > >>>
> > >>
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > ch
> > >>
> >
> work.ozlabs.org%2Fcover%2F985326%2F&amp;data=02%7C01%7Crajat.srivas
> t
> > >>
> >
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b
> 4
> > >>
> >
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=M2aU
> > >>
> >
> WUxSn9wmlBlYj336%2Bay5rwOddG%2Br7Qn5kH%2Bf1uw%3D&amp;reserv
> ed=
> > >> 0
> > >>>
> > >>
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > ch
> > >>
> >
> work.ozlabs.org%2Fpatch%2F985327%2F&amp;data=02%7C01%7Crajat.srivas
> t
> > >>
> >
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b
> 4
> > >>
> >
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=IIzUJuI
> > >> 9nL5Wn7K5uAqjig9edpW6YIIcSOExNJNB5qE%3D&amp;reserved=0
> > >>>
> > >>
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > ch
> > >>
> >
> work.ozlabs.org%2Fpatch%2F985329%2F&amp;data=02%7C01%7Crajat.srivas
> t
> > >>
> >
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b
> 4
> > >>
> >
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=N5qQJ
> > >> E1776Wb3siJApPDCkUyY4vn0ZVLjCebn4hi6bk%3D&amp;reserved=0
> > >>>
> > >>
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > ch
> > >>
> >
> work.ozlabs.org%2Fpatch%2F985328%2F&amp;data=02%7C01%7Crajat.srivas
> t
> > >>
> >
> ava%40nxp.com%7C56b81d26a6954324bebd08d637ee0c35%7C686ea1d3bc2b
> 4
> > >>
> >
> c6fa92cd99c5c301635%7C0%7C0%7C636757892882342025&amp;sdata=tC%2F
> > >>
> >
> %2FsGVwV%2FrHBPX1gJ5TNYmVnJOL13XpAjgP87w3%2Bx0%3D&amp;reserv
> ed
> > >> =0
> > >>
> > >> I've just applied your 3 patches and have added SFDP support for
> > >> our equipped SPI chip (with my patch not applied):
> > >>
> > >> -       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> > >> WR_QPP) },
> > >> +       {"mx25l25635f",    INFO(0xc22019, 0x0, 64 * 1024,   512, RD_FULL |
> > >> WR_QPP | SPI_FLASH_USE_SFDP) },
> > >>
> > >> This does not seem to work though:
> > >
> > > Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP
> work.
> > You'll
> > > need to add the driver code corresponding to the mtd layer code (in
> > spi_flash.c)
> > > which will send the actual READ SFDP command to flash.
> > >
> > > The patch-set I floated adds driver code in fsl_qspi.c
> > > (Freescale/NXP QSPI
> > driver).
> > > Please find the patch at
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> chwork.ozlabs.org%2Fpatch%2F985329&amp;data=02%7C01%7Crajat.srivast
> ava%40nxp.com%7Cc967de2c68464c6561b508d639091e21%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C636759108670133878&amp;sdata=uZfP5
> lWvGmpA%2Fxi5AS06c15PKqYwwndgU%2BC3zYu2K3w%3D&amp;reserved=
> 0.
> > >
> > > After the mtd layer calls spi_flash_read_common() function to send
> > > any read command to flash, it lands on driver which ultimately fires
> > > the command (in this case 0x5A command to read SFDP) to flash.
> >
> > So you say, that each SPI driver needs to get some changes to support
> > the SFDP reading? That does not sound like a generic approach to me.
> > But maybe I misunderstood this.
> 
> To read SFDP parameters, a READ_SFDP command (0x5A) needs to be sent
> to flash which can be sent only with help of a SPI driver.
> Even if one wants to initiate a simple basic 1-byte read operation, there must
> exist a SPI driver that actually sends the read command to flash.
> 
> I think this is a generic approach because that is how mtd framework is
> designed.
> 
> >
> > > Since you say the flash is designed to support only 4-byte
> > > addressing mode
> >
> > No. The flash itself (as you have seen in the datasheet) supports
> > both. But the special chip version equipped on the LinkIt MT7688
> > boards is somehow strapped to only support 4-byte mode. This is not
> > documented anywhere (and did cost me quite some time to figure it
> > out).
> 
> Ok I get it. I think this flash does not follow standard SFDP framework. Can
> you please confirm?
> 
> >
> > > then it is possible that the READ_SFDP command (0x5A) is also
> > > required to be sent in 4-byte mode (My patch sends 0x5A in 3-byte
> > > addressing mode which is also the SFDP standard that every other flash
> supports).
> > > Although I looked up the datasheet of mx25l25635f and it says that
> > > the READ_SFDP command will be sent in 3-byte mode (as supported by
> my patch).
> > >
> > >>
> > >> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB,
> > >> total 0
> > Bytes
> > >> *** Warning - bad CRC, using default environment
> > >>
> > >> Please note that I'm not opposed to using your SFDP support. But in
> > >> our case it does not work - at least not without the
> > >> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
> > >> fully detect the chip parameters. So I would prefer to go ahead
> > >> with my patch for now.
> > >
> > > My point is that if your case can be handled by adding a generic
> > > code, instead of flash specific code then we should prefer the
> > > generic approach,
> > what
> > > do you say?
> >
> > Please see above, my questions about SPI driver additions for this
> > SFDP support. This does not sound very "generic" to me.
> 
> As mentioned above, this is the generic approach and this is how Linux has
> also implemented SFDP. By design, the SPI driver should adapt to changes in
> mtd framework.
> 
> >
> > But I think both solutions, your SFDP support and my "simple"
> > pre-configured 3-byte vs 4-byte addressing mode detection and usage
> > can co-exist.
> 
> Ok.
> Let us wait for the SPI maintainer to comment on this since for both solutions
> to co-exist we may have to resolve code conflicts.

Are you ok with the decision on co-existence of both methods?

Thanks
Rajat

> 
> >
> > Thanks,
> > Stefan