from:"Insop Song"

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

2013-06-06 Thread Insop Song

 -Original Message-
 From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
 Sent: Wednesday, June 05, 2013 2:00 AM
 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
  And how about this?  can we add flag_status_check flag somewhere, and
 if the device is needed then we set the flag during device discovery.
  Then call the flag check function if the flag is set.
  I think this way might be more generic then what you did in your patch.

  What do you think?

  Thank you,

 Can u just send pseudo code about your logic, i couldn't get u exactly sorry.

Here is what I think, it is similar with your patch in the sense that do the 
different status check for different device.
However, the difference is that your patch checks based on device type at the 
status checking function.
Mine is proposing to add the information to the devices' struct (spi_flash) 
instead, so each device can have a flexibility to call flag status check. In 
this way if new device use this flag_status check, you can just hide the 
setting in devices' probe function, and not worrying about updating generic 
function call (spi_flash_cmd_poll_bit() as in your patch)

I am not so happy to add flag_status in struct spi_flash; so I am open to 
your idea.
Please see below and let me know what you think?

// 1. adding flag status
struct spi_flash {
struct spi_slave *spi;

const char  *name;

/* Total flash size */
u32 size;
/* Write (page) size */
u32 page_size;
/* Erase (sector) size */
u32 sector_size;

/* if flag_status is use or not */
u8  flag_status;

int (*read)(struct spi_flash *flash, u32 offset,
size_t len, void *buf);
int (*write)(struct spi_flash *flash, u32 offset,
size_t len, const void *buf);
int (*erase)(struct spi_flash *flash, u32 offset,
size_t len);
};

// 2. in probe function, set the flag_status per vendor

struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
{

flash-spi = spi;
flash-name = params-name;

flash-write = spi_flash_cmd_write_multi;
flash-erase = spi_flash_cmd_erase;
flash-read = spi_flash_cmd_read_fast;
flash-page_size = 256;
flash-sector_size = 256 * params-pages_per_sector;
flash-size = flash-sector_size * params-nr_sectors;

// we do this for Micron, and will do the same if any other device 
needs this
flash-flag_status = 1;

return flash;
}

// 3. call flag status check if flash-flag_status is set

int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
{
if (flash-flag_status)
return spi_flash_cmd_poll_bit(flash, timeout,
CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC);
else
return spi_flash_cmd_poll_bit(flash, timeout,
CMD_READ_STATUS, STATUS_WIP, 0);
}

Thank you,

IS

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-06 Thread Insop Song

Hi Jagan,

Thank you for your feedback,

 -Original Message-
 From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
 Sent: Wednesday, June 05, 2013 11:34 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

 Thank you very much for your interest on this.

   What do you think?

   Thank you,

  Can u just send pseudo code about your logic, i couldn't get u exactly
 sorry.

  Here is what I think, it is similar with your patch in the sense that do 
  the
 different status check for different device.
  However, the difference is that your patch checks based on device type at
 the status checking function.
  Mine is proposing to add the information to the devices' struct
  (spi_flash) instead, so each device can have a flexibility to call
  flag status check. In this way if new device use this flag_status
  check, you can just hide the setting in devices' probe function, and
  not worrying about updating generic function call
  (spi_flash_cmd_poll_bit() as in your patch)

 Your idea seems to be clean and reasonable, but

  I am not so happy to add flag_status in struct spi_flash; so I am open to
 your idea.
  Please see below and let me know what you think?

  // 1. adding flag status
  struct spi_flash {
  struct spi_slave *spi;

  const char  *name;

  /* Total flash size */
  u32 size;
  /* Write (page) size */
  u32 page_size;
  /* Erase (sector) size */
  u32 sector_size;

  /* if flag_status is use or not */
  u8  flag_status;

  int (*read)(struct spi_flash *flash, u32 offset,
  size_t len, void *buf);
  int (*write)(struct spi_flash *flash, u32 offset,
  size_t len, const void *buf);
  int (*erase)(struct spi_flash *flash, u32 offset,
  size_t len); };

  // 2. in probe function, set the flag_status per vendor

  struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 *
  idcode) {

  flash-spi = spi;
  flash-name = params-name;

  flash-write = spi_flash_cmd_write_multi;
  flash-erase = spi_flash_cmd_erase;
  flash-read = spi_flash_cmd_read_fast;
  flash-page_size = 256;
  flash-sector_size = 256 * params-pages_per_sector;
  flash-size = flash-sector_size * params-nr_sectors;

  // we do this for Micron, and will do the same if any other device 
  needs
 this
  flash-flag_status = 1;

  return flash;
  }

  // 3. call flag status check if flash-flag_status is set

  int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long
  timeout) {
  if (flash-flag_status)
  return spi_flash_cmd_poll_bit(flash, timeout,
  CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC);
  else
  return spi_flash_cmd_poll_bit(flash, timeout,
  CMD_READ_STATUS, STATUS_WIP, 0); }

 APAIK. flag status required only for stmicro flashes which has = 512MB
 flashes.
 And as this is specific to a particular flash with particular size, i don't 
 see any
 reason to add extra variable in spi_flash and then initialized at probe.

Agree with you, that's why I also was not so happy to put flag_status to a 
generic struct.

 Your idea seems to be reasonable if we have more numbers of flash vendors
 require this with respective sizes.

 ie, the reason I have gave a condition for the particular state like
 + if ((flash-idcode0 == 0x20) 
 + (flash-size = SPI_FLASH_512MB_STMIC))

 And  I have removed spi_flash_cmd_poll_bit as these is no separate caller for
 this except the spi_flash_cmd_wait_ready() and did everything on
 spi_flash_cmd_wait_ready().

Sounds good. I think your updated patch is coming on line now, so I will take a 
look at your v2 patch and I will comment on that thread directly.

I think we could close this discussion here.

Thank you.

Insop
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-04 Thread Insop Song

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.


 
  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_STATUS0x05
  +#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

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

2013-06-04 Thread Insop Song

Hi Jagan,

Thank you for your feedback.

 -Original Message-
 From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
 Sent: Monday, June 03, 2013 10:31 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'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.

I've tested on the N25Q512A erase without the flag check went okay.
However, adding the check to the erase would be safe, and I've tested erase 
with flag status check.
Here is the diff of this change. I will use the format-patch when we finalize 
the change.

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 4240e9d..9cf5aaf 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -247,6 +247,10 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 
offset, size_t len)
if (ret)
goto out;

+ ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT);
+ if (ret)
+ break;
+
ret = spi_flash_cmd_wait_ready(flash, 
SPI_FLASH_PAGE_ERASE_TIMEOUT);
if (ret)
goto out;

  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.

To your comment on confusion to users, I agree on that.
With that argument, your patch is better since it hides the flag status check.

And how about this?  can we add flag_status_check flag somewhere, and if the 
device is needed then we set the flag during device discovery.
Then call the flag check function if the flag is set.
I think this way might be more generic then what you did in your patch.

What do you think? 

Thank you,

IS

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-03 Thread Insop Song

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;
+   }
 
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_STATUS0x05
+#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,
+   .pages_per_sector = 256,
+   .nr_sectors = 1024,
+   .name = N25Q512A,
+   },
 };
 
 struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
-- 
1.7.9.5


0001-Add-SPI-Flash-STMicro-s-N25Q512A-add-flag-status-che.patch
Description: 0001-Add-SPI-Flash-STMicro-s-N25Q512A-add-flag-status-che.patch
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information