On Tue, Jun 4, 2013 at 10:34 AM, Insop Song <insop.s...@cohere.net> wrote:
> Hi,
>
> Thank you for your feedback.
>
>
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
>> Sent: Monday, June 03, 2013 12:14 PM
>> To: Insop Song
>> Cc: u-boot@lists.denx.de; york...@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.s...@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.s...@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.s...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
