Hi, Thank you for your feedback.
> -----Original Message----- > From: Jagan Teki [mailto:[email protected]] > Sent: Monday, June 03, 2013 12:14 PM > To: Insop Song > Cc: [email protected]; [email protected] > 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 <[email protected]> > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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. > 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 <[email protected]> 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 <[email protected]> --- 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); -- 1.7.9.5 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
