[U-Boot] [PATCH v2 01/16] sf: Add bank address register writing support

[U-Boot] [PATCH v2 01/16] sf: Add bank address register writing support

[Jagannadha Sutradharudu Teki]

This patch provides support to program a flash bank address
register.

extended/bank address register contains an information to access
the 4th byte addressing in 3-byte address mode.

Currently added an bank address register writing support for
spansion flashes.

reff' the spec for more details about bank addr register
in Page-63, Table 8.16
http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
	- none

 drivers/mtd/spi/spi_flash.c          | 37 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi/spi_flash_internal.h |  6 ++++++
 include/spi_flash.h                  |  2 ++
 3 files changed, 45 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 0e38f59..7aba520 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -278,6 +278,40 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
 	return 0;
 }
 
+int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel)
+{
+	u8 cmd, idcode0;
+	int ret;
+
+	idcode0 = flash->idcode0;
+	if (idcode0 == 0x01) {
+		cmd = CMD_BANKADDR_BRWR;
+	} else {
+		printf("SF: Unsupported bank addr write %02x\n", idcode0);
+		return -1;
+	}
+
+	ret = spi_flash_cmd_write_enable(flash);
+	if (ret < 0) {
+		debug("SF: enabling write failed\n");
+		return ret;
+	}
+
+	ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &bank_sel, 1);
+	if (ret) {
+		debug("SF: fail to write bank addr register\n");
+		return ret;
+	}
+
+	ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
+	if (ret < 0) {
+		debug("SF: write bank addr register timed out\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_OF_CONTROL
 int spi_flash_decode_fdt(const void *blob, struct spi_flash *flash)
 {
@@ -422,6 +456,9 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 		goto err_manufacturer_probe;
 	}
 
+	/* store the manuf id, for performing specific flash ops */
+	flash->idcode0 = *idp;
+
 #ifdef CONFIG_OF_CONTROL
 	if (spi_flash_decode_fdt(gd->fdt_blob, flash)) {
 		debug("SF: FDT decode error\n");
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 141cfa8..6e38494 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -28,6 +28,9 @@
 #define CMD_ERASE_64K			0xd8
 #define CMD_ERASE_CHIP			0xc7
 
+/* Bank addr acess commands */
+#define CMD_BANKADDR_BRWR		0x17
+
 /* Common status */
 #define STATUS_WIP			0x01
 
@@ -77,6 +80,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
 /* Program the status register. */
 int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
 
+/* Program the bank address register */
+int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel);
+
 /*
  * Same as spi_flash_cmd_read() except it also claims/releases the SPI
  * bus. Used as common part of the ->read() operation.
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 3b6a44e..5ea42e1 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -38,6 +38,8 @@ struct spi_flash {
 	u32		page_size;
 	/* Erase (sector) size */
 	u32		sector_size;
+	/* ID code0 */
+	u8              idcode0;
 
 	void *memory_map;	/* Address of read-only SPI flash access */
 	int		(*read)(struct spi_flash *flash, u32 offset,
-- 
1.8.3

[U-Boot] [PATCH v2 02/16] sf: Add bank address register reading support

[Jagannadha Sutradharudu Teki]

This patch provides support to read a flash bank address register.

reading extended/bank address register will give whether the flash
is operated on extended/bank addressing or normal addressing in
3-byte address mode.

Currently added an extended/bank address register reading support
for spansion flashes.

reff' the spec for more details about bank addr register
in Page-63, Table 8.16
http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 15 +++++++++++++++
 drivers/mtd/spi/spi_flash_internal.h |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 7aba520..193de42 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -312,6 +312,21 @@ int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel)
 	return 0;
 }
 
+int spi_flash_cmd_bankaddr_read(struct spi_flash *flash, void *data)
+{
+	u8 cmd;
+	u8 idcode0 = flash->idcode0;
+
+	if (idcode0 == 0x01) {
+		cmd = CMD_BANKADDR_BRRD;
+	} else {
+		printf("SF: Unsupported bank addr read %02x\n", idcode0);
+		return -1;
+	}
+
+	return spi_flash_read_common(flash, &cmd, 1, data, 1);
+}
+
 #ifdef CONFIG_OF_CONTROL
 int spi_flash_decode_fdt(const void *blob, struct spi_flash *flash)
 {
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 6e38494..2567bbc 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -30,6 +30,7 @@
 
 /* Bank addr acess commands */
 #define CMD_BANKADDR_BRWR		0x17
+#define CMD_BANKADDR_BRRD		0x16
 
 /* Common status */
 #define STATUS_WIP			0x01
@@ -83,6 +84,9 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
 /* Program the bank address register */
 int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel);
 
+/* Read the bank address register */
+int spi_flash_cmd_bankaddr_read(struct spi_flash *flash, void *data);
+
 /*
  * Same as spi_flash_cmd_read() except it also claims/releases the SPI
  * bus. Used as common part of the ->read() operation.
-- 
1.8.3

[U-Boot] [PATCH v2 03/16] sf: Add extended addr write support for winbond|stmicro

[Jagannadha Sutradharudu Teki]

This patch provides support to program a flash extended address
register for winbond and stmicro SPI flashes.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 2 ++
 drivers/mtd/spi/spi_flash_internal.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 193de42..05d1792 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -286,6 +286,8 @@ int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel)
 	idcode0 = flash->idcode0;
 	if (idcode0 == 0x01) {
 		cmd = CMD_BANKADDR_BRWR;
+	} else if ((idcode0 == 0xef) || (idcode0 == 0x20)) {
+		cmd = CMD_EXTNADDR_WREAR;
 	} else {
 		printf("SF: Unsupported bank addr write %02x\n", idcode0);
 		return -1;
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 2567bbc..de1a0df 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -31,6 +31,7 @@
 /* Bank addr acess commands */
 #define CMD_BANKADDR_BRWR		0x17
 #define CMD_BANKADDR_BRRD		0x16
+#define CMD_EXTNADDR_WREAR		0xC5
 
 /* Common status */
 #define STATUS_WIP			0x01
-- 
1.8.3

[U-Boot] [PATCH v2 04/16] sf: Add extended addr read support for winbond|stmicro

[Jagannadha Sutradharudu Teki]

This patch provides support to read a flash extended address
register for winbond and stmicro SPI flashes.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 2 ++
 drivers/mtd/spi/spi_flash_internal.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 05d1792..66b6b14 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -321,6 +321,8 @@ int spi_flash_cmd_bankaddr_read(struct spi_flash *flash, void *data)
 
 	if (idcode0 == 0x01) {
 		cmd = CMD_BANKADDR_BRRD;
+	} else if ((idcode0 == 0xef) || (idcode0 == 0x20)) {
+		cmd = CMD_EXTNADDR_RDEAR;
 	} else {
 		printf("SF: Unsupported bank addr read %02x\n", idcode0);
 		return -1;
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index de1a0df..1b29e2d 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -32,6 +32,7 @@
 #define CMD_BANKADDR_BRWR		0x17
 #define CMD_BANKADDR_BRRD		0x16
 #define CMD_EXTNADDR_WREAR		0xC5
+#define CMD_EXTNADDR_RDEAR		0xC8
 
 /* Common status */
 #define STATUS_WIP			0x01
-- 
1.8.3

[U-Boot] [PATCH v2 05/16] sf: read flash bank addr register at probe time

[Jagannadha Sutradharudu Teki]

Read the flash bank addr register to get the state of bank in
a perticular flash. and also bank write happens only when there is
a change in bank selection from user.

bank read only valid for flashes which has > 16Mbytes those are
opearted in 3-byte addr mode, each bank occupies 16Mytes.

Suppose if the flash has 64Mbytes size consists of 4 banks like
bank0, bank1, bank2 and bank3.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 17 +++++++++++++++++
 drivers/mtd/spi/spi_flash_internal.h |  2 ++
 include/spi_flash.h                  |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 66b6b14..4576a16 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -283,6 +283,11 @@ int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel)
 	u8 cmd, idcode0;
 	int ret;
 
+	if (flash->bank_curr == bank_sel) {
+		debug("SF: not require to enable bank%d\n", bank_sel);
+		return 0;
+	}
+
 	idcode0 = flash->idcode0;
 	if (idcode0 == 0x01) {
 		cmd = CMD_BANKADDR_BRWR;
@@ -304,6 +309,7 @@ int spi_flash_cmd_bankaddr_write(struct spi_flash *flash, u8 bank_sel)
 		debug("SF: fail to write bank addr register\n");
 		return ret;
 	}
+	flash->bank_curr = bank_sel;
 
 	ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
 	if (ret < 0) {
@@ -432,6 +438,7 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 	struct spi_flash *flash = NULL;
 	int ret, i, shift;
 	u8 idcode[IDCODE_LEN], *idp;
+	u8 curr_bank = 0;
 
 	spi = spi_setup_slave(bus, cs, max_hz, spi_mode);
 	if (!spi) {
@@ -491,6 +498,16 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 		printf(", mapped@%p", flash->memory_map);
 	puts("\n");
 
+	if (flash->size > SPI_FLASH_16MB_BOUN) {
+		if (spi_flash_cmd_bankaddr_read(flash, &curr_bank)) {
+			debug("SF: fail to read bank addr register\n");
+			goto err_manufacturer_probe;
+		}
+		flash->bank_curr = curr_bank;
+	} else {
+		flash->bank_curr = curr_bank;
+	}
+
 	spi_release_bus(spi);
 
 	return flash;
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 1b29e2d..455dc02 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -12,6 +12,8 @@
 #define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONFIG_SYS_HZ)
 #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONFIG_SYS_HZ)
 
+#define SPI_FLASH_16MB_BOUN		0x1000000
+
 /* Common commands */
 #define CMD_READ_ID			0x9f
 
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 5ea42e1..acac17a 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -40,6 +40,8 @@ struct spi_flash {
 	u32		sector_size;
 	/* ID code0 */
 	u8              idcode0;
+	/* Current flash bank */
+	u8		bank_curr;
 
 	void *memory_map;	/* Address of read-only SPI flash access */
 	int		(*read)(struct spi_flash *flash, u32 offset,
-- 
1.8.3

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagannadha Sutradharudu Teki]

Updated the spi_flash framework to handle all sizes of flashes
using bank/extd addr reg facility

The current implementation in spi_flash supports 3-byte address mode
due to this up to 16Mbytes amount of flash is able to access for those
flashes which has an actual size of > 16MB.

As most of the flashes introduces a bank/extd address registers
for accessing the flashes in 16Mbytes of banks if the flash size
is > 16Mbytes, this new scheme will add the bank selection feature
for performing write/erase operations on all flashes.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 4576a16..5386884 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
 	unsigned long page_addr, byte_addr, page_size;
 	size_t chunk_len, actual;
 	int ret;
-	u8 cmd[4];
+	u8 cmd[4], bank_sel;
 
 	page_size = flash->page_size;
-	page_addr = offset / page_size;
-	byte_addr = offset % page_size;
 
 	ret = spi_claim_bus(flash->spi);
 	if (ret) {
@@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
 
 	cmd[0] = CMD_PAGE_PROGRAM;
 	for (actual = 0; actual < len; actual += chunk_len) {
+		bank_sel = offset / SPI_FLASH_16MB_BOUN;
+
+		ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
+		if (ret) {
+			debug("SF: fail to set bank%d\n", bank_sel);
+			return ret;
+		}
+
+		page_addr = offset / page_size;
+		byte_addr = offset % page_size;
 		chunk_len = min(len - actual, page_size - byte_addr);
 
 		if (flash->spi->max_write_size)
@@ -117,11 +125,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
 		if (ret)
 			break;
 
-		byte_addr += chunk_len;
-		if (byte_addr == page_size) {
-			page_addr++;
-			byte_addr = 0;
-		}
+		offset += chunk_len;
 	}
 
 	spi_release_bus(flash->spi);
@@ -204,9 +208,9 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
 
 int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
-	u32 end, erase_size;
+	u32 erase_size;
 	int ret;
-	u8 cmd[4];
+	u8 cmd[4], bank_sel;
 
 	erase_size = flash->sector_size;
 	if (offset % erase_size || len % erase_size) {
@@ -224,11 +228,17 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
 		cmd[0] = CMD_ERASE_4K;
 	else
 		cmd[0] = CMD_ERASE_64K;
-	end = offset + len;
 
-	while (offset < end) {
+	while (len) {
+		bank_sel = offset / SPI_FLASH_16MB_BOUN;
+
+		ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
+		if (ret) {
+			debug("SF: fail to set bank%d\n", bank_sel);
+			return ret;
+		}
+
 		spi_flash_addr(offset, cmd);
-		offset += erase_size;
 
 		debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
 		      cmd[2], cmd[3], offset);
@@ -244,6 +254,9 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
 		ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT);
 		if (ret)
 			goto out;
+
+		offset += erase_size;
+		len -= erase_size;
 	}
 
  out:
-- 
1.8.3

[U-Boot] [PATCH v2 07/16] sf: Update sf read to support all sizes of flashes

[Jagannadha Sutradharudu Teki]

This patch updated the spi_flash read func to support all
sizes of flashes using bank reg addr facility.

The same support has been added in below patch for erase/write
spi_flash functions:
"sf: Support all sizes of flashes using bank addr reg facility"
(sha1: c956f600cbb0943d0afe1004cdb503f4fcd8f415)

With these new updates on sf framework, the flashes which has < 16MB
are not effected as per as performance is concern and but the
u-boot.bin size incrased ~600 bytes.

sf update(for first 16MBytes), Changes before:
U-Boot> sf update 0x1000000 0x0 0x1000000
- N25Q256
  16777216 bytes written, 0 bytes skipped in 199.72s, speed 86480 B/s
- W25Q128BV
  16777216 bytes written, 0 bytes skipped in 351.739s, speed 48913 B/s
- S25FL256S_64K
  16777216 bytes written, 0 bytes skipped in 65.659s, speed 262144 B/s

sf update(for first 16MBytes), Changes before:
U-Boot> sf update 0x1000000 0x0 0x1000000
- N25Q256
  16777216 bytes written, 0 bytes skipped in 198.953s, speed 86480 B/s
- W25Q128BV
  16777216 bytes written, 0 bytes skipped in 350.90s, speed 49200 B/s
- S25FL256S_64K
  16777216 bytes written, 0 bytes skipped in 66.521s, speed 262144 B/s

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 5386884..40c0389 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -148,7 +148,9 @@ int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,
 int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset,
 		size_t len, void *data)
 {
-	u8 cmd[5];
+	u8 cmd[5], bank_sel;
+	u32 remain_len, read_len;
+	int ret = -1;
 
 	/* Handle memory-mapped SPI */
 	if (flash->memory_map) {
@@ -157,10 +159,38 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset,
 	}
 
 	cmd[0] = CMD_READ_ARRAY_FAST;
-	spi_flash_addr(offset, cmd);
 	cmd[4] = 0x00;
 
-	return spi_flash_read_common(flash, cmd, sizeof(cmd), data, len);
+	while (len) {
+		bank_sel = offset / SPI_FLASH_16MB_BOUN;
+
+		ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
+		if (ret) {
+			debug("SF: fail to set bank%d\n", bank_sel);
+			return ret;
+		}
+
+		remain_len = (SPI_FLASH_16MB_BOUN * (bank_sel + 1) - offset);
+		if (len < remain_len)
+			read_len = len;
+		else
+			read_len = remain_len;
+
+		spi_flash_addr(offset, cmd);
+
+		ret = spi_flash_read_common(flash, cmd, sizeof(cmd),
+							data, read_len);
+		if (ret < 0) {
+			debug("SF: read failed\n");
+			break;
+		}
+
+		offset += read_len;
+		len -= read_len;
+		data += read_len;
+	}
+
+	return ret;
 }
 
 int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
-- 
1.8.3

[U-Boot] [PATCH v2 08/16] sf: Use spi_flash_addr() in write call

[Jagannadha Sutradharudu Teki]

Use the existing spi_flash_addr() for 3-byte addressing
cmd filling in write call.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 40c0389..821aa2e 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -71,7 +71,7 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len,
 int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
 		size_t len, const void *buf)
 {
-	unsigned long page_addr, byte_addr, page_size;
+	unsigned long byte_addr, page_size;
 	size_t chunk_len, actual;
 	int ret;
 	u8 cmd[4], bank_sel;
@@ -94,16 +94,13 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
 			return ret;
 		}
 
-		page_addr = offset / page_size;
 		byte_addr = offset % page_size;
 		chunk_len = min(len - actual, page_size - byte_addr);
 
 		if (flash->spi->max_write_size)
 			chunk_len = min(chunk_len, flash->spi->max_write_size);
 
-		cmd[1] = page_addr >> 8;
-		cmd[2] = page_addr;
-		cmd[3] = byte_addr;
+		spi_flash_addr(offset, cmd);
 
 		debug("PP: 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);
-- 
1.8.3

[U-Boot] [PATCH v2 09/16] sf: stmicro: Add support for N25Q512

[Jagannadha Sutradharudu Teki]

Add support for Numonyx N25Q512 SPI flash.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/stmicro.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index 2a9972b..e9adfa5 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -140,6 +140,12 @@ static const struct stmicro_spi_flash_params stmicro_spi_flash_table[] = {
 		.nr_sectors = 512,
 		.name = "N25Q256A",
 	},
+	{
+		.id = 0xba20,
+		.pages_per_sector = 256,
+		.nr_sectors = 1024,
+		.name = "N25Q512",
+	},
 };
 
 struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
-- 
1.8.3

[U-Boot] [PATCH v2 10/16] sf: stmicro: Add support for N25Q512A

[Jagannadha Sutradharudu Teki]

Add support for Numonyx N25Q512A SPI flash.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/stmicro.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index e9adfa5..bf61a37 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -146,6 +146,12 @@ static const struct stmicro_spi_flash_params stmicro_spi_flash_table[] = {
 		.nr_sectors = 1024,
 		.name = "N25Q512",
 	},
+	{
+		.id = 0xbb20,
+		.pages_per_sector = 256,
+		.nr_sectors = 1024,
+		.name = "N25Q512A",
+	},
 };
 
 struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
-- 
1.8.3

[U-Boot] [PATCH v2 11/16] sf: stmicro: Add support for N25Q1024

[Jagannadha Sutradharudu Teki]

Add support for Numonyx N25Q1024 SPI flash.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/stmicro.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index bf61a37..cac1013 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -152,6 +152,12 @@ static const struct stmicro_spi_flash_params stmicro_spi_flash_table[] = {
 		.nr_sectors = 1024,
 		.name = "N25Q512A",
 	},
+	{
+		.id = 0xba21,
+		.pages_per_sector = 256,
+		.nr_sectors = 2048,
+		.name = "N25Q1024",
+	},
 };
 
 struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
-- 
1.8.3

[U-Boot] [PATCH v2 12/16] sf: stmicro: Add support for N25Q1024A

[Jagannadha Sutradharudu Teki]

Add support for Numonyx N25Q1024A SPI flash.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/stmicro.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index cac1013..ef4b911 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -158,6 +158,12 @@ static const struct stmicro_spi_flash_params stmicro_spi_flash_table[] = {
 		.nr_sectors = 2048,
 		.name = "N25Q1024",
 	},
+	{
+		.id = 0xbb21,
+		.pages_per_sector = 256,
+		.nr_sectors = 2048,
+		.name = "N25Q1024A",
+	},
 };
 
 struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
-- 
1.8.3

[U-Boot] [PATCH v2 13/16] sf: spansion: Add support for S25FL512S_256K

[Jagannadha Sutradharudu Teki]

Add support for Spansion S25FL512S_256K SPI flash.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spansion.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
index dad4fbb..1ded39f 100644
--- a/drivers/mtd/spi/spansion.c
+++ b/drivers/mtd/spi/spansion.c
@@ -110,6 +110,13 @@ static const struct spansion_spi_flash_params spansion_spi_flash_table[] = {
 		.nr_sectors = 512,
 		.name = "S25FL256S_64K",
 	},
+	{
+		.idcode1 = 0x0220,
+		.idcode2 = 0x4d00,
+		.pages_per_sector = 256,
+		.nr_sectors = 1024,
+		.name = "S25FL512S_256K",
+	},
 };
 
 struct spi_flash *spi_flash_probe_spansion(struct spi_slave *spi, u8 *idcode)
-- 
1.8.3

[U-Boot] [PATCH v2 14/16] sf: Use spi_flash_read_common() in write status poll

[Jagannadha Sutradharudu Teki]

Instead of using spi_xfer for SPI_XFER_BEGIN and SPI_XFER_END
separatley use common read call spi_flash_read_common() which
does the same.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 821aa2e..765d4bc 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -193,32 +193,25 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset,
 int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
 			   u8 cmd, u8 poll_bit)
 {
-	struct spi_slave *spi = flash->spi;
 	unsigned long timebase;
 	int ret;
 	u8 status;
 
-	ret = spi_xfer(spi, 8, &cmd, NULL, SPI_XFER_BEGIN);
-	if (ret) {
-		debug("SF: Failed to send command %02x: %d\n", cmd, ret);
-		return ret;
-	}
-
 	timebase = get_timer(0);
 	do {
 		WATCHDOG_RESET();
 
-		ret = spi_xfer(spi, 8, NULL, &status, 0);
-		if (ret)
-			return -1;
+		ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
+		if (ret < 0) {
+			debug("SF: fail to read read status register\n");
+			return ret;
+		}
 
 		if ((status & poll_bit) == 0)
 			break;
 
 	} while (get_timer(timebase) < timeout);
 
-	spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
-
 	if ((status & poll_bit) == 0)
 		return 0;
 
-- 
1.8.3

[U-Boot] [PATCH v2 15/16] sf: Remove spi_flash_cmd_poll_bit()

[Jagannadha Sutradharudu Teki]

There is no other call other than spi_flash_cmd_wait_ready(),
hence removed spi_flash_cmd_poll_bit and use the poll status code
spi_flash_cmd_wait_ready() itself.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 11 +++--------
 drivers/mtd/spi/spi_flash_internal.h |  4 ----
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 765d4bc..527423d 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -190,12 +190,13 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset,
 	return ret;
 }
 
-int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
-			   u8 cmd, u8 poll_bit)
+int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
 {
 	unsigned long timebase;
 	int ret;
 	u8 status;
+	u8 poll_bit = STATUS_WIP;
+	u8 cmd = CMD_READ_STATUS;
 
 	timebase = get_timer(0);
 	do {
@@ -220,12 +221,6 @@ int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
 	return -1;
 }
 
-int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
-{
-	return spi_flash_cmd_poll_bit(flash, timeout,
-		CMD_READ_STATUS, STATUS_WIP);
-}
-
 int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	u32 erase_size;
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 455dc02..ac4530f 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -98,10 +98,6 @@ int spi_flash_cmd_bankaddr_read(struct spi_flash *flash, void *data);
 int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,
 		size_t cmd_len, void *data, size_t data_len);
 
-/* Send a command to the device and wait for some bit to clear itself. */
-int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
-			   u8 cmd, u8 poll_bit);
-
 /*
  * Send the read status command to the device and wait for the wip
  * (write-in-progress) bit to clear itself.
-- 
1.8.3

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagannadha Sutradharudu Teki]

Flag status register polling is required for micron 512Mb flash
devices onwards, for performing erase/program operations.

Like polling for WIP(Write-In-Progress) bit in read status register,
spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
bit in flag status register.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
---
Changes for v2:
        - none

 drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
 drivers/mtd/spi/spi_flash_internal.h |  3 +++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 527423d..8cd2988 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
 	unsigned long timebase;
 	int ret;
 	u8 status;
+	u8 check_status = 0x0;
 	u8 poll_bit = STATUS_WIP;
 	u8 cmd = CMD_READ_STATUS;
 
+	if ((flash->idcode0 == 0x20) &&
+			(flash->size >= SPI_FLASH_512MB_STMIC)) {
+		poll_bit = STATUS_PEC;
+		check_status = poll_bit;
+		cmd = CMD_FLAG_STATUS;
+	}
+
 	timebase = get_timer(0);
 	do {
 		WATCHDOG_RESET();
 
 		ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
 		if (ret < 0) {
-			debug("SF: fail to read read status register\n");
+			debug("SF: fail to read %s status register\n",
+				cmd == CMD_READ_STATUS ? "read" : "flag");
 			return ret;
 		}
 
-		if ((status & poll_bit) == 0)
+		if ((status & poll_bit) == check_status)
 			break;
 
 	} while (get_timer(timebase) < timeout);
 
-	if ((status & poll_bit) == 0)
+	if ((status & poll_bit) == check_status)
 		return 0;
 
 	/* Timed out */
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index ac4530f..cb7a505 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -13,6 +13,7 @@
 #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONFIG_SYS_HZ)
 
 #define SPI_FLASH_16MB_BOUN		0x1000000
+#define SPI_FLASH_512MB_STMIC		0x4000000
 
 /* Common commands */
 #define CMD_READ_ID			0x9f
@@ -24,6 +25,7 @@
 #define CMD_PAGE_PROGRAM		0x02
 #define CMD_WRITE_DISABLE		0x04
 #define CMD_READ_STATUS			0x05
+#define CMD_FLAG_STATUS			0x70
 #define CMD_WRITE_ENABLE		0x06
 #define CMD_ERASE_4K			0x20
 #define CMD_ERASE_32K			0x52
@@ -38,6 +40,7 @@
 
 /* Common status */
 #define STATUS_WIP			0x01
+#define STATUS_PEC			0x80
 
 /* Send a single-byte command to the device and read the response */
 int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t len);
-- 
1.8.3

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Simon Glass]

Hi Jagan,

On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki <
jagannadha.sutradharudu-teki@xilinx.com> wrote:

> Updated the spi_flash framework to handle all sizes of flashes
> using bank/extd addr reg facility
>
> The current implementation in spi_flash supports 3-byte address mode
> due to this up to 16Mbytes amount of flash is able to access for those
> flashes which has an actual size of > 16MB.
>
> As most of the flashes introduces a bank/extd address registers
> for accessing the flashes in 16Mbytes of banks if the flash size
> is > 16Mbytes, this new scheme will add the bank selection feature
> for performing write/erase operations on all flashes.
>
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>

I have a few comments on this series, but it mostly looks fine. I think the
new code is correct.

The patches did not apply cleanly for me. Perhaps I am missing something.
My tree looks like this after I did a bit of merging:

5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash devices
via address shift
f700095 sf: Add Flag status register polling support
42f4b70 sf: Remove spi_flash_cmd_poll_bit()
fc31387 sf: Use spi_flash_read_common() in write status poll
923e40e sf: spansion: Add support for S25FL512S_256K
c72e52a sf: stmicro: Add support for N25Q1024A
4066f71 sf: stmicro: Add support for N25Q1024
0efaf86 sf: stmicro: Add support for N25Q512A
8fd962f sf: stmicro: Add support for N25Q512
f1a2080 sf: Use spi_flash_addr() in write call
31ed498 sf: Update sf read to support all sizes of flashes
0f77642 sf: Update sf to support all sizes of flashes
9e57220 sf: read flash bank addr register at probe time
e99a43d sf: Add extended addr read support for winbond|stmicro
2f06d56 sf: Add extended addr write support for winbond|stmicro
f02ecf1 sf: Add bank address register reading support
02ba27c sf: Add bank address register writing support

Also do you think spi_flash_cmd_bankaddr_write() and related stuff should
be behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space
does this add?

In your change to spi_flash_cmd_poll_bit() the effect is not the same I
think. It is designed to hold CS active and read the status byte
continuously until it changes. But your implementation asserts CS, reads
the status byte, de-asserts CS, then repeats. Why do we want to change
this?



---
> Changes for v2:
>         - none
>
>  drivers/mtd/spi/spi_flash.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 4576a16..5386884 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
> u32 offset,
>         unsigned long page_addr, byte_addr, page_size;
>         size_t chunk_len, actual;
>         int ret;
> -       u8 cmd[4];
> +       u8 cmd[4], bank_sel;
>
>         page_size = flash->page_size;
> -       page_addr = offset / page_size;
> -       byte_addr = offset % page_size;
>
>         ret = spi_claim_bus(flash->spi);
>         if (ret) {
> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
> u32 offset,
>
>         cmd[0] = CMD_PAGE_PROGRAM;
>         for (actual = 0; actual < len; actual += chunk_len) {
> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> +
> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> +               if (ret) {
> +                       debug("SF: fail to set bank%d\n", bank_sel);
> +                       return ret;
> +               }
>

So we are now doing this for all chips. But isn't it true that only some
chips (>16MB?) have a bank address. If so, then I think we should have a
flag somewhere to enable this feature


> +
> +               page_addr = offset / page_size;
> +               byte_addr = offset % page_size;
>

This is OK I think. We really don't care about the slower divide so it is
not worth optimising for I think.


>                 chunk_len = min(len - actual, page_size - byte_addr);
>
>                 if (flash->spi->max_write_size)
> @@ -117,11 +125,7 @@ int spi_flash_cmd_write_multi(struct spi_flash
> *flash, u32 offset,
>                 if (ret)
>                         break;
>
> -               byte_addr += chunk_len;
> -               if (byte_addr == page_size) {
> -                       page_addr++;
> -                       byte_addr = 0;
> -               }
> +               offset += chunk_len;
>         }
>
>         spi_release_bus(flash->spi);
> @@ -204,9 +208,9 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash,
> unsigned long timeout)
>
>  int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
>  {
> -       u32 end, erase_size;
> +       u32 erase_size;
>         int ret;
> -       u8 cmd[4];
> +       u8 cmd[4], bank_sel;
>
>         erase_size = flash->sector_size;
>         if (offset % erase_size || len % erase_size) {
> @@ -224,11 +228,17 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32
> offset, size_t len)
>                 cmd[0] = CMD_ERASE_4K;
>         else
>                 cmd[0] = CMD_ERASE_64K;
> -       end = offset + len;
>
> -       while (offset < end) {
> +       while (len) {
> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> +
> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> +               if (ret) {
> +                       debug("SF: fail to set bank%d\n", bank_sel);
> +                       return ret;
> +               }
> +
>                 spi_flash_addr(offset, cmd);
> -               offset += erase_size;
>
>                 debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
>                       cmd[2], cmd[3], offset);
> @@ -244,6 +254,9 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32
> offset, size_t len)
>                 ret = spi_flash_cmd_wait_ready(flash,
> SPI_FLASH_PAGE_ERASE_TIMEOUT);
>                 if (ret)
>                         goto out;
> +
> +               offset += erase_size;
> +               len -= erase_size;
>         }
>
>   out:
> --
> 1.8.3
>
>
>
Regards,
Simon

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Simon,

On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>>
>> Updated the spi_flash framework to handle all sizes of flashes
>> using bank/extd addr reg facility
>>
>> The current implementation in spi_flash supports 3-byte address mode
>> due to this up to 16Mbytes amount of flash is able to access for those
>> flashes which has an actual size of > 16MB.
>>
>> As most of the flashes introduces a bank/extd address registers
>> for accessing the flashes in 16Mbytes of banks if the flash size
>> is > 16Mbytes, this new scheme will add the bank selection feature
>> for performing write/erase operations on all flashes.
>>
>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>
>
> I have a few comments on this series, but it mostly looks fine. I think the
> new code is correct.
>
> The patches did not apply cleanly for me. Perhaps I am missing something. My
> tree looks like this after I did a bit of merging:

Which patches you had an issues while applying,we have few patches on
u-boot-spi.git i created these on top of it.

>
> 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash devices
> via address shift
> f700095 sf: Add Flag status register polling support
> 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> fc31387 sf: Use spi_flash_read_common() in write status poll
> 923e40e sf: spansion: Add support for S25FL512S_256K
> c72e52a sf: stmicro: Add support for N25Q1024A
> 4066f71 sf: stmicro: Add support for N25Q1024
> 0efaf86 sf: stmicro: Add support for N25Q512A
> 8fd962f sf: stmicro: Add support for N25Q512
> f1a2080 sf: Use spi_flash_addr() in write call
> 31ed498 sf: Update sf read to support all sizes of flashes
> 0f77642 sf: Update sf to support all sizes of flashes
> 9e57220 sf: read flash bank addr register at probe time
> e99a43d sf: Add extended addr read support for winbond|stmicro
> 2f06d56 sf: Add extended addr write support for winbond|stmicro
> f02ecf1 sf: Add bank address register reading support
> 02ba27c sf: Add bank address register writing support
>
> Also do you think spi_flash_cmd_bankaddr_write() and related stuff should be
> behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space does
> this add?

Initially i thought of the same, but I just updated sf which is
generic to all sizes of flashes.
due to this i haven't included the bank read/write on macros, and the
flash ops will call these
bank write call irrespective of flash sizes.

As flashes which has below 16Mbytes will have a bank_curr value 0
so-that even bank write will exit for
bank 0 operations.

+	if (flash->bank_curr == bank_sel) {
+		debug("SF: not require to enable bank%d\n", bank_sel);
+		return 0;
+	}
+

And due to these framework changes bank+flash ops addition, bin size
increases appr' ~600bytes
by enabling stmicro, winbond and spansion flash drivers.(please check
the size from your end also if required)

Please see the commit body on below thread for more info
http://patchwork.ozlabs.org/patch/247954/

>
> In your change to spi_flash_cmd_poll_bit() the effect is not the same I
> think. It is designed to hold CS active and read the status byte
> continuously until it changes. But your implementation asserts CS, reads the
> status byte, de-asserts CS, then repeats. Why do we want to change this?

I commented on the actual patch thread, please refer,

>
>
>
>> ---
>> Changes for v2:
>>         - none
>>
>>  drivers/mtd/spi/spi_flash.c | 39 ++++++++++++++++++++++++++-------------
>>  1 file changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>> index 4576a16..5386884 100644
>> --- a/drivers/mtd/spi/spi_flash.c
>> +++ b/drivers/mtd/spi/spi_flash.c
>> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
>> u32 offset,
>>         unsigned long page_addr, byte_addr, page_size;
>>         size_t chunk_len, actual;
>>         int ret;
>> -       u8 cmd[4];
>> +       u8 cmd[4], bank_sel;
>>
>>         page_size = flash->page_size;
>> -       page_addr = offset / page_size;
>> -       byte_addr = offset % page_size;
>>
>>         ret = spi_claim_bus(flash->spi);
>>         if (ret) {
>> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
>> u32 offset,
>>
>>         cmd[0] = CMD_PAGE_PROGRAM;
>>         for (actual = 0; actual < len; actual += chunk_len) {
>> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
>> +
>> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
>> +               if (ret) {
>> +                       debug("SF: fail to set bank%d\n", bank_sel);
>> +                       return ret;
>> +               }
>
>
> So we are now doing this for all chips. But isn't it true that only some
> chips (>16MB?) have a bank address. If so, then I think we should have a
> flag somewhere to enable this feature

APAMK, currently stmicro, winbond, spansion and macronix have a
flashes which has > 16Mbytes flashes.

And macronix is also have same bank setup like stmicro, extended addr
read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
We need to add this in near future.

I have added Prafulla Wadaskar on this thread (initial contributor for
macronix.c), may be he will give some more information
for accessing > 16Mbytes flashes in 3-byte addr mode.

I think we can go ahead for now, may be we will tune sf some more in
future based on the availability of different flash which crosses
16Mbytes
with different apparoch (other than banking/extended), what do you say?

>
>>
>> +
>> +               page_addr = offset / page_size;
>> +               byte_addr = offset % page_size;
>
>
> This is OK I think. We really don't care about the slower divide so it is
> not worth optimising for I think.

Yes, I just used the existing spi_flash_addr with offset as an
argument, anyway sf write have a logic
to use writing in terms of page sizes and even offset is also varies
page sizes or requested sizes.

Thanks,
Jagan.

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagan Teki]

Hi Simon,

Please let know your comments.

I have changed the logic, but removed spi_flash_cmd_poll_bit() use
poll code on spi_flash_cmd_wait_ready()
as no other call for spi_flash_cmd_poll_bit() this.

And also for read_status the check_status i assigned as 0,earlier it
has direct 0 (w/o check_status variable).

To add the support for flag status on the same code, i define this check_status.
I don't see any coding functionality change for now, compared to before.

--
Thanks,
Jagan.

On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
<jagannadha.sutradharudu-teki@xilinx.com> wrote:
> Flag status register polling is required for micron 512Mb flash
> devices onwards, for performing erase/program operations.
>
> Like polling for WIP(Write-In-Progress) bit in read status register,
> spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
> bit in flag status register.
>
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> ---
> Changes for v2:
>         - none
>
>  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 527423d..8cd2988 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
>         unsigned long timebase;
>         int ret;
>         u8 status;
> +       u8 check_status = 0x0;
>         u8 poll_bit = STATUS_WIP;
>         u8 cmd = CMD_READ_STATUS;
>
> +       if ((flash->idcode0 == 0x20) &&
> +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
> +               poll_bit = STATUS_PEC;
> +               check_status = poll_bit;
> +               cmd = CMD_FLAG_STATUS;
> +       }
> +
>         timebase = get_timer(0);
>         do {
>                 WATCHDOG_RESET();
>
>                 ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
>                 if (ret < 0) {
> -                       debug("SF: fail to read read status register\n");
> +                       debug("SF: fail to read %s status register\n",
> +                               cmd == CMD_READ_STATUS ? "read" : "flag");
>                         return ret;
>                 }
>
> -               if ((status & poll_bit) == 0)
> +               if ((status & poll_bit) == check_status)
>                         break;
>
>         } while (get_timer(timebase) < timeout);
>
> -       if ((status & poll_bit) == 0)
> +       if ((status & poll_bit) == check_status)
>                 return 0;
>
>         /* Timed out */
> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
> index ac4530f..cb7a505 100644
> --- a/drivers/mtd/spi/spi_flash_internal.h
> +++ b/drivers/mtd/spi/spi_flash_internal.h
> @@ -13,6 +13,7 @@
>  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>
>  #define SPI_FLASH_16MB_BOUN            0x1000000
> +#define SPI_FLASH_512MB_STMIC          0x4000000
>
>  /* Common commands */
>  #define CMD_READ_ID                    0x9f
> @@ -24,6 +25,7 @@
>  #define CMD_PAGE_PROGRAM               0x02
>  #define CMD_WRITE_DISABLE              0x04
>  #define CMD_READ_STATUS                        0x05
> +#define CMD_FLAG_STATUS                        0x70
>  #define CMD_WRITE_ENABLE               0x06
>  #define CMD_ERASE_4K                   0x20
>  #define CMD_ERASE_32K                  0x52
> @@ -38,6 +40,7 @@
>
>  /* Common status */
>  #define STATUS_WIP                     0x01
> +#define STATUS_PEC                     0x80
>
>  /* Send a single-byte command to the device and read the response */
>  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t len);
> --
> 1.8.3
>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Simon Glass]

Hi Jagan,

On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:

> Hi Simon,
>
> Please let know your comments.
>
> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
> poll code on spi_flash_cmd_wait_ready()
> as no other call for spi_flash_cmd_poll_bit() this.
>
> And also for read_status the check_status i assigned as 0,earlier it
> has direct 0 (w/o check_status variable).
>
> To add the support for flag status on the same code, i define this
> check_status.
> I don't see any coding functionality change for now, compared to before.
>

This is not the right patch, but in one of them you remove
spi_flash_cmd_poll_bit(),
so that it no longer works the same way. You will get lots of individual
SPI transactions on the bus instead of a single one that reads the status
byte continuously.

Do we need to change this?

Regards,
Simon


>
> --
> Thanks,
> Jagan.
>
> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> > Flag status register polling is required for micron 512Mb flash
> > devices onwards, for performing erase/program operations.
> >
> > Like polling for WIP(Write-In-Progress) bit in read status register,
> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
> > bit in flag status register.
> >
> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> > ---
> > Changes for v2:
> >         - none
> >
> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> > index 527423d..8cd2988 100644
> > --- a/drivers/mtd/spi/spi_flash.c
> > +++ b/drivers/mtd/spi/spi_flash.c
> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
> *flash, unsigned long timeout)
> >         unsigned long timebase;
> >         int ret;
> >         u8 status;
> > +       u8 check_status = 0x0;
> >         u8 poll_bit = STATUS_WIP;
> >         u8 cmd = CMD_READ_STATUS;
> >
> > +       if ((flash->idcode0 == 0x20) &&
> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
> > +               poll_bit = STATUS_PEC;
> > +               check_status = poll_bit;
> > +               cmd = CMD_FLAG_STATUS;
> > +       }
> > +
> >         timebase = get_timer(0);
> >         do {
> >                 WATCHDOG_RESET();
> >
> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
> >                 if (ret < 0) {
> > -                       debug("SF: fail to read read status register\n");
> > +                       debug("SF: fail to read %s status register\n",
> > +                               cmd == CMD_READ_STATUS ? "read" :
> "flag");
> >                         return ret;
> >                 }
> >
> > -               if ((status & poll_bit) == 0)
> > +               if ((status & poll_bit) == check_status)
> >                         break;
> >
> >         } while (get_timer(timebase) < timeout);
> >
> > -       if ((status & poll_bit) == 0)
> > +       if ((status & poll_bit) == check_status)
> >                 return 0;
> >
> >         /* Timed out */
> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
> b/drivers/mtd/spi/spi_flash_internal.h
> > index ac4530f..cb7a505 100644
> > --- a/drivers/mtd/spi/spi_flash_internal.h
> > +++ b/drivers/mtd/spi/spi_flash_internal.h
> > @@ -13,6 +13,7 @@
> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
> >
> >  #define SPI_FLASH_16MB_BOUN            0x1000000
> > +#define SPI_FLASH_512MB_STMIC          0x4000000
> >
> >  /* Common commands */
> >  #define CMD_READ_ID                    0x9f
> > @@ -24,6 +25,7 @@
> >  #define CMD_PAGE_PROGRAM               0x02
> >  #define CMD_WRITE_DISABLE              0x04
> >  #define CMD_READ_STATUS                        0x05
> > +#define CMD_FLAG_STATUS                        0x70
> >  #define CMD_WRITE_ENABLE               0x06
> >  #define CMD_ERASE_4K                   0x20
> >  #define CMD_ERASE_32K                  0x52
> > @@ -38,6 +40,7 @@
> >
> >  /* Common status */
> >  #define STATUS_WIP                     0x01
> > +#define STATUS_PEC                     0x80
> >
> >  /* Send a single-byte command to the device and read the response */
> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t
> len);
> > --
> > 1.8.3
> >
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Simon Glass]

Hi,

On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:

> Hi Simon,
>
> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
> > Hi Jagan,
> >
> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >>
> >> Updated the spi_flash framework to handle all sizes of flashes
> >> using bank/extd addr reg facility
> >>
> >> The current implementation in spi_flash supports 3-byte address mode
> >> due to this up to 16Mbytes amount of flash is able to access for those
> >> flashes which has an actual size of > 16MB.
> >>
> >> As most of the flashes introduces a bank/extd address registers
> >> for accessing the flashes in 16Mbytes of banks if the flash size
> >> is > 16Mbytes, this new scheme will add the bank selection feature
> >> for performing write/erase operations on all flashes.
> >>
> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >
> >
> > I have a few comments on this series, but it mostly looks fine. I think
> the
> > new code is correct.
> >
> > The patches did not apply cleanly for me. Perhaps I am missing
> something. My
> > tree looks like this after I did a bit of merging:
>
> Which patches you had an issues while applying,we have few patches on
> u-boot-spi.git i created these on top of it.
>

I am not sure now - sorry I did not keep a record. But the bundle I used is
here, and it doesn't apply cleanly.

http://patchwork.ozlabs.org/bundle/sjg/jagan/

git am ~/Downloads/bundle-4407-jagan.mbox
Applying: sf: Add bank address register writing support
Applying: sf: Add bank address register reading support
Applying: sf: Add extended addr write support for winbond|stmicro
Applying: sf: Add extended addr read support for winbond|stmicro
Applying: sf: read flash bank addr register at probe time
Applying: sf: Update sf to support all sizes of flashes
error: patch failed: drivers/mtd/spi/spi_flash.c:117
error: drivers/mtd/spi/spi_flash.c: patch does not apply
Patch failed at 0006 sf: Update sf to support all sizes of flashes
The copy of the patch that failed is found in:
   /home/sjg/u/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort"


>
> >
> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
> devices
> > via address shift
> > f700095 sf: Add Flag status register polling support
> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> > fc31387 sf: Use spi_flash_read_common() in write status poll
> > 923e40e sf: spansion: Add support for S25FL512S_256K
> > c72e52a sf: stmicro: Add support for N25Q1024A
> > 4066f71 sf: stmicro: Add support for N25Q1024
> > 0efaf86 sf: stmicro: Add support for N25Q512A
> > 8fd962f sf: stmicro: Add support for N25Q512
> > f1a2080 sf: Use spi_flash_addr() in write call
> > 31ed498 sf: Update sf read to support all sizes of flashes
> > 0f77642 sf: Update sf to support all sizes of flashes
> > 9e57220 sf: read flash bank addr register at probe time
> > e99a43d sf: Add extended addr read support for winbond|stmicro
> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
> > f02ecf1 sf: Add bank address register reading support
> > 02ba27c sf: Add bank address register writing support
> >
> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
> should be
> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space
> does
> > this add?
>
> Initially i thought of the same, but I just updated sf which is
> generic to all sizes of flashes.
> due to this i haven't included the bank read/write on macros, and the
> flash ops will call these
> bank write call irrespective of flash sizes.
>
> As flashes which has below 16Mbytes will have a bank_curr value 0
> so-that even bank write will exit for
> bank 0 operations.
>

Yes this is fine.


>
> +       if (flash->bank_curr == bank_sel) {
> +               debug("SF: not require to enable bank%d\n", bank_sel);
> +               return 0;
> +       }
> +
>
> And due to these framework changes bank+flash ops addition, bin size
> increases appr' ~600bytes
> by enabling stmicro, winbond and spansion flash drivers.(please check
> the size from your end also if required)
>

I suggest you make that function a nop (perhaps using an #ifdef
CONFIG_SPI_BANK_ADDR
inside it or some other name) so that your patches don't increase U-Boot
code size for those boards that don't need support larger devices (which I
guess is almost all of them, right now). U-Boot is quite concerned about
code size.

Tom may chime in and decide it is fine, though.


>
> Please see the commit body on below thread for more info
> http://patchwork.ozlabs.org/patch/247954/
>
> >
> > In your change to spi_flash_cmd_poll_bit() the effect is not the same I
> > think. It is designed to hold CS active and read the status byte
> > continuously until it changes. But your implementation asserts CS, reads
> the
> > status byte, de-asserts CS, then repeats. Why do we want to change this?
>
> I commented on the actual patch thread, please refer,
>

OK I will take a look.


>
> >
> >
> >
> >> ---
> >> Changes for v2:
> >>         - none
> >>
> >>  drivers/mtd/spi/spi_flash.c | 39
> ++++++++++++++++++++++++++-------------
> >>  1 file changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> >> index 4576a16..5386884 100644
> >> --- a/drivers/mtd/spi/spi_flash.c
> >> +++ b/drivers/mtd/spi/spi_flash.c
> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
> *flash,
> >> u32 offset,
> >>         unsigned long page_addr, byte_addr, page_size;
> >>         size_t chunk_len, actual;
> >>         int ret;
> >> -       u8 cmd[4];
> >> +       u8 cmd[4], bank_sel;
> >>
> >>         page_size = flash->page_size;
> >> -       page_addr = offset / page_size;
> >> -       byte_addr = offset % page_size;
> >>
> >>         ret = spi_claim_bus(flash->spi);
> >>         if (ret) {
> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
> *flash,
> >> u32 offset,
> >>
> >>         cmd[0] = CMD_PAGE_PROGRAM;
> >>         for (actual = 0; actual < len; actual += chunk_len) {
> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> >> +
> >> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> >> +               if (ret) {
> >> +                       debug("SF: fail to set bank%d\n", bank_sel);
> >> +                       return ret;
> >> +               }
> >
> >
> > So we are now doing this for all chips. But isn't it true that only some
> > chips (>16MB?) have a bank address. If so, then I think we should have a
> > flag somewhere to enable this feature
>
> APAMK, currently stmicro, winbond, spansion and macronix have a
> flashes which has > 16Mbytes flashes.
>
> And macronix is also have same bank setup like stmicro, extended addr
> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
> We need to add this in near future.
>
> I have added Prafulla Wadaskar on this thread (initial contributor for
> macronix.c), may be he will give some more information
> for accessing > 16Mbytes flashes in 3-byte addr mode.
>
> I think we can go ahead for now, may be we will tune sf some more in
> future based on the availability of different flash which crosses
> 16Mbytes
> with different apparoch (other than banking/extended), what do you say?
>

OK, well we don't need a flag since you will never issue the bank address
command unless the chip is larger than 16MB.,


>
> >
> >>
> >> +
> >> +               page_addr = offset / page_size;
> >> +               byte_addr = offset % page_size;
> >
> >
> > This is OK I think. We really don't care about the slower divide so it is
> > not worth optimising for I think.
>
> Yes, I just used the existing spi_flash_addr with offset as an
> argument, anyway sf write have a logic
> to use writing in terms of page sizes and even offset is also varies
> page sizes or requested sizes.
>
> Thanks,
> Jagan.
>


Regards,
Simon

[U-Boot] [PATCH v2 14/16] sf: Use spi_flash_read_common() in write status poll

[Simon Glass]

Hi Jagan,

On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki <
jagannadha.sutradharudu-teki@xilinx.com> wrote:

> Instead of using spi_xfer for SPI_XFER_BEGIN and SPI_XFER_END
> separatley use common read call spi_flash_read_common() which
> does the same.
>
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> ---
> Changes for v2:
>         - none
>
>  drivers/mtd/spi/spi_flash.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 821aa2e..765d4bc 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -193,32 +193,25 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash,
> u32 offset,
>  int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,
>                            u8 cmd, u8 poll_bit)
>  {
> -       struct spi_slave *spi = flash->spi;
>         unsigned long timebase;
>         int ret;
>         u8 status;
>
> -       ret = spi_xfer(spi, 8, &cmd, NULL, SPI_XFER_BEGIN);
> -       if (ret) {
> -               debug("SF: Failed to send command %02x: %d\n", cmd, ret);
> -               return ret;
> -       }
> -
>         timebase = get_timer(0);
>         do {
>                 WATCHDOG_RESET();
>
> -               ret = spi_xfer(spi, 8, NULL, &status, 0);
> -               if (ret)
> -                       return -1;
> +               ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
> +               if (ret < 0) {
> +                       debug("SF: fail to read read status register\n");
> +                       return ret;
> +               }
>

As mentioned elsewhere, do you need to make this change? Is it not possible
to read the status register in the same way as now?

spi_flash_read_common() does a separate SPI transaction for each read.


>
>                 if ((status & poll_bit) == 0)
>                         break;
>
>         } while (get_timer(timebase) < timeout);
>
> -       spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
> -
>         if ((status & poll_bit) == 0)
>                 return 0;
>
> --
> 1.8.3
>
>
>

Regards,
Simon

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagan Teki]

Hi Simon,

On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>
>> Hi Simon,
>>
>> Please let know your comments.
>>
>> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
>> poll code on spi_flash_cmd_wait_ready()
>> as no other call for spi_flash_cmd_poll_bit() this.
>>
>> And also for read_status the check_status i assigned as 0,earlier it
>> has direct 0 (w/o check_status variable).
>>
>> To add the support for flag status on the same code, i define this
>> check_status.
>> I don't see any coding functionality change for now, compared to before.
>
>
> This is not the right patch, but in one of them you remove
> spi_flash_cmd_poll_bit(), so that it no longer works the same way. You will
> get lots of individual SPI transactions on the bus instead of a single one
> that reads the status byte continuously.

I couldn't understand you clearly.

Earlier all erase/write they were calling spi_flash_cmd_wait_ready
with flash, timeout values.
spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
with CMD_READ_STATUS and STATUS_WIP.

In my changes I have removed spi_flash_cmd_poll_bit() as there is no
other caller except spi_flash_cmd_wait_ready()
so all polling stuff on spi_flash_cmd_wait_ready() means, still
erase/program can call spi_flash_cmd_wait_ready() with
flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
used on spi_flash_cmd_wait_ready() instead
of passing through spi_flash_cmd_poll_bit().

Any wrong here, please comment.

Thanks,
Jagan.

>
> Do we need to change this?
>
>
>>
>>
>> --
>> Thanks,
>> Jagan.
>>
>> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
>> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> > Flag status register polling is required for micron 512Mb flash
>> > devices onwards, for performing erase/program operations.
>> >
>> > Like polling for WIP(Write-In-Progress) bit in read status register,
>> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
>> > bit in flag status register.
>> >
>> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> > ---
>> > Changes for v2:
>> >         - none
>> >
>> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>> >  2 files changed, 15 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>> > index 527423d..8cd2988 100644
>> > --- a/drivers/mtd/spi/spi_flash.c
>> > +++ b/drivers/mtd/spi/spi_flash.c
>> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
>> > *flash, unsigned long timeout)
>> >         unsigned long timebase;
>> >         int ret;
>> >         u8 status;
>> > +       u8 check_status = 0x0;
>> >         u8 poll_bit = STATUS_WIP;
>> >         u8 cmd = CMD_READ_STATUS;
>> >
>> > +       if ((flash->idcode0 == 0x20) &&
>> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
>> > +               poll_bit = STATUS_PEC;
>> > +               check_status = poll_bit;
>> > +               cmd = CMD_FLAG_STATUS;
>> > +       }
>> > +
>> >         timebase = get_timer(0);
>> >         do {
>> >                 WATCHDOG_RESET();
>> >
>> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status, 1);
>> >                 if (ret < 0) {
>> > -                       debug("SF: fail to read read status
>> > register\n");
>> > +                       debug("SF: fail to read %s status register\n",
>> > +                               cmd == CMD_READ_STATUS ? "read" :
>> > "flag");
>> >                         return ret;
>> >                 }
>> >
>> > -               if ((status & poll_bit) == 0)
>> > +               if ((status & poll_bit) == check_status)
>> >                         break;
>> >
>> >         } while (get_timer(timebase) < timeout);
>> >
>> > -       if ((status & poll_bit) == 0)
>> > +       if ((status & poll_bit) == check_status)
>> >                 return 0;
>> >
>> >         /* Timed out */
>> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
>> > b/drivers/mtd/spi/spi_flash_internal.h
>> > index ac4530f..cb7a505 100644
>> > --- a/drivers/mtd/spi/spi_flash_internal.h
>> > +++ b/drivers/mtd/spi/spi_flash_internal.h
>> > @@ -13,6 +13,7 @@
>> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>> >
>> >  #define SPI_FLASH_16MB_BOUN            0x1000000
>> > +#define SPI_FLASH_512MB_STMIC          0x4000000
>> >
>> >  /* Common commands */
>> >  #define CMD_READ_ID                    0x9f
>> > @@ -24,6 +25,7 @@
>> >  #define CMD_PAGE_PROGRAM               0x02
>> >  #define CMD_WRITE_DISABLE              0x04
>> >  #define CMD_READ_STATUS                        0x05
>> > +#define CMD_FLAG_STATUS                        0x70
>> >  #define CMD_WRITE_ENABLE               0x06
>> >  #define CMD_ERASE_4K                   0x20
>> >  #define CMD_ERASE_32K                  0x52
>> > @@ -38,6 +40,7 @@
>> >
>> >  /* Common status */
>> >  #define STATUS_WIP                     0x01
>> > +#define STATUS_PEC                     0x80
>> >
>> >  /* Send a single-byte command to the device and read the response */
>> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t
>> > len);
>> > --
>> > 1.8.3
>>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Simon,

On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>
>> Hi Simon,
>>
>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
>> > Hi Jagan,
>> >
>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> >>
>> >> Updated the spi_flash framework to handle all sizes of flashes
>> >> using bank/extd addr reg facility
>> >>
>> >> The current implementation in spi_flash supports 3-byte address mode
>> >> due to this up to 16Mbytes amount of flash is able to access for those
>> >> flashes which has an actual size of > 16MB.
>> >>
>> >> As most of the flashes introduces a bank/extd address registers
>> >> for accessing the flashes in 16Mbytes of banks if the flash size
>> >> is > 16Mbytes, this new scheme will add the bank selection feature
>> >> for performing write/erase operations on all flashes.
>> >>
>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> >
>> >
>> > I have a few comments on this series, but it mostly looks fine. I think
>> > the
>> > new code is correct.
>> >
>> > The patches did not apply cleanly for me. Perhaps I am missing
>> > something. My
>> > tree looks like this after I did a bit of merging:
>>
>> Which patches you had an issues while applying,we have few patches on
>> u-boot-spi.git i created these on top of it.
>
>
> I am not sure now - sorry I did not keep a record. But the bundle I used is
> here, and it doesn't apply cleanly.
>
> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>
> git am ~/Downloads/bundle-4407-jagan.mbox
> Applying: sf: Add bank address register writing support
> Applying: sf: Add bank address register reading support
> Applying: sf: Add extended addr write support for winbond|stmicro
> Applying: sf: Add extended addr read support for winbond|stmicro
> Applying: sf: read flash bank addr register at probe time
> Applying: sf: Update sf to support all sizes of flashes
> error: patch failed: drivers/mtd/spi/spi_flash.c:117
> error: drivers/mtd/spi/spi_flash.c: patch does not apply
> Patch failed at 0006 sf: Update sf to support all sizes of flashes
> The copy of the patch that failed is found in:
>    /home/sjg/u/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --resolved".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort"
>
>>
>>
>> >
>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
>> > devices
>> > via address shift
>> > f700095 sf: Add Flag status register polling support
>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
>> > fc31387 sf: Use spi_flash_read_common() in write status poll
>> > 923e40e sf: spansion: Add support for S25FL512S_256K
>> > c72e52a sf: stmicro: Add support for N25Q1024A
>> > 4066f71 sf: stmicro: Add support for N25Q1024
>> > 0efaf86 sf: stmicro: Add support for N25Q512A
>> > 8fd962f sf: stmicro: Add support for N25Q512
>> > f1a2080 sf: Use spi_flash_addr() in write call
>> > 31ed498 sf: Update sf read to support all sizes of flashes
>> > 0f77642 sf: Update sf to support all sizes of flashes
>> > 9e57220 sf: read flash bank addr register at probe time
>> > e99a43d sf: Add extended addr read support for winbond|stmicro
>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
>> > f02ecf1 sf: Add bank address register reading support
>> > 02ba27c sf: Add bank address register writing support
>> >
>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
>> > should be
>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space
>> > does
>> > this add?
>>
>> Initially i thought of the same, but I just updated sf which is
>> generic to all sizes of flashes.
>> due to this i haven't included the bank read/write on macros, and the
>> flash ops will call these
>> bank write call irrespective of flash sizes.
>>
>> As flashes which has below 16Mbytes will have a bank_curr value 0
>> so-that even bank write will exit for
>> bank 0 operations.
>
>
> Yes this is fine.
>
>>
>>
>> +       if (flash->bank_curr == bank_sel) {
>> +               debug("SF: not require to enable bank%d\n", bank_sel);
>> +               return 0;
>> +       }
>> +
>>
>> And due to these framework changes bank+flash ops addition, bin size
>> increases appr' ~600bytes
>> by enabling stmicro, winbond and spansion flash drivers.(please check
>> the size from your end also if required)
>
>
> I suggest you make that function a nop (perhaps using an #ifdef
> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
> don't increase U-Boot code size for those boards that don't need support
> larger devices (which I guess is almost all of them, right now). U-Boot is
> quite concerned about code size.

Little concern here, the point here I stated is this new changes is
common for all sizes of flashes.(which are less or greater than
16Mbytes).
and yes it increase the code size little bit but i don't think it will
require the separate macro.

Thanks,
Jagan.

>
> Tom may chime in and decide it is fine, though.
>
>>
>>
>> Please see the commit body on below thread for more info
>> http://patchwork.ozlabs.org/patch/247954/
>>
>> >
>> > In your change to spi_flash_cmd_poll_bit() the effect is not the same I
>> > think. It is designed to hold CS active and read the status byte
>> > continuously until it changes. But your implementation asserts CS, reads
>> > the
>> > status byte, de-asserts CS, then repeats. Why do we want to change this?
>>
>> I commented on the actual patch thread, please refer,
>
>
> OK I will take a look.
>
>>
>>
>> >
>> >
>> >
>> >> ---
>> >> Changes for v2:
>> >>         - none
>> >>
>> >>  drivers/mtd/spi/spi_flash.c | 39
>> >> ++++++++++++++++++++++++++-------------
>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>> >> index 4576a16..5386884 100644
>> >> --- a/drivers/mtd/spi/spi_flash.c
>> >> +++ b/drivers/mtd/spi/spi_flash.c
>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
>> >> *flash,
>> >> u32 offset,
>> >>         unsigned long page_addr, byte_addr, page_size;
>> >>         size_t chunk_len, actual;
>> >>         int ret;
>> >> -       u8 cmd[4];
>> >> +       u8 cmd[4], bank_sel;
>> >>
>> >>         page_size = flash->page_size;
>> >> -       page_addr = offset / page_size;
>> >> -       byte_addr = offset % page_size;
>> >>
>> >>         ret = spi_claim_bus(flash->spi);
>> >>         if (ret) {
>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
>> >> *flash,
>> >> u32 offset,
>> >>
>> >>         cmd[0] = CMD_PAGE_PROGRAM;
>> >>         for (actual = 0; actual < len; actual += chunk_len) {
>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
>> >> +
>> >> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
>> >> +               if (ret) {
>> >> +                       debug("SF: fail to set bank%d\n", bank_sel);
>> >> +                       return ret;
>> >> +               }
>> >
>> >
>> > So we are now doing this for all chips. But isn't it true that only some
>> > chips (>16MB?) have a bank address. If so, then I think we should have a
>> > flag somewhere to enable this feature
>>
>> APAMK, currently stmicro, winbond, spansion and macronix have a
>> flashes which has > 16Mbytes flashes.
>>
>> And macronix is also have same bank setup like stmicro, extended addr
>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
>> We need to add this in near future.
>>
>> I have added Prafulla Wadaskar on this thread (initial contributor for
>> macronix.c), may be he will give some more information
>> for accessing > 16Mbytes flashes in 3-byte addr mode.
>>
>> I think we can go ahead for now, may be we will tune sf some more in
>> future based on the availability of different flash which crosses
>> 16Mbytes
>> with different apparoch (other than banking/extended), what do you say?
>
>
> OK, well we don't need a flag since you will never issue the bank address
> command unless the chip is larger than 16MB.,
>
>>
>>
>> >
>> >>
>> >> +
>> >> +               page_addr = offset / page_size;
>> >> +               byte_addr = offset % page_size;
>> >
>> >
>> > This is OK I think. We really don't care about the slower divide so it
>> > is
>> > not worth optimising for I think.
>>
>> Yes, I just used the existing spi_flash_addr with offset as an
>> argument, anyway sf write have a logic
>> to use writing in terms of page sizes and even offset is also varies
>> page sizes or requested sizes.
>>
>> Thanks,
>> Jagan.
>
>
>
> Regards,
> Simon
>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Simon Glass]

Hi Jagan,

On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:

> Hi Simon,
>
> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
> > Hi Jagan,
> >
> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
> >>
> >> Hi Simon,
> >>
> >> Please let know your comments.
> >>
> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
> >> poll code on spi_flash_cmd_wait_ready()
> >> as no other call for spi_flash_cmd_poll_bit() this.
> >>
> >> And also for read_status the check_status i assigned as 0,earlier it
> >> has direct 0 (w/o check_status variable).
> >>
> >> To add the support for flag status on the same code, i define this
> >> check_status.
> >> I don't see any coding functionality change for now, compared to before.
> >
> >
> > This is not the right patch, but in one of them you remove
> > spi_flash_cmd_poll_bit(), so that it no longer works the same way. You
> will
> > get lots of individual SPI transactions on the bus instead of a single
> one
> > that reads the status byte continuously.
>
> I couldn't understand you clearly.
>
> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
> with flash, timeout values.
> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
> with CMD_READ_STATUS and STATUS_WIP.
>
> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
> other caller except spi_flash_cmd_wait_ready()
> so all polling stuff on spi_flash_cmd_wait_ready() means, still
> erase/program can call spi_flash_cmd_wait_ready() with
> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
> used on spi_flash_cmd_wait_ready() instead
> of passing through spi_flash_cmd_poll_bit().
>
> Any wrong here, please comment.
>

You have changed the implementation of spi_flash_cmd_wait_ready() so that
instead of polling for the bit within a single SPI transaction (CS staying
asserted) you are now doing multiple transactions.

The old code for spi_flash_cmd_wait_ready() ended up with an algorithm like
this, when you look inside the nested functions:

spi_claim_bus(spi);
ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
do {
ret = spi_xfer(spi, 8, NULL, &status, 0);
        ...
}
spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
spi_release_bus(spi);

The new code is:

do {
spi_claim_bus(spi);
        ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
        ret = spi_xfer(spi, data_len * 8, data_out, data_in, SPI_XFER_END);
spi_release_bus(spi);
        ...
} while (...)


Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
inside the loop.

Is there a reason why this change is needed? Does updating the bank address
not work with the old code?

It is find to move the spi_flash_cmd_poll_bit() function into
spi_flash_cmd_wait_ready() - I am not worried about that. I am only worried
about your change of algorithm as above.

Regards,
Simon


>
> Thanks,
> Jagan.
>
> >
> > Do we need to change this?
> >
> >
> >>
> >>
> >> --
> >> Thanks,
> >> Jagan.
> >>
> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
> >> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >> > Flag status register polling is required for micron 512Mb flash
> >> > devices onwards, for performing erase/program operations.
> >> >
> >> > Like polling for WIP(Write-In-Progress) bit in read status register,
> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
> >> > bit in flag status register.
> >> >
> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >> > ---
> >> > Changes for v2:
> >> >         - none
> >> >
> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> >> > index 527423d..8cd2988 100644
> >> > --- a/drivers/mtd/spi/spi_flash.c
> >> > +++ b/drivers/mtd/spi/spi_flash.c
> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
> >> > *flash, unsigned long timeout)
> >> >         unsigned long timebase;
> >> >         int ret;
> >> >         u8 status;
> >> > +       u8 check_status = 0x0;
> >> >         u8 poll_bit = STATUS_WIP;
> >> >         u8 cmd = CMD_READ_STATUS;
> >> >
> >> > +       if ((flash->idcode0 == 0x20) &&
> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
> >> > +               poll_bit = STATUS_PEC;
> >> > +               check_status = poll_bit;
> >> > +               cmd = CMD_FLAG_STATUS;
> >> > +       }
> >> > +
> >> >         timebase = get_timer(0);
> >> >         do {
> >> >                 WATCHDOG_RESET();
> >> >
> >> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status,
> 1);
> >> >                 if (ret < 0) {
> >> > -                       debug("SF: fail to read read status
> >> > register\n");
> >> > +                       debug("SF: fail to read %s status register\n",
> >> > +                               cmd == CMD_READ_STATUS ? "read" :
> >> > "flag");
> >> >                         return ret;
> >> >                 }
> >> >
> >> > -               if ((status & poll_bit) == 0)
> >> > +               if ((status & poll_bit) == check_status)
> >> >                         break;
> >> >
> >> >         } while (get_timer(timebase) < timeout);
> >> >
> >> > -       if ((status & poll_bit) == 0)
> >> > +       if ((status & poll_bit) == check_status)
> >> >                 return 0;
> >> >
> >> >         /* Timed out */
> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
> >> > b/drivers/mtd/spi/spi_flash_internal.h
> >> > index ac4530f..cb7a505 100644
> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
> >> > @@ -13,6 +13,7 @@
> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
> >> >
> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
> >> >
> >> >  /* Common commands */
> >> >  #define CMD_READ_ID                    0x9f
> >> > @@ -24,6 +25,7 @@
> >> >  #define CMD_PAGE_PROGRAM               0x02
> >> >  #define CMD_WRITE_DISABLE              0x04
> >> >  #define CMD_READ_STATUS                        0x05
> >> > +#define CMD_FLAG_STATUS                        0x70
> >> >  #define CMD_WRITE_ENABLE               0x06
> >> >  #define CMD_ERASE_4K                   0x20
> >> >  #define CMD_ERASE_32K                  0x52
> >> > @@ -38,6 +40,7 @@
> >> >
> >> >  /* Common status */
> >> >  #define STATUS_WIP                     0x01
> >> > +#define STATUS_PEC                     0x80
> >> >
> >> >  /* Send a single-byte command to the device and read the response */
> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
> size_t
> >> > len);
> >> > --
> >> > 1.8.3
> >>
>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagan Teki]

Hi Simon,

On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>
>> Hi Simon,
>>
>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
>> > Hi Jagan,
>> >
>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com>
>> > wrote:
>> >>
>> >> Hi Simon,
>> >>
>> >> Please let know your comments.
>> >>
>> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
>> >> poll code on spi_flash_cmd_wait_ready()
>> >> as no other call for spi_flash_cmd_poll_bit() this.
>> >>
>> >> And also for read_status the check_status i assigned as 0,earlier it
>> >> has direct 0 (w/o check_status variable).
>> >>
>> >> To add the support for flag status on the same code, i define this
>> >> check_status.
>> >> I don't see any coding functionality change for now, compared to
>> >> before.
>> >
>> >
>> > This is not the right patch, but in one of them you remove
>> > spi_flash_cmd_poll_bit(), so that it no longer works the same way. You
>> > will
>> > get lots of individual SPI transactions on the bus instead of a single
>> > one
>> > that reads the status byte continuously.
>>
>> I couldn't understand you clearly.
>>
>> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
>> with flash, timeout values.
>> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
>> with CMD_READ_STATUS and STATUS_WIP.
>>
>> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
>> other caller except spi_flash_cmd_wait_ready()
>> so all polling stuff on spi_flash_cmd_wait_ready() means, still
>> erase/program can call spi_flash_cmd_wait_ready() with
>> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
>> used on spi_flash_cmd_wait_ready() instead
>> of passing through spi_flash_cmd_poll_bit().
>>
>> Any wrong here, please comment.
>
>
> You have changed the implementation of spi_flash_cmd_wait_ready() so that
> instead of polling for the bit within a single SPI transaction (CS staying
> asserted) you are now doing multiple transactions.
>
> The old code for spi_flash_cmd_wait_ready() ended up with an algorithm like
> this, when you look inside the nested functions:
>
> spi_claim_bus(spi);
> ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
> do {
> ret = spi_xfer(spi, 8, NULL, &status, 0);
>         ...
> }
> spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
> spi_release_bus(spi);
>
> The new code is:
>
> do {
> spi_claim_bus(spi);
>         ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>         ret = spi_xfer(spi, data_len * 8, data_out, data_in, SPI_XFER_END);
> spi_release_bus(spi);
>         ...
> } while (...)
>
>
> Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
> inside the loop.

Understand, i thought as we do _END transfer in old code as well.
It could be fine to go with using common function as it will do _BEGIN and _END.

What could be the problem you see if we do _BEGIN and _END on loop...?

>
> Is there a reason why this change is needed? Does updating the bank address
> not work with the old code?

It doesn't effect with banking.

Thanks,
Jagan.

>
> It is find to move the spi_flash_cmd_poll_bit() function into
> spi_flash_cmd_wait_ready() - I am not worried about that. I am only worried
> about your change of algorithm as above.
>
> Regards,
> Simon
>
>>
>>
>> Thanks,
>> Jagan.
>>
>> >
>> > Do we need to change this?
>> >
>> >
>> >>
>> >>
>> >> --
>> >> Thanks,
>> >> Jagan.
>> >>
>> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
>> >> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> >> > Flag status register polling is required for micron 512Mb flash
>> >> > devices onwards, for performing erase/program operations.
>> >> >
>> >> > Like polling for WIP(Write-In-Progress) bit in read status register,
>> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
>> >> > bit in flag status register.
>> >> >
>> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> >> > ---
>> >> > Changes for v2:
>> >> >         - none
>> >> >
>> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/drivers/mtd/spi/spi_flash.c
>> >> > b/drivers/mtd/spi/spi_flash.c
>> >> > index 527423d..8cd2988 100644
>> >> > --- a/drivers/mtd/spi/spi_flash.c
>> >> > +++ b/drivers/mtd/spi/spi_flash.c
>> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
>> >> > *flash, unsigned long timeout)
>> >> >         unsigned long timebase;
>> >> >         int ret;
>> >> >         u8 status;
>> >> > +       u8 check_status = 0x0;
>> >> >         u8 poll_bit = STATUS_WIP;
>> >> >         u8 cmd = CMD_READ_STATUS;
>> >> >
>> >> > +       if ((flash->idcode0 == 0x20) &&
>> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
>> >> > +               poll_bit = STATUS_PEC;
>> >> > +               check_status = poll_bit;
>> >> > +               cmd = CMD_FLAG_STATUS;
>> >> > +       }
>> >> > +
>> >> >         timebase = get_timer(0);
>> >> >         do {
>> >> >                 WATCHDOG_RESET();
>> >> >
>> >> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status,
>> >> > 1);
>> >> >                 if (ret < 0) {
>> >> > -                       debug("SF: fail to read read status
>> >> > register\n");
>> >> > +                       debug("SF: fail to read %s status
>> >> > register\n",
>> >> > +                               cmd == CMD_READ_STATUS ? "read" :
>> >> > "flag");
>> >> >                         return ret;
>> >> >                 }
>> >> >
>> >> > -               if ((status & poll_bit) == 0)
>> >> > +               if ((status & poll_bit) == check_status)
>> >> >                         break;
>> >> >
>> >> >         } while (get_timer(timebase) < timeout);
>> >> >
>> >> > -       if ((status & poll_bit) == 0)
>> >> > +       if ((status & poll_bit) == check_status)
>> >> >                 return 0;
>> >> >
>> >> >         /* Timed out */
>> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
>> >> > b/drivers/mtd/spi/spi_flash_internal.h
>> >> > index ac4530f..cb7a505 100644
>> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
>> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
>> >> > @@ -13,6 +13,7 @@
>> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>> >> >
>> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
>> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
>> >> >
>> >> >  /* Common commands */
>> >> >  #define CMD_READ_ID                    0x9f
>> >> > @@ -24,6 +25,7 @@
>> >> >  #define CMD_PAGE_PROGRAM               0x02
>> >> >  #define CMD_WRITE_DISABLE              0x04
>> >> >  #define CMD_READ_STATUS                        0x05
>> >> > +#define CMD_FLAG_STATUS                        0x70
>> >> >  #define CMD_WRITE_ENABLE               0x06
>> >> >  #define CMD_ERASE_4K                   0x20
>> >> >  #define CMD_ERASE_32K                  0x52
>> >> > @@ -38,6 +40,7 @@
>> >> >
>> >> >  /* Common status */
>> >> >  #define STATUS_WIP                     0x01
>> >> > +#define STATUS_PEC                     0x80
>> >> >
>> >> >  /* Send a single-byte command to the device and read the response */
>> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
>> >> > size_t
>> >> > len);
>> >> > --
>> >> > 1.8.3
>> >>
>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagan Teki]

Hi Simon,

On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
>>> > Hi Jagan,
>>> >
>>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com>
>>> > wrote:
>>> >>
>>> >> Hi Simon,
>>> >>
>>> >> Please let know your comments.
>>> >>
>>> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
>>> >> poll code on spi_flash_cmd_wait_ready()
>>> >> as no other call for spi_flash_cmd_poll_bit() this.
>>> >>
>>> >> And also for read_status the check_status i assigned as 0,earlier it
>>> >> has direct 0 (w/o check_status variable).
>>> >>
>>> >> To add the support for flag status on the same code, i define this
>>> >> check_status.
>>> >> I don't see any coding functionality change for now, compared to
>>> >> before.
>>> >
>>> >
>>> > This is not the right patch, but in one of them you remove
>>> > spi_flash_cmd_poll_bit(), so that it no longer works the same way. You
>>> > will
>>> > get lots of individual SPI transactions on the bus instead of a single
>>> > one
>>> > that reads the status byte continuously.
>>>
>>> I couldn't understand you clearly.
>>>
>>> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
>>> with flash, timeout values.
>>> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
>>> with CMD_READ_STATUS and STATUS_WIP.
>>>
>>> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
>>> other caller except spi_flash_cmd_wait_ready()
>>> so all polling stuff on spi_flash_cmd_wait_ready() means, still
>>> erase/program can call spi_flash_cmd_wait_ready() with
>>> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
>>> used on spi_flash_cmd_wait_ready() instead
>>> of passing through spi_flash_cmd_poll_bit().
>>>
>>> Any wrong here, please comment.
>>
>>
>> You have changed the implementation of spi_flash_cmd_wait_ready() so that
>> instead of polling for the bit within a single SPI transaction (CS staying
>> asserted) you are now doing multiple transactions.
>>
>> The old code for spi_flash_cmd_wait_ready() ended up with an algorithm like
>> this, when you look inside the nested functions:
>>
>> spi_claim_bus(spi);
>> ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>> do {
>> ret = spi_xfer(spi, 8, NULL, &status, 0);
>>         ...
>> }
>> spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
>> spi_release_bus(spi);
>>
>> The new code is:
>>
>> do {
>> spi_claim_bus(spi);
>>         ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>>         ret = spi_xfer(spi, data_len * 8, data_out, data_in, SPI_XFER_END);
>> spi_release_bus(spi);
>>         ...
>> } while (...)
>>
>>
>> Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
>> inside the loop.
>
> Understand, i thought as we do _END transfer in old code as well.
> It could be fine to go with using common function as it will do _BEGIN and _END.
>
> What could be the problem you see if we do _BEGIN and _END on loop...?
>
>>
>> Is there a reason why this change is needed? Does updating the bank address
>> not work with the old code?
>
> It doesn't effect with banking.
>
> Thanks,
> Jagan.
>
>>
>> It is find to move the spi_flash_cmd_poll_bit() function into
>> spi_flash_cmd_wait_ready() - I am not worried about that. I am only worried
>> about your change of algorithm as above.
>>
>> Regards,
>> Simon
>>
>>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>> >
>>> > Do we need to change this?
>>> >
>>> >
>>> >>
>>> >>
>>> >> --
>>> >> Thanks,
>>> >> Jagan.
>>> >>
>>> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
>>> >> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>>> >> > Flag status register polling is required for micron 512Mb flash
>>> >> > devices onwards, for performing erase/program operations.
>>> >> >
>>> >> > Like polling for WIP(Write-In-Progress) bit in read status register,
>>> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
>>> >> > bit in flag status register.
>>> >> >
>>> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>>> >> > ---
>>> >> > Changes for v2:
>>> >> >         - none
>>> >> >
>>> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>>> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>>> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
>>> >> >
>>> >> > diff --git a/drivers/mtd/spi/spi_flash.c
>>> >> > b/drivers/mtd/spi/spi_flash.c
>>> >> > index 527423d..8cd2988 100644
>>> >> > --- a/drivers/mtd/spi/spi_flash.c
>>> >> > +++ b/drivers/mtd/spi/spi_flash.c
>>> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
>>> >> > *flash, unsigned long timeout)
>>> >> >         unsigned long timebase;
>>> >> >         int ret;
>>> >> >         u8 status;
>>> >> > +       u8 check_status = 0x0;
>>> >> >         u8 poll_bit = STATUS_WIP;
>>> >> >         u8 cmd = CMD_READ_STATUS;
>>> >> >
>>> >> > +       if ((flash->idcode0 == 0x20) &&
>>> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
>>> >> > +               poll_bit = STATUS_PEC;
>>> >> > +               check_status = poll_bit;
>>> >> > +               cmd = CMD_FLAG_STATUS;
>>> >> > +       }
>>> >> > +
>>> >> >         timebase = get_timer(0);
>>> >> >         do {
>>> >> >                 WATCHDOG_RESET();
>>> >> >
>>> >> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status,
>>> >> > 1);
>>> >> >                 if (ret < 0) {
>>> >> > -                       debug("SF: fail to read read status
>>> >> > register\n");
>>> >> > +                       debug("SF: fail to read %s status
>>> >> > register\n",
>>> >> > +                               cmd == CMD_READ_STATUS ? "read" :
>>> >> > "flag");
>>> >> >                         return ret;
>>> >> >                 }
>>> >> >
>>> >> > -               if ((status & poll_bit) == 0)
>>> >> > +               if ((status & poll_bit) == check_status)
>>> >> >                         break;
>>> >> >
>>> >> >         } while (get_timer(timebase) < timeout);
>>> >> >
>>> >> > -       if ((status & poll_bit) == 0)
>>> >> > +       if ((status & poll_bit) == check_status)
>>> >> >                 return 0;
>>> >> >
>>> >> >         /* Timed out */
>>> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
>>> >> > b/drivers/mtd/spi/spi_flash_internal.h
>>> >> > index ac4530f..cb7a505 100644
>>> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
>>> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
>>> >> > @@ -13,6 +13,7 @@
>>> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>>> >> >
>>> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
>>> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
>>> >> >
>>> >> >  /* Common commands */
>>> >> >  #define CMD_READ_ID                    0x9f
>>> >> > @@ -24,6 +25,7 @@
>>> >> >  #define CMD_PAGE_PROGRAM               0x02
>>> >> >  #define CMD_WRITE_DISABLE              0x04
>>> >> >  #define CMD_READ_STATUS                        0x05
>>> >> > +#define CMD_FLAG_STATUS                        0x70
>>> >> >  #define CMD_WRITE_ENABLE               0x06
>>> >> >  #define CMD_ERASE_4K                   0x20
>>> >> >  #define CMD_ERASE_32K                  0x52
>>> >> > @@ -38,6 +40,7 @@
>>> >> >
>>> >> >  /* Common status */
>>> >> >  #define STATUS_WIP                     0x01
>>> >> > +#define STATUS_PEC                     0x80
>>> >> >
>>> >> >  /* Send a single-byte command to the device and read the response */
>>> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
>>> >> > size_t
>>> >> > len);
>>> >> > --
>>> >> > 1.8.3
>>> >>
>>

May be I will explore much for this, update the changes later.
and i will remains the code as before as of now.

Do you have any more comments on this series.
I will going to send v3 and planning to apply.

--
Thanks,
Jagan.

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Tom,

On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > Hi Jagan,
>>> >
>>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
>>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>>> >>
>>> >> Updated the spi_flash framework to handle all sizes of flashes
>>> >> using bank/extd addr reg facility
>>> >>
>>> >> The current implementation in spi_flash supports 3-byte address mode
>>> >> due to this up to 16Mbytes amount of flash is able to access for those
>>> >> flashes which has an actual size of > 16MB.
>>> >>
>>> >> As most of the flashes introduces a bank/extd address registers
>>> >> for accessing the flashes in 16Mbytes of banks if the flash size
>>> >> is > 16Mbytes, this new scheme will add the bank selection feature
>>> >> for performing write/erase operations on all flashes.
>>> >>
>>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>>> >
>>> >
>>> > I have a few comments on this series, but it mostly looks fine. I think
>>> > the
>>> > new code is correct.
>>> >
>>> > The patches did not apply cleanly for me. Perhaps I am missing
>>> > something. My
>>> > tree looks like this after I did a bit of merging:
>>>
>>> Which patches you had an issues while applying,we have few patches on
>>> u-boot-spi.git i created these on top of it.
>>
>>
>> I am not sure now - sorry I did not keep a record. But the bundle I used is
>> here, and it doesn't apply cleanly.
>>
>> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>>
>> git am ~/Downloads/bundle-4407-jagan.mbox
>> Applying: sf: Add bank address register writing support
>> Applying: sf: Add bank address register reading support
>> Applying: sf: Add extended addr write support for winbond|stmicro
>> Applying: sf: Add extended addr read support for winbond|stmicro
>> Applying: sf: read flash bank addr register at probe time
>> Applying: sf: Update sf to support all sizes of flashes
>> error: patch failed: drivers/mtd/spi/spi_flash.c:117
>> error: drivers/mtd/spi/spi_flash.c: patch does not apply
>> Patch failed at 0006 sf: Update sf to support all sizes of flashes
>> The copy of the patch that failed is found in:
>>    /home/sjg/u/.git/rebase-apply/patch
>> When you have resolved this problem, run "git am --resolved".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort"
>>
>>>
>>>
>>> >
>>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
>>> > devices
>>> > via address shift
>>> > f700095 sf: Add Flag status register polling support
>>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
>>> > fc31387 sf: Use spi_flash_read_common() in write status poll
>>> > 923e40e sf: spansion: Add support for S25FL512S_256K
>>> > c72e52a sf: stmicro: Add support for N25Q1024A
>>> > 4066f71 sf: stmicro: Add support for N25Q1024
>>> > 0efaf86 sf: stmicro: Add support for N25Q512A
>>> > 8fd962f sf: stmicro: Add support for N25Q512
>>> > f1a2080 sf: Use spi_flash_addr() in write call
>>> > 31ed498 sf: Update sf read to support all sizes of flashes
>>> > 0f77642 sf: Update sf to support all sizes of flashes
>>> > 9e57220 sf: read flash bank addr register at probe time
>>> > e99a43d sf: Add extended addr read support for winbond|stmicro
>>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
>>> > f02ecf1 sf: Add bank address register reading support
>>> > 02ba27c sf: Add bank address register writing support
>>> >
>>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
>>> > should be
>>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space
>>> > does
>>> > this add?
>>>
>>> Initially i thought of the same, but I just updated sf which is
>>> generic to all sizes of flashes.
>>> due to this i haven't included the bank read/write on macros, and the
>>> flash ops will call these
>>> bank write call irrespective of flash sizes.
>>>
>>> As flashes which has below 16Mbytes will have a bank_curr value 0
>>> so-that even bank write will exit for
>>> bank 0 operations.
>>
>>
>> Yes this is fine.
>>
>>>
>>>
>>> +       if (flash->bank_curr == bank_sel) {
>>> +               debug("SF: not require to enable bank%d\n", bank_sel);
>>> +               return 0;
>>> +       }
>>> +
>>>
>>> And due to these framework changes bank+flash ops addition, bin size
>>> increases appr' ~600bytes
>>> by enabling stmicro, winbond and spansion flash drivers.(please check
>>> the size from your end also if required)
>>
>>
>> I suggest you make that function a nop (perhaps using an #ifdef
>> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
>> don't increase U-Boot code size for those boards that don't need support
>> larger devices (which I guess is almost all of them, right now). U-Boot is
>> quite concerned about code size.
>
> Little concern here, the point here I stated that these new changes is
> common for all sizes of flashes.(which are less or greater than
> 16Mbytes).
> and yes it increase the code size little bit but i don't think it will
> require the separate macro.

Any comments.

--
Thanks,
Jagan.

>
> Thanks,
> Jagan.
>
>>
>> Tom may chime in and decide it is fine, though.
>>
>>>
>>>
>>> Please see the commit body on below thread for more info
>>> http://patchwork.ozlabs.org/patch/247954/
>>>
>>> >
>>> > In your change to spi_flash_cmd_poll_bit() the effect is not the same I
>>> > think. It is designed to hold CS active and read the status byte
>>> > continuously until it changes. But your implementation asserts CS, reads
>>> > the
>>> > status byte, de-asserts CS, then repeats. Why do we want to change this?
>>>
>>> I commented on the actual patch thread, please refer,
>>
>>
>> OK I will take a look.
>>
>>>
>>>
>>> >
>>> >
>>> >
>>> >> ---
>>> >> Changes for v2:
>>> >>         - none
>>> >>
>>> >>  drivers/mtd/spi/spi_flash.c | 39
>>> >> ++++++++++++++++++++++++++-------------
>>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
>>> >>
>>> >> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>> >> index 4576a16..5386884 100644
>>> >> --- a/drivers/mtd/spi/spi_flash.c
>>> >> +++ b/drivers/mtd/spi/spi_flash.c
>>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
>>> >> *flash,
>>> >> u32 offset,
>>> >>         unsigned long page_addr, byte_addr, page_size;
>>> >>         size_t chunk_len, actual;
>>> >>         int ret;
>>> >> -       u8 cmd[4];
>>> >> +       u8 cmd[4], bank_sel;
>>> >>
>>> >>         page_size = flash->page_size;
>>> >> -       page_addr = offset / page_size;
>>> >> -       byte_addr = offset % page_size;
>>> >>
>>> >>         ret = spi_claim_bus(flash->spi);
>>> >>         if (ret) {
>>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
>>> >> *flash,
>>> >> u32 offset,
>>> >>
>>> >>         cmd[0] = CMD_PAGE_PROGRAM;
>>> >>         for (actual = 0; actual < len; actual += chunk_len) {
>>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
>>> >> +
>>> >> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
>>> >> +               if (ret) {
>>> >> +                       debug("SF: fail to set bank%d\n", bank_sel);
>>> >> +                       return ret;
>>> >> +               }
>>> >
>>> >
>>> > So we are now doing this for all chips. But isn't it true that only some
>>> > chips (>16MB?) have a bank address. If so, then I think we should have a
>>> > flag somewhere to enable this feature
>>>
>>> APAMK, currently stmicro, winbond, spansion and macronix have a
>>> flashes which has > 16Mbytes flashes.
>>>
>>> And macronix is also have same bank setup like stmicro, extended addr
>>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
>>> We need to add this in near future.
>>>
>>> I have added Prafulla Wadaskar on this thread (initial contributor for
>>> macronix.c), may be he will give some more information
>>> for accessing > 16Mbytes flashes in 3-byte addr mode.
>>>
>>> I think we can go ahead for now, may be we will tune sf some more in
>>> future based on the availability of different flash which crosses
>>> 16Mbytes
>>> with different apparoch (other than banking/extended), what do you say?
>>
>>
>> OK, well we don't need a flag since you will never issue the bank address
>> command unless the chip is larger than 16MB.,
>>
>>>
>>>
>>> >
>>> >>
>>> >> +
>>> >> +               page_addr = offset / page_size;
>>> >> +               byte_addr = offset % page_size;
>>> >
>>> >
>>> > This is OK I think. We really don't care about the slower divide so it
>>> > is
>>> > not worth optimising for I think.
>>>
>>> Yes, I just used the existing spi_flash_addr with offset as an
>>> argument, anyway sf write have a logic
>>> to use writing in terms of page sizes and even offset is also varies
>>> page sizes or requested sizes.
>>>
>>> Thanks,
>>> Jagan.
>>
>>
>>
>> Regards,
>> Simon
>>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Simon Glass]

Hi Jagan,

On Mon, Jun 10, 2013 at 8:27 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:

> Hi Simon,
>
> On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
> > Hi Simon,
> >
> > On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
> >> Hi Jagan,
> >>
> >> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
> >>>
> >>> Hi Simon,
> >>>
> >>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
> >>> > Hi Jagan,
> >>> >
> >>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@gmail.com
> >
> >>> > wrote:
> >>> >>
> >>> >> Hi Simon,
> >>> >>
> >>> >> Please let know your comments.
> >>> >>
> >>> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
> >>> >> poll code on spi_flash_cmd_wait_ready()
> >>> >> as no other call for spi_flash_cmd_poll_bit() this.
> >>> >>
> >>> >> And also for read_status the check_status i assigned as 0,earlier it
> >>> >> has direct 0 (w/o check_status variable).
> >>> >>
> >>> >> To add the support for flag status on the same code, i define this
> >>> >> check_status.
> >>> >> I don't see any coding functionality change for now, compared to
> >>> >> before.
> >>> >
> >>> >
> >>> > This is not the right patch, but in one of them you remove
> >>> > spi_flash_cmd_poll_bit(), so that it no longer works the same way.
> You
> >>> > will
> >>> > get lots of individual SPI transactions on the bus instead of a
> single
> >>> > one
> >>> > that reads the status byte continuously.
> >>>
> >>> I couldn't understand you clearly.
> >>>
> >>> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
> >>> with flash, timeout values.
> >>> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
> >>> with CMD_READ_STATUS and STATUS_WIP.
> >>>
> >>> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
> >>> other caller except spi_flash_cmd_wait_ready()
> >>> so all polling stuff on spi_flash_cmd_wait_ready() means, still
> >>> erase/program can call spi_flash_cmd_wait_ready() with
> >>> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
> >>> used on spi_flash_cmd_wait_ready() instead
> >>> of passing through spi_flash_cmd_poll_bit().
> >>>
> >>> Any wrong here, please comment.
> >>
> >>
> >> You have changed the implementation of spi_flash_cmd_wait_ready() so
> that
> >> instead of polling for the bit within a single SPI transaction (CS
> staying
> >> asserted) you are now doing multiple transactions.
> >>
> >> The old code for spi_flash_cmd_wait_ready() ended up with an algorithm
> like
> >> this, when you look inside the nested functions:
> >>
> >> spi_claim_bus(spi);
> >> ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
> >> do {
> >> ret = spi_xfer(spi, 8, NULL, &status, 0);
> >>         ...
> >> }
> >> spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
> >> spi_release_bus(spi);
> >>
> >> The new code is:
> >>
> >> do {
> >> spi_claim_bus(spi);
> >>         ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
> >>         ret = spi_xfer(spi, data_len * 8, data_out, data_in,
> SPI_XFER_END);
> >> spi_release_bus(spi);
> >>         ...
> >> } while (...)
> >>
> >>
> >> Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
> >> inside the loop.
> >
> > Understand, i thought as we do _END transfer in old code as well.
> > It could be fine to go with using common function as it will do _BEGIN
> and _END.
> >
> > What could be the problem you see if we do _BEGIN and _END on loop...?
> >
> >>
> >> Is there a reason why this change is needed? Does updating the bank
> address
> >> not work with the old code?
> >
> > It doesn't effect with banking.
> >
> > Thanks,
> > Jagan.
> >
> >>
> >> It is find to move the spi_flash_cmd_poll_bit() function into
> >> spi_flash_cmd_wait_ready() - I am not worried about that. I am only
> worried
> >> about your change of algorithm as above.
> >>
> >> Regards,
> >> Simon
> >>
> >>>
> >>>
> >>> Thanks,
> >>> Jagan.
> >>>
> >>> >
> >>> > Do we need to change this?
> >>> >
> >>> >
> >>> >>
> >>> >>
> >>> >> --
> >>> >> Thanks,
> >>> >> Jagan.
> >>> >>
> >>> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
> >>> >> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >>> >> > Flag status register polling is required for micron 512Mb flash
> >>> >> > devices onwards, for performing erase/program operations.
> >>> >> >
> >>> >> > Like polling for WIP(Write-In-Progress) bit in read status
> register,
> >>> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
> >>> >> > bit in flag status register.
> >>> >> >
> >>> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >>> >> > ---
> >>> >> > Changes for v2:
> >>> >> >         - none
> >>> >> >
> >>> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
> >>> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
> >>> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >>> >> >
> >>> >> > diff --git a/drivers/mtd/spi/spi_flash.c
> >>> >> > b/drivers/mtd/spi/spi_flash.c
> >>> >> > index 527423d..8cd2988 100644
> >>> >> > --- a/drivers/mtd/spi/spi_flash.c
> >>> >> > +++ b/drivers/mtd/spi/spi_flash.c
> >>> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct
> spi_flash
> >>> >> > *flash, unsigned long timeout)
> >>> >> >         unsigned long timebase;
> >>> >> >         int ret;
> >>> >> >         u8 status;
> >>> >> > +       u8 check_status = 0x0;
> >>> >> >         u8 poll_bit = STATUS_WIP;
> >>> >> >         u8 cmd = CMD_READ_STATUS;
> >>> >> >
> >>> >> > +       if ((flash->idcode0 == 0x20) &&
> >>> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
> >>> >> > +               poll_bit = STATUS_PEC;
> >>> >> > +               check_status = poll_bit;
> >>> >> > +               cmd = CMD_FLAG_STATUS;
> >>> >> > +       }
> >>> >> > +
> >>> >> >         timebase = get_timer(0);
> >>> >> >         do {
> >>> >> >                 WATCHDOG_RESET();
> >>> >> >
> >>> >> >                 ret = spi_flash_read_common(flash, &cmd, 1,
> &status,
> >>> >> > 1);
> >>> >> >                 if (ret < 0) {
> >>> >> > -                       debug("SF: fail to read read status
> >>> >> > register\n");
> >>> >> > +                       debug("SF: fail to read %s status
> >>> >> > register\n",
> >>> >> > +                               cmd == CMD_READ_STATUS ? "read" :
> >>> >> > "flag");
> >>> >> >                         return ret;
> >>> >> >                 }
> >>> >> >
> >>> >> > -               if ((status & poll_bit) == 0)
> >>> >> > +               if ((status & poll_bit) == check_status)
> >>> >> >                         break;
> >>> >> >
> >>> >> >         } while (get_timer(timebase) < timeout);
> >>> >> >
> >>> >> > -       if ((status & poll_bit) == 0)
> >>> >> > +       if ((status & poll_bit) == check_status)
> >>> >> >                 return 0;
> >>> >> >
> >>> >> >         /* Timed out */
> >>> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
> >>> >> > b/drivers/mtd/spi/spi_flash_internal.h
> >>> >> > index ac4530f..cb7a505 100644
> >>> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
> >>> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
> >>> >> > @@ -13,6 +13,7 @@
> >>> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
> >>> >> >
> >>> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
> >>> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
> >>> >> >
> >>> >> >  /* Common commands */
> >>> >> >  #define CMD_READ_ID                    0x9f
> >>> >> > @@ -24,6 +25,7 @@
> >>> >> >  #define CMD_PAGE_PROGRAM               0x02
> >>> >> >  #define CMD_WRITE_DISABLE              0x04
> >>> >> >  #define CMD_READ_STATUS                        0x05
> >>> >> > +#define CMD_FLAG_STATUS                        0x70
> >>> >> >  #define CMD_WRITE_ENABLE               0x06
> >>> >> >  #define CMD_ERASE_4K                   0x20
> >>> >> >  #define CMD_ERASE_32K                  0x52
> >>> >> > @@ -38,6 +40,7 @@
> >>> >> >
> >>> >> >  /* Common status */
> >>> >> >  #define STATUS_WIP                     0x01
> >>> >> > +#define STATUS_PEC                     0x80
> >>> >> >
> >>> >> >  /* Send a single-byte command to the device and read the
> response */
> >>> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
> >>> >> > size_t
> >>> >> > len);
> >>> >> > --
> >>> >> > 1.8.3
> >>> >>
> >>
>
> May be I will explore much for this, update the changes later.
> and i will remains the code as before as of now.
>

Do you mean you will keep the old status-reading algorithm, or change to
your new one?


>
> Do you have any more comments on this series.
> I will going to send v3 and planning to apply.
>

I don't have any other comments.


>
> --
> Thanks,
> Jagan.
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Simon Glass]

On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:

> Hi Tom,
>
> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
> > Hi Simon,
> >
> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
> >> Hi,
> >>
> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
> >>>
> >>> Hi Simon,
> >>>
> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
> >>> > Hi Jagan,
> >>> >
> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >>> >>
> >>> >> Updated the spi_flash framework to handle all sizes of flashes
> >>> >> using bank/extd addr reg facility
> >>> >>
> >>> >> The current implementation in spi_flash supports 3-byte address mode
> >>> >> due to this up to 16Mbytes amount of flash is able to access for
> those
> >>> >> flashes which has an actual size of > 16MB.
> >>> >>
> >>> >> As most of the flashes introduces a bank/extd address registers
> >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
> >>> >> is > 16Mbytes, this new scheme will add the bank selection feature
> >>> >> for performing write/erase operations on all flashes.
> >>> >>
> >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >>> >
> >>> >
> >>> > I have a few comments on this series, but it mostly looks fine. I
> think
> >>> > the
> >>> > new code is correct.
> >>> >
> >>> > The patches did not apply cleanly for me. Perhaps I am missing
> >>> > something. My
> >>> > tree looks like this after I did a bit of merging:
> >>>
> >>> Which patches you had an issues while applying,we have few patches on
> >>> u-boot-spi.git i created these on top of it.
> >>
> >>
> >> I am not sure now - sorry I did not keep a record. But the bundle I
> used is
> >> here, and it doesn't apply cleanly.
> >>
> >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
> >>
> >> git am ~/Downloads/bundle-4407-jagan.mbox
> >> Applying: sf: Add bank address register writing support
> >> Applying: sf: Add bank address register reading support
> >> Applying: sf: Add extended addr write support for winbond|stmicro
> >> Applying: sf: Add extended addr read support for winbond|stmicro
> >> Applying: sf: read flash bank addr register at probe time
> >> Applying: sf: Update sf to support all sizes of flashes
> >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
> >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
> >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
> >> The copy of the patch that failed is found in:
> >>    /home/sjg/u/.git/rebase-apply/patch
> >> When you have resolved this problem, run "git am --resolved".
> >> If you prefer to skip this patch, run "git am --skip" instead.
> >> To restore the original branch and stop patching, run "git am --abort"
> >>
> >>>
> >>>
> >>> >
> >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
> >>> > devices
> >>> > via address shift
> >>> > f700095 sf: Add Flag status register polling support
> >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
> >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
> >>> > c72e52a sf: stmicro: Add support for N25Q1024A
> >>> > 4066f71 sf: stmicro: Add support for N25Q1024
> >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
> >>> > 8fd962f sf: stmicro: Add support for N25Q512
> >>> > f1a2080 sf: Use spi_flash_addr() in write call
> >>> > 31ed498 sf: Update sf read to support all sizes of flashes
> >>> > 0f77642 sf: Update sf to support all sizes of flashes
> >>> > 9e57220 sf: read flash bank addr register at probe time
> >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
> >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
> >>> > f02ecf1 sf: Add bank address register reading support
> >>> > 02ba27c sf: Add bank address register writing support
> >>> >
> >>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
> >>> > should be
> >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
> space
> >>> > does
> >>> > this add?
> >>>
> >>> Initially i thought of the same, but I just updated sf which is
> >>> generic to all sizes of flashes.
> >>> due to this i haven't included the bank read/write on macros, and the
> >>> flash ops will call these
> >>> bank write call irrespective of flash sizes.
> >>>
> >>> As flashes which has below 16Mbytes will have a bank_curr value 0
> >>> so-that even bank write will exit for
> >>> bank 0 operations.
> >>
> >>
> >> Yes this is fine.
> >>
> >>>
> >>>
> >>> +       if (flash->bank_curr == bank_sel) {
> >>> +               debug("SF: not require to enable bank%d\n", bank_sel);
> >>> +               return 0;
> >>> +       }
> >>> +
> >>>
> >>> And due to these framework changes bank+flash ops addition, bin size
> >>> increases appr' ~600bytes
> >>> by enabling stmicro, winbond and spansion flash drivers.(please check
> >>> the size from your end also if required)
> >>
> >>
> >> I suggest you make that function a nop (perhaps using an #ifdef
> >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
> >> don't increase U-Boot code size for those boards that don't need support
> >> larger devices (which I guess is almost all of them, right now). U-Boot
> is
> >> quite concerned about code size.
> >
> > Little concern here, the point here I stated that these new changes is
> > common for all sizes of flashes.(which are less or greater than
> > 16Mbytes).
> > and yes it increase the code size little bit but i don't think it will
> > require the separate macro.
>
> Any comments.
>

In U-Boot it is generally not acceptable to increase code size for existing
boards when adding a new feature that they don't use. So I suspect in this
case you should add a new CONFIG to enable your feature. It seems to
increase code by more than 200 bytes for snow, for example.

Tom may have further comments.

Also my buildman run of your commit gave an error on this commit:

07: sf: Update sf to support all sizes of flashes


Regards,
Simon


>
> --
> Thanks,
> Jagan.
>
> >
> > Thanks,
> > Jagan.
> >
> >>
> >> Tom may chime in and decide it is fine, though.
> >>
> >>>
> >>>
> >>> Please see the commit body on below thread for more info
> >>> http://patchwork.ozlabs.org/patch/247954/
> >>>
> >>> >
> >>> > In your change to spi_flash_cmd_poll_bit() the effect is not the
> same I
> >>> > think. It is designed to hold CS active and read the status byte
> >>> > continuously until it changes. But your implementation asserts CS,
> reads
> >>> > the
> >>> > status byte, de-asserts CS, then repeats. Why do we want to change
> this?
> >>>
> >>> I commented on the actual patch thread, please refer,
> >>
> >>
> >> OK I will take a look.
> >>
> >>>
> >>>
> >>> >
> >>> >
> >>> >
> >>> >> ---
> >>> >> Changes for v2:
> >>> >>         - none
> >>> >>
> >>> >>  drivers/mtd/spi/spi_flash.c | 39
> >>> >> ++++++++++++++++++++++++++-------------
> >>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
> >>> >>
> >>> >> diff --git a/drivers/mtd/spi/spi_flash.c
> b/drivers/mtd/spi/spi_flash.c
> >>> >> index 4576a16..5386884 100644
> >>> >> --- a/drivers/mtd/spi/spi_flash.c
> >>> >> +++ b/drivers/mtd/spi/spi_flash.c
> >>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
> >>> >> *flash,
> >>> >> u32 offset,
> >>> >>         unsigned long page_addr, byte_addr, page_size;
> >>> >>         size_t chunk_len, actual;
> >>> >>         int ret;
> >>> >> -       u8 cmd[4];
> >>> >> +       u8 cmd[4], bank_sel;
> >>> >>
> >>> >>         page_size = flash->page_size;
> >>> >> -       page_addr = offset / page_size;
> >>> >> -       byte_addr = offset % page_size;
> >>> >>
> >>> >>         ret = spi_claim_bus(flash->spi);
> >>> >>         if (ret) {
> >>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
> >>> >> *flash,
> >>> >> u32 offset,
> >>> >>
> >>> >>         cmd[0] = CMD_PAGE_PROGRAM;
> >>> >>         for (actual = 0; actual < len; actual += chunk_len) {
> >>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> >>> >> +
> >>> >> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> >>> >> +               if (ret) {
> >>> >> +                       debug("SF: fail to set bank%d\n", bank_sel);
> >>> >> +                       return ret;
> >>> >> +               }
> >>> >
> >>> >
> >>> > So we are now doing this for all chips. But isn't it true that only
> some
> >>> > chips (>16MB?) have a bank address. If so, then I think we should
> have a
> >>> > flag somewhere to enable this feature
> >>>
> >>> APAMK, currently stmicro, winbond, spansion and macronix have a
> >>> flashes which has > 16Mbytes flashes.
> >>>
> >>> And macronix is also have same bank setup like stmicro, extended addr
> >>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
> >>> We need to add this in near future.
> >>>
> >>> I have added Prafulla Wadaskar on this thread (initial contributor for
> >>> macronix.c), may be he will give some more information
> >>> for accessing > 16Mbytes flashes in 3-byte addr mode.
> >>>
> >>> I think we can go ahead for now, may be we will tune sf some more in
> >>> future based on the availability of different flash which crosses
> >>> 16Mbytes
> >>> with different apparoch (other than banking/extended), what do you say?
> >>
> >>
> >> OK, well we don't need a flag since you will never issue the bank
> address
> >> command unless the chip is larger than 16MB.,
> >>
> >>>
> >>>
> >>> >
> >>> >>
> >>> >> +
> >>> >> +               page_addr = offset / page_size;
> >>> >> +               byte_addr = offset % page_size;
> >>> >
> >>> >
> >>> > This is OK I think. We really don't care about the slower divide so
> it
> >>> > is
> >>> > not worth optimising for I think.
> >>>
> >>> Yes, I just used the existing spi_flash_addr with offset as an
> >>> argument, anyway sf write have a logic
> >>> to use writing in terms of page sizes and even offset is also varies
> >>> page sizes or requested sizes.
> >>>
> >>> Thanks,
> >>> Jagan.
> >>
> >>
> >>
> >> Regards,
> >> Simon
> >>
>

[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

[Jagan Teki]

On Tue, Jun 11, 2013 at 2:01 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Mon, Jun 10, 2013 at 8:27 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
>>
>> Hi Simon,
>>
>> On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki <jagannadh.teki@gmail.com>
>> wrote:
>> > Hi Simon,
>> >
>> > On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
>> >> Hi Jagan,
>> >>
>> >> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki@gmail.com>
>> >> wrote:
>> >>>
>> >>> Hi Simon,
>> >>>
>> >>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg@chromium.org> wrote:
>> >>> > Hi Jagan,
>> >>> >
>> >>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki
>> >>> > <jagannadh.teki@gmail.com>
>> >>> > wrote:
>> >>> >>
>> >>> >> Hi Simon,
>> >>> >>
>> >>> >> Please let know your comments.
>> >>> >>
>> >>> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
>> >>> >> poll code on spi_flash_cmd_wait_ready()
>> >>> >> as no other call for spi_flash_cmd_poll_bit() this.
>> >>> >>
>> >>> >> And also for read_status the check_status i assigned as 0,earlier
>> >>> >> it
>> >>> >> has direct 0 (w/o check_status variable).
>> >>> >>
>> >>> >> To add the support for flag status on the same code, i define this
>> >>> >> check_status.
>> >>> >> I don't see any coding functionality change for now, compared to
>> >>> >> before.
>> >>> >
>> >>> >
>> >>> > This is not the right patch, but in one of them you remove
>> >>> > spi_flash_cmd_poll_bit(), so that it no longer works the same way.
>> >>> > You
>> >>> > will
>> >>> > get lots of individual SPI transactions on the bus instead of a
>> >>> > single
>> >>> > one
>> >>> > that reads the status byte continuously.
>> >>>
>> >>> I couldn't understand you clearly.
>> >>>
>> >>> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
>> >>> with flash, timeout values.
>> >>> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
>> >>> with CMD_READ_STATUS and STATUS_WIP.
>> >>>
>> >>> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
>> >>> other caller except spi_flash_cmd_wait_ready()
>> >>> so all polling stuff on spi_flash_cmd_wait_ready() means, still
>> >>> erase/program can call spi_flash_cmd_wait_ready() with
>> >>> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
>> >>> used on spi_flash_cmd_wait_ready() instead
>> >>> of passing through spi_flash_cmd_poll_bit().
>> >>>
>> >>> Any wrong here, please comment.
>> >>
>> >>
>> >> You have changed the implementation of spi_flash_cmd_wait_ready() so
>> >> that
>> >> instead of polling for the bit within a single SPI transaction (CS
>> >> staying
>> >> asserted) you are now doing multiple transactions.
>> >>
>> >> The old code for spi_flash_cmd_wait_ready() ended up with an algorithm
>> >> like
>> >> this, when you look inside the nested functions:
>> >>
>> >> spi_claim_bus(spi);
>> >> ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>> >> do {
>> >> ret = spi_xfer(spi, 8, NULL, &status, 0);
>> >>         ...
>> >> }
>> >> spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
>> >> spi_release_bus(spi);
>> >>
>> >> The new code is:
>> >>
>> >> do {
>> >> spi_claim_bus(spi);
>> >>         ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>> >>         ret = spi_xfer(spi, data_len * 8, data_out, data_in,
>> >> SPI_XFER_END);
>> >> spi_release_bus(spi);
>> >>         ...
>> >> } while (...)
>> >>
>> >>
>> >> Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
>> >> inside the loop.
>> >
>> > Understand, i thought as we do _END transfer in old code as well.
>> > It could be fine to go with using common function as it will do _BEGIN
>> > and _END.
>> >
>> > What could be the problem you see if we do _BEGIN and _END on loop...?
>> >
>> >>
>> >> Is there a reason why this change is needed? Does updating the bank
>> >> address
>> >> not work with the old code?
>> >
>> > It doesn't effect with banking.
>> >
>> > Thanks,
>> > Jagan.
>> >
>> >>
>> >> It is find to move the spi_flash_cmd_poll_bit() function into
>> >> spi_flash_cmd_wait_ready() - I am not worried about that. I am only
>> >> worried
>> >> about your change of algorithm as above.
>> >>
>> >> Regards,
>> >> Simon
>> >>
>> >>>
>> >>>
>> >>> Thanks,
>> >>> Jagan.
>> >>>
>> >>> >
>> >>> > Do we need to change this?
>> >>> >
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> --
>> >>> >> Thanks,
>> >>> >> Jagan.
>> >>> >>
>> >>> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
>> >>> >> <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> >>> >> > Flag status register polling is required for micron 512Mb flash
>> >>> >> > devices onwards, for performing erase/program operations.
>> >>> >> >
>> >>> >> > Like polling for WIP(Write-In-Progress) bit in read status
>> >>> >> > register,
>> >>> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
>> >>> >> > bit in flag status register.
>> >>> >> >
>> >>> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> >>> >> > ---
>> >>> >> > Changes for v2:
>> >>> >> >         - none
>> >>> >> >
>> >>> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>> >>> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>> >>> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
>> >>> >> >
>> >>> >> > diff --git a/drivers/mtd/spi/spi_flash.c
>> >>> >> > b/drivers/mtd/spi/spi_flash.c
>> >>> >> > index 527423d..8cd2988 100644
>> >>> >> > --- a/drivers/mtd/spi/spi_flash.c
>> >>> >> > +++ b/drivers/mtd/spi/spi_flash.c
>> >>> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct
>> >>> >> > spi_flash
>> >>> >> > *flash, unsigned long timeout)
>> >>> >> >         unsigned long timebase;
>> >>> >> >         int ret;
>> >>> >> >         u8 status;
>> >>> >> > +       u8 check_status = 0x0;
>> >>> >> >         u8 poll_bit = STATUS_WIP;
>> >>> >> >         u8 cmd = CMD_READ_STATUS;
>> >>> >> >
>> >>> >> > +       if ((flash->idcode0 == 0x20) &&
>> >>> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
>> >>> >> > +               poll_bit = STATUS_PEC;
>> >>> >> > +               check_status = poll_bit;
>> >>> >> > +               cmd = CMD_FLAG_STATUS;
>> >>> >> > +       }
>> >>> >> > +
>> >>> >> >         timebase = get_timer(0);
>> >>> >> >         do {
>> >>> >> >                 WATCHDOG_RESET();
>> >>> >> >
>> >>> >> >                 ret = spi_flash_read_common(flash, &cmd, 1,
>> >>> >> > &status,
>> >>> >> > 1);
>> >>> >> >                 if (ret < 0) {
>> >>> >> > -                       debug("SF: fail to read read status
>> >>> >> > register\n");
>> >>> >> > +                       debug("SF: fail to read %s status
>> >>> >> > register\n",
>> >>> >> > +                               cmd == CMD_READ_STATUS ? "read" :
>> >>> >> > "flag");
>> >>> >> >                         return ret;
>> >>> >> >                 }
>> >>> >> >
>> >>> >> > -               if ((status & poll_bit) == 0)
>> >>> >> > +               if ((status & poll_bit) == check_status)
>> >>> >> >                         break;
>> >>> >> >
>> >>> >> >         } while (get_timer(timebase) < timeout);
>> >>> >> >
>> >>> >> > -       if ((status & poll_bit) == 0)
>> >>> >> > +       if ((status & poll_bit) == check_status)
>> >>> >> >                 return 0;
>> >>> >> >
>> >>> >> >         /* Timed out */
>> >>> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
>> >>> >> > b/drivers/mtd/spi/spi_flash_internal.h
>> >>> >> > index ac4530f..cb7a505 100644
>> >>> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
>> >>> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
>> >>> >> > @@ -13,6 +13,7 @@
>> >>> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>> >>> >> >
>> >>> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
>> >>> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
>> >>> >> >
>> >>> >> >  /* Common commands */
>> >>> >> >  #define CMD_READ_ID                    0x9f
>> >>> >> > @@ -24,6 +25,7 @@
>> >>> >> >  #define CMD_PAGE_PROGRAM               0x02
>> >>> >> >  #define CMD_WRITE_DISABLE              0x04
>> >>> >> >  #define CMD_READ_STATUS                        0x05
>> >>> >> > +#define CMD_FLAG_STATUS                        0x70
>> >>> >> >  #define CMD_WRITE_ENABLE               0x06
>> >>> >> >  #define CMD_ERASE_4K                   0x20
>> >>> >> >  #define CMD_ERASE_32K                  0x52
>> >>> >> > @@ -38,6 +40,7 @@
>> >>> >> >
>> >>> >> >  /* Common status */
>> >>> >> >  #define STATUS_WIP                     0x01
>> >>> >> > +#define STATUS_PEC                     0x80
>> >>> >> >
>> >>> >> >  /* Send a single-byte command to the device and read the
>> >>> >> > response */
>> >>> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
>> >>> >> > size_t
>> >>> >> > len);
>> >>> >> > --
>> >>> >> > 1.8.3
>> >>> >>
>> >>
>>
>> May be I will explore much for this, update the changes later.
>> and i will remains the code as before as of now.
>
>
> Do you mean you will keep the old status-reading algorithm, or change to
> your new one?

Will keep the old status-read algo.

--
Thanks,
Jagan.

>
>>
>>
>> Do you have any more comments on this series.
>> I will going to send v3 and planning to apply.
>
>
> I don't have any other comments.
>
>>
>>
>> --
>> Thanks,
>> Jagan.
>
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Simon,

On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <sjg@chromium.org> wrote:
> On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
>>
>> Hi Tom,
>>
>> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
>> wrote:
>> > Hi Simon,
>> >
>> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
>> >> Hi,
>> >>
>> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com>
>> >> wrote:
>> >>>
>> >>> Hi Simon,
>> >>>
>> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
>> >>> > Hi Jagan,
>> >>> >
>> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
>> >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> >>> >>
>> >>> >> Updated the spi_flash framework to handle all sizes of flashes
>> >>> >> using bank/extd addr reg facility
>> >>> >>
>> >>> >> The current implementation in spi_flash supports 3-byte address
>> >>> >> mode
>> >>> >> due to this up to 16Mbytes amount of flash is able to access for
>> >>> >> those
>> >>> >> flashes which has an actual size of > 16MB.
>> >>> >>
>> >>> >> As most of the flashes introduces a bank/extd address registers
>> >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
>> >>> >> is > 16Mbytes, this new scheme will add the bank selection feature
>> >>> >> for performing write/erase operations on all flashes.
>> >>> >>
>> >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> >>> >
>> >>> >
>> >>> > I have a few comments on this series, but it mostly looks fine. I
>> >>> > think
>> >>> > the
>> >>> > new code is correct.
>> >>> >
>> >>> > The patches did not apply cleanly for me. Perhaps I am missing
>> >>> > something. My
>> >>> > tree looks like this after I did a bit of merging:
>> >>>
>> >>> Which patches you had an issues while applying,we have few patches on
>> >>> u-boot-spi.git i created these on top of it.
>> >>
>> >>
>> >> I am not sure now - sorry I did not keep a record. But the bundle I
>> >> used is
>> >> here, and it doesn't apply cleanly.
>> >>
>> >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>> >>
>> >> git am ~/Downloads/bundle-4407-jagan.mbox
>> >> Applying: sf: Add bank address register writing support
>> >> Applying: sf: Add bank address register reading support
>> >> Applying: sf: Add extended addr write support for winbond|stmicro
>> >> Applying: sf: Add extended addr read support for winbond|stmicro
>> >> Applying: sf: read flash bank addr register at probe time
>> >> Applying: sf: Update sf to support all sizes of flashes
>> >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
>> >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
>> >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
>> >> The copy of the patch that failed is found in:
>> >>    /home/sjg/u/.git/rebase-apply/patch
>> >> When you have resolved this problem, run "git am --resolved".
>> >> If you prefer to skip this patch, run "git am --skip" instead.
>> >> To restore the original branch and stop patching, run "git am --abort"
>> >>
>> >>>
>> >>>
>> >>> >
>> >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
>> >>> > devices
>> >>> > via address shift
>> >>> > f700095 sf: Add Flag status register polling support
>> >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
>> >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
>> >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
>> >>> > c72e52a sf: stmicro: Add support for N25Q1024A
>> >>> > 4066f71 sf: stmicro: Add support for N25Q1024
>> >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
>> >>> > 8fd962f sf: stmicro: Add support for N25Q512
>> >>> > f1a2080 sf: Use spi_flash_addr() in write call
>> >>> > 31ed498 sf: Update sf read to support all sizes of flashes
>> >>> > 0f77642 sf: Update sf to support all sizes of flashes
>> >>> > 9e57220 sf: read flash bank addr register at probe time
>> >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
>> >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
>> >>> > f02ecf1 sf: Add bank address register reading support
>> >>> > 02ba27c sf: Add bank address register writing support
>> >>> >
>> >>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
>> >>> > should be
>> >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
>> >>> > space
>> >>> > does
>> >>> > this add?
>> >>>
>> >>> Initially i thought of the same, but I just updated sf which is
>> >>> generic to all sizes of flashes.
>> >>> due to this i haven't included the bank read/write on macros, and the
>> >>> flash ops will call these
>> >>> bank write call irrespective of flash sizes.
>> >>>
>> >>> As flashes which has below 16Mbytes will have a bank_curr value 0
>> >>> so-that even bank write will exit for
>> >>> bank 0 operations.
>> >>
>> >>
>> >> Yes this is fine.
>> >>
>> >>>
>> >>>
>> >>> +       if (flash->bank_curr == bank_sel) {
>> >>> +               debug("SF: not require to enable bank%d\n", bank_sel);
>> >>> +               return 0;
>> >>> +       }
>> >>> +
>> >>>
>> >>> And due to these framework changes bank+flash ops addition, bin size
>> >>> increases appr' ~600bytes
>> >>> by enabling stmicro, winbond and spansion flash drivers.(please check
>> >>> the size from your end also if required)
>> >>
>> >>
>> >> I suggest you make that function a nop (perhaps using an #ifdef
>> >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
>> >> don't increase U-Boot code size for those boards that don't need
>> >> support
>> >> larger devices (which I guess is almost all of them, right now). U-Boot
>> >> is
>> >> quite concerned about code size.
>> >
>> > Little concern here, the point here I stated that these new changes is
>> > common for all sizes of flashes.(which are less or greater than
>> > 16Mbytes).
>> > and yes it increase the code size little bit but i don't think it will
>> > require the separate macro.
>>
>> Any comments.
>
>
> In U-Boot it is generally not acceptable to increase code size for existing
> boards when adding a new feature that they don't use. So I suspect in this
> case you should add a new CONFIG to enable your feature. It seems to
> increase code by more than 200 bytes for snow, for example.

OK, I did coding on sf to have a common framework for all sizes of
flashes in mind.
but as you said it's increasing the size of u-boot.bin > 200 bytes.

Seems like no choice to comprise, I am going to create v3 series for
these changes.
Will that be OK?

>
> Tom may have further comments.
>
> Also my buildman run of your commit gave an error on this commit:
>
> 07: sf: Update sf to support all sizes of flashes

I am created these patches on top of u-boot-spi.git, there some
patches already available on sf.
may be you used master.

Thanks,
Jagan.

>
>
> Regards,
> Simon
>
>>
>>
>> --
>> Thanks,
>> Jagan.
>>
>> >
>> > Thanks,
>> > Jagan.
>> >
>> >>
>> >> Tom may chime in and decide it is fine, though.
>> >>
>> >>>
>> >>>
>> >>> Please see the commit body on below thread for more info
>> >>> http://patchwork.ozlabs.org/patch/247954/
>> >>>
>> >>> >
>> >>> > In your change to spi_flash_cmd_poll_bit() the effect is not the
>> >>> > same I
>> >>> > think. It is designed to hold CS active and read the status byte
>> >>> > continuously until it changes. But your implementation asserts CS,
>> >>> > reads
>> >>> > the
>> >>> > status byte, de-asserts CS, then repeats. Why do we want to change
>> >>> > this?
>> >>>
>> >>> I commented on the actual patch thread, please refer,
>> >>
>> >>
>> >> OK I will take a look.
>> >>
>> >>>
>> >>>
>> >>> >
>> >>> >
>> >>> >
>> >>> >> ---
>> >>> >> Changes for v2:
>> >>> >>         - none
>> >>> >>
>> >>> >>  drivers/mtd/spi/spi_flash.c | 39
>> >>> >> ++++++++++++++++++++++++++-------------
>> >>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
>> >>> >>
>> >>> >> diff --git a/drivers/mtd/spi/spi_flash.c
>> >>> >> b/drivers/mtd/spi/spi_flash.c
>> >>> >> index 4576a16..5386884 100644
>> >>> >> --- a/drivers/mtd/spi/spi_flash.c
>> >>> >> +++ b/drivers/mtd/spi/spi_flash.c
>> >>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
>> >>> >> *flash,
>> >>> >> u32 offset,
>> >>> >>         unsigned long page_addr, byte_addr, page_size;
>> >>> >>         size_t chunk_len, actual;
>> >>> >>         int ret;
>> >>> >> -       u8 cmd[4];
>> >>> >> +       u8 cmd[4], bank_sel;
>> >>> >>
>> >>> >>         page_size = flash->page_size;
>> >>> >> -       page_addr = offset / page_size;
>> >>> >> -       byte_addr = offset % page_size;
>> >>> >>
>> >>> >>         ret = spi_claim_bus(flash->spi);
>> >>> >>         if (ret) {
>> >>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
>> >>> >> *flash,
>> >>> >> u32 offset,
>> >>> >>
>> >>> >>         cmd[0] = CMD_PAGE_PROGRAM;
>> >>> >>         for (actual = 0; actual < len; actual += chunk_len) {
>> >>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
>> >>> >> +
>> >>> >> +               ret = spi_flash_cmd_bankaddr_write(flash,
>> >>> >> bank_sel);
>> >>> >> +               if (ret) {
>> >>> >> +                       debug("SF: fail to set bank%d\n",
>> >>> >> bank_sel);
>> >>> >> +                       return ret;
>> >>> >> +               }
>> >>> >
>> >>> >
>> >>> > So we are now doing this for all chips. But isn't it true that only
>> >>> > some
>> >>> > chips (>16MB?) have a bank address. If so, then I think we should
>> >>> > have a
>> >>> > flag somewhere to enable this feature
>> >>>
>> >>> APAMK, currently stmicro, winbond, spansion and macronix have a
>> >>> flashes which has > 16Mbytes flashes.
>> >>>
>> >>> And macronix is also have same bank setup like stmicro, extended addr
>> >>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
>> >>> We need to add this in near future.
>> >>>
>> >>> I have added Prafulla Wadaskar on this thread (initial contributor for
>> >>> macronix.c), may be he will give some more information
>> >>> for accessing > 16Mbytes flashes in 3-byte addr mode.
>> >>>
>> >>> I think we can go ahead for now, may be we will tune sf some more in
>> >>> future based on the availability of different flash which crosses
>> >>> 16Mbytes
>> >>> with different apparoch (other than banking/extended), what do you
>> >>> say?
>> >>
>> >>
>> >> OK, well we don't need a flag since you will never issue the bank
>> >> address
>> >> command unless the chip is larger than 16MB.,
>> >>
>> >>>
>> >>>
>> >>> >
>> >>> >>
>> >>> >> +
>> >>> >> +               page_addr = offset / page_size;
>> >>> >> +               byte_addr = offset % page_size;
>> >>> >
>> >>> >
>> >>> > This is OK I think. We really don't care about the slower divide so
>> >>> > it
>> >>> > is
>> >>> > not worth optimising for I think.
>> >>>
>> >>> Yes, I just used the existing spi_flash_addr with offset as an
>> >>> argument, anyway sf write have a logic
>> >>> to use writing in terms of page sizes and even offset is also varies
>> >>> page sizes or requested sizes.
>> >>>
>> >>> Thanks,
>> >>> Jagan.
>> >>
>> >>
>> >>
>> >> Regards,
>> >> Simon
>> >>
>
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Simon Glass]

Hi Jagan,

On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:

> Hi Simon,
>
> On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <sjg@chromium.org> wrote:
> > On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>
> > wrote:
> >>
> >> Hi Tom,
> >>
> >> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
> >> wrote:
> >> > Hi Simon,
> >> >
> >> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
> >> >> Hi,
> >> >>
> >> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com
> >
> >> >> wrote:
> >> >>>
> >> >>> Hi Simon,
> >> >>>
> >> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org>
> wrote:
> >> >>> > Hi Jagan,
> >> >>> >
> >> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> >> >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >> >>> >>
> >> >>> >> Updated the spi_flash framework to handle all sizes of flashes
> >> >>> >> using bank/extd addr reg facility
> >> >>> >>
> >> >>> >> The current implementation in spi_flash supports 3-byte address
> >> >>> >> mode
> >> >>> >> due to this up to 16Mbytes amount of flash is able to access for
> >> >>> >> those
> >> >>> >> flashes which has an actual size of > 16MB.
> >> >>> >>
> >> >>> >> As most of the flashes introduces a bank/extd address registers
> >> >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
> >> >>> >> is > 16Mbytes, this new scheme will add the bank selection
> feature
> >> >>> >> for performing write/erase operations on all flashes.
> >> >>> >>
> >> >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >> >>> >
> >> >>> >
> >> >>> > I have a few comments on this series, but it mostly looks fine. I
> >> >>> > think
> >> >>> > the
> >> >>> > new code is correct.
> >> >>> >
> >> >>> > The patches did not apply cleanly for me. Perhaps I am missing
> >> >>> > something. My
> >> >>> > tree looks like this after I did a bit of merging:
> >> >>>
> >> >>> Which patches you had an issues while applying,we have few patches
> on
> >> >>> u-boot-spi.git i created these on top of it.
> >> >>
> >> >>
> >> >> I am not sure now - sorry I did not keep a record. But the bundle I
> >> >> used is
> >> >> here, and it doesn't apply cleanly.
> >> >>
> >> >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
> >> >>
> >> >> git am ~/Downloads/bundle-4407-jagan.mbox
> >> >> Applying: sf: Add bank address register writing support
> >> >> Applying: sf: Add bank address register reading support
> >> >> Applying: sf: Add extended addr write support for winbond|stmicro
> >> >> Applying: sf: Add extended addr read support for winbond|stmicro
> >> >> Applying: sf: read flash bank addr register at probe time
> >> >> Applying: sf: Update sf to support all sizes of flashes
> >> >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
> >> >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
> >> >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
> >> >> The copy of the patch that failed is found in:
> >> >>    /home/sjg/u/.git/rebase-apply/patch
> >> >> When you have resolved this problem, run "git am --resolved".
> >> >> If you prefer to skip this patch, run "git am --skip" instead.
> >> >> To restore the original branch and stop patching, run "git am
> --abort"
> >> >>
> >> >>>
> >> >>>
> >> >>> >
> >> >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus
> flash
> >> >>> > devices
> >> >>> > via address shift
> >> >>> > f700095 sf: Add Flag status register polling support
> >> >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> >> >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
> >> >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
> >> >>> > c72e52a sf: stmicro: Add support for N25Q1024A
> >> >>> > 4066f71 sf: stmicro: Add support for N25Q1024
> >> >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
> >> >>> > 8fd962f sf: stmicro: Add support for N25Q512
> >> >>> > f1a2080 sf: Use spi_flash_addr() in write call
> >> >>> > 31ed498 sf: Update sf read to support all sizes of flashes
> >> >>> > 0f77642 sf: Update sf to support all sizes of flashes
> >> >>> > 9e57220 sf: read flash bank addr register at probe time
> >> >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
> >> >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
> >> >>> > f02ecf1 sf: Add bank address register reading support
> >> >>> > 02ba27c sf: Add bank address register writing support
> >> >>> >
> >> >>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
> >> >>> > should be
> >> >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
> >> >>> > space
> >> >>> > does
> >> >>> > this add?
> >> >>>
> >> >>> Initially i thought of the same, but I just updated sf which is
> >> >>> generic to all sizes of flashes.
> >> >>> due to this i haven't included the bank read/write on macros, and
> the
> >> >>> flash ops will call these
> >> >>> bank write call irrespective of flash sizes.
> >> >>>
> >> >>> As flashes which has below 16Mbytes will have a bank_curr value 0
> >> >>> so-that even bank write will exit for
> >> >>> bank 0 operations.
> >> >>
> >> >>
> >> >> Yes this is fine.
> >> >>
> >> >>>
> >> >>>
> >> >>> +       if (flash->bank_curr == bank_sel) {
> >> >>> +               debug("SF: not require to enable bank%d\n",
> bank_sel);
> >> >>> +               return 0;
> >> >>> +       }
> >> >>> +
> >> >>>
> >> >>> And due to these framework changes bank+flash ops addition, bin size
> >> >>> increases appr' ~600bytes
> >> >>> by enabling stmicro, winbond and spansion flash drivers.(please
> check
> >> >>> the size from your end also if required)
> >> >>
> >> >>
> >> >> I suggest you make that function a nop (perhaps using an #ifdef
> >> >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your
> patches
> >> >> don't increase U-Boot code size for those boards that don't need
> >> >> support
> >> >> larger devices (which I guess is almost all of them, right now).
> U-Boot
> >> >> is
> >> >> quite concerned about code size.
> >> >
> >> > Little concern here, the point here I stated that these new changes is
> >> > common for all sizes of flashes.(which are less or greater than
> >> > 16Mbytes).
> >> > and yes it increase the code size little bit but i don't think it will
> >> > require the separate macro.
> >>
> >> Any comments.
> >
> >
> > In U-Boot it is generally not acceptable to increase code size for
> existing
> > boards when adding a new feature that they don't use. So I suspect in
> this
> > case you should add a new CONFIG to enable your feature. It seems to
> > increase code by more than 200 bytes for snow, for example.
>
> OK, I did coding on sf to have a common framework for all sizes of
> flashes in mind.
> but as you said it's increasing the size of u-boot.bin > 200 bytes.
>
> Seems like no choice to comprise, I am going to create v3 series for
> these changes.
> Will that be OK?
>

What does 'comprise' mean in this context?


>
> >
> > Tom may have further comments.
> >
> > Also my buildman run of your commit gave an error on this commit:
> >
> > 07: sf: Update sf to support all sizes of flashes
>
> I am created these patches on top of u-boot-spi.git, there some
> patches already available on sf.
> may be you used master.
>

OK, sorry, I didn't know about that tree...thanks for pointing me to it.

Regards,
Simon


>
> Thanks,
> Jagan.
>
> >
> >
> > Regards,
> > Simon
> >
> >>
> >>
> >> --
> >> Thanks,
> >> Jagan.
> >>
> >> >
> >> > Thanks,
> >> > Jagan.
> >> >
> >> >>
> >> >> Tom may chime in and decide it is fine, though.
> >> >>
> >> >>>
> >> >>>
> >> >>> Please see the commit body on below thread for more info
> >> >>> http://patchwork.ozlabs.org/patch/247954/
> >> >>>
> >> >>> >
> >> >>> > In your change to spi_flash_cmd_poll_bit() the effect is not the
> >> >>> > same I
> >> >>> > think. It is designed to hold CS active and read the status byte
> >> >>> > continuously until it changes. But your implementation asserts CS,
> >> >>> > reads
> >> >>> > the
> >> >>> > status byte, de-asserts CS, then repeats. Why do we want to change
> >> >>> > this?
> >> >>>
> >> >>> I commented on the actual patch thread, please refer,
> >> >>
> >> >>
> >> >> OK I will take a look.
> >> >>
> >> >>>
> >> >>>
> >> >>> >
> >> >>> >
> >> >>> >
> >> >>> >> ---
> >> >>> >> Changes for v2:
> >> >>> >>         - none
> >> >>> >>
> >> >>> >>  drivers/mtd/spi/spi_flash.c | 39
> >> >>> >> ++++++++++++++++++++++++++-------------
> >> >>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
> >> >>> >>
> >> >>> >> diff --git a/drivers/mtd/spi/spi_flash.c
> >> >>> >> b/drivers/mtd/spi/spi_flash.c
> >> >>> >> index 4576a16..5386884 100644
> >> >>> >> --- a/drivers/mtd/spi/spi_flash.c
> >> >>> >> +++ b/drivers/mtd/spi/spi_flash.c
> >> >>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash
> >> >>> >> *flash,
> >> >>> >> u32 offset,
> >> >>> >>         unsigned long page_addr, byte_addr, page_size;
> >> >>> >>         size_t chunk_len, actual;
> >> >>> >>         int ret;
> >> >>> >> -       u8 cmd[4];
> >> >>> >> +       u8 cmd[4], bank_sel;
> >> >>> >>
> >> >>> >>         page_size = flash->page_size;
> >> >>> >> -       page_addr = offset / page_size;
> >> >>> >> -       byte_addr = offset % page_size;
> >> >>> >>
> >> >>> >>         ret = spi_claim_bus(flash->spi);
> >> >>> >>         if (ret) {
> >> >>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash
> >> >>> >> *flash,
> >> >>> >> u32 offset,
> >> >>> >>
> >> >>> >>         cmd[0] = CMD_PAGE_PROGRAM;
> >> >>> >>         for (actual = 0; actual < len; actual += chunk_len) {
> >> >>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> >> >>> >> +
> >> >>> >> +               ret = spi_flash_cmd_bankaddr_write(flash,
> >> >>> >> bank_sel);
> >> >>> >> +               if (ret) {
> >> >>> >> +                       debug("SF: fail to set bank%d\n",
> >> >>> >> bank_sel);
> >> >>> >> +                       return ret;
> >> >>> >> +               }
> >> >>> >
> >> >>> >
> >> >>> > So we are now doing this for all chips. But isn't it true that
> only
> >> >>> > some
> >> >>> > chips (>16MB?) have a bank address. If so, then I think we should
> >> >>> > have a
> >> >>> > flag somewhere to enable this feature
> >> >>>
> >> >>> APAMK, currently stmicro, winbond, spansion and macronix have a
> >> >>> flashes which has > 16Mbytes flashes.
> >> >>>
> >> >>> And macronix is also have same bank setup like stmicro, extended
> addr
> >> >>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
> >> >>> We need to add this in near future.
> >> >>>
> >> >>> I have added Prafulla Wadaskar on this thread (initial contributor
> for
> >> >>> macronix.c), may be he will give some more information
> >> >>> for accessing > 16Mbytes flashes in 3-byte addr mode.
> >> >>>
> >> >>> I think we can go ahead for now, may be we will tune sf some more in
> >> >>> future based on the availability of different flash which crosses
> >> >>> 16Mbytes
> >> >>> with different apparoch (other than banking/extended), what do you
> >> >>> say?
> >> >>
> >> >>
> >> >> OK, well we don't need a flag since you will never issue the bank
> >> >> address
> >> >> command unless the chip is larger than 16MB.,
> >> >>
> >> >>>
> >> >>>
> >> >>> >
> >> >>> >>
> >> >>> >> +
> >> >>> >> +               page_addr = offset / page_size;
> >> >>> >> +               byte_addr = offset % page_size;
> >> >>> >
> >> >>> >
> >> >>> > This is OK I think. We really don't care about the slower divide
> so
> >> >>> > it
> >> >>> > is
> >> >>> > not worth optimising for I think.
> >> >>>
> >> >>> Yes, I just used the existing spi_flash_addr with offset as an
> >> >>> argument, anyway sf write have a logic
> >> >>> to use writing in terms of page sizes and even offset is also varies
> >> >>> page sizes or requested sizes.
> >> >>>
> >> >>> Thanks,
> >> >>> Jagan.
> >> >>
> >> >>
> >> >>
> >> >> Regards,
> >> >> Simon
> >> >>
> >
> >
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Tom Rini]

On Mon, Jun 10, 2013 at 02:49:35PM -0700, Simon Glass wrote:
> On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:
> 
> > Hi Tom,
> >
> > On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
> > wrote:
> > > Hi Simon,
> > >
> > > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
> > >> Hi,
> > >>
> > >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com>
> > wrote:
> > >>>
> > >>> Hi Simon,
> > >>>
> > >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
> > >>> > Hi Jagan,
> > >>> >
> > >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> > >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> > >>> >>
> > >>> >> Updated the spi_flash framework to handle all sizes of flashes
> > >>> >> using bank/extd addr reg facility
> > >>> >>
> > >>> >> The current implementation in spi_flash supports 3-byte address mode
> > >>> >> due to this up to 16Mbytes amount of flash is able to access for
> > those
> > >>> >> flashes which has an actual size of > 16MB.
> > >>> >>
> > >>> >> As most of the flashes introduces a bank/extd address registers
> > >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
> > >>> >> is > 16Mbytes, this new scheme will add the bank selection feature
> > >>> >> for performing write/erase operations on all flashes.
> > >>> >>
> > >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> > >>> >
> > >>> >
> > >>> > I have a few comments on this series, but it mostly looks fine. I
> > think
> > >>> > the
> > >>> > new code is correct.
> > >>> >
> > >>> > The patches did not apply cleanly for me. Perhaps I am missing
> > >>> > something. My
> > >>> > tree looks like this after I did a bit of merging:
> > >>>
> > >>> Which patches you had an issues while applying,we have few patches on
> > >>> u-boot-spi.git i created these on top of it.
> > >>
> > >>
> > >> I am not sure now - sorry I did not keep a record. But the bundle I
> > used is
> > >> here, and it doesn't apply cleanly.
> > >>
> > >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
> > >>
> > >> git am ~/Downloads/bundle-4407-jagan.mbox
> > >> Applying: sf: Add bank address register writing support
> > >> Applying: sf: Add bank address register reading support
> > >> Applying: sf: Add extended addr write support for winbond|stmicro
> > >> Applying: sf: Add extended addr read support for winbond|stmicro
> > >> Applying: sf: read flash bank addr register at probe time
> > >> Applying: sf: Update sf to support all sizes of flashes
> > >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
> > >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
> > >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
> > >> The copy of the patch that failed is found in:
> > >>    /home/sjg/u/.git/rebase-apply/patch
> > >> When you have resolved this problem, run "git am --resolved".
> > >> If you prefer to skip this patch, run "git am --skip" instead.
> > >> To restore the original branch and stop patching, run "git am --abort"
> > >>
> > >>>
> > >>>
> > >>> >
> > >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
> > >>> > devices
> > >>> > via address shift
> > >>> > f700095 sf: Add Flag status register polling support
> > >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> > >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
> > >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
> > >>> > c72e52a sf: stmicro: Add support for N25Q1024A
> > >>> > 4066f71 sf: stmicro: Add support for N25Q1024
> > >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
> > >>> > 8fd962f sf: stmicro: Add support for N25Q512
> > >>> > f1a2080 sf: Use spi_flash_addr() in write call
> > >>> > 31ed498 sf: Update sf read to support all sizes of flashes
> > >>> > 0f77642 sf: Update sf to support all sizes of flashes
> > >>> > 9e57220 sf: read flash bank addr register at probe time
> > >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
> > >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
> > >>> > f02ecf1 sf: Add bank address register reading support
> > >>> > 02ba27c sf: Add bank address register writing support
> > >>> >
> > >>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
> > >>> > should be
> > >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
> > space
> > >>> > does
> > >>> > this add?
> > >>>
> > >>> Initially i thought of the same, but I just updated sf which is
> > >>> generic to all sizes of flashes.
> > >>> due to this i haven't included the bank read/write on macros, and the
> > >>> flash ops will call these
> > >>> bank write call irrespective of flash sizes.
> > >>>
> > >>> As flashes which has below 16Mbytes will have a bank_curr value 0
> > >>> so-that even bank write will exit for
> > >>> bank 0 operations.
> > >>
> > >>
> > >> Yes this is fine.
> > >>
> > >>>
> > >>>
> > >>> +       if (flash->bank_curr == bank_sel) {
> > >>> +               debug("SF: not require to enable bank%d\n", bank_sel);
> > >>> +               return 0;
> > >>> +       }
> > >>> +
> > >>>
> > >>> And due to these framework changes bank+flash ops addition, bin size
> > >>> increases appr' ~600bytes
> > >>> by enabling stmicro, winbond and spansion flash drivers.(please check
> > >>> the size from your end also if required)
> > >>
> > >>
> > >> I suggest you make that function a nop (perhaps using an #ifdef
> > >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
> > >> don't increase U-Boot code size for those boards that don't need support
> > >> larger devices (which I guess is almost all of them, right now). U-Boot
> > is
> > >> quite concerned about code size.
> > >
> > > Little concern here, the point here I stated that these new changes is
> > > common for all sizes of flashes.(which are less or greater than
> > > 16Mbytes).
> > > and yes it increase the code size little bit but i don't think it will
> > > require the separate macro.
> >
> > Any comments.
> >
> 
> In U-Boot it is generally not acceptable to increase code size for existing
> boards when adding a new feature that they don't use. So I suspect in this
> case you should add a new CONFIG to enable your feature. It seems to
> increase code by more than 200 bytes for snow, for example.
> 
> Tom may have further comments.

I think it's important we keep both binary size and code complexity in
mind when we say "no binary size increase".  It really has to mean "no
unreasonable binary size increase, when compared with code complexity
changes".  The question has to be "can we easily make things opt-in".

Perhaps the answer here is that we need to add
CONFIG_SPI_FLASH_STMICRO_STATIC_TABLE and a corresponding set of
CONFIG_SPI_FLASH_STMICRO_ID/PPS/NR_SECT/NAME (and Winbond variant) so
that boards with size constrains can opt to define the single chip they
support, and other boards can just get the full probe table.  This will
allow boards to get a net size decrease here if desired.

Heck, for ARM we are, or have just, reclaimed a bunch of space at least
on SPL using boards in the main binary now that we --gc-sections U-Boot.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130611/3399dec4/attachment.pgp>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Simon,

On Tue, Jun 11, 2013 at 9:26 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki <jagannadh.teki@gmail.com>
> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <sjg@chromium.org> wrote:
>> > On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>
>> > wrote:
>> >>
>> >> Hi Tom,
>> >>
>> >> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
>> >> wrote:
>> >> > Hi Simon,
>> >> >
>> >> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
>> >> >> Hi,
>> >> >>
>> >> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki
>> >> >> <jagannadh.teki@gmail.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> Hi Simon,
>> >> >>>
>> >> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org>
>> >> >>> wrote:
>> >> >>> > Hi Jagan,
>> >> >>> >
>> >> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
>> >> >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> >> >>> >>
>> >> >>> >> Updated the spi_flash framework to handle all sizes of flashes
>> >> >>> >> using bank/extd addr reg facility
>> >> >>> >>
>> >> >>> >> The current implementation in spi_flash supports 3-byte address
>> >> >>> >> mode
>> >> >>> >> due to this up to 16Mbytes amount of flash is able to access for
>> >> >>> >> those
>> >> >>> >> flashes which has an actual size of > 16MB.
>> >> >>> >>
>> >> >>> >> As most of the flashes introduces a bank/extd address registers
>> >> >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
>> >> >>> >> is > 16Mbytes, this new scheme will add the bank selection
>> >> >>> >> feature
>> >> >>> >> for performing write/erase operations on all flashes.
>> >> >>> >>
>> >> >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> >> >>> >
>> >> >>> >
>> >> >>> > I have a few comments on this series, but it mostly looks fine. I
>> >> >>> > think
>> >> >>> > the
>> >> >>> > new code is correct.
>> >> >>> >
>> >> >>> > The patches did not apply cleanly for me. Perhaps I am missing
>> >> >>> > something. My
>> >> >>> > tree looks like this after I did a bit of merging:
>> >> >>>
>> >> >>> Which patches you had an issues while applying,we have few patches
>> >> >>> on
>> >> >>> u-boot-spi.git i created these on top of it.
>> >> >>
>> >> >>
>> >> >> I am not sure now - sorry I did not keep a record. But the bundle I
>> >> >> used is
>> >> >> here, and it doesn't apply cleanly.
>> >> >>
>> >> >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>> >> >>
>> >> >> git am ~/Downloads/bundle-4407-jagan.mbox
>> >> >> Applying: sf: Add bank address register writing support
>> >> >> Applying: sf: Add bank address register reading support
>> >> >> Applying: sf: Add extended addr write support for winbond|stmicro
>> >> >> Applying: sf: Add extended addr read support for winbond|stmicro
>> >> >> Applying: sf: read flash bank addr register at probe time
>> >> >> Applying: sf: Update sf to support all sizes of flashes
>> >> >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
>> >> >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
>> >> >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
>> >> >> The copy of the patch that failed is found in:
>> >> >>    /home/sjg/u/.git/rebase-apply/patch
>> >> >> When you have resolved this problem, run "git am --resolved".
>> >> >> If you prefer to skip this patch, run "git am --skip" instead.
>> >> >> To restore the original branch and stop patching, run "git am
>> >> >> --abort"
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>> >
>> >> >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus
>> >> >>> > flash
>> >> >>> > devices
>> >> >>> > via address shift
>> >> >>> > f700095 sf: Add Flag status register polling support
>> >> >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
>> >> >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
>> >> >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
>> >> >>> > c72e52a sf: stmicro: Add support for N25Q1024A
>> >> >>> > 4066f71 sf: stmicro: Add support for N25Q1024
>> >> >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
>> >> >>> > 8fd962f sf: stmicro: Add support for N25Q512
>> >> >>> > f1a2080 sf: Use spi_flash_addr() in write call
>> >> >>> > 31ed498 sf: Update sf read to support all sizes of flashes
>> >> >>> > 0f77642 sf: Update sf to support all sizes of flashes
>> >> >>> > 9e57220 sf: read flash bank addr register at probe time
>> >> >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
>> >> >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
>> >> >>> > f02ecf1 sf: Add bank address register reading support
>> >> >>> > 02ba27c sf: Add bank address register writing support
>> >> >>> >
>> >> >>> > Also do you think spi_flash_cmd_bankaddr_write() and related
>> >> >>> > stuff
>> >> >>> > should be
>> >> >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
>> >> >>> > space
>> >> >>> > does
>> >> >>> > this add?
>> >> >>>
>> >> >>> Initially i thought of the same, but I just updated sf which is
>> >> >>> generic to all sizes of flashes.
>> >> >>> due to this i haven't included the bank read/write on macros, and
>> >> >>> the
>> >> >>> flash ops will call these
>> >> >>> bank write call irrespective of flash sizes.
>> >> >>>
>> >> >>> As flashes which has below 16Mbytes will have a bank_curr value 0
>> >> >>> so-that even bank write will exit for
>> >> >>> bank 0 operations.
>> >> >>
>> >> >>
>> >> >> Yes this is fine.
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>> +       if (flash->bank_curr == bank_sel) {
>> >> >>> +               debug("SF: not require to enable bank%d\n",
>> >> >>> bank_sel);
>> >> >>> +               return 0;
>> >> >>> +       }
>> >> >>> +
>> >> >>>
>> >> >>> And due to these framework changes bank+flash ops addition, bin
>> >> >>> size
>> >> >>> increases appr' ~600bytes
>> >> >>> by enabling stmicro, winbond and spansion flash drivers.(please
>> >> >>> check
>> >> >>> the size from your end also if required)
>> >> >>
>> >> >>
>> >> >> I suggest you make that function a nop (perhaps using an #ifdef
>> >> >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your
>> >> >> patches
>> >> >> don't increase U-Boot code size for those boards that don't need
>> >> >> support
>> >> >> larger devices (which I guess is almost all of them, right now).
>> >> >> U-Boot
>> >> >> is
>> >> >> quite concerned about code size.
>> >> >
>> >> > Little concern here, the point here I stated that these new changes
>> >> > is
>> >> > common for all sizes of flashes.(which are less or greater than
>> >> > 16Mbytes).
>> >> > and yes it increase the code size little bit but i don't think it
>> >> > will
>> >> > require the separate macro.
>> >>
>> >> Any comments.
>> >
>> >
>> > In U-Boot it is generally not acceptable to increase code size for
>> > existing
>> > boards when adding a new feature that they don't use. So I suspect in
>> > this
>> > case you should add a new CONFIG to enable your feature. It seems to
>> > increase code by more than 200 bytes for snow, for example.
>>
>> OK, I did coding on sf to have a common framework for all sizes of
>> flashes in mind.
>> but as you said it's increasing the size of u-boot.bin > 200 bytes.
>>
>> Seems like no choice to comprise, I am going to create v3 series for
>> these changes.
>> Will that be OK?
>
>
> What does 'comprise' mean in this context?

I am saying as uboot.bin size increases for these new updates I must
compromise use the macros for reducing the sizes.

Am i clear.

Thanks,
Jagan.

>
>>
>>
>> >
>> > Tom may have further comments.
>> >
>> > Also my buildman run of your commit gave an error on this commit:
>> >
>> > 07: sf: Update sf to support all sizes of flashes
>>
>> I am created these patches on top of u-boot-spi.git, there some
>> patches already available on sf.
>> may be you used master.
>
>
> OK, sorry, I didn't know about that tree...thanks for pointing me to it.
>
> Regards,
> Simon
>
>>
>>
>> Thanks,
>> Jagan.
>>
>> >
>> >
>> > Regards,
>> > Simon
>> >
>> >>
>> >>
>> >> --
>> >> Thanks,
>> >> Jagan.
>> >>
>> >> >
>> >> > Thanks,
>> >> > Jagan.
>> >> >
>> >> >>
>> >> >> Tom may chime in and decide it is fine, though.
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>> Please see the commit body on below thread for more info
>> >> >>> http://patchwork.ozlabs.org/patch/247954/
>> >> >>>
>> >> >>> >
>> >> >>> > In your change to spi_flash_cmd_poll_bit() the effect is not the
>> >> >>> > same I
>> >> >>> > think. It is designed to hold CS active and read the status byte
>> >> >>> > continuously until it changes. But your implementation asserts
>> >> >>> > CS,
>> >> >>> > reads
>> >> >>> > the
>> >> >>> > status byte, de-asserts CS, then repeats. Why do we want to
>> >> >>> > change
>> >> >>> > this?
>> >> >>>
>> >> >>> I commented on the actual patch thread, please refer,
>> >> >>
>> >> >>
>> >> >> OK I will take a look.
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>> >> ---
>> >> >>> >> Changes for v2:
>> >> >>> >>         - none
>> >> >>> >>
>> >> >>> >>  drivers/mtd/spi/spi_flash.c | 39
>> >> >>> >> ++++++++++++++++++++++++++-------------
>> >> >>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
>> >> >>> >>
>> >> >>> >> diff --git a/drivers/mtd/spi/spi_flash.c
>> >> >>> >> b/drivers/mtd/spi/spi_flash.c
>> >> >>> >> index 4576a16..5386884 100644
>> >> >>> >> --- a/drivers/mtd/spi/spi_flash.c
>> >> >>> >> +++ b/drivers/mtd/spi/spi_flash.c
>> >> >>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct
>> >> >>> >> spi_flash
>> >> >>> >> *flash,
>> >> >>> >> u32 offset,
>> >> >>> >>         unsigned long page_addr, byte_addr, page_size;
>> >> >>> >>         size_t chunk_len, actual;
>> >> >>> >>         int ret;
>> >> >>> >> -       u8 cmd[4];
>> >> >>> >> +       u8 cmd[4], bank_sel;
>> >> >>> >>
>> >> >>> >>         page_size = flash->page_size;
>> >> >>> >> -       page_addr = offset / page_size;
>> >> >>> >> -       byte_addr = offset % page_size;
>> >> >>> >>
>> >> >>> >>         ret = spi_claim_bus(flash->spi);
>> >> >>> >>         if (ret) {
>> >> >>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct
>> >> >>> >> spi_flash
>> >> >>> >> *flash,
>> >> >>> >> u32 offset,
>> >> >>> >>
>> >> >>> >>         cmd[0] = CMD_PAGE_PROGRAM;
>> >> >>> >>         for (actual = 0; actual < len; actual += chunk_len) {
>> >> >>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
>> >> >>> >> +
>> >> >>> >> +               ret = spi_flash_cmd_bankaddr_write(flash,
>> >> >>> >> bank_sel);
>> >> >>> >> +               if (ret) {
>> >> >>> >> +                       debug("SF: fail to set bank%d\n",
>> >> >>> >> bank_sel);
>> >> >>> >> +                       return ret;
>> >> >>> >> +               }
>> >> >>> >
>> >> >>> >
>> >> >>> > So we are now doing this for all chips. But isn't it true that
>> >> >>> > only
>> >> >>> > some
>> >> >>> > chips (>16MB?) have a bank address. If so, then I think we should
>> >> >>> > have a
>> >> >>> > flag somewhere to enable this feature
>> >> >>>
>> >> >>> APAMK, currently stmicro, winbond, spansion and macronix have a
>> >> >>> flashes which has > 16Mbytes flashes.
>> >> >>>
>> >> >>> And macronix is also have same bank setup like stmicro, extended
>> >> >>> addr
>> >> >>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
>> >> >>> We need to add this in near future.
>> >> >>>
>> >> >>> I have added Prafulla Wadaskar on this thread (initial contributor
>> >> >>> for
>> >> >>> macronix.c), may be he will give some more information
>> >> >>> for accessing > 16Mbytes flashes in 3-byte addr mode.
>> >> >>>
>> >> >>> I think we can go ahead for now, may be we will tune sf some more
>> >> >>> in
>> >> >>> future based on the availability of different flash which crosses
>> >> >>> 16Mbytes
>> >> >>> with different apparoch (other than banking/extended), what do you
>> >> >>> say?
>> >> >>
>> >> >>
>> >> >> OK, well we don't need a flag since you will never issue the bank
>> >> >> address
>> >> >> command unless the chip is larger than 16MB.,
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>> >
>> >> >>> >>
>> >> >>> >> +
>> >> >>> >> +               page_addr = offset / page_size;
>> >> >>> >> +               byte_addr = offset % page_size;
>> >> >>> >
>> >> >>> >
>> >> >>> > This is OK I think. We really don't care about the slower divide
>> >> >>> > so
>> >> >>> > it
>> >> >>> > is
>> >> >>> > not worth optimising for I think.
>> >> >>>
>> >> >>> Yes, I just used the existing spi_flash_addr with offset as an
>> >> >>> argument, anyway sf write have a logic
>> >> >>> to use writing in terms of page sizes and even offset is also
>> >> >>> varies
>> >> >>> page sizes or requested sizes.
>> >> >>>
>> >> >>> Thanks,
>> >> >>> Jagan.
>> >> >>
>> >> >>
>> >> >>
>> >> >> Regards,
>> >> >> Simon
>> >> >>
>> >
>> >
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Simon Glass]

Hi,

On Tue, Jun 11, 2013 at 9:19 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:

> Hi Simon,
>
> On Tue, Jun 11, 2013 at 9:26 PM, Simon Glass <sjg@chromium.org> wrote:
> > Hi Jagan,
> >
> > On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki <jagannadh.teki@gmail.com>
> > wrote:
> >>
> >> Hi Simon,
> >>
> >> On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <sjg@chromium.org> wrote:
> >> > On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com
> >
> >> > wrote:
> >> >>
> >> >> Hi Tom,
> >> >>
> >> >> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com
> >
> >> >> wrote:
> >> >> > Hi Simon,
> >> >> >
> >> >> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org>
> wrote:
> >> >> >> Hi,
> >> >> >>
> >> >> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki
> >> >> >> <jagannadh.teki@gmail.com>
> >> >> >> wrote:
> >> >> >>>
> >> >> >>> Hi Simon,
> >> >> >>>
> >> >> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org>
> >> >> >>> wrote:
> >> >> >>> > Hi Jagan,
> >> >> >>> >
> >> >> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
> >> >> >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
> >> >> >>> >>
> >> >> >>> >> Updated the spi_flash framework to handle all sizes of flashes
> >> >> >>> >> using bank/extd addr reg facility
> >> >> >>> >>
> >> >> >>> >> The current implementation in spi_flash supports 3-byte
> address
> >> >> >>> >> mode
> >> >> >>> >> due to this up to 16Mbytes amount of flash is able to access
> for
> >> >> >>> >> those
> >> >> >>> >> flashes which has an actual size of > 16MB.
> >> >> >>> >>
> >> >> >>> >> As most of the flashes introduces a bank/extd address
> registers
> >> >> >>> >> for accessing the flashes in 16Mbytes of banks if the flash
> size
> >> >> >>> >> is > 16Mbytes, this new scheme will add the bank selection
> >> >> >>> >> feature
> >> >> >>> >> for performing write/erase operations on all flashes.
> >> >> >>> >>
> >> >> >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <
> jaganna at xilinx.com>
> >> >> >>> >
> >> >> >>> >
> >> >> >>> > I have a few comments on this series, but it mostly looks
> fine. I
> >> >> >>> > think
> >> >> >>> > the
> >> >> >>> > new code is correct.
> >> >> >>> >
> >> >> >>> > The patches did not apply cleanly for me. Perhaps I am missing
> >> >> >>> > something. My
> >> >> >>> > tree looks like this after I did a bit of merging:
> >> >> >>>
> >> >> >>> Which patches you had an issues while applying,we have few
> patches
> >> >> >>> on
> >> >> >>> u-boot-spi.git i created these on top of it.
> >> >> >>
> >> >> >>
> >> >> >> I am not sure now - sorry I did not keep a record. But the bundle
> I
> >> >> >> used is
> >> >> >> here, and it doesn't apply cleanly.
> >> >> >>
> >> >> >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
> >> >> >>
> >> >> >> git am ~/Downloads/bundle-4407-jagan.mbox
> >> >> >> Applying: sf: Add bank address register writing support
> >> >> >> Applying: sf: Add bank address register reading support
> >> >> >> Applying: sf: Add extended addr write support for winbond|stmicro
> >> >> >> Applying: sf: Add extended addr read support for winbond|stmicro
> >> >> >> Applying: sf: read flash bank addr register at probe time
> >> >> >> Applying: sf: Update sf to support all sizes of flashes
> >> >> >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
> >> >> >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
> >> >> >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
> >> >> >> The copy of the patch that failed is found in:
> >> >> >>    /home/sjg/u/.git/rebase-apply/patch
> >> >> >> When you have resolved this problem, run "git am --resolved".
> >> >> >> If you prefer to skip this patch, run "git am --skip" instead.
> >> >> >> To restore the original branch and stop patching, run "git am
> >> >> >> --abort"
> >> >> >>
> >> >> >>>
> >> >> >>>
> >> >> >>> >
> >> >> >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus
> >> >> >>> > flash
> >> >> >>> > devices
> >> >> >>> > via address shift
> >> >> >>> > f700095 sf: Add Flag status register polling support
> >> >> >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
> >> >> >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
> >> >> >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
> >> >> >>> > c72e52a sf: stmicro: Add support for N25Q1024A
> >> >> >>> > 4066f71 sf: stmicro: Add support for N25Q1024
> >> >> >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
> >> >> >>> > 8fd962f sf: stmicro: Add support for N25Q512
> >> >> >>> > f1a2080 sf: Use spi_flash_addr() in write call
> >> >> >>> > 31ed498 sf: Update sf read to support all sizes of flashes
> >> >> >>> > 0f77642 sf: Update sf to support all sizes of flashes
> >> >> >>> > 9e57220 sf: read flash bank addr register at probe time
> >> >> >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
> >> >> >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
> >> >> >>> > f02ecf1 sf: Add bank address register reading support
> >> >> >>> > 02ba27c sf: Add bank address register writing support
> >> >> >>> >
> >> >> >>> > Also do you think spi_flash_cmd_bankaddr_write() and related
> >> >> >>> > stuff
> >> >> >>> > should be
> >> >> >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much
> code
> >> >> >>> > space
> >> >> >>> > does
> >> >> >>> > this add?
> >> >> >>>
> >> >> >>> Initially i thought of the same, but I just updated sf which is
> >> >> >>> generic to all sizes of flashes.
> >> >> >>> due to this i haven't included the bank read/write on macros, and
> >> >> >>> the
> >> >> >>> flash ops will call these
> >> >> >>> bank write call irrespective of flash sizes.
> >> >> >>>
> >> >> >>> As flashes which has below 16Mbytes will have a bank_curr value 0
> >> >> >>> so-that even bank write will exit for
> >> >> >>> bank 0 operations.
> >> >> >>
> >> >> >>
> >> >> >> Yes this is fine.
> >> >> >>
> >> >> >>>
> >> >> >>>
> >> >> >>> +       if (flash->bank_curr == bank_sel) {
> >> >> >>> +               debug("SF: not require to enable bank%d\n",
> >> >> >>> bank_sel);
> >> >> >>> +               return 0;
> >> >> >>> +       }
> >> >> >>> +
> >> >> >>>
> >> >> >>> And due to these framework changes bank+flash ops addition, bin
> >> >> >>> size
> >> >> >>> increases appr' ~600bytes
> >> >> >>> by enabling stmicro, winbond and spansion flash drivers.(please
> >> >> >>> check
> >> >> >>> the size from your end also if required)
> >> >> >>
> >> >> >>
> >> >> >> I suggest you make that function a nop (perhaps using an #ifdef
> >> >> >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your
> >> >> >> patches
> >> >> >> don't increase U-Boot code size for those boards that don't need
> >> >> >> support
> >> >> >> larger devices (which I guess is almost all of them, right now).
> >> >> >> U-Boot
> >> >> >> is
> >> >> >> quite concerned about code size.
> >> >> >
> >> >> > Little concern here, the point here I stated that these new changes
> >> >> > is
> >> >> > common for all sizes of flashes.(which are less or greater than
> >> >> > 16Mbytes).
> >> >> > and yes it increase the code size little bit but i don't think it
> >> >> > will
> >> >> > require the separate macro.
> >> >>
> >> >> Any comments.
> >> >
> >> >
> >> > In U-Boot it is generally not acceptable to increase code size for
> >> > existing
> >> > boards when adding a new feature that they don't use. So I suspect in
> >> > this
> >> > case you should add a new CONFIG to enable your feature. It seems to
> >> > increase code by more than 200 bytes for snow, for example.
> >>
> >> OK, I did coding on sf to have a common framework for all sizes of
> >> flashes in mind.
> >> but as you said it's increasing the size of u-boot.bin > 200 bytes.
> >>
> >> Seems like no choice to comprise, I am going to create v3 series for
> >> these changes.
> >> Will that be OK?
> >
> >
> > What does 'comprise' mean in this context?
>
> I am saying as uboot.bin size increases for these new updates I must
> compromise use the macros for reducing the sizes.
>

Yes, I think it is OK to increase size by a few bytes, but if you are
adding a new feature that will not be used by existing boards, suggest
putting it behind a CONFIG_... flag.

Also see Tom's comment.

Regards,
Simon



>
> Am i clear.
>
> Thanks,
> Jagan.
>
> >
> >>
> >>
> >> >
> >> > Tom may have further comments.
> >> >
> >> > Also my buildman run of your commit gave an error on this commit:
> >> >
> >> > 07: sf: Update sf to support all sizes of flashes
> >>
> >> I am created these patches on top of u-boot-spi.git, there some
> >> patches already available on sf.
> >> may be you used master.
> >
> >
> > OK, sorry, I didn't know about that tree...thanks for pointing me to it.
> >
> > Regards,
> > Simon
> >
> >>
> >>
> >> Thanks,
> >> Jagan.
> >>
> >> >
> >> >
> >> > Regards,
> >> > Simon
> >> >
> >> >>
> >> >>
> >> >> --
> >> >> Thanks,
> >> >> Jagan.
> >> >>
> >> >> >
> >> >> > Thanks,
> >> >> > Jagan.
> >> >> >
> >> >> >>
> >> >> >> Tom may chime in and decide it is fine, though.
> >> >> >>
> >> >> >>>
> >> >> >>>
> >> >> >>> Please see the commit body on below thread for more info
> >> >> >>> http://patchwork.ozlabs.org/patch/247954/
> >> >> >>>
> >> >> >>> >
> >> >> >>> > In your change to spi_flash_cmd_poll_bit() the effect is not
> the
> >> >> >>> > same I
> >> >> >>> > think. It is designed to hold CS active and read the status
> byte
> >> >> >>> > continuously until it changes. But your implementation asserts
> >> >> >>> > CS,
> >> >> >>> > reads
> >> >> >>> > the
> >> >> >>> > status byte, de-asserts CS, then repeats. Why do we want to
> >> >> >>> > change
> >> >> >>> > this?
> >> >> >>>
> >> >> >>> I commented on the actual patch thread, please refer,
> >> >> >>
> >> >> >>
> >> >> >> OK I will take a look.
> >> >> >>
> >> >> >>>
> >> >> >>>
> >> >> >>> >
> >> >> >>> >
> >> >> >>> >
> >> >> >>> >> ---
> >> >> >>> >> Changes for v2:
> >> >> >>> >>         - none
> >> >> >>> >>
> >> >> >>> >>  drivers/mtd/spi/spi_flash.c | 39
> >> >> >>> >> ++++++++++++++++++++++++++-------------
> >> >> >>> >>  1 file changed, 26 insertions(+), 13 deletions(-)
> >> >> >>> >>
> >> >> >>> >> diff --git a/drivers/mtd/spi/spi_flash.c
> >> >> >>> >> b/drivers/mtd/spi/spi_flash.c
> >> >> >>> >> index 4576a16..5386884 100644
> >> >> >>> >> --- a/drivers/mtd/spi/spi_flash.c
> >> >> >>> >> +++ b/drivers/mtd/spi/spi_flash.c
> >> >> >>> >> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct
> >> >> >>> >> spi_flash
> >> >> >>> >> *flash,
> >> >> >>> >> u32 offset,
> >> >> >>> >>         unsigned long page_addr, byte_addr, page_size;
> >> >> >>> >>         size_t chunk_len, actual;
> >> >> >>> >>         int ret;
> >> >> >>> >> -       u8 cmd[4];
> >> >> >>> >> +       u8 cmd[4], bank_sel;
> >> >> >>> >>
> >> >> >>> >>         page_size = flash->page_size;
> >> >> >>> >> -       page_addr = offset / page_size;
> >> >> >>> >> -       byte_addr = offset % page_size;
> >> >> >>> >>
> >> >> >>> >>         ret = spi_claim_bus(flash->spi);
> >> >> >>> >>         if (ret) {
> >> >> >>> >> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct
> >> >> >>> >> spi_flash
> >> >> >>> >> *flash,
> >> >> >>> >> u32 offset,
> >> >> >>> >>
> >> >> >>> >>         cmd[0] = CMD_PAGE_PROGRAM;
> >> >> >>> >>         for (actual = 0; actual < len; actual += chunk_len) {
> >> >> >>> >> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> >> >> >>> >> +
> >> >> >>> >> +               ret = spi_flash_cmd_bankaddr_write(flash,
> >> >> >>> >> bank_sel);
> >> >> >>> >> +               if (ret) {
> >> >> >>> >> +                       debug("SF: fail to set bank%d\n",
> >> >> >>> >> bank_sel);
> >> >> >>> >> +                       return ret;
> >> >> >>> >> +               }
> >> >> >>> >
> >> >> >>> >
> >> >> >>> > So we are now doing this for all chips. But isn't it true that
> >> >> >>> > only
> >> >> >>> > some
> >> >> >>> > chips (>16MB?) have a bank address. If so, then I think we
> should
> >> >> >>> > have a
> >> >> >>> > flag somewhere to enable this feature
> >> >> >>>
> >> >> >>> APAMK, currently stmicro, winbond, spansion and macronix have a
> >> >> >>> flashes which has > 16Mbytes flashes.
> >> >> >>>
> >> >> >>> And macronix is also have same bank setup like stmicro, extended
> >> >> >>> addr
> >> >> >>> read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
> >> >> >>> We need to add this in near future.
> >> >> >>>
> >> >> >>> I have added Prafulla Wadaskar on this thread (initial
> contributor
> >> >> >>> for
> >> >> >>> macronix.c), may be he will give some more information
> >> >> >>> for accessing > 16Mbytes flashes in 3-byte addr mode.
> >> >> >>>
> >> >> >>> I think we can go ahead for now, may be we will tune sf some more
> >> >> >>> in
> >> >> >>> future based on the availability of different flash which crosses
> >> >> >>> 16Mbytes
> >> >> >>> with different apparoch (other than banking/extended), what do
> you
> >> >> >>> say?
> >> >> >>
> >> >> >>
> >> >> >> OK, well we don't need a flag since you will never issue the bank
> >> >> >> address
> >> >> >> command unless the chip is larger than 16MB.,
> >> >> >>
> >> >> >>>
> >> >> >>>
> >> >> >>> >
> >> >> >>> >>
> >> >> >>> >> +
> >> >> >>> >> +               page_addr = offset / page_size;
> >> >> >>> >> +               byte_addr = offset % page_size;
> >> >> >>> >
> >> >> >>> >
> >> >> >>> > This is OK I think. We really don't care about the slower
> divide
> >> >> >>> > so
> >> >> >>> > it
> >> >> >>> > is
> >> >> >>> > not worth optimising for I think.
> >> >> >>>
> >> >> >>> Yes, I just used the existing spi_flash_addr with offset as an
> >> >> >>> argument, anyway sf write have a logic
> >> >> >>> to use writing in terms of page sizes and even offset is also
> >> >> >>> varies
> >> >> >>> page sizes or requested sizes.
> >> >> >>>
> >> >> >>> Thanks,
> >> >> >>> Jagan.
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> Regards,
> >> >> >> Simon
> >> >> >>
> >> >
> >> >
> >
>

[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

[Jagan Teki]

Hi Tom,

On Tue, Jun 11, 2013 at 9:46 PM, Tom Rini <trini@ti.com> wrote:
> On Mon, Jun 10, 2013 at 02:49:35PM -0700, Simon Glass wrote:
>> On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:
>>
>> > Hi Tom,
>> >
>> > On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com>
>> > wrote:
>> > > Hi Simon,
>> > >
>> > > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg@chromium.org> wrote:
>> > >> Hi,
>> > >>
>> > >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki@gmail.com>
>> > wrote:
>> > >>>
>> > >>> Hi Simon,
>> > >>>
>> > >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg@chromium.org> wrote:
>> > >>> > Hi Jagan,
>> > >>> >
>> > >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki
>> > >>> > <jagannadha.sutradharudu-teki@xilinx.com> wrote:
>> > >>> >>
>> > >>> >> Updated the spi_flash framework to handle all sizes of flashes
>> > >>> >> using bank/extd addr reg facility
>> > >>> >>
>> > >>> >> The current implementation in spi_flash supports 3-byte address mode
>> > >>> >> due to this up to 16Mbytes amount of flash is able to access for
>> > those
>> > >>> >> flashes which has an actual size of > 16MB.
>> > >>> >>
>> > >>> >> As most of the flashes introduces a bank/extd address registers
>> > >>> >> for accessing the flashes in 16Mbytes of banks if the flash size
>> > >>> >> is > 16Mbytes, this new scheme will add the bank selection feature
>> > >>> >> for performing write/erase operations on all flashes.
>> > >>> >>
>> > >>> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> > >>> >
>> > >>> >
>> > >>> > I have a few comments on this series, but it mostly looks fine. I
>> > think
>> > >>> > the
>> > >>> > new code is correct.
>> > >>> >
>> > >>> > The patches did not apply cleanly for me. Perhaps I am missing
>> > >>> > something. My
>> > >>> > tree looks like this after I did a bit of merging:
>> > >>>
>> > >>> Which patches you had an issues while applying,we have few patches on
>> > >>> u-boot-spi.git i created these on top of it.
>> > >>
>> > >>
>> > >> I am not sure now - sorry I did not keep a record. But the bundle I
>> > used is
>> > >> here, and it doesn't apply cleanly.
>> > >>
>> > >> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>> > >>
>> > >> git am ~/Downloads/bundle-4407-jagan.mbox
>> > >> Applying: sf: Add bank address register writing support
>> > >> Applying: sf: Add bank address register reading support
>> > >> Applying: sf: Add extended addr write support for winbond|stmicro
>> > >> Applying: sf: Add extended addr read support for winbond|stmicro
>> > >> Applying: sf: read flash bank addr register at probe time
>> > >> Applying: sf: Update sf to support all sizes of flashes
>> > >> error: patch failed: drivers/mtd/spi/spi_flash.c:117
>> > >> error: drivers/mtd/spi/spi_flash.c: patch does not apply
>> > >> Patch failed at 0006 sf: Update sf to support all sizes of flashes
>> > >> The copy of the patch that failed is found in:
>> > >>    /home/sjg/u/.git/rebase-apply/patch
>> > >> When you have resolved this problem, run "git am --resolved".
>> > >> If you prefer to skip this patch, run "git am --skip" instead.
>> > >> To restore the original branch and stop patching, run "git am --abort"
>> > >>
>> > >>>
>> > >>>
>> > >>> >
>> > >>> > 5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash
>> > >>> > devices
>> > >>> > via address shift
>> > >>> > f700095 sf: Add Flag status register polling support
>> > >>> > 42f4b70 sf: Remove spi_flash_cmd_poll_bit()
>> > >>> > fc31387 sf: Use spi_flash_read_common() in write status poll
>> > >>> > 923e40e sf: spansion: Add support for S25FL512S_256K
>> > >>> > c72e52a sf: stmicro: Add support for N25Q1024A
>> > >>> > 4066f71 sf: stmicro: Add support for N25Q1024
>> > >>> > 0efaf86 sf: stmicro: Add support for N25Q512A
>> > >>> > 8fd962f sf: stmicro: Add support for N25Q512
>> > >>> > f1a2080 sf: Use spi_flash_addr() in write call
>> > >>> > 31ed498 sf: Update sf read to support all sizes of flashes
>> > >>> > 0f77642 sf: Update sf to support all sizes of flashes
>> > >>> > 9e57220 sf: read flash bank addr register at probe time
>> > >>> > e99a43d sf: Add extended addr read support for winbond|stmicro
>> > >>> > 2f06d56 sf: Add extended addr write support for winbond|stmicro
>> > >>> > f02ecf1 sf: Add bank address register reading support
>> > >>> > 02ba27c sf: Add bank address register writing support
>> > >>> >
>> > >>> > Also do you think spi_flash_cmd_bankaddr_write() and related stuff
>> > >>> > should be
>> > >>> > behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code
>> > space
>> > >>> > does
>> > >>> > this add?
>> > >>>
>> > >>> Initially i thought of the same, but I just updated sf which is
>> > >>> generic to all sizes of flashes.
>> > >>> due to this i haven't included the bank read/write on macros, and the
>> > >>> flash ops will call these
>> > >>> bank write call irrespective of flash sizes.
>> > >>>
>> > >>> As flashes which has below 16Mbytes will have a bank_curr value 0
>> > >>> so-that even bank write will exit for
>> > >>> bank 0 operations.
>> > >>
>> > >>
>> > >> Yes this is fine.
>> > >>
>> > >>>
>> > >>>
>> > >>> +       if (flash->bank_curr == bank_sel) {
>> > >>> +               debug("SF: not require to enable bank%d\n", bank_sel);
>> > >>> +               return 0;
>> > >>> +       }
>> > >>> +
>> > >>>
>> > >>> And due to these framework changes bank+flash ops addition, bin size
>> > >>> increases appr' ~600bytes
>> > >>> by enabling stmicro, winbond and spansion flash drivers.(please check
>> > >>> the size from your end also if required)
>> > >>
>> > >>
>> > >> I suggest you make that function a nop (perhaps using an #ifdef
>> > >> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
>> > >> don't increase U-Boot code size for those boards that don't need support
>> > >> larger devices (which I guess is almost all of them, right now). U-Boot
>> > is
>> > >> quite concerned about code size.
>> > >
>> > > Little concern here, the point here I stated that these new changes is
>> > > common for all sizes of flashes.(which are less or greater than
>> > > 16Mbytes).
>> > > and yes it increase the code size little bit but i don't think it will
>> > > require the separate macro.
>> >
>> > Any comments.
>> >
>>
>> In U-Boot it is generally not acceptable to increase code size for existing
>> boards when adding a new feature that they don't use. So I suspect in this
>> case you should add a new CONFIG to enable your feature. It seems to
>> increase code by more than 200 bytes for snow, for example.
>>
>> Tom may have further comments.
>
> I think it's important we keep both binary size and code complexity in
> mind when we say "no binary size increase".  It really has to mean "no
> unreasonable binary size increase, when compared with code complexity
> changes".  The question has to be "can we easily make things opt-in".

Understood.

>
> Perhaps the answer here is that we need to add
> CONFIG_SPI_FLASH_STMICRO_STATIC_TABLE and a corresponding set of
> CONFIG_SPI_FLASH_STMICRO_ID/PPS/NR_SECT/NAME (and Winbond variant) so
> that boards with size constrains can opt to define the single chip they
> support, and other boards can just get the full probe table.  This will
> allow boards to get a net size decrease here if desired.

Yes, we should think about this.

I will come to this again for better more idea, if any.

--
Thanks,
Jagan.

>
> Heck, for ARM we are, or have just, reclaimed a bunch of space at least
> on SPL using boards in the main binary now that we --gc-sections U-Boot.
>
> --
> Tom