Hi Simon,
On Fri, Dec 14, 2012 at 7:06 AM, Simon Glass sjg@chromium.org wrote:
+Mike
Hi,
On Thu, Dec 13, 2012 at 8:21 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Thu, Dec 13, 2012 at 4:08 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Wed, Dec 12, 2012 at 8:52 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Dec 12, 2012 at 12:11 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:
This patch provides support to program a flash using Quad-input Page Program(32h) instruction.
This will effectively increases the data transfer rate by up to four times, as compared to the Page Program(PP) instruction.
Respective flash drivers need to use spi_flash_cmd_write_multi_qpp() based on their usage.
Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com
It's great to have this support. A few comments below.
Thanks.
README | 1 + common/cmd_sf.c | 12 +++- drivers/mtd/spi/spi_flash.c | 108 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 13 ++++ include/spi_flash.h | 12 ++++ 5 files changed, 144 insertions(+), 2 deletions(-)
diff --git a/README b/README index 5a86ae9..a01de13 100644 --- a/README +++ b/README @@ -869,6 +869,7 @@ The following options need to be configured: CONFIG_CMD_SETGETDCR Support for DCR Register access (4xx only) CONFIG_CMD_SF * Read/write/erase SPI NOR flash
CONFIG_CMD_SF_QPP * Program SPI flash using Quad-input Page Program CONFIG_CMD_SHA1SUM print sha1 memory digest (requires CONFIG_CMD_MEMORY) CONFIG_CMD_SOURCE "source" command Supportdiff --git a/common/cmd_sf.c b/common/cmd_sf.c index 5ac1d0c..a449d2c 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -228,6 +228,10 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) ret = spi_flash_update(flash, offset, len, buf); else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); +#ifdef CONFIG_CMD_SF_QPP
else if (strcmp(argv[0], "write.qpp") == 0)ret = spi_flash_write_qpp(flash, offset, len, buf);+#endif else ret = spi_flash_write(flash, offset, len, buf);
@@ -300,7 +304,7 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ }
if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 ||
strcmp(cmd, "update") == 0)
strcmp(cmd, "update") == 0 || strcmp(cmd, "write.qpp") == 0) ret = do_spi_flash_read_write(argc, argv); else if (strcmp(cmd, "erase") == 0) ret = do_spi_flash_erase(argc, argv);@@ -327,5 +331,9 @@ U_BOOT_CMD( "sf erase offset [+]len - erase `len' bytes from `offset'\n" " `+len' round up `len' to block size\n" "sf update addr offset len - erase and write `len' bytes from memory\n"
" at `addr' to flash at `offset'"
" at `addr' to flash at `offset'\n"+#ifdef CONFIG_CMD_SF_QPP
"sf write.qpp addr offset len - write `len' bytes from memory\n"" at `addr' to flash at `offset' using Quad-input Page Program(32h)"+#endif ); diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index a8f0af0..3545f59 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -122,6 +122,114 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }
+#ifdef CONFIG_CMD_SF_QPP +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) +{
u8 cmd = CMD_READ_CONFIG;u8 data = 0;int ret;ret = spi_flash_read_common(flash,&cmd, sizeof(cmd), &data, sizeof(data));if (ret < 0) {debug("SF: fail to read config register\n");return ret;}if (data & 0x2) {Can we have defines/enums for this please?
Yes I will do.
debug("SF: quad enable bit is already set.\n");return ret;} else {debug("SF: need to set quad enable bit\n");ret = spi_flash_cmd_write_config(flash, 0x0200);and here?
Ok.
if (ret < 0) {debug("SF: fail to write quad enable bit\n");return ret;}ret = spi_flash_read_common(flash,&cmd, sizeof(cmd), &data, sizeof(data));if (ret < 0) {debug("SF: fail to read config register\n");return ret;}It almost seems like you could have a loop here: read it, return if ok, write it, then loop again (up to two times). Might reduce the code length, but perhaps your way is better.
I just check the config reg for quad bit if it is set already, return. otherwise set the bit and read it again.
Any better way we need to do?
Very rough code:
for (pass = 0; pass < 2; pass++) { spi_flash_read_common() if (data & 0x2) return 0; spi_flash_cmd_write_config(flash, 0x0200); } debug("could not set it...."); return -some error;
I liked this logic, may be I will try.
Please ignore this if you can't make it work, just trying to reduce duplication.
if (data & 0x2)debug("SF: successfully set quad enable bit\n");else {printf("SF: fail to set quad enable bit\n");return -1;}}return ret;+}
+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,
size_t len, const void *buf)+{
unsigned long page_addr, byte_addr, page_size;size_t chunk_len, actual;int ret;u8 cmd[4];page_size = flash->page_size;page_addr = offset / page_size;byte_addr = offset % page_size;ret = spi_claim_bus(flash->spi);if (ret) {debug("SF: unable to claim SPI bus\n");return ret;}ret = spi_flash_set_quad_enable_bit(flash);if (ret) {debug("SF: set quad enable bit failed\n");return ret;}cmd[0] = CMD_QUAD_PAGE_PROGRAM;for (actual = 0; actual < len; actual += chunk_len) {chunk_len = min(len - actual, page_size - byte_addr);cmd[1] = page_addr >> 8;cmd[2] = page_addr;cmd[3] = byte_addr;debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);ret = spi_flash_cmd_write_enable(flash);if (ret < 0) {debug("SF: enabling write failed\n");break;}ret = spi_flash_cmd_write(flash->spi, cmd, 4,buf + actual, chunk_len);if (ret < 0) {debug("SF: write failed\n");break;}ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);if (ret)break;page_addr++;byte_addr = 0;This seems very similar to the existing write function. Can you pull out the common code?
Yes, most of the code is redundant.. But my plan will short it up in near future a/f more testing. (not touch the existing write call as of now)
make sense?
OK, in that case you should probably do your testing after you shorten the code, and then submit again? Or are you just looking for overall comments so far? Overall to me it seems like a useful feature and we should have it.
}printf("SF: program %s %zu bytes @ %#x\n",ret ? "failure" : "success", len, offset);I think this should go in the cmdline code, not here. I may have mentioned that already :-)
spi_release_bus(flash->spi);return ret;+} +#endif
int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd, size_t cmd_len, void *data, size_t data_len) { diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 9287778..cb157ea 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -20,8 +20,10 @@
#define CMD_WRITE_STATUS 0x01 #define CMD_PAGE_PROGRAM 0x02 +#define CMD_QUAD_PAGE_PROGRAM 0x32 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_CONFIG 0x35 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 @@ -54,10 +56,21 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, /*
- Write the requested data out breaking it up into multiple write
- commands as needed per the write size.
*/
- Programming a flash using Page Program(PP, 02h)
int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, size_t len, const void *buf);
+#ifdef CONFIG_CMD_SF_QPP +/*
- Write the requested data out breaking it up into multiple write
- commands as needed per the write size.
- Programming a flash using Quad-input Page Program(QPP, 32h)
- */
+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,
size_t len, const void *buf);+#endif
/*
- Enable writing on the SPI flash.
*/ diff --git a/include/spi_flash.h b/include/spi_flash.h index 9da9062..61f0c19 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -43,6 +43,10 @@ struct spi_flash { size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); +#ifdef CONFIG_CMD_SF_QPP
int (*write_qpp)(struct spi_flash *flash, u32 offset,size_t len, const void *buf);+#endif
This is ok, but it almost feels like we should add a flags parameter here?
I couldn't understand the flag parameter here..can u elaborate?
Well the only difference between the standard write() and your write_qpp() is the status write and the command byte that is sent. Is that right?
If so, then perhaps a flags word as a 5th parameter to write() would make some sense - you could have a QUAD flag. But there is a lot of code to change, so perhaps we should wait until there is a second need for flags.
OK. I agree with your comments.
BTW. I have an other plan to pass the instruction(PP, QPP) to spi_flash_write with a separate flag and only common standard write call can use for both the instructions. may be this way we can reduce the duplicate.
Some how i have liked this, what do you say..any side effects...?
And also I am planning to create a separate commands for QUAD bit enable and disable.. so if any one use the quad write(qpp) and quad read(will add in future) he should enable the QUAD bit first. please comment?
Actually I wonder whether the single/dual/quad setting should be kept in struct spi_flash, and you could add a new function to set the width in bits. Then the existing read/write functions can use the selected width. That way you don't need to change the read()/write() functions, although you will need to add a new function to set the width.
I little bit unclear with your thoughts.
Let me explain my idea.
=Code snippet++ static int do_spi_flash_read_write(int argc, char * const argv[]) { ............ if (strcmp(argv[0], "update") == 0) ret = spi_flash_update(flash, offset, len, buf); else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); #ifdef CONFIG_CMD_SF_QPP else if (strcmp(argv[0], "write.qpp") == 0) ret = spi_flash_write_qpp(flash, offset, len, buf, CMD_QUAD_PAGE_PROGRAM); #endif else ret = spi_flash_write(flash, offset, len, buf, CMD_PAGE_PROGRAM);
............... } =Code snippet--
spi_flash_write() is same for PP and QPP but we need to send the instruction as 5th argument. May be this way we can use the same spi_flash_write() for different instructions.?
But the issue with this there is a header file in drivers/mtd/spi/spi_flash_internal.h. Can we use this header in common/cmd_sf.c or can I move this into include folder?
Let me know if you have any info.
Thanks, Jagan.