Hi Jagan, On Wed, Jan 16, 2013 at 2:52 AM, Jagan Teki <jagannadh.t...@gmail.com> wrote: > Hi Simon, > > On Fri, Jan 11, 2013 at 7:41 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Jagannadha, >> >> On Mon, Dec 31, 2012 at 3:13 AM, Jagannadha Sutradharudu Teki >> <jagannadh.t...@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.t...@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.t...@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@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