[PATCH] flashrom: Refactor SPI reads

[Carl-Daniel Hailfinger]

Every SPI host controller implemented its own way to read flash chips.
This was partly due to a design problem in the abstraction layer.

There should be exactly two different functions for reading SPI chips:
- memory mapped reads
- SPI command reads.

Each of them should be contained in a separate function, optionally
taking parameters where needed.

This patch solves the problems mentioned above, shortens the code and
makes the code logic a lot more obvious.

Since open-coding the min() function leads to errors, include it in this
patch as well.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-spi_read_refactor/flash.h
===================================================================
--- flashrom-spi_read_refactor/flash.h	(Revision 563)
+++ flashrom-spi_read_refactor/flash.h	(Arbeitskopie)
@@ -679,6 +679,7 @@
 extern int verbose;
 #define printf_debug(x...) { if (verbose) printf(x); }
 void map_flash_registers(struct flashchip *flash);
+int min(int a, int b);
 int read_memmapped(struct flashchip *flash, uint8_t *buf);
 extern char *pcidev_bdf;

@@ -726,6 +727,7 @@
 int spi_disable_blockprotect(void);
 void spi_byte_program(int address, uint8_t byte);
 int spi_nbyte_read(int address, uint8_t *bytes, int len);
+int spi_read_generic(struct flashchip *flash, uint8_t *buf, int chunksize);
 int spi_aai_write(struct flashchip *flash, uint8_t *buf);
 uint32_t spi_get_valid_read_addr(void);

Index: flashrom-spi_read_refactor/it87spi.c
===================================================================
--- flashrom-spi_read_refactor/it87spi.c	(Revision 563)
+++ flashrom-spi_read_refactor/it87spi.c	(Arbeitskopie)
@@ -260,18 +260,12 @@
 int it8716f_spi_chip_read(struct flashchip *flash, uint8_t *buf)
 {
    int total_size = 1024 * flash->total_size;
-	int i;
    fast_spi = 0;

if ((programmer == PROGRAMMER_IT87SPI) || (total_size > 512 * 1024)) {
-		for (i = 0; i < total_size; i += 3) {
-			int toread = 3;
-			if (total_size - i < toread)
-				toread = total_size - i;
-			spi_nbyte_read(i, buf + i, toread);
-		}
+		spi_read_generic(flash, buf, 3);
    } else {
-		memcpy(buf, (const char *)flash->virtual_memory, total_size);
+		read_memmapped(flash, buf);
    }

return 0;
Index: flashrom-spi_read_refactor/spi.c
===================================================================
--- flashrom-spi_read_refactor/spi.c	(Revision 563)
+++ flashrom-spi_read_refactor/spi.c	(Arbeitskopie)
@@ -652,6 +652,33 @@
    return spi_command(sizeof(cmd), len, cmd, bytes);
 }

+/*
+ * Read a complete flash chip.
+ * Each page is read separately in chunks with a maximum size of chunksize.
+ */
+int spi_read_generic(struct flashchip *flash, uint8_t *buf, int chunksize)
+{
+	int rc = 0;
+	int i, j;
+	int total_size = flash->total_size * 1024;
+	int page_size = flash->page_size;
+	int toread;
+
+	for (j = 0; j < total_size / page_size; j++) {
+		for (i = 0; i < page_size; i += chunksize) {
+			toread = min(chunksize, page_size - i);
+			rc = spi_nbyte_read(j * page_size + i,
+					    buf + j * page_size + i, toread);
+			if (rc)
+				break;
+		}
+		if (rc)
+			break;
+	}
+
+	return rc;
+}
+
 int spi_chip_read(struct flashchip *flash, uint8_t *buf)
 {
    switch (spi_controller) {
Index: flashrom-spi_read_refactor/wbsio_spi.c
===================================================================
--- flashrom-spi_read_refactor/wbsio_spi.c	(Revision 563)
+++ flashrom-spi_read_refactor/wbsio_spi.c	(Arbeitskopie)
@@ -177,12 +177,12 @@
 {
    int size = flash->total_size * 1024;

-	if (flash->total_size > 1024) {
+	if (size > 1024 * 1024) {
    	fprintf(stderr, "%s: Winbond saved on 4 register bits so max chip size is 1024 KB!\n", __func__);
    	return 1;
    }

-	memcpy(buf, (const char *)flash->virtual_memory, size);
+	read_memmapped(flash, buf);
    return 0;
 }

Index: flashrom-spi_read_refactor/sb600spi.c
===================================================================
--- flashrom-spi_read_refactor/sb600spi.c	(Revision 563)
+++ flashrom-spi_read_refactor/sb600spi.c	(Arbeitskopie)
@@ -41,14 +41,8 @@

int sb600_spi_read(struct flashchip *flash, uint8_t *buf)
 {
-	int rc = 0, i;
-	int total_size = flash->total_size * 1024;
-	int page_size = 8;
-
-	for (i = 0; i < total_size / page_size; i++)
-		spi_nbyte_read(i * page_size, (void *)(buf + i * page_size),
-			       page_size);
-	return rc;
+	/* Maximum read length is 8 bytes. */
+	return spi_read_generic(flash, buf, 8);
 }

uint8_t sb600_read_status_register(void)
Index: flashrom-spi_read_refactor/flashrom.c
===================================================================
--- flashrom-spi_read_refactor/flashrom.c	(Revision 563)
+++ flashrom-spi_read_refactor/flashrom.c	(Arbeitskopie)
@@ -174,6 +174,11 @@
    return 0;
 }

+int min(int a, int b)
+{
+	return (a < b) ? a : b;
+}
+
 char *strcat_realloc(char *dest, const char *src)
 {
    dest = realloc(dest, strlen(dest) + strlen(src) + 1);
Index: flashrom-spi_read_refactor/ichspi.c
===================================================================
--- flashrom-spi_read_refactor/ichspi.c	(Revision 563)
+++ flashrom-spi_read_refactor/ichspi.c	(Arbeitskopie)
@@ -148,8 +148,6 @@
 static int program_opcodes(OPCODES * op);
 static int run_opcode(OPCODE op, uint32_t offset,
    	      uint8_t datalength, uint8_t * data);
-static int ich_spi_read_page(struct flashchip *flash, uint8_t * buf,
-			     int offset, int maxdata);
 static int ich_spi_write_page(struct flashchip *flash, uint8_t * bytes,
    		      int offset, int maxdata);

@@ -614,38 +612,6 @@
    return -1;
 }

-static int ich_spi_read_page(struct flashchip *flash, uint8_t * buf, int offset,
-			     int maxdata)
-{
-	int page_size = flash->page_size;
-	uint32_t remaining = flash->page_size;
-	int a;
-
-	printf_debug("ich_spi_read_page: offset=%d, number=%d, buf=%p\n",
-		     offset, page_size, buf);
-
-	for (a = 0; a < page_size; a += maxdata) {
-		if (remaining < maxdata) {
-
-			if (spi_nbyte_read(offset + (page_size - remaining),
-				&buf[page_size - remaining], remaining)) {
-				printf_debug("Error reading");
-				return 1;
-			}
-			remaining = 0;
-		} else {
-			if (spi_nbyte_read(offset + (page_size - remaining),
-				&buf[page_size - remaining], maxdata)) {
-				printf_debug("Error reading");
-				return 1;
-			}
-			remaining -= maxdata;
-		}
-	}
-
-	return 0;
-}
-
 static int ich_spi_write_page(struct flashchip *flash, uint8_t * bytes,
    		      int offset, int maxdata)
 {
@@ -683,21 +649,12 @@

int ich_spi_read(struct flashchip *flash, uint8_t * buf)
 {
-	int i, rc = 0;
-	int total_size = flash->total_size * 1024;
-	int page_size = flash->page_size;
    int maxdata = 64;

-	if (spi_controller == SPI_CONTROLLER_VIA) {
+	if (spi_controller == SPI_CONTROLLER_VIA)
    	maxdata = 16;
-	}

-	for (i = 0; (i < total_size / page_size) && (rc == 0); i++) {
-		rc = ich_spi_read_page(flash, (void *)(buf + i * page_size),
-				       i * page_size, maxdata);
-	}
-
-	return rc;
+	return spi_read_generic(flash, buf, maxdata);
 }

int ich_spi_write_256(struct flashchip *flash, uint8_t * buf)

[Carl-Daniel Hailfinger]

On 02.06.2009 01:19, Carl-Daniel Hailfinger wrote:
> Every SPI host controller implemented its own way to read flash chips.
> This was partly due to a design problem in the abstraction layer.
>
> There should be exactly two different functions for reading SPI chips:
> memory mapped reads
> SPI command reads.
>
> Each of them should be contained in a separate function, optionally
> taking parameters where needed.
>
> This patch solves the problems mentioned above, shortens the code and
> makes the code logic a lot more obvious.
>
> Since open-coding the min() function leads to errors, include it in this
> patch as well.
>
> Signed-off-by: Carl-Daniel Hailfinger

Ron pointed out that I should not only explain what a patch does, but
also why we need it.

Once this patch is merged, we get partial read support practically for free.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 02.06.2009 01:19, Carl-Daniel Hailfinger wrote:
> Every SPI host controller implemented its own way to read flash chips.
> This was partly due to a design problem in the abstraction layer.
>
> Fix it.
>
> Signed-off-by: Carl-Daniel Hailfinger

Ping.

Regards,
Carl-Daniel

[ron minnich]

Acked-by: Ronald G. Minnich

[Carl-Daniel Hailfinger]

On 13.06.2009 12:24, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Thanks, committed with one minor cosmetic change in r589.

Regards,
Carl-Daniel