[U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write

Jagan Teki jagannadh.teki at gmail.com
Tue Jun 4 07:31:18 CEST 2013


On Tue, Jun 4, 2013 at 10:34 AM, Insop Song <Insop.Song at cohere.net> wrote:
> 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.

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.


More information about the U-Boot mailing list