Hi,
Thank you for your feedback.
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Monday, June 03, 2013 12:14 PM To: Insop Song Cc: u-boot@lists.denx.de; yorksun@freescale.com Subject: Re: [U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write
I don't know may be you sent these changes intentionally. I personally not encouraging these as you sent all changes in one patch, attached a patch series to mail and did n't follow commit message header. http://www.denx.de/wiki/view/U- Boot/Patches#Commit_message_conventions
I've put two changes in one patch because flag status check is needed for Micron's device. This is already started mailing thread, so I will keep as it is, but I will follow the patch convention as the link you told me.
On Mon, Jun 3, 2013 at 10:16 PM, Insop Song Insop.Song@cohere.net wrote:
Hi,
I've made two changes while I was working on STMidro's SPI Flash N25Q512A
- add the device entry for STMidro's SPI Flash N25Q512A
- add Flag status check during Without this flag checking "sf write" will fail and SPI flash
is locked up
Please see the patch and let me know your feedback.
From 97572b32c49d06ca6f8548ed88e6e381fb719a08 Mon Sep 17 00:00:00
2001
From: Insop Song insop.song@cohere.net Date: Sun, 2 Jun 2013 13:33:37 -0700 Subject: [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write
Signed-off-by: Insop Song insop.song@cohere.net
drivers/mtd/spi/spi_flash.c | 25 +++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 1 + drivers/mtd/spi/stmicro.c | 6 ++++++ 3 files changed, 32 insertions(+)
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 00aece9..f53756d 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -72,6 +72,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
u32 offset,
size_t chunk_len, actual; int ret; u8 cmd[4];
u8 flag_status; page_size = flash->page_size; page_addr = offset / page_size; @@ -83,6 +84,18 @@ intspi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }
+wait_flag:
ret = spi_flash_cmd(flash->spi, CMD_READ_FLAG_STATUS,&flag_status, sizeof(flag_status));
if (ret < 0) {printf("SF: reading flag failed\n");return ret;}debug("flag_status %d\n", flag_status);if ((flag_status & 0x80) == 0) {udelay(10);goto wait_flag;}cmd[0] = CMD_PAGE_PROGRAM; for (actual = 0; actual < len; actual += chunk_len) { chunk_len = min(len - actual, page_size - byte_addr);@@ -106,6 +119,18 @@ int spi_flash_cmd_write_multi(struct spi_flash
*flash, u32 offset,
debug("SF: write failed\n"); break; }
/* check status */+flag_check:
ret = spi_flash_cmd(flash->spi, CMD_READ_FLAG_STATUS,&flag_status, sizeof(flag_status));
if (ret < 0) {printf("SF: reading flag failed\n");break;}debug("flag_status %d\n", flag_status);if (!(flag_status & 0x80)) {udelay(100);goto flag_check;}Instead doing this poll on actaual write, may be do it on spi_flash_cmd_wait_ready() for code compatibility. Did you tested these changes, i think the same Flag_status must require on erase as well.
I've made a change and add spi_flash_cmd_wait_program to align with existing code structure. Please see the patch below. Erase is okay without checking, so I didn't add the check.
ret = spi_flash_cmd_wait_ready(flash,SPI_FLASH_PROG_TIMEOUT);
if (ret)diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 141cfa8..90b6993 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -22,6 +22,7 @@ #define CMD_PAGE_PROGRAM 0x02 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_FLAG_STATUS 0x70 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c index 30b626a..ad94ad2 100644 --- a/drivers/mtd/spi/stmicro.c +++ b/drivers/mtd/spi/stmicro.c @@ -110,6 +110,12 @@ static const struct stmicro_spi_flash_params
stmicro_spi_flash_table[] = {
.nr_sectors = 512, .name = "N25Q256", },
{.id = 0xba20,This is wrong, 0xba20 is for N25Q512, 0xbb20 is for N25Q512A., agree?
I think that it is correct, it is 0xba20 not 0xbb20. Here is from the datasheet from the Micron chip N25Q512A
JEDEC-standard 2-byte signature (BA20h)
Please see there is a patch available in spi bucket http://patchwork.ozlabs.org/patch/247953/
The main agenda about this patch is trying to use same wait_poll func which is used for read_status register, to make code reliable and modular.
Please test the above patch http://patchwork.ozlabs.org/patch/247953/ Let me know if you have any concerns/issues.
I am not sure I'd agree with you on that you are putting the device check in spi_flash_cmd_poll_bit(). I am more inclined to make the change at the level where we call spi_flash_cmd_poll_bit() if we have to distinguish per devices.
Please let me know what do you think.
Thank you,
IS
----------------------------------------------------
From 86f1d778caea02029f58706d4614c28c08ffd937 Mon Sep 17 00:00:00 2001
From: Insop Song insop.song@cohere.net Date: Mon, 3 Jun 2013 21:55:19 -0700 Subject: [PATCH] Add flag status during SPI Flash write
Add spi_flash_cmd_wait_program() to align with existing code structure Use existing spi_flash_cmd_poll_bit to check flag status for program is done Update spi_flash_cmd_poll_bit() so that it can check against check_bit instead of zero
Tested SPI flash: STMicro's N25Q512A
Signed-off-by: Insop Song insop.song@cohere.net --- drivers/mtd/spi/spi_flash.c | 25 +++++++++++++++++++++---- drivers/mtd/spi/spi_flash_internal.h | 10 +++++++++- 2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 00aece9..4240e9d 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -83,6 +83,10 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }
+ ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT); + if (ret) + return ret; + cmd[0] = CMD_PAGE_PROGRAM; for (actual = 0; actual < len; actual += chunk_len) { chunk_len = min(len - actual, page_size - byte_addr); @@ -107,6 +111,13 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, break; }
+ /* + * check flag status for program is done + */ + ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT); + if (ret) + break; + ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT); if (ret) break; @@ -148,7 +159,7 @@ 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) + u8 cmd, u8 poll_bit, u8 check_bit) { struct spi_slave *spi = flash->spi; unsigned long timebase; @@ -169,14 +180,14 @@ int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout, if (ret) return -1;
- if ((status & poll_bit) == 0) + if ((status & poll_bit) == check_bit) break;
} while (get_timer(timebase) < timeout);
spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
- if ((status & poll_bit) == 0) + if ((status & poll_bit) == check_bit) return 0;
/* Timed out */ @@ -187,7 +198,13 @@ int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout, 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); + CMD_READ_STATUS, STATUS_WIP, 0); +} + +int spi_flash_cmd_wait_program(struct spi_flash *flash, unsigned long timeout) +{ + return spi_flash_cmd_poll_bit(flash, timeout, + CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); }
int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 141cfa8..670a722 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -22,6 +22,7 @@ #define CMD_PAGE_PROGRAM 0x02 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_FLAG_STATUS 0x70 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 @@ -30,6 +31,7 @@
/* Common status */ #define STATUS_WIP 0x01 +#define STATUS_PEC 0x80 /* program or erase controller */
/* 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); @@ -86,7 +88,7 @@ int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,
/* 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); + u8 cmd, u8 poll_bit, u8 check_bit);
/* * Send the read status command to the device and wait for the wip @@ -94,6 +96,12 @@ int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout, */ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout);
+/* + * Send the read flag status command to the device and wait for the program + * is done and ready + */ +int spi_flash_cmd_wait_program(struct spi_flash *flash, unsigned long timeout); + /* Erase sectors. */ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len);