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

Hi Jagannadha,

On Mon, Dec 31, 2012 at 3:13 AM, Jagannadha Sutradharudu Teki jagannadh.teki@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@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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon