[U-Boot] [PATCH] Fixing spi erase for S25FL128P_256K

[U-Boot] [PATCH] Fixing spi erase for S25FL128P_256K

[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 <marc-andre.hebert@humanware.com>
---
 drivers/mtd/spi/spansion.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
index d6c1a5f..94489af 100644
--- a/drivers/mtd/spi/spansion.c
+++ b/drivers/mtd/spi/spansion.c
@@ -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;
-- 
1.6.0.4

[U-Boot] [PATCH] Fixing spi erase for S25FL128P_256K

[Sergei Shtylyov]

Hello.

Marc-Andr? H?bert wrote:

> 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 <marc-andre.hebert@humanware.com>
> ---
>  drivers/mtd/spi/spansion.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)

> diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
> index d6c1a5f..94489af 100644
> --- a/drivers/mtd/spi/spansion.c
> +++ b/drivers/mtd/spi/spansion.c
[...]
> @@ -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

[U-Boot] [PATCH v2] Fixing spi erase for S25FL128P_256K

[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 <marc-andre.hebert@humanware.com>
---
 drivers/mtd/spi/spansion.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c
index d6c1a5f..94489af 100644
--- a/drivers/mtd/spi/spansion.c
+++ b/drivers/mtd/spi/spansion.c
@@ -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;
-- 
1.6.0.4

[U-Boot] [PATCH v2] Fixing spi erase for S25FL128P_256K

[Mike Frysinger]

2010/8/10 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.

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

> --- a/drivers/mtd/spi/spansion.c
> +++ b/drivers/mtd/spi/spansion.c
> @@ -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

[U-Boot] [PATCH v2] Fixing spi erase for S25FL128P_256K

[Marc-André Hébert]

2010/8/10 Mike Frysinger <vapier@gentoo.org>:
> 2010/8/10 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.
>
> ugh, this is why the current sf framework annoys me. ?so much
> duplication across drivers including bugs.
>
>> --- a/drivers/mtd/spi/spansion.c
>> +++ b/drivers/mtd/spi/spansion.c
>> @@ -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
>
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

[U-Boot] [PATCH v2] Fixing spi erase for S25FL128P_256K

[Mike Frysinger]

2010/8/11 Marc-Andr? H?bert :
> 2010/8/10 Mike Frysinger:
>> 2010/8/10 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.
>>
>> ugh, this is why the current sf framework annoys me. ?so much
>> duplication across drivers including bugs.
>>
>>> --- a/drivers/mtd/spi/spansion.c
>>> +++ b/drivers/mtd/spi/spansion.c
>>> @@ -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;
>
> 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

[U-Boot] [PATCH] Fixing spi erase for S25FL128P_256K

[Mike Frysinger]

On Tuesday, August 10, 2010 09:02:09 Marc-Andr? H?bert wrote:
> @@ -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 

[U-Boot] [PATCH] Fixing spi erase for S25FL128P_256K

[Mike Frysinger]

On Monday, August 23, 2010 17:37:44 Mike Frysinger wrote:
> On Tuesday, August 10, 2010 09:02:09 Marc-Andr? H?bert wrote:
> > @@ -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