+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.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
