[PATCH] flashrom: something

[Carl-Daniel Hailfinger]

This is probably the weirdest changelog ever.

OK, I forgot what this patch does, but I'm pretty sure it's justified.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-fwh/sst39sf020.c
===================================================================
--- flashrom-fwh/sst39sf020.c	(Revision 3324)
+++ flashrom-fwh/sst39sf020.c	(Arbeitskopie)
@@ -22,21 +22,6 @@
 #include <stdint.h>
 #include "flash.h"

-#define AUTO_PG_ERASE1		0x20
-#define AUTO_PG_ERASE2		0xD0
-
-static __inline__ int erase_sector_39sf020(volatile uint8_t *bios,
-					   unsigned long address)
-{
-	*bios = AUTO_PG_ERASE1;
-	*(bios + address) = AUTO_PG_ERASE2;
-
-	/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
-
-	return 0;
-}
-
 int write_39sf020(struct flashchip *flash, uint8_t *buf)
 {
    int i;
Index: flashrom-fwh/flash.h
===================================================================
--- flashrom-fwh/flash.h	(Revision 3324)
+++ flashrom-fwh/flash.h	(Arbeitskopie)
@@ -464,6 +464,8 @@
 /* sst28sf040.c */
 int probe_28sf040(struct flashchip *flash);
 int erase_28sf040(struct flashchip *flash);
+int erase_all_sectors_28sf040(struct flashchip *flash);
+int erase_sector_28sf040(volatile uint8_t *bios, unsigned long address);
 int write_28sf040(struct flashchip *flash, uint8_t *buf);

/* sst39sf020.c */
Index: flashrom-fwh/sst28sf040.c
===================================================================
--- flashrom-fwh/sst28sf040.c	(Revision 3324)
+++ flashrom-fwh/sst28sf040.c	(Arbeitskopie)
@@ -58,8 +58,7 @@
    tmp = *(volatile uint8_t *)(bios + 0x041A);
 }

-static __inline__ int erase_sector_28sf040(volatile uint8_t *bios,
-					   unsigned long address)
+int erase_sector_28sf040(volatile uint8_t *bios, unsigned long address)
 {
    *bios = AUTO_PG_ERASE1;
    *(bios + address) = AUTO_PG_ERASE2;
@@ -119,6 +118,20 @@
    return 0;
 }

+int erase_all_sectors_28sf040(struct flashchip *flash)
+{
+	int i;
+	int total_size = flash->total_size * 1024;
+	int page_size = flash->page_size;
+	volatile uint8_t *bios = flash->virtual_memory;
+
+	for (i = 0; i < total_size / page_size; i++) {
+		/* erase the page before programming */
+		erase_sector_28sf040(bios, i * page_size);
+	}
+	return 0;
+}
+
 int erase_28sf040(struct flashchip *flash)
 {
    volatile uint8_t *bios = flash->virtual_memory;

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> This is probably the weirdest changelog ever.
>
> OK, I forgot what this patch does, but I'm pretty sure it's justified.

Fine. Please resend, when you figured it out.

Until then, NACK.

[Peter Stuge]

On Fri, May 16, 2008 at 03:10:10AM +0200, Stefan Reinauer wrote:
> > OK, I forgot what this patch does, but I'm pretty sure it's justified.
>
> Fine. Please resend, when you figured it out.
>
> Until then, NACK.

NAK from me too.

Looking at the patch it does not change flashrom behavior in any way:

It removes one function, sst39sf020.c:erase_sector_39sf020()
It makes two functions public in flash.h
It changes sst28sf040.c:erase_sector_28sf040() from static to public
It adds sst28sf040.c:erase_all_sectors_28sf040()

It does not add or change calls to any of these functions, so
apparently they are not used and not important at the moment.

Maybe erase_sector_39sf020() does not work. After testing, I could
agree to remove it.

The static vs. public functions simply do not seem to matter. AFAIK
sector erasing is never done outside the chip drivers though? I think
that's a good thing.

Please try to find why these changes came about.

//Peter

[Carl-Daniel Hailfinger]

On 16.05.2008 03:30, Peter Stuge wrote:
> On Fri, May 16, 2008 at 03:10:10AM +0200, Stefan Reinauer wrote:
> > > OK, I forgot what this patch does, but I'm pretty sure it's justified.
> >
> > Fine. Please resend, when you figured it out.
> >
> > Until then, NACK.
>
> NAK from me too.

OK, I figured one part out:
Drop erase_sector_39sf020() which is an identical copy of
erase_sector_28sf040().

Signed-off-by: Carl-Daniel Hailfinger

Index: sst39sf020.c
===================================================================
--- sst39sf020.c	(Revision 3326)
+++ sst39sf020.c	(Arbeitskopie)
@@ -22,21 +22,6 @@
 #include <stdint.h>
 #include "flash.h"

-#define AUTO_PG_ERASE1		0x20
-#define AUTO_PG_ERASE2		0xD0
-
-static __inline__ int erase_sector_39sf020(volatile uint8_t *bios,
-					   unsigned long address)
-{
-	*bios = AUTO_PG_ERASE1;
-	*(bios + address) = AUTO_PG_ERASE2;
-
-	/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
-
-	return 0;
-}
-
 int write_39sf020(struct flashchip *flash, uint8_t *buf)
 {
    int i;
> Looking at the patch it does not change flashrom behavior in any way:
>
> It removes one function, sst39sf020.c:erase_sector_39sf020()

Explained above.
> It makes two functions public in flash.h

Dropped.
> It changes sst28sf040.c:erase_sector_28sf040() from static to public

And it removes __inline__. This brings erase_sector_28sf040() more in
line with the style of most other chip-specific functions in the
flashrom tree.
> It adds sst28sf040.c:erase_all_sectors_28sf040()

That's a preparation for better per-sector erasing.
> It does not add or change calls to any of these functions, so
> apparently they are not used and not important at the moment.
>
> Maybe erase_sector_39sf020() does not work. After testing, I could
> agree to remove it.

No, it just was a duplicate.
> The static vs. public functions simply do not seem to matter. AFAIK
> sector erasing is never done outside the chip drivers though? I think
> that's a good thing.
>
> Please try to find why these changes came about.

And the other part:
Add a per-sector chip erase function for SST 28SF* and compatible.

Signed-off-by: Carl-Daniel Hailfinger

Index: sst28sf040.c
===================================================================
--- sst28sf040.c	(Revision 3326)
+++ sst28sf040.c	(Arbeitskopie)
@@ -58,8 +58,7 @@
    tmp = *(volatile uint8_t *)(bios + 0x041A);
 }

-static __inline__ int erase_sector_28sf040(volatile uint8_t *bios,
-					   unsigned long address)
+int erase_sector_28sf040(volatile uint8_t *bios, unsigned long address)
 {
    *bios = AUTO_PG_ERASE1;
    *(bios + address) = AUTO_PG_ERASE2;
@@ -119,6 +118,20 @@
    return 0;
 }

+int erase_all_sectors_28sf040(struct flashchip *flash)
+{
+	int i;
+	int total_size = flash->total_size * 1024;
+	int page_size = flash->page_size;
+	volatile uint8_t *bios = flash->virtual_memory;
+
+	for (i = 0; i < total_size / page_size; i++) {
+		/* erase the page before programming */
+		erase_sector_28sf040(bios, i * page_size);
+	}
+	return 0;
+}
+
 int erase_28sf040(struct flashchip *flash)
 {
    volatile uint8_t *bios = flash->virtual_memory;

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> OK, I figured one part out:
> Drop erase_sector_39sf020() which is an identical copy of
> erase_sector_28sf040().

Alright. This starts making more sense. I still say NACK, as the name
for that function is obviously very wrong then.
If the erase sector function works on several chips, even from different
generations, it should not carry the name of one specific chip.

If we find a good name for it, that makes clear when supporters of new
chips can use it, I suggest lets commit your patches.
> +int erase_sector_28sf040(volatile uint8_t *bios, unsigned long address)
>  {
>    *bios = AUTO_PG_ERASE1;
>    *(bios + address) = AUTO_PG_ERASE2;
> @@ -119,6 +118,20 @@
>    return 0;
>  }
>
> +int erase_all_sectors_28sf040(struct flashchip *flash)
> +{
> int i;
> int total_size = flash->total_size * 1024;
> int page_size = flash->page_size;
> volatile uint8_t *bios = flash->virtual_memory;
> for (i = 0; i < total_size / page_size; i++) {
> /* erase the page before programming */
> erase_sector_28sf040(bios, i * page_size);
> }
> return 0;
>
> +}
>
> int erase_28sf040(struct flashchip *flash)
>  {
>    volatile uint8_t *bios = flash->virtual_memory;

[Carl-Daniel Hailfinger]

On 16.05.2008 15:19, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > OK, I figured one part out:
> > Drop erase_sector_39sf020() which is an identical copy of
> > erase_sector_28sf040().
>
> Alright. This starts making more sense. I still say NACK, as the name
> for that function is obviously very wrong then.
> If the erase sector function works on several chips, even from different
> generations, it should not carry the name of one specific chip.
>
> If we find a good name for it, that makes clear when supporters of new
> chips can use it, I suggest lets commit your patches.

OK, I looked at the datasheet and erase_sector_39sf020() is totally and
completely wrong. It was a straight cut'n'paste from SST 28SF040 code
and the person doing the cut'n'paste didn't even bother to check the
data sheet. The SST 39SF020 is completely incompatible with the 28SF040.

Can I have an Ack for dropping that code?

Regards,
Carl-Daniel

[Joseph Smith]

On Fri, 16 May 2008 17:20:40 +0200, Carl-Daniel Hailfinger
 wrote:
> On 16.05.2008 15:19, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > OK, I figured one part out:
> > > Drop erase_sector_39sf020() which is an identical copy of
> > > erase_sector_28sf040().
> >
> > Alright. This starts making more sense. I still say NACK, as the name
> > for that function is obviously very wrong then.
> > If the erase sector function works on several chips, even from different
> > generations, it should not carry the name of one specific chip.
> >
> > If we find a good name for it, that makes clear when supporters of new
> > chips can use it, I suggest lets commit your patches.
>
> OK, I looked at the datasheet and erase_sector_39sf020() is totally and
> completely wrong. It was a straight cut'n'paste from SST 28SF040 code
> and the person doing the cut'n'paste didn't even bother to check the
> data sheet. The SST 39SF020 is completely incompatible with the 28SF040.
>
> Can I have an Ack for dropping that code?

Are you going to just drop the code or replace it with the correct code??

[Carl-Daniel Hailfinger]

On 16.05.2008 17:23, Joseph Smith wrote:
> On Fri, 16 May 2008 17:20:40 +0200, Carl-Daniel Hailfinger
>  wrote:
> > On 16.05.2008 15:19, Stefan Reinauer wrote:
> > > Carl-Daniel Hailfinger wrote:
> > > > OK, I figured one part out:
> > > > Drop erase_sector_39sf020() which is an identical copy of
> > > > erase_sector_28sf040().
> > >
> > > Alright. This starts making more sense. I still say NACK, as the name
> > > for that function is obviously very wrong then.
> > > If the erase sector function works on several chips, even from different
> > > generations, it should not carry the name of one specific chip.
> > >
> > > If we find a good name for it, that makes clear when supporters of new
> > > chips can use it, I suggest lets commit your patches.
> >
> > OK, I looked at the datasheet and erase_sector_39sf020() is totally and
> > completely wrong. It was a straight cut'n'paste from SST 28SF040 code
> > and the person doing the cut'n'paste didn't even bother to check the
> > data sheet. The SST 39SF020 is completely incompatible with the 28SF040.
> >
> > Can I have an Ack for dropping that code?
>
> Are you going to just drop the code or replace it with the correct code??

No need for replacement. According to the data sheet, standard JEDEC
commands will work and we have those commands in the tree already.

Regards,
Carl-Daniel