Fixing spi erase for S25FL128P_256K

PATCH Superseded Marc-André Hébert 3 participants 8 messages Aug 10, 2010 – Aug 23, 2010 Memory technology devices (MTD)

lore

Share & tools

lore

Marc-André Hébert

author

2010-08-10 13:02 UTC #0 lore raw

PATCH 1/1 Fixing spi erase for S25FL128P_256K

The spansion_erase currently only works when the sector size is 64KB. cmd[1] should contain the higher 8 bit of the 24 bit address of the sector to be erased. Currently it is holding the sector index to be erased which happens to be the same thing when the sector size is 64KB.

Signed-off-by Marc-Andre Hebert <[email protected]>

---
 drivers/mtd/spi/spansion.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

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

index d6c1a5f..94489af 100644
@@ -262,7 +262,6 @@ int spansion_erase(struct spi_flash *flash, u32

offset, size_t len)
return -1;
}

- len /= sector_size;
cmd[0] = CMD_S25FLXX_SE;
cmd[2] = 0x00;
cmd[3] = 0x00;
@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
}

ret = 0;
- for (actual = 0; actual < len; actual++) {
- cmd[1] = (offset / sector_size) + actual;
+ for (actual = 0; actual < len; actual+=sector_size) {
+ cmd[1] = (offset + actual) >> 16;

ret = spi_flash_cmd(flash->spi, CMD_S25FLXX_WREN, NULL, 0); if (ret < 0) {
@@ -298,7 +297,7 @@ int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
}

debug("SF: SPANSION: Successfully erased %u bytes @ 0x%x\n",
- len * sector_size, offset);
+ len, offset);

spi_release_bus(flash->spi);
return ret;

Signature · 1 line

1.6.0.4

Sergei Shtylyov

2010-08-10 13:14 UTC #1 lore raw

PATCH 1/1 Fixing spi erase for S25FL128P_256K

Hello.

Quoted 15 lines · Marc-Andr? H?bert

The spansion_erase currently only works when the sector size is 64KB. cmd[1] should contain the higher 8 bit of the 24 bit address of the sector to be erased. Currently it is holding the sector index to be erased which happens to be the same thing when the sector size is 64KB.
Signed-off-by Marc-Andre Hebert <[email protected]>
---
 drivers/mtd/spi/spansion.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/mtd/spi/spansion.c +0 -0
index d6c1a5f..94489af 100644

[...]

drivers/mtd/spi/spansion.c int spansion_erase(struct spi_flash *flash, u32 · line 277

Quoted 8 lines

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

@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 	}
 
 	ret = 0;
-	for (actual = 0; actual < len; actual++) {
-		cmd[1] = (offset / sector_size) + actual;
+	for (actual = 0; actual < len; actual+=sector_size) {

    Keep the  coding style consistent please -- add spaces around +=.

WBR, Sergei

Marc-André Hébert

author

2010-08-10 13:35 UTC #2 lore raw

PATCH v2 1/1 Fixing spi erase for S25FL128P_256K

Signed-off-by Marc-Andre Hebert <[email protected]>

---
 drivers/mtd/spi/spansion.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

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

index d6c1a5f..94489af 100644
@@ -262,7 +262,6 @@ int spansion_erase(struct spi_flash *flash, u32

offset, size_t len)
return -1;
}

- len /= sector_size;
cmd[0] = CMD_S25FLXX_SE;
cmd[2] = 0x00;
cmd[3] = 0x00;
@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
}

ret = 0;
- for (actual = 0; actual < len; actual++) {
- cmd[1] = (offset / sector_size) + actual;
+ for (actual = 0; actual < len; actual += sector_size) {
+ cmd[1] = (offset + actual) >> 16;

ret = spi_flash_cmd(flash->spi, CMD_S25FLXX_WREN, NULL, 0); if (ret < 0) {
@@ -298,7 +297,7 @@ int spansion_erase(struct spi_flash *flash, u32 offset, size_t len)
}

debug("SF: SPANSION: Successfully erased %u bytes @ 0x%x\n",
- len * sector_size, offset);
+ len, offset);

spi_release_bus(flash->spi);
return ret;

Signature · 1 line

1.6.0.4

Mike Frysinger

2010-08-10 20:55 UTC #3 lore raw

PATCH v2 1/1 Fixing spi erase for S25FL128P_256K

2010/8/10 Marc-Andr? H?bert:

Quoted 5 lines

The spansion_erase currently only works when the sector size is 64KB. cmd[1] should contain the higher 8 bit of the 24 bit address of the sector to be erased. Currently it is holding the sector index to be erased which happens to be the same thing when the sector size is 64KB.

ugh, this is why the current sf framework annoys me. so much duplication across drivers including bugs.

drivers/mtd/spi/spansion.c int spansion_erase(struct spi_flash *flash, u32 · line 278

Quoted 11 lines

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

@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 ? ? ? ?}
 
 ? ? ? ?ret = 0;
- ? ? ? for (actual = 0; actual < len; actual++) {
- ? ? ? ? ? ? ? cmd[1] = (offset / sector_size) + actual;
+ ? ? ? for (actual = 0; actual < len; actual += sector_size) {
+ ? ? ? ? ? ? ? cmd[1] = (offset + actual) >> 16;

how about the bug fix that went into stmicro:

-       cmd[1] = (offset / sector_size) + actual;
+       cmd[1] = offset >> 16;
+       offset += sector_size;

-mike

Marc-André Hébert

author

2010-08-11 11:23 UTC #4 lore raw

PATCH v2 1/1 Fixing spi erase for S25FL128P_256K

2010/8/10 Mike Frysinger <[email protected]>:

Quoted 16 lines

2010/8/10 Marc-Andr? H?bert:
Quoted 5 lines
The spansion_erase currently only works when the sector size is 64KB. cmd[1] should contain the higher 8 bit of the 24 bit address of the sector to be erased. Currently it is holding the sector index to be erased which happens to be the same thing when the sector size is 64KB.
ugh, this is why the current sf framework annoys me. ?so much duplication across drivers including bugs.
Quoted 6 lines
drivers/mtd/spi/spansion.c +0 -0
@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 ? ? ? ?}
 

Quoted 12 lines

drivers/mtd/spi/spansion.c int spansion_erase(struct spi_flash *flash, u32 · line 278
Quoted 5 lines
drivers/mtd/spi/spansion.c +2 -2
 ? ? ? ?ret = 0;
- ? ? ? for (actual = 0; actual < len; actual++) {
- ? ? ? ? ? ? ? cmd[1] = (offset / sector_size) + actual;
+ ? ? ? for (actual = 0; actual < len; actual += sector_size) {
+ ? ? ? ? ? ? ? cmd[1] = (offset + actual) >> 16;
how about the bug fix that went into stmicro:
- ? ? ? cmd[1] = (offset / sector_size) + actual;
+ ? ? ? cmd[1] = offset >> 16;
+ ? ? ? offset += sector_size;
-mike

Yeah I saw how stmicro did it, but I didn't do it that way simply because modifying the offset invalidates the debug statement afterwards:

debug("SF: SPANSION: Successfully erased %u bytes @ 0x%x\n",

	      len, offset);

Marc

Mike Frysinger

2010-08-11 17:01 UTC #5 lore raw

PATCH v2 1/1 Fixing spi erase for S25FL128P_256K

2010/8/11 Marc-Andr? H?bert :

Quoted 28 lines

2010/8/10 Mike Frysinger:
Quoted 16 lines
2010/8/10 Marc-Andr? H?bert:
Quoted 5 lines
The spansion_erase currently only works when the sector size is 64KB. cmd[1] should contain the higher 8 bit of the 24 bit address of the sector to be erased. Currently it is holding the sector index to be erased which happens to be the same thing when the sector size is 64KB.
ugh, this is why the current sf framework annoys me. ?so much duplication across drivers including bugs.
Quoted 6 lines
drivers/mtd/spi/spansion.c +0 -0
@@ -274,8 +273,8 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 ? ? ? ?}
 
Quoted 10 lines
drivers/mtd/spi/spansion.c int spansion_erase(struct spi_flash *flash, u32 · line 278
Quoted 5 lines
drivers/mtd/spi/spansion.c +2 -2
 ? ? ? ?ret = 0;
- ? ? ? for (actual = 0; actual < len; actual++) {
- ? ? ? ? ? ? ? cmd[1] = (offset / sector_size) + actual;
+ ? ? ? for (actual = 0; actual < len; actual += sector_size) {
+ ? ? ? ? ? ? ? cmd[1] = (offset + actual) >> 16;
how about the bug fix that went into stmicro:
- ? ? ? cmd[1] = (offset / sector_size) + actual;
+ ? ? ? cmd[1] = offset >> 16;
+ ? ? ? offset += sector_size;

Quoted 6 lines

Yeah I saw how stmicro did it, but I didn't do it that way simply because modifying the offset invalidates the debug statement afterwards:
debug("SF: SPANSION: Successfully erased %u bytes @ 0x%x\n", ? ? ? ? ? ? ?len, offset);

and this just reinforces my point. you're basically saying the stmicro flash code has a broken debug() statement now. -mike

Mike Frysinger

2010-08-23 21:37 UTC #6 lore raw

PATCH 1/1 Fixing spi erase for S25FL128P_256K

In reply to Marc-André Hébert

Review int spansion_erase(struct spi_flash *flash, u32 · line 263

Quoted 3 lines · On Tuesday, August 10, 2010 09:02:09 Marc-Andr? H?bert wrote:

(quoted diff) +0 -0

@@ -262,7 +262,6 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 		return -1;

seems your mailer too is broken. please use git send-email when posting patches so they dont get screwed up.
-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/20100823/3e3a828e/attachment.pgp

Mike Frysinger

2010-08-23 22:08 UTC #7 lore raw

PATCH 1/1 Fixing spi erase for S25FL128P_256K

Quoted 8 lines · On Monday, August 23, 2010 17:37:44 Mike Frysinger wrote:

Review int spansion_erase(struct spi_flash *flash, u32 · line 264
Quoted 4 lines · On Tuesday, August 10, 2010 09:02:09 Marc-Andr? H?bert wrote:
(quoted diff) +0 -0
@@ -262,7 +262,6 @@ int spansion_erase(struct spi_flash *flash, u32
 offset, size_t len)
 
 		return -1;
seems your mailer too is broken. please use git send-email when posting patches so they dont get screwed up.

although i fixed up the commit and merged it manually. but you only get one pass, so next time i'll wait for a proper resend :P.

this is in my sf branch so it wont get lost
-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/20100823/ea213bad/attachment.pgp