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

login
register
mail settings
Submitter Jagannadha Sutradharudu Teki
Date Dec. 31, 2012, 11:13 a.m.
Message ID <1356952428-19824-2-git-send-email-jagannadh.teki@gmail.com>
Download mbox | patch
Permalink /patch/208831/
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Comments

Jagannadha Sutradharudu Teki - Dec. 31, 2012, 11:13 a.m.
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).

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
Simon Glass - Jan. 11, 2013, 2:11 a.m.
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
Simon Glass - Feb. 14, 2013, 2:35 p.m.
Hi Jagan,

On Wed, Jan 16, 2013 at 2:52 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jan 11, 2013 at 7:41 AM, Simon Glass <sjg@chromium.org> wrote:
>> 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
>
> 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@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@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

Patch

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",
+					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"
+	"				- write `len' bytes from memory\n"
+	"				  at `addr' to flash at `offset' using\n"
+	"				  pp `wr_inst' write instruction\n"
+	"				  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);
 	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_
+#define _SPI_FLASH_INST_H_
+
+/* Write commands */
+#define CMD_PAGE_PROGRAM		0x02
+
+#endif /* _SPI_FLASH_INST_H_ */