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.