[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[Richard Retanubun]

Hello,

I am working on hooks to build into my board to allow for SPI flash (STMicro M25P40 and M25P16) reflash.
When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing
what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params?

There are things like the "to_stmicro_spi_flash" #define in stmicro.c, but that is private.

Will adding this API be okay?

int spi_flash_get_sector_size(struct spi_flash *sf) that will simply return the sector_size number or an errno.

thanks for everyone's time

- Richard Retanubun

[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[Wolfgang Denk]

Dear Richard Retanubun,

In message <4D514F6E.6040101@RuggedCom.com> you wrote:
> 
> When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing
> what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params?

Why would you need to know the sector size?

To align your erase command accordingly?  I think you are taking the
wrong approach here.  Look at how NOR flash is handled: we don't need
to know the sector sizes there either.  Instead we support the format
"+N" (often used as "+${fileszize}") which will _internally_ round up
N to match the next erase block boundary.

Do the same for SPI flash.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first thing we do is kill all the lawyers.
(Shakespeare. II Henry VI, Act IV, scene ii)

[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[Richard Retanubun]

On 02/08/11 09:40, Wolfgang Denk wrote:

Thanks for the speedy response Wolfgang,

> Dear Richard Retanubun,
>
> In message<4D514F6E.6040101@RuggedCom.com>  you wrote:
>>
>> When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing
>> what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params?
>
> Why would you need to know the sector size?
>
> To align your erase command accordingly?
Yep.

I think you are taking the wrong approach here.  Look at how NOR flash is handled: we don't need
> to know the sector sizes there either.  Instead we support the format
> "+N" (often used as "+${fileszize}") which will _internally_ round up
> N to match the next erase block boundary.
>
> Do the same for SPI flash.

If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this
but I need to know what number to round it to, no?

In the context of cmd_flash, it has access to flash_info_t
which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size()
that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify.


To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size.
Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check:

<quoted_code>
sector_size = stm->params->page_size * stm->params->pages_per_sector;

if (offset % sector_size || len % sector_size) {
	debug("SF: Erase offset/length not multiple of sector size\n");
	return -1;
}
</quoted_code>

I am worried about unintentionally erasing more than what the caller requested.
Would it be better to round up manually and then calls the erase?

Thanks for your time

- Richard

[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[Wolfgang Denk]

Dear Richard Retanubun,

In message <4D515D06.7020209@RuggedCom.com> you wrote:
> 
> If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this
> but I need to know what number to round it to, no?

The code needs to know that, you don't ;-)

> In the context of cmd_flash, it has access to flash_info_t
> which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size()
> that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify.

You are right.

> To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size.
> Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check:
> 
> <quoted_code>
> sector_size = stm->params->page_size * stm->params->pages_per_sector;
> 
> if (offset % sector_size || len % sector_size) {
> 	debug("SF: Erase offset/length not multiple of sector size\n");
> 	return -1;
> }
> </quoted_code>

This is ok if an explicit size is given - you will se ethe same
behaviour on NOR flash when trying something like

	erase 40000000 40002345

> I am worried about unintentionally erasing more than what the caller requested.
> Would it be better to round up manually and then calls the erase?

No.  If the caller uses the "+<size>" notation he explictly requests
to round up.  He is supposed to know what he is doing.

Being able to use "+${filesize}" allows for many powerful solutions
to standard tasks that without this would result in cumbersome
manipulation of the size - which would then probably break if you
issue a new revision of your hardware with different flash chips fit.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If you'll excuse me a minute, I'm going to have a cup of coffee."
- broadcast from Apollo 11's LEM, "Eagle", to Johnson  Space  Center,
Houston July 20, 1969, 7:27 P.M.

[U-Boot] [U-boot] sf: API for spi_flash_get_sector_size

[Richard Retanubun]

On 02/08/11 10:19, Wolfgang Denk wrote:
> Dear Richard Retanubun,
>
> In message<4D515D06.7020209@RuggedCom.com>  you wrote:
>>
>> If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this
>> but I need to know what number to round it to, no?
>
> The code needs to know that, you don't ;-)
>
>> In the context of cmd_flash, it has access to flash_info_t
>> which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size()
>> that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify.
>
> You are right.

<sensing a wavering in your resolve> ;-)

Thus we come to my original question, in the same way that cfi_flash.c provides flash_sector_size();

I propose we add spi_flash.c::spi_flash_sector_size() which returns the same information so that other code
can validate (or round up if requested) the length of the operation to the nearest sector size.

right now (unless I misunderstood), there is no easy way to get the spi flash sector_size, no?

would this be acceptable?

>
>> To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size.
>> Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check:
>>
>> <quoted_code>
>> sector_size = stm->params->page_size * stm->params->pages_per_sector;
>>
>> if (offset % sector_size || len % sector_size) {
>> 	debug("SF: Erase offset/length not multiple of sector size\n");
>> 	return -1;
>> }
>> </quoted_code>
>
> This is ok if an explicit size is given - you will see the same
> behaviour on NOR flash when trying something like
>
> 	erase 40000000 40002345

Understood. So the spi flash driver erase function itself should only validate the args and not auto round up.
This means that the decision of [warn or auto-round-up] should be handled by other code that parses the user's commands
that is operating on spi flash.

To do this, they need a way to ask the spi flash driver what the sector size to round up to is.

>
>> I am worried about unintentionally erasing more than what the caller requested.
>> Would it be better to round up manually and then calls the erase?
>
> No.  If the caller uses the "+<size>" notation he explictly requests
> to round up.  He is supposed to know what he is doing.
>
> Being able to use "+${filesize}" allows for many powerful solutions
> to standard tasks that without this would result in cumbersome
> manipulation of the size - which would then probably break if you
> issue a new revision of your hardware with different flash chips fit.

This is a cool convention. Should we add this to "sf erase command?" if it is not there already.
if so, what command (or better yet, git commit) shows how to add handling of "+N"

Thanks!

- Richard

[U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler.

[Richard Retanubun]

[U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler.

[Mike Frysinger]

On Wednesday, February 09, 2011 16:16:12 Richard Retanubun wrote:
> From hints by Wolfgang, this patch adds the ability to handle +len
> argument for spi flash erase, which will round up the length to the
> nearest [sector|page|block]_size.

this should be split up into two patches.  one that unifies the erase sizes 
and one that modifies cmd_sf.c to use the new field.

ive already mostly unified the erase functions here:
	git://git.denx.de/u-boot-blackfin.git sf

but the one piece missing is what you're proposing.  so i'll want to merge the 
unification part you have here into that patch.  if you could test out that sf 
branch now to see if it works for you, that'd be nice ;).

> This is done by adding a new member to
> struct spi_flash::u32 block_size
> 
> The name 'block_size' is chosen to mean:
> "the smallest unit erase commands will check against".

let's use "sector_size" as this is what Linux already uses

> +static int sf_parse_len_arg(char *arg, ulong *len)

constify arg

> +
> +

one new line only

> +	if (arg && *arg == '+'){

NULL check is useless as the caller already took care of it

> +	if (round_up_len) {
> +		/* Get sector size from flash and round up */
> +		sector_size = 	flash->block_size;
> +		if (sector_size > 0) {
> +			*len = ((((len_arg -1)/sector_size) + 1) * sector_size);

we have a DIV_ROUND_UP macro already

> +			if (*len > flash->size) {
> +				return -1;
> +			}
> +		} else {
> +			return -1;
> +		}
> +	} else {
> +		*len = len_arg;
> +	}

pretty much all these braces can be punted

> @@ -149,6 +204,7 @@ static int do_spi_flash_erase(int argc, char * const
> argv[])
> 
>  usage:
>  	puts("Usage: sf erase offset len\n");
> +	puts("       sf erase offset +len\n");
>  	return 1;
>  }

hrm, a sep patch should be written and sent out before yours that drops this 
"usage" label and converts all "goto usage" to "return cmd_usage(cmdtp)".

> --- a/drivers/mtd/spi/macronix.c
> +++ b/drivers/mtd/spi/macronix.c

i'll ignore all the spi flash changes per my earlier highlight of rewrites 
pending in this area.

> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -38,6 +38,8 @@ struct spi_flash {
> 
>  	u32		size;
> 
> +	u32		block_size;

not sure what it'll do to code size, but a u16 should be large enough to hold 
the base erase size.

> @@ -67,5 +69,4 @@ static inline int spi_flash_erase(struct spi_flash
> *flash, u32 offset, {
>  	return flash->erase(flash, offset, len);
>  }
> -
>  #endif /* _SPI_FLASH_H_ */

please avoid useless whitespace changes
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110215/a24def07/attachment.pgp 

[U-Boot] (no subject)

[Richard Retanubun]

Hi Mike,

Thanks for the feedback, and for the cool unification.
Here is the updated patch that implements your comments and is
based on the head of the blackfin.git sf branch.

[U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter

[Richard Retanubun]

This patch adds a new member to struct spi_flash (u16 sector_size)
and updates the spi flash drivers to start populating it.

This parameter can be used by spi flash commands that need to round
up units of operation to the flash's sector_size.
---
 drivers/mtd/spi/atmel.c    |    1 +
 drivers/mtd/spi/macronix.c |    1 +
 drivers/mtd/spi/spansion.c |    4 ++--
 drivers/mtd/spi/sst.c      |    3 ++-
 drivers/mtd/spi/stmicro.c  |    4 ++--
 drivers/mtd/spi/winbond.c  |    5 +++--
 include/spi_flash.h        |    2 ++
 7 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c
index a9910b1..180a52b 100644
--- a/drivers/mtd/spi/atmel.c
+++ b/drivers/mtd/spi/atmel.c
@@ -498,6 +498,7 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode)
 	asf->flash.size = page_size * params->pages_per_block
 				* params->blocks_per_sector
 				* params->nr_sectors;
+	asf->flash.sector_size = page_size;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, page_size);
diff --git a/drivers/mtd/spi/macronix.c b/drivers/mtd/spi/macronix.c
index 4155d4d..4a8e17f 100644
--- a/drivers/mtd/spi/macronix.c
+++ b/drivers/mtd/spi/macronix.c
@@ -217,6 +217,7 @@ struct spi_flash *spi_flash_probe_macronix(struct spi_slave *spi, u8 *idcode)
 	mcx->flash.read = spi_flash_cmd_read_fast;
 	mcx->flash.size = params->page_size * params->pages_per_sector
 	    * params->sectors_per_block * params->nr_blocks;
+	mcx->flash.sector_size = mcx->flash.size/params->nr_blocks;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
index d54a5fa..c88457b 100644
--- a/drivers/mtd/spi/spansion.c
+++ b/drivers/mtd/spi/spansion.c
@@ -199,8 +199,7 @@ static int spansion_write(struct spi_flash *flash,
 int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct spansion_spi_flash *spsn = to_spansion_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE,
-		spsn->params->page_size * spsn->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -242,6 +241,7 @@ struct spi_flash *spi_flash_probe_spansion(struct spi_slave *spi, u8 *idcode)
 	spsn->flash.read = spi_flash_cmd_read_fast;
 	spsn->flash.size = params->page_size * params->pages_per_sector
 	    * params->nr_sectors;
+	spsn->flash.sector_size = spsn->flash.size/params->nr_sectors;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/sst.c b/drivers/mtd/spi/sst.c
index 792d04d..15de12b 100644
--- a/drivers/mtd/spi/sst.c
+++ b/drivers/mtd/spi/sst.c
@@ -201,7 +201,7 @@ sst_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf)
 
 int sst_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
-	return spi_flash_cmd_erase(flash, CMD_SST_SE, SST_SECTOR_SIZE,
+	return spi_flash_cmd_erase(flash, CMD_SST_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -257,6 +257,7 @@ spi_flash_probe_sst(struct spi_slave *spi, u8 *idcode)
 	stm->flash.write = sst_write;
 	stm->flash.erase = sst_erase;
 	stm->flash.size = SST_SECTOR_SIZE * params->nr_sectors;
+	stm->flash.sector_size = SST_SECTOR_SIZE;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, SST_SECTOR_SIZE);
diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index 7ef690d..80d838e 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -200,8 +200,7 @@ static int stmicro_write(struct spi_flash *flash,
 int stmicro_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct stmicro_spi_flash *stm = to_stmicro_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_M25PXX_SE,
-		stm->params->page_size * stm->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -251,6 +250,7 @@ struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
 	stm->flash.read = spi_flash_cmd_read_fast;
 	stm->flash.size = params->page_size * params->pages_per_sector
 	    * params->nr_sectors;
+	stm->flash.sector_size = stm->flash.size/params->nr_sectors;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/winbond.c b/drivers/mtd/spi/winbond.c
index e88802f..ec6fb79 100644
--- a/drivers/mtd/spi/winbond.c
+++ b/drivers/mtd/spi/winbond.c
@@ -173,8 +173,7 @@ out:
 int winbond_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct winbond_spi_flash *stm = to_winbond_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_W25_SE,
-		(1 << stm->params->l2_page_size) * stm->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_W25_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -216,6 +215,8 @@ struct spi_flash *spi_flash_probe_winbond(struct spi_slave *spi, u8 *idcode)
 	stm->flash.size = page_size * params->pages_per_sector
 				* params->sectors_per_block
 				* params->nr_blocks;
+	stm->flash.sector_size = (1 << stm->params->l2_page_size) *
+		stm->params->pages_per_sector;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, page_size);
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 1f8ba29..039b94e 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -38,6 +38,8 @@ struct spi_flash {
 
 	u32		size;
 
+	u16		sector_size;
+
 	int		(*read)(struct spi_flash *flash, u32 offset,
 				size_t len, void *buf);
 	int		(*write)(struct spi_flash *flash, u32 offset,
-- 
1.7.2.3

[U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command.

[Richard Retanubun]

This patch adds [+]len handler for the erase command that will
automatically round up the requested erase length to the flash's
sector_size.
---
 common/cmd_sf.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 6e7be81..bbd4842 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -19,6 +19,48 @@
 
 static struct spi_flash *flash;
 
+
+/*
+ * This function computes the length argument for the erase command.
+ * The length on which the command is to operate can be given in two forms:
+ * 1. <cmd> offset len  - operate on <'offset',  'len')
+ * 2. <cmd> offset +len - operate on <'offset',  'round_up(len)')
+ * If the second form is used and the length doesn't fall on the
+ * sector boundary, than it will be adjusted to the next sector boundary.
+ * If it isn't in the flash, the function will fail (return -1).
+ * Input:
+ *    arg: length specification (i.e. both command arguments)
+ * Output:
+ *    len: computed length for operation
+ * Return:
+ *    1: success
+ *   -1: failure (bad format, bad address).
+*/
+static int sf_parse_len_arg(char *arg, ulong *len)
+{
+	char *ep;
+	char round_up_len; /* indicates if the "+length" form used */
+	ulong len_arg;
+
+
+	round_up_len = 0;
+	if (*arg == '+'){
+		round_up_len = 1;
+		++arg;
+	}
+
+	len_arg = simple_strtoul(arg, &ep, 16);
+	if (ep == arg || *ep != '\0')
+		return -1;
+
+	if (round_up_len && flash->sector_size > 0)
+		*len = DIV_ROUND_UP(len_arg, flash->sector_size);
+	else
+		*len = len_arg;
+
+	return 1;
+}
+
 static int do_spi_flash_probe(int argc, char * const argv[])
 {
 	unsigned int bus = 0;
@@ -135,9 +177,11 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	offset = simple_strtoul(argv[1], &endp, 16);
 	if (*argv[1] == 0 || *endp != 0)
 		goto usage;
-	len = simple_strtoul(argv[2], &endp, 16);
-	if (*argv[2] == 0 || *endp != 0)
+
+	ret = sf_parse_len_arg(argv[2], &len);
+	if (ret != 1) {
 		goto usage;
+	}
 
 	ret = spi_flash_erase(flash, offset, len);
 	if (ret) {
@@ -148,7 +192,7 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	return 0;
 
 usage:
-	puts("Usage: sf erase offset len\n");
+	puts("Usage: sf erase offset [+]len\n");
 	return 1;
 }
 
@@ -189,5 +233,6 @@ U_BOOT_CMD(
 	"				  `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 erase offset len		- erase `len' bytes from `offset'"
+	"sf erase offset [+]len		- erase `len' bytes from `offset'\n"
+	"				  `+len' round up `len' to block size"
 );
-- 
1.7.2.3

[U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter

[Wolfgang Denk]

Dear Richard Retanubun,

In message <1297888074-8344-2-git-send-email-RichardRetanubun@RuggedCom.com> you wrote:
> This patch adds a new member to struct spi_flash (u16 sector_size)
> and updates the spi flash drivers to start populating it.
> 
> This parameter can be used by spi flash commands that need to round
> up units of operation to the flash's sector_size.
> ---
>  drivers/mtd/spi/atmel.c    |    1 +
>  drivers/mtd/spi/macronix.c |    1 +
>  drivers/mtd/spi/spansion.c |    4 ++--
>  drivers/mtd/spi/sst.c      |    3 ++-
>  drivers/mtd/spi/stmicro.c  |    4 ++--
>  drivers/mtd/spi/winbond.c  |    5 +++--
>  include/spi_flash.h        |    2 ++
>  7 files changed, 13 insertions(+), 7 deletions(-)

Rejected - Signed-off-by: line missing.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
God made the integers; all else is the work of Man.       - Kronecker

[U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command.

[Wolfgang Denk]

Dear Richard Retanubun,

In message <1297888074-8344-3-git-send-email-RichardRetanubun@RuggedCom.com> you wrote:
> This patch adds [+]len handler for the erase command that will
> automatically round up the requested erase length to the flash's
> sector_size.
> ---
>  common/cmd_sf.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 49 insertions(+), 4 deletions(-)

Please run your patches through checkpatch - this helps avoiding the
frustration of seeing rejects due to simple issues like these:

ERROR: Missing Signed-off-by: line(s)

> +	if (*arg == '+'){

ERROR: space required before the open brace '{'

> +	if (ret != 1) {
>  		goto usage;
> +	}

WARNING: braces {} are not necessary for single statement blocks

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
All these theories, diverse as they are, have two things  in  common:
they explain the observed facts, and they are completeley and utterly
wrong.                       - Terry Pratchett, _The Light Fantastic_

[U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter

[Richard Retanubun]

This patch adds a new member to struct spi_flash (u16 sector_size)
and updates the spi flash drivers to start populating it.

This parameter can be used by spi flash commands that need to round
up units of operation to the flash's sector_size.

Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com>
---
v2: scrubbed via checkpatch, thanks WD!

 drivers/mtd/spi/atmel.c    |    1 +
 drivers/mtd/spi/macronix.c |    1 +
 drivers/mtd/spi/spansion.c |    4 ++--
 drivers/mtd/spi/sst.c      |    3 ++-
 drivers/mtd/spi/stmicro.c  |    4 ++--
 drivers/mtd/spi/winbond.c  |    5 +++--
 include/spi_flash.h        |    2 ++
 7 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c
index a9910b1..180a52b 100644
--- a/drivers/mtd/spi/atmel.c
+++ b/drivers/mtd/spi/atmel.c
@@ -498,6 +498,7 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode)
 	asf->flash.size = page_size * params->pages_per_block
 				* params->blocks_per_sector
 				* params->nr_sectors;
+	asf->flash.sector_size = page_size;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, page_size);
diff --git a/drivers/mtd/spi/macronix.c b/drivers/mtd/spi/macronix.c
index 4155d4d..4a8e17f 100644
--- a/drivers/mtd/spi/macronix.c
+++ b/drivers/mtd/spi/macronix.c
@@ -217,6 +217,7 @@ struct spi_flash *spi_flash_probe_macronix(struct spi_slave *spi, u8 *idcode)
 	mcx->flash.read = spi_flash_cmd_read_fast;
 	mcx->flash.size = params->page_size * params->pages_per_sector
 	    * params->sectors_per_block * params->nr_blocks;
+	mcx->flash.sector_size = mcx->flash.size/params->nr_blocks;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
index d54a5fa..c88457b 100644
--- a/drivers/mtd/spi/spansion.c
+++ b/drivers/mtd/spi/spansion.c
@@ -199,8 +199,7 @@ static int spansion_write(struct spi_flash *flash,
 int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct spansion_spi_flash *spsn = to_spansion_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE,
-		spsn->params->page_size * spsn->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -242,6 +241,7 @@ struct spi_flash *spi_flash_probe_spansion(struct spi_slave *spi, u8 *idcode)
 	spsn->flash.read = spi_flash_cmd_read_fast;
 	spsn->flash.size = params->page_size * params->pages_per_sector
 	    * params->nr_sectors;
+	spsn->flash.sector_size = spsn->flash.size/params->nr_sectors;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/sst.c b/drivers/mtd/spi/sst.c
index 792d04d..15de12b 100644
--- a/drivers/mtd/spi/sst.c
+++ b/drivers/mtd/spi/sst.c
@@ -201,7 +201,7 @@ sst_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf)
 
 int sst_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
-	return spi_flash_cmd_erase(flash, CMD_SST_SE, SST_SECTOR_SIZE,
+	return spi_flash_cmd_erase(flash, CMD_SST_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -257,6 +257,7 @@ spi_flash_probe_sst(struct spi_slave *spi, u8 *idcode)
 	stm->flash.write = sst_write;
 	stm->flash.erase = sst_erase;
 	stm->flash.size = SST_SECTOR_SIZE * params->nr_sectors;
+	stm->flash.sector_size = SST_SECTOR_SIZE;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, SST_SECTOR_SIZE);
diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c
index 7ef690d..80d838e 100644
--- a/drivers/mtd/spi/stmicro.c
+++ b/drivers/mtd/spi/stmicro.c
@@ -200,8 +200,7 @@ static int stmicro_write(struct spi_flash *flash,
 int stmicro_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct stmicro_spi_flash *stm = to_stmicro_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_M25PXX_SE,
-		stm->params->page_size * stm->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -251,6 +250,7 @@ struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
 	stm->flash.read = spi_flash_cmd_read_fast;
 	stm->flash.size = params->page_size * params->pages_per_sector
 	    * params->nr_sectors;
+	stm->flash.sector_size = stm->flash.size/params->nr_sectors;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, params->page_size);
diff --git a/drivers/mtd/spi/winbond.c b/drivers/mtd/spi/winbond.c
index e88802f..ec6fb79 100644
--- a/drivers/mtd/spi/winbond.c
+++ b/drivers/mtd/spi/winbond.c
@@ -173,8 +173,7 @@ out:
 int winbond_erase(struct spi_flash *flash, u32 offset, size_t len)
 {
 	struct winbond_spi_flash *stm = to_winbond_spi_flash(flash);
-	return spi_flash_cmd_erase(flash, CMD_W25_SE,
-		(1 << stm->params->l2_page_size) * stm->params->pages_per_sector,
+	return spi_flash_cmd_erase(flash, CMD_W25_SE, flash->sector_size,
 		offset, len);
 }
 
@@ -216,6 +215,8 @@ struct spi_flash *spi_flash_probe_winbond(struct spi_slave *spi, u8 *idcode)
 	stm->flash.size = page_size * params->pages_per_sector
 				* params->sectors_per_block
 				* params->nr_blocks;
+	stm->flash.sector_size = (1 << stm->params->l2_page_size) *
+		stm->params->pages_per_sector;
 
 	printf("SF: Detected %s with page size %u, total ",
 	       params->name, page_size);
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 1f8ba29..039b94e 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -38,6 +38,8 @@ struct spi_flash {
 
 	u32		size;
 
+	u16		sector_size;
+
 	int		(*read)(struct spi_flash *flash, u32 offset,
 				size_t len, void *buf);
 	int		(*write)(struct spi_flash *flash, u32 offset,
-- 
1.7.2.3

[U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command.

[Richard Retanubun]

This patch adds [+]len handler for the erase command that will
automatically round up the requested erase length to the flash's
sector_size.

Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com>
---
v2: scrubbed via checkpatch, thanks WD!

 common/cmd_sf.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 6e7be81..17a4dd8 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -19,6 +19,48 @@
 
 static struct spi_flash *flash;
 
+
+/*
+ * This function computes the length argument for the erase command.
+ * The length on which the command is to operate can be given in two forms:
+ * 1. <cmd> offset len  - operate on <'offset',  'len')
+ * 2. <cmd> offset +len - operate on <'offset',  'round_up(len)')
+ * If the second form is used and the length doesn't fall on the
+ * sector boundary, than it will be adjusted to the next sector boundary.
+ * If it isn't in the flash, the function will fail (return -1).
+ * Input:
+ *    arg: length specification (i.e. both command arguments)
+ * Output:
+ *    len: computed length for operation
+ * Return:
+ *    1: success
+ *   -1: failure (bad format, bad address).
+*/
+static int sf_parse_len_arg(char *arg, ulong *len)
+{
+	char *ep;
+	char round_up_len; /* indicates if the "+length" form used */
+	ulong len_arg;
+
+
+	round_up_len = 0;
+	if (*arg == '+') {
+		round_up_len = 1;
+		++arg;
+	}
+
+	len_arg = simple_strtoul(arg, &ep, 16);
+	if (ep == arg || *ep != '\0')
+		return -1;
+
+	if (round_up_len && flash->sector_size > 0)
+		*len = DIV_ROUND_UP(len_arg, flash->sector_size);
+	else
+		*len = len_arg;
+
+	return 1;
+}
+
 static int do_spi_flash_probe(int argc, char * const argv[])
 {
 	unsigned int bus = 0;
@@ -135,8 +177,9 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	offset = simple_strtoul(argv[1], &endp, 16);
 	if (*argv[1] == 0 || *endp != 0)
 		goto usage;
-	len = simple_strtoul(argv[2], &endp, 16);
-	if (*argv[2] == 0 || *endp != 0)
+
+	ret = sf_parse_len_arg(argv[2], &len);
+	if (ret != 1)
 		goto usage;
 
 	ret = spi_flash_erase(flash, offset, len);
@@ -148,7 +191,7 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	return 0;
 
 usage:
-	puts("Usage: sf erase offset len\n");
+	puts("Usage: sf erase offset [+]len\n");
 	return 1;
 }
 
@@ -189,5 +232,6 @@ U_BOOT_CMD(
 	"				  `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 erase offset len		- erase `len' bytes from `offset'"
+	"sf erase offset [+]len		- erase `len' bytes from `offset'\n"
+	"				  `+len' round up `len' to block size"
 );
-- 
1.7.2.3

[U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command.

[Scott Wood]

On Wed, 16 Feb 2011 15:27:54 -0500
Richard Retanubun <RichardRetanubun@ruggedcom.com> wrote:

> This patch adds [+]len handler for the erase command that will
> automatically round up the requested erase length to the flash's
> sector_size.
> ---
>  common/cmd_sf.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index 6e7be81..bbd4842 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -19,6 +19,48 @@
>  
>  static struct spi_flash *flash;
>  
> +
> +/*
> + * This function computes the length argument for the erase command.
> + * The length on which the command is to operate can be given in two forms:
> + * 1. <cmd> offset len  - operate on <'offset',  'len')
> + * 2. <cmd> offset +len - operate on <'offset',  'round_up(len)')
> + * If the second form is used and the length doesn't fall on the
> + * sector boundary, than it will be adjusted to the next sector boundary.
> + * If it isn't in the flash, the function will fail (return -1).

On NOR, + is used to indicate that the second argument is a length,
as opposed to an ending address.  Rounding seems like a side effect of
length mode.

On NAND we unconditionally round up erase lengths, as we don't support
ending-address mode (looks like SPI doesn't either).

-Scott

[U-Boot] (no subject)

[Mike Frysinger]

On Wednesday, February 16, 2011 15:27:52 Richard Retanubun wrote:
> Thanks for the feedback, and for the cool unification.
> Here is the updated patch that implements your comments and is
> based on the head of the blackfin.git sf branch.

thanks !  i'll try to get to it this weekend, but no promises as i currently 
have non-u-boot stuff pressing for my time.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110217/6b1f2553/attachment.pgp 

[U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter

[Mike Frysinger]

these patches have been integrated/superseded by my recent sf patchset
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110412/4005fbe8/attachment.pgp