[U-Boot] [PATCH 01/12] cmd_sf: Add wr_inst argument to 'sf write' command

Simon Glass sjg at chromium.org
Thu Feb 14 15:35:54 CET 2013


Hi Jagan,

On Wed, Jan 16, 2013 at 2:52 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jan 11, 2013 at 7:41 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagannadha,
>>
>> On Mon, Dec 31, 2012 at 3:13 AM, Jagannadha Sutradharudu Teki
>> <jagannadh.teki at gmail.com> wrote:
>>> This patch provides a support to add a write instruction(wr_inst)
>>> argument to 'sf write' command.
>>>
>>> User will dynamically use the specific write instruction while
>>> programming the flash using 'sf write' command.
>>> Currently added an existing write instruction called pp(Page Program).
>>
>> I wonder if you might look at using an hyphen option system like:
>>
>> -p for page program
>> -x for something else
>
> May be required, I will look into this.
>
>>
>>>
>>> Example:
>>> write 0x2000 length bytes from memory at 0x10000 address to
>>> flash offset at 0x0 using pp write instruction.
>>> u-boot> sf write pp 0x10000 0x0 0x2000
>>>
>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki at gmail.com>
>>> ---
>>>  common/cmd_sf.c                      |   36 +++++++++++++++++++++++----------
>>>  drivers/mtd/spi/spi_flash.c          |    4 +-
>>>  drivers/mtd/spi/spi_flash_internal.h |    2 +-
>>>  include/spi_flash.h                  |   10 ++++----
>>>  include/spi_flash_inst.h             |   30 ++++++++++++++++++++++++++++
>>>  5 files changed, 63 insertions(+), 19 deletions(-)
>>>  create mode 100644 include/spi_flash_inst.h
>>>
>>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>>> index b175358..e7843f3 100644
>>> --- a/common/cmd_sf.c
>>> +++ b/common/cmd_sf.c
>>> @@ -9,6 +9,7 @@
>>>  #include <common.h>
>>>  #include <malloc.h>
>>>  #include <spi_flash.h>
>>> +#include <spi_flash_inst.h>
>>>
>>>  #include <asm/io.h>
>>>
>>> @@ -234,20 +235,21 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
>>>         unsigned long len;
>>>         void *buf;
>>>         char *endp;
>>> +       u8 wr_inst;
>>>         int ret;
>>>
>>> -       if (argc < 4)
>>> +       if (argc < 5)
>>>                 return -1;
>>>
>>> -       addr = simple_strtoul(argv[1], &endp, 16);
>>> -       if (*argv[1] == 0 || *endp != 0)
>>> -               return -1;
>>> -       offset = simple_strtoul(argv[2], &endp, 16);
>>> +       addr = simple_strtoul(argv[2], &endp, 16);
>>>         if (*argv[2] == 0 || *endp != 0)
>>>                 return -1;
>>> -       len = simple_strtoul(argv[3], &endp, 16);
>>> +       offset = simple_strtoul(argv[3], &endp, 16);
>>>         if (*argv[3] == 0 || *endp != 0)
>>>                 return -1;
>>> +       len = simple_strtoul(argv[4], &endp, 16);
>>> +       if (*argv[4] == 0 || *endp != 0)
>>> +               return -1;
>>>
>>>         /* Consistency checking */
>>>         if (offset + len > flash->size) {
>>> @@ -266,8 +268,17 @@ 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);
>>> -       else
>>> -               ret = spi_flash_write(flash, offset, len, buf);
>>> +       else {
>>> +               if (strcmp(argv[1], "pp") == 0)
>>> +                       wr_inst = CMD_PAGE_PROGRAM;
>>> +               else {
>>> +                       printf("SF: Unknown %s wr_inst on 'sf write'\n",
>>
>> Single quotes around %s I think
>
> Ok.
>
>>
>>> +                                       argv[1]);
>>> +                       return 1;
>>> +               }
>>> +
>>> +               ret = spi_flash_write(flash, wr_inst, offset, len, buf);
>>> +       }
>>>
>>>         unmap_physmem(buf, len);
>>>
>>> @@ -520,14 +531,17 @@ usage:
>>>  #endif
>>>
>>>  U_BOOT_CMD(
>>> -       sf,     5,      1,      do_spi_flash,
>>> +       sf,     6,      1,      do_spi_flash,
>>>         "SPI flash sub-system",
>>>         "probe [[bus:]cs] [hz] [mode]   - init flash device on given SPI bus\n"
>>>         "                                 and chip select\n"
>>>         "sf read addr offset len        - read `len' bytes starting at\n"
>>>         "                                 `offset' to memory at `addr'\n"
>>> -       "sf write addr offset len       - write `len' bytes from memory\n"
>>> -       "                                 at `addr' to flash at `offset'\n"
>>> +       "sf write wr_inst addr offset len\n"
>>
>> I think wr_inst is optional, so should have [] around it
>>
>>> +       "                               - write `len' bytes from memory\n"
>>> +       "                                 at `addr' to flash at `offset' using\n"
>>> +       "                                 pp `wr_inst' write instruction\n"
>>
>> options are: pp (only)
>>
>> W
>
> I think your idea is to use optional read/write inst for existing
> supported instruction is it?

Yes, please have a think about whether this makes sense.

>
>>
>>> +       "                                 pp (Page Program, 02h)\n"
>>>         "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"
>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>> index 4dacdc7..8ba2c65 100644
>>> --- a/drivers/mtd/spi/spi_flash.c
>>> +++ b/drivers/mtd/spi/spi_flash.c
>>> @@ -65,7 +65,7 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len,
>>>         return spi_flash_read_write(spi, cmd, cmd_len, data, NULL, data_len);
>>>  }
>>>
>>> -int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
>>> +int spi_flash_cmd_write_multi(struct spi_flash *flash, u8 wr_inst, u32 offset,
>>>                 size_t len, const void *buf)
>>>  {
>>>         unsigned long page_addr, byte_addr, page_size;
>>> @@ -83,7 +83,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
>>>                 return ret;
>>>         }
>>>
>>> -       cmd[0] = CMD_PAGE_PROGRAM;
>>> +       cmd[0] = wr_inst;
>>>         for (actual = 0; actual < len; actual += chunk_len) {
>>>                 chunk_len = min(len - actual, page_size - byte_addr);
>>>
>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>> index 15c7ac4..0d416b3 100644
>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>> @@ -57,7 +57,7 @@ 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.
>>>   */
>>> -int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
>>> +int spi_flash_cmd_write_multi(struct spi_flash *flash, u8 wr_inst, u32 offset,
>>>                 size_t len, const void *buf);
>>>
>>>  /*
>>> diff --git a/include/spi_flash.h b/include/spi_flash.h
>>> index 9da9062..9b3a6a1 100644
>>> --- a/include/spi_flash.h
>>> +++ b/include/spi_flash.h
>>> @@ -41,8 +41,8 @@ struct spi_flash {
>>>
>>>         int             (*read)(struct spi_flash *flash, u32 offset,
>>>                                 size_t len, void *buf);
>>> -       int             (*write)(struct spi_flash *flash, u32 offset,
>>> -                               size_t len, const void *buf);
>>> +       int             (*write)(struct spi_flash *flash, u8 wr_inst,
>>> +                               u32 offset, size_t len, const void *buf);
>>
>> I think wr_inst can just be int? Does it need to be u8? This can
>> increase code size.
>
> wr_inst is single byte command instruction that why I have used u8.

Sure - but for function arguments it is often better to use native
type sizes (reduces code size). Still in this case I doubt it matters.

>
>>
>>>         int             (*erase)(struct spi_flash *flash, u32 offset,
>>>                                 size_t len);
>>>  };
>>> @@ -57,10 +57,10 @@ static inline int spi_flash_read(struct spi_flash *flash, u32 offset,
>>>         return flash->read(flash, offset, len, buf);
>>>  }
>>>
>>> -static inline int spi_flash_write(struct spi_flash *flash, u32 offset,
>>> -               size_t len, const void *buf)
>>> +static inline int spi_flash_write(struct spi_flash *flash, u8 wr_inst,
>>> +               u32 offset, size_t len, const void *buf)
>>>  {
>>> -       return flash->write(flash, offset, len, buf);
>>> +       return flash->write(flash, wr_inst, offset, len, buf);
>>>  }
>>>
>>>  static inline int spi_flash_erase(struct spi_flash *flash, u32 offset,
>>> diff --git a/include/spi_flash_inst.h b/include/spi_flash_inst.h
>>> new file mode 100644
>>> index 0000000..139f45b
>>> --- /dev/null
>>> +++ b/include/spi_flash_inst.h
>>> @@ -0,0 +1,30 @@
>>> +/*
>>> + * (C) Copyright 2012
>>> + * Jagannadha Sutradharudu Teki <jagannadh.teki at gmail.com>
>>> + *
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#ifndef _SPI_FLASH_INST_H_
>>
>> Any reason not to put this into spi_flash_internal.h?
>
> I thought header file from drivers/mtd/spi/spi_flash_internal.h
> couldn't be use directly in non driver directory area.
> am I correct?

Yes.

>
> this is the reason why I have newly added new header file in include
> folder for using it on common folder area.

Then perhaps just put this stuff in include/spi_flash.h? Are you
wanting to avoid it being available to other drivers?

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>>> +#define _SPI_FLASH_INST_H_
>>> +
>>> +/* Write commands */
>>> +#define CMD_PAGE_PROGRAM               0x02
>>> +
>>> +#endif /* _SPI_FLASH_INST_H_ */
>>> --
>>> 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