| Message ID | 1355150521-3339-5-git-send-email-jagannadh.teki@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Mike Frysinger |
| Headers | show |
Hello Jagannadha, I have some remarks and questions, as I currently work on a hw platform, which also would allow to support dual- or quad-io accesses. So my first question: why is this restricted to write only? If you have a hardware, which is capable of supporting this, the read will definitely benefit from it, while the speedup for write depends on the internal programming time of the flash. The other questions: On which hardware platform was this tested and for which flashes? Patches for both components are missing from this patch series. And for both I have more remarks below. Jagannadha Sutradharudu Teki wrote on 2012-12-10: > 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. > > +#ifdef CONFIG_CMD_SF_QPP > + else if (strcmp(argv[0], "write.qpp") == 0) > + ret = spi_flash_write_qpp(flash, offset, len, buf); > +#endif Is it really necessary to have a dedicated command here? Wouldn't it be better, if the SF layer or the driver will use it automatically, if the hardware supports it and the driver has detected the feature of the flash? > +#ifdef CONFIG_CMD_SF_QPP > +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) > +{ [...] > + > + if (data & 0x2) { > + debug("SF: quad enable bit is already set.\n"); Is this bit common for all flashes? Otherwise add some comments on tested flashed and/or TODO for an extension to provide this info from the flash driver. > + return ret; > + } else { > + debug("SF: need to set quad enable bit\n"); > + > + ret = spi_flash_cmd_write_config(flash, 0x0200); Same here. And why is this a 16-bit value here? > +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); I don't like this implicit setting here. And as far as I know, this bit is sticky/non-volatile. It is not necessary to write this each time. Maybe it make more sense to have an interactive command to write this bit (enabled or disabled) to the flash? And then the flash probe function can check the bit and map to the appropriate read and write functions. > + 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; > + } Except for the config bit and the different command code, I don't see any difference to the "regular" spi_flash_cmd_write_multi function. So the question is: How does the SPI framework knows when to send 4 bits in parallel to 4 pins instead of the serialization to one signal pin? This extension to the SPI framework is completely missing! > 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); The flash probe should detect, if QPP is possible, and then map the existing (*read) and (*write) pointers to the relevant functions. No new pointers required here. > +#endif > int (*erase)(struct spi_flash *flash, u32 offset, > size_t len); > }; > +#ifdef CONFIG_CMD_SF_QPP > +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, > + size_t len, const void *buf) > +{ > + return flash->write_qpp(flash, offset, len, buf); > +} > +#endif See above, this is also not required. I would be happy to support you by adapting and testing this extension on my platform. The plan on my side is to send the pieces to support my platform in the near future (not sure if my schedule will fit for the next merge window, but anyway..) Adding this feature on top would be very nice ;-) Best Regards, Thomas
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. > --- > 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 Support > diff --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? > + 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? > + 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. > + > + 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? > + } > + > + 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? > int (*erase)(struct spi_flash *flash, u32 offset, > size_t len); > }; > @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, > return flash->write(flash, offset, len, buf); > } > > +#ifdef CONFIG_CMD_SF_QPP > +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, > + size_t len, const void *buf) > +{ > + return flash->write_qpp(flash, offset, len, buf); > +} > +#endif > + > static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, > size_t len) > { > -- > 1.7.0.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon
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 Support >> diff --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? > >> + >> + 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? > >> + } >> + >> + 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? Thanks, Jagan. > >> int (*erase)(struct spi_flash *flash, u32 offset, >> size_t len); >> }; >> @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, >> return flash->write(flash, offset, len, buf); >> } >> >> +#ifdef CONFIG_CMD_SF_QPP >> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, >> + size_t len, const void *buf) >> +{ >> + return flash->write_qpp(flash, offset, len, buf); >> +} >> +#endif >> + >> static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, >> size_t len) >> { >> -- >> 1.7.0.4 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > > Regards, > Simon
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 Support >>> diff --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; 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. Regards, Simon > > Thanks, > Jagan. > >> >>> int (*erase)(struct spi_flash *flash, u32 offset, >>> size_t len); >>> }; >>> @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, >>> return flash->write(flash, offset, len, buf); >>> } >>> >>> +#ifdef CONFIG_CMD_SF_QPP >>> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, >>> + size_t len, const void *buf) >>> +{ >>> + return flash->write_qpp(flash, offset, len, buf); >>> +} >>> +#endif >>> + >>> static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, >>> size_t len) >>> { >>> -- >>> 1.7.0.4 >>> >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >> >> Regards, >> Simon
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 Support >>>> diff --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? Thanks, Jagan. > > Regards, > Simon > >> >> Thanks, >> Jagan. >> >>> >>>> int (*erase)(struct spi_flash *flash, u32 offset, >>>> size_t len); >>>> }; >>>> @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, >>>> return flash->write(flash, offset, len, buf); >>>> } >>>> >>>> +#ifdef CONFIG_CMD_SF_QPP >>>> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, >>>> + size_t len, const void *buf) >>>> +{ >>>> + return flash->write_qpp(flash, offset, len, buf); >>>> +} >>>> +#endif >>>> + >>>> static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, >>>> size_t len) >>>> { >>>> -- >>>> 1.7.0.4 >>>> >>>> _______________________________________________ >>>> U-Boot mailing list >>>> U-Boot@lists.denx.de >>>> http://lists.denx.de/mailman/listinfo/u-boot >>> >>> Regards, >>> Simon
+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 Support >>>>> diff --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. Regards, Simon > > Thanks, > Jagan. > >> >> Regards, >> Simon >> >>> >>> Thanks, >>> Jagan. >>> >>>> >>>>> int (*erase)(struct spi_flash *flash, u32 offset, >>>>> size_t len); >>>>> }; >>>>> @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, >>>>> return flash->write(flash, offset, len, buf); >>>>> } >>>>> >>>>> +#ifdef CONFIG_CMD_SF_QPP >>>>> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, >>>>> + size_t len, const void *buf) >>>>> +{ >>>>> + return flash->write_qpp(flash, offset, len, buf); >>>>> +} >>>>> +#endif >>>>> + >>>>> static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, >>>>> size_t len) >>>>> { >>>>> -- >>>>> 1.7.0.4 >>>>> >>>>> _______________________________________________ >>>>> U-Boot mailing list >>>>> U-Boot@lists.denx.de >>>>> http://lists.denx.de/mailman/listinfo/u-boot >>>> >>>> Regards, >>>> Simon
Hi Thomas, On Tue, Dec 11, 2012 at 5:00 PM, Langer Thomas (LQDE RD ST PON SW) <thomas.langer@lantiq.com> wrote: > Hello Jagannadha, > > I have some remarks and questions, as I currently work on a hw platform, which also would allow > to support dual- or quad-io accesses. Planning to add reads too.. > > So my first question: why is this restricted to write only? If you have a hardware, which is capable > of supporting this, the read will definitely benefit from it, while the speedup for write depends on > the internal programming time of the flash. Yes I will try to add these in near feature. > > The other questions: On which hardware platform was this tested and for which flashes? > Patches for both components are missing from this patch series. Yes I am missing flash info..My plan will add the flash source. Once this code is in a recommended position. > And for both I have more remarks below. > > Jagannadha Sutradharudu Teki wrote on 2012-12-10: >> 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. >> > > >> +#ifdef CONFIG_CMD_SF_QPP >> + else if (strcmp(argv[0], "write.qpp") == 0) >> + ret = spi_flash_write_qpp(flash, offset, len, buf); >> +#endif > Is it really necessary to have a dedicated command here? Wouldn't it be better, if the SF layer or > the driver will use it automatically, if the hardware supports it and the driver has detected the > feature of the flash? But I couldn't see any way to detect the read/write modes though dedicated flash register. Appreciate if you give any suggestion. > >> +#ifdef CONFIG_CMD_SF_QPP >> +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) >> +{ > [...] >> + >> + if (data & 0x2) { >> + debug("SF: quad enable bit is already set.\n"); > Is this bit common for all flashes? Otherwise add some comments on tested flashed > and/or TODO for an extension to provide this info from the flash driver. OK, Thanks. > >> + return ret; >> + } else { >> + debug("SF: need to set quad enable bit\n"); >> + >> + ret = spi_flash_cmd_write_config(flash, 0x0200); > Same here. And why is this a 16-bit value here? Yes WRR with 16-bit data. WRR CMD+status reg+config reg. 8 + 8 + 8 = 8-bit CMD and 16-bit DATA. > >> +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); > I don't like this implicit setting here. And as far as I know, this bit is sticky/non-volatile. It is not > necessary to write this each time. I just check the config reg for quad bit if it is set already, return. otherwise set the bit and read it again. > Maybe it make more sense to have an interactive command to write this bit (enabled or disabled) > to the flash? yes i have the same thought, but thinking..this bit needs to set only on QUAD modes. Why this needs to be a separate commands. > And then the flash probe function can check the bit and map to the appropriate read and write > functions. I am unclear. Can you elaborate? > >> + 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; >> + } > Except for the config bit and the different command code, I don't see any difference to the > "regular" spi_flash_cmd_write_multi function. So the question is: How does the SPI framework 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) > knows when to send 4 bits in parallel to 4 pins instead of the serialization to one signal pin? > > This extension to the SPI framework is completely missing! Yes the entire bit logic is controller ed by specific controller driver. MTD flash layer must send the respective commands. Please add any inputs and let me know for any information. Thanks, Jagan. > >> 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); > > The flash probe should detect, if QPP is possible, and then map the existing (*read) and (*write) > pointers to the relevant functions. No new pointers required here. > >> +#endif >> int (*erase)(struct spi_flash *flash, u32 offset, >> size_t len); >> }; > >> +#ifdef CONFIG_CMD_SF_QPP >> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, >> + size_t len, const void *buf) >> +{ >> + return flash->write_qpp(flash, offset, len, buf); >> +} >> +#endif > See above, this is also not required. > > I would be happy to support you by adapting and testing this extension on my platform. > > The plan on my side is to send the pieces to support my platform in the near future (not sure if > my schedule will fit for the next merge window, but anyway..) > > Adding this feature on top would be very nice ;-) > > Best Regards, > Thomas >
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 Support >>>>>> diff --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.
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 Support diff --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) { + 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); + 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; + } + + 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; + } + + printf("SF: program %s %zu bytes @ %#x\n", + ret ? "failure" : "success", len, offset); + + 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 int (*erase)(struct spi_flash *flash, u32 offset, size_t len); }; @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, return flash->write(flash, offset, len, buf); } +#ifdef CONFIG_CMD_SF_QPP +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset, + size_t len, const void *buf) +{ + return flash->write_qpp(flash, offset, len, buf); +} +#endif + static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, size_t len) {
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> --- 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(-)