[U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write
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 @@ int spi_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; + }
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, + .pages_per_sector = 256, + .nr_sectors = 1024, + .name = "N25Q512A", + }, };
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
Hi,
Thanks for your new changes.
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
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 @@ int spi_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.
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?
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.
-- Thanks, Jagan.
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);
On Tue, Jun 4, 2013 at 10:34 AM, Insop Song Insop.Song@cohere.net wrote:
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.
No i see without the check in erase when i erase and read back i couldn't see 0xff Please 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.
spi_flash_cmd_poll_bit() is common call for poll for read_status, as write call is common to all making changes to spi_flash_cmd_poll_bit() is valid i guess. Write call is a global which is used by so many user, i don't like to add the flag status there and make the confusion user.
-- Thanks, Jagan.
Hi Jagan,
Thank you for your feedback.
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Monday, June 03, 2013 10:31 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'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.
No i see without the check in erase when i erase and read back i couldn't see 0xff Please check.
I've tested on the N25Q512A erase without the flag check went okay. However, adding the check to the erase would be safe, and I've tested erase with flag status check. Here is the diff of this change. I will use the format-patch when we finalize the change.
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 4240e9d..9cf5aaf 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -247,6 +247,10 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) if (ret) goto out;
+ ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT); + if (ret) + break; + ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT); if (ret) goto out;
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.
spi_flash_cmd_poll_bit() is common call for poll for read_status, as write call is common to all making changes to spi_flash_cmd_poll_bit() is valid i guess. Write call is a global which is used by so many user, i don't like to add the flag status there and make the confusion user.
To your comment on "confusion to users", I agree on that. With that argument, your patch is better since it hides the flag status check.
And how about this? can we add "flag_status_check" flag somewhere, and if the device is needed then we set the flag during device discovery. Then call the flag check function if the flag is set. I think this way might be more generic then what you did in your patch.
What do you think?
Thank you,
IS
On Wed, Jun 5, 2013 at 10:10 AM, Insop Song Insop.Song@cohere.net wrote:
Hi Jagan,
Thank you for your feedback.
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Monday, June 03, 2013 10:31 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'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.
No i see without the check in erase when i erase and read back i couldn't see 0xff Please check.
I've tested on the N25Q512A erase without the flag check went okay. However, adding the check to the erase would be safe, and I've tested erase with flag status check. Here is the diff of this change. I will use the format-patch when we finalize the change.
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 4240e9d..9cf5aaf 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -247,6 +247,10 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) if (ret) goto out;
ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT);if (ret)break;ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT); if (ret) goto out;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.
spi_flash_cmd_poll_bit() is common call for poll for read_status, as write call is common to all making changes to spi_flash_cmd_poll_bit() is valid i guess. Write call is a global which is used by so many user, i don't like to add the flag status there and make the confusion user.
To your comment on "confusion to users", I agree on that. With that argument, your patch is better since it hides the flag status check.
And how about this? can we add "flag_status_check" flag somewhere, and if the device is needed then we set the flag during device discovery. Then call the flag check function if the flag is set. I think this way might be more generic then what you did in your patch.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
-- Thanks, Jagan.
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Wednesday, June 05, 2013 2:00 AM 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
And how about this? can we add "flag_status_check" flag somewhere, and
if the device is needed then we set the flag during device discovery.
Then call the flag check function if the flag is set. I think this way might be more generic then what you did in your patch.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
Here is what I think, it is similar with your patch in the sense that "do the different status" check for different device. However, the difference is that your patch checks based on device type at the status checking function. Mine is proposing to add the information to the devices' struct (spi_flash) instead, so each device can have a flexibility to call flag status check. In this way if new device use this flag_status check, you can just hide the setting in devices' probe function, and not worrying about updating generic function call (spi_flash_cmd_poll_bit() as in your patch)
I am not so happy to add "flag_status" in struct spi_flash; so I am open to your idea. Please see below and let me know what you think?
// 1. adding flag status struct spi_flash { struct spi_slave *spi;
const char *name;
/* Total flash size */ u32 size; /* Write (page) size */ u32 page_size; /* Erase (sector) size */ u32 sector_size;
/* if flag_status is use or not */ u8 flag_status;
int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); int (*erase)(struct spi_flash *flash, u32 offset, size_t len); };
// 2. in probe function, set the flag_status per vendor
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) {
flash->spi = spi; flash->name = params->name;
flash->write = spi_flash_cmd_write_multi; flash->erase = spi_flash_cmd_erase; flash->read = spi_flash_cmd_read_fast; flash->page_size = 256; flash->sector_size = 256 * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors;
// we do this for Micron, and will do the same if any other device needs this flash->flag_status = 1;
return flash; }
// 3. call flag status check if flash->flag_status is set
int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout) { if (flash->flag_status) return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); else return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_STATUS, STATUS_WIP, 0); }
Thank you,
IS
Thank you very much for your interest on this.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
Here is what I think, it is similar with your patch in the sense that "do the different status" check for different device. However, the difference is that your patch checks based on device type at the status checking function. Mine is proposing to add the information to the devices' struct (spi_flash) instead, so each device can have a flexibility to call flag status check. In this way if new device use this flag_status check, you can just hide the setting in devices' probe function, and not worrying about updating generic function call (spi_flash_cmd_poll_bit() as in your patch)
Your idea seems to be clean and reasonable, but
I am not so happy to add "flag_status" in struct spi_flash; so I am open to your idea. Please see below and let me know what you think?
// 1. adding flag status struct spi_flash { struct spi_slave *spi;
const char *name; /* Total flash size */ u32 size; /* Write (page) size */ u32 page_size; /* Erase (sector) size */ u32 sector_size; /* if flag_status is use or not */ u8 flag_status; int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); int (*erase)(struct spi_flash *flash, u32 offset, size_t len);};
// 2. in probe function, set the flag_status per vendor
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) {
flash->spi = spi; flash->name = params->name; flash->write = spi_flash_cmd_write_multi; flash->erase = spi_flash_cmd_erase; flash->read = spi_flash_cmd_read_fast; flash->page_size = 256; flash->sector_size = 256 * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors; // we do this for Micron, and will do the same if any other device needs this flash->flag_status = 1; return flash;}
// 3. call flag status check if flash->flag_status is set
int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout) { if (flash->flag_status) return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); else return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_STATUS, STATUS_WIP, 0); }
APAIK. flag status required only for stmicro flashes which has >= 512MB flashes. And as this is specific to a particular flash with particular size, i don't see any reason to add extra variable in spi_flash and then initialized at probe. Your idea seems to be reasonable if we have more numbers of flash vendors require this with respective sizes.
ie, the reason I have gave a condition for the particular state like + if ((flash->idcode0 == 0x20) && + (flash->size >= SPI_FLASH_512MB_STMIC))
And I have removed spi_flash_cmd_poll_bit as these is no separate caller for this except the spi_flash_cmd_wait_ready() and did everything on spi_flash_cmd_wait_ready().
Comments.
-- Thanks, Jagan.
Hi Jagan,
Thank you for your feedback,
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Wednesday, June 05, 2013 11:34 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
Thank you very much for your interest on this.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly
sorry.
Here is what I think, it is similar with your patch in the sense that "do the
different status" check for different device.
However, the difference is that your patch checks based on device type at
the status checking function.
Mine is proposing to add the information to the devices' struct (spi_flash) instead, so each device can have a flexibility to call flag status check. In this way if new device use this flag_status check, you can just hide the setting in devices' probe function, and not worrying about updating generic function call (spi_flash_cmd_poll_bit() as in your patch)
Your idea seems to be clean and reasonable, but
I am not so happy to add "flag_status" in struct spi_flash; so I am open to
your idea.
Please see below and let me know what you think?
// 1. adding flag status struct spi_flash { struct spi_slave *spi;
const char *name; /* Total flash size */ u32 size; /* Write (page) size */ u32 page_size; /* Erase (sector) size */ u32 sector_size; /* if flag_status is use or not */ u8 flag_status; int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); int (*erase)(struct spi_flash *flash, u32 offset, size_t len); };// 2. in probe function, set the flag_status per vendor
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) {
flash->spi = spi; flash->name = params->name; flash->write = spi_flash_cmd_write_multi; flash->erase = spi_flash_cmd_erase; flash->read = spi_flash_cmd_read_fast; flash->page_size = 256; flash->sector_size = 256 * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors; // we do this for Micron, and will do the same if any other device needsthis
flash->flag_status = 1; return flash;}
// 3. call flag status check if flash->flag_status is set
int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout) { if (flash->flag_status) return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); else return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_STATUS, STATUS_WIP, 0); }
APAIK. flag status required only for stmicro flashes which has >= 512MB flashes. And as this is specific to a particular flash with particular size, i don't see any reason to add extra variable in spi_flash and then initialized at probe.
Agree with you, that's why I also was not so happy to put flag_status to a generic struct.
Your idea seems to be reasonable if we have more numbers of flash vendors require this with respective sizes.
ie, the reason I have gave a condition for the particular state like
- if ((flash->idcode0 == 0x20) &&
(flash->size >= SPI_FLASH_512MB_STMIC))And I have removed spi_flash_cmd_poll_bit as these is no separate caller for this except the spi_flash_cmd_wait_ready() and did everything on spi_flash_cmd_wait_ready().
Sounds good. I think your updated patch is coming on line now, so I will take a look at your v2 patch and I will comment on that thread directly.
I think we could close this discussion here.
Thank you.
Insop
On Thu, Jun 6, 2013 at 12:18 PM, Insop Song Insop.Song@cohere.net wrote:
Hi Jagan,
Thank you for your feedback,
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Wednesday, June 05, 2013 11:34 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
Thank you very much for your interest on this.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly
sorry.
Here is what I think, it is similar with your patch in the sense that "do the
different status" check for different device.
However, the difference is that your patch checks based on device type at
the status checking function.
Mine is proposing to add the information to the devices' struct (spi_flash) instead, so each device can have a flexibility to call flag status check. In this way if new device use this flag_status check, you can just hide the setting in devices' probe function, and not worrying about updating generic function call (spi_flash_cmd_poll_bit() as in your patch)
Your idea seems to be clean and reasonable, but
I am not so happy to add "flag_status" in struct spi_flash; so I am open to
your idea.
Please see below and let me know what you think?
// 1. adding flag status struct spi_flash { struct spi_slave *spi;
const char *name; /* Total flash size */ u32 size; /* Write (page) size */ u32 page_size; /* Erase (sector) size */ u32 sector_size; /* if flag_status is use or not */ u8 flag_status; int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); int (*erase)(struct spi_flash *flash, u32 offset, size_t len); };// 2. in probe function, set the flag_status per vendor
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) {
flash->spi = spi; flash->name = params->name; flash->write = spi_flash_cmd_write_multi; flash->erase = spi_flash_cmd_erase; flash->read = spi_flash_cmd_read_fast; flash->page_size = 256; flash->sector_size = 256 * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors; // we do this for Micron, and will do the same if any other device needsthis
flash->flag_status = 1; return flash;}
// 3. call flag status check if flash->flag_status is set
int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout) { if (flash->flag_status) return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); else return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_STATUS, STATUS_WIP, 0); }
APAIK. flag status required only for stmicro flashes which has >= 512MB flashes. And as this is specific to a particular flash with particular size, i don't see any reason to add extra variable in spi_flash and then initialized at probe.
Agree with you, that's why I also was not so happy to put flag_status to a generic struct.
Your idea seems to be reasonable if we have more numbers of flash vendors require this with respective sizes.
ie, the reason I have gave a condition for the particular state like
if ((flash->idcode0 == 0x20) &&(flash->size >= SPI_FLASH_512MB_STMIC))And I have removed spi_flash_cmd_poll_bit as these is no separate caller for this except the spi_flash_cmd_wait_ready() and did everything on spi_flash_cmd_wait_ready().
Sounds good. I think your updated patch is coming on line now, so I will take a look at your v2 patch and I will comment on that thread directly.
I think we could close this discussion here.
Thank you.
Insop
Thank you, if possible please test with new sf framework updates.
-- Thanks, Jagan.
participants (2)
-
Insop Song -
Jagan Teki