[U-Boot] [PATCH 4/4] sf: Add Quad-input Page Program(32h) instruction support
Jagan Teki
jagannadh.teki at gmail.com
Wed Dec 12 17:52:41 CET 2012
Hi Simon,
On Wed, Dec 12, 2012 at 12:11 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
> <jagannadh.teki at 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 at 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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon
More information about the U-Boot
mailing list