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