Hi Simon,
On Fri, Dec 14, 2012 at 7:06 AM, Simon Glass <s...@chromium.org> wrote:
> +Mike
>
> Hi,
>
> On Thu, Dec 13, 2012 at 8:21 AM, Jagan Teki <jagannadh.t...@gmail.com> wrote:
>> Hi Simon,
>>
>> On Thu, Dec 13, 2012 at 4:08 AM, Simon Glass <s...@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On Wed, Dec 12, 2012 at 8:52 AM, Jagan Teki <jagannadh.t...@gmail.com>
>>> wrote:
>>>> Hi Simon,
>>>>
>>>> On Wed, Dec 12, 2012 at 12:11 PM, Simon Glass <s...@chromium.org> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>>>> <jagannadh.t...@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.t...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
