[U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write
Insop Song
Insop.Song at cohere.net
Tue Jun 4 07:04:45 CEST 2013
Hi,
Thank you for your feedback.
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Monday, June 03, 2013 12:14 PM
> To: Insop Song
> Cc: u-boot at lists.denx.de; yorksun at 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 at 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 at 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 at 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.
>
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
----------------------------------------------------
More information about the U-Boot
mailing list