[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