sf: API for spi_flash_get_sector_size

U-Boot New Richard Retanubun 4 participants 17 messages Feb 8, 2011 – Apr 12, 2011

Subsystems (3)

Memory technology devices (MTD) Arm STM32 Arm/Microchip (AT91) SoC support

lore

Share & tools

lore

Richard Retanubun

author

2011-02-08 14:13 UTC #0 lore raw

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

Wolfgang Denk

2011-02-08 14:40 UTC #1 lore raw

Dear Richard Retanubun,

Quoted 3 lines · In message <[email protected]> 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

Signature · 5 lines

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)

Richard Retanubun

author

2011-02-08 15:11 UTC #2 lore raw

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

Thanks for the speedy response Wolfgang,

Quoted 10 lines

Dear Richard Retanubun,
Quoted 3 lines · In message<[email protected]> 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

Quoted 5 lines

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

Wolfgang Denk

2011-02-08 15:19 UTC #3 lore raw

Dear Richard Retanubun,

Quoted 3 lines · In message <[email protected]> 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 ;-)

Quoted 3 lines

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.

Quoted 11 lines

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

Quoted 2 lines

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

Signature · 6 lines

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.

Richard Retanubun

author

2011-02-08 15:33 UTC #4 lore raw

Quoted 7 lines · Wolfgang Denk

Dear Richard Retanubun,
Quoted 3 lines · In message<[email protected]> 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?

Quoted 7 lines

The code needs to know that, you don't ;-)
Quoted 3 lines
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?

Quoted 13 lines

Quoted 4 lines
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 8 lines
<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>

Quoted 4 lines

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.

Quoted 11 lines

Quoted 3 lines
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

Richard Retanubun

author

2011-02-09 21:16 UTC #5 lore raw

RFC PATCH 1/1 SF: Add "sf erase offset +len" command handler.

In reply to Wolfgang Denk

Mike Frysinger

2011-02-15 09:34 UTC #6 lore raw

RFC PATCH 1/1 SF: Add "sf erase offset +len" command handler.

Quoted 3 lines · 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 ;).

Quoted 5 lines

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

Quoted 1 line

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

constify arg

Review

Quoted 2 lines

(quoted diff) +2 -0
+
+

one new line only

Review

Quoted 1 line

(quoted diff) +1 -0
+	if (arg && *arg == '+'){

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

Review

Quoted 5 lines

(quoted diff) +5 -0

+	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

Review

Quoted 9 lines

(quoted diff) +9 -0

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

pretty much all these braces can be punted

Review static int do_spi_flash_erase(int argc, char * const · line 210

Quoted 8 lines

(quoted diff) +1 -0

@@ -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)".

drivers/mtd/spi/macronix.c

Quoted 2 lines

drivers/mtd/spi/macronix.c +0 -0

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

include/spi_flash.h struct spi_flash { · line 41

Quoted 7 lines

include/spi_flash.h +1 -0

@@ -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.

include/spi_flash.h static inline int spi_flash_erase(struct spi_flash · line 72

Quoted 6 lines

include/spi_flash.h +0 -1

@@ -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

Richard Retanubun

author

2011-02-16 20:27 UTC #7 lore raw

[U-Boot] (no subject)

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.

Mike Frysinger

2011-02-17 05:46 UTC #8 lore raw

[U-Boot] (no subject)

Quoted 3 lines · 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

Richard Retanubun

author

2011-02-16 20:27 UTC #9 lore raw

PATCH 1/2 SPI: Add struct spi_flash.sector_size parameter

In reply to Mike Frysinger

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(-)

drivers/mtd/spi/atmel.c +1 -0

index a9910b1..180a52b 100644
@@ -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);

drivers/mtd/spi/macronix.c +1 -0

index 4155d4d..4a8e17f 100644
@@ -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);

drivers/mtd/spi/spansion.c +2 -2

index d54a5fa..c88457b 100644
@@ -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);

drivers/mtd/spi/sst.c +2 -1

index 792d04d..15de12b 100644
@@ -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);

drivers/mtd/spi/stmicro.c +2 -2

index 7ef690d..80d838e 100644
@@ -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);

drivers/mtd/spi/winbond.c +3 -2

index e88802f..ec6fb79 100644
@@ -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);

include/spi_flash.h +2 -0

index 1f8ba29..039b94e 100644
@@ -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,

Signature · 1 line

1.7.2.3

Wolfgang Denk

2011-02-16 21:00 UTC #10 lore raw

PATCH 1/2 SPI: Add struct spi_flash.sector_size parameter

Dear Richard Retanubun,

Quoted 5 lines · In message <[email protected]>…

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.

Quoted 9 lines

---
 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

Signature · 4 lines

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

Richard Retanubun

author

2011-02-16 20:27 UTC #11 lore raw

PATCH 2/2 cmd_sf: Add handler for +len arg for erase command.

In reply to Mike Frysinger

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(-)

common/cmd_sf.c +49 -4

index 6e7be81..bbd4842 100644
@@ -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"
 );

Signature · 1 line

1.7.2.3

Wolfgang Denk

2011-02-16 21:06 UTC #12 lore raw

PATCH 2/2 cmd_sf: Add handler for +len arg for erase command.

Dear Richard Retanubun,

Quoted 6 lines · In message <[email protected]>…

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)

common/cmd_sf.c line 47

Quoted 1 line

common/cmd_sf.c +1 -0
+	if (*arg == '+'){

ERROR: space required before the open brace '{'

common/cmd_sf.c static int do_spi_flash_erase(int argc, char * const argv[]) · line 184

Quoted 3 lines

common/cmd_sf.c +2 -0

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

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

Best regards,

Wolfgang Denk

Signature · 6 lines

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_

Richard Retanubun

author

2011-02-16 21:37 UTC #13 lore raw

PATCH 1/2 SPI: Add struct spi_flash.sector_size parameter

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 <[email protected]>

---
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(-)

drivers/mtd/spi/atmel.c +1 -0

index a9910b1..180a52b 100644
@@ -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);

drivers/mtd/spi/macronix.c +1 -0

index 4155d4d..4a8e17f 100644
@@ -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);

drivers/mtd/spi/spansion.c +2 -2

index d54a5fa..c88457b 100644
@@ -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);

drivers/mtd/spi/sst.c +2 -1

index 792d04d..15de12b 100644
@@ -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);

drivers/mtd/spi/stmicro.c +2 -2

index 7ef690d..80d838e 100644
@@ -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);

drivers/mtd/spi/winbond.c +3 -2

index e88802f..ec6fb79 100644
@@ -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);

include/spi_flash.h +2 -0

index 1f8ba29..039b94e 100644
@@ -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,

Signature · 1 line

1.7.2.3

Mike Frysinger

2011-04-12 06:34 UTC #14 lore raw

PATCH 1/2 SPI: Add struct spi_flash.sector_size parameter

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

Richard Retanubun

author

2011-02-16 21:39 UTC #15 lore raw

PATCH 2/2 cmd_sf: Add handler for +len arg for erase command.

In reply to Wolfgang Denk

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 <[email protected]>

---
v2: scrubbed via checkpatch, thanks WD!

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

common/cmd_sf.c +48 -4

index 6e7be81..17a4dd8 100644
@@ -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"
 );

Signature · 1 line

1.7.2.3

Scott Wood

2011-02-16 21:47 UTC #16 lore raw

PATCH 2/2 cmd_sf: Add handler for +len arg for erase command.

In reply to Richard Retanubun

Quoted 14 lines · On Wed, 16 Feb 2011 15:27:54 -0500 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(-)
common/cmd_sf.c +0 -0
index 6e7be81..bbd4842 100644
@@ -19,6 +19,48 @@
 
 static struct spi_flash *flash;

common/cmd_sf.c line 30

Quoted 10 lines

common/cmd_sf.c +9 -0

 
+
+/*
+ * 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