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

Simon Glass sjg at chromium.org
Fri Jan 11 03:11:59 CET 2013


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

>
> 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

> +                                       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

> +       "                                 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.

>         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?

> +#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