[U-Boot] [PATCH 4/4] sf: Add Quad-input Page Program(32h) instruction support

Jagan Teki jagannadh.teki at gmail.com
Wed Dec 19 12:28:30 CET 2012


Hi Simon,

On Fri, Dec 14, 2012 at 7:06 AM, Simon Glass <sjg at chromium.org> wrote:
> +Mike
>
> Hi,
>
> On Thu, Dec 13, 2012 at 8:21 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> 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?
>
> 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.


More information about the U-Boot mailing list