Change in em100[master]: Drop hard coded SPI flash size in firmware.c

[Stefan Reinauer (Code Review)]

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/36835 )

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Drop hard coded SPI flash size in firmware.c

Decide the flash size by examining by looking at the SPI flash id.
Yes, the size can be automatically derived from from the ID number,
but it makes sense to me to have people report flash chips with
other IDs instead of adding some automatism. You can guess, why.

Signed-off-by: Stefan Reinauer 
Change-Id: Id6851ab5017dc63ed16b5c289c17ff4ea5323d8f
---
M em100.h
M firmware.c
M spi.c
3 files changed, 24 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/35/36835/1

diff --git a/em100.h b/em100.h
index 7d12dd6..c1b9241 100644
--- a/em100.h
+++ b/em100.h
@@ -58,7 +58,7 @@
    	int length);

/* spi.c */
-int get_spi_flash_id(struct em100 *em100);
+uint32_t get_spi_flash_id(struct em100 *em100);
 int erase_spi_flash(struct em100 *em100);
 int poll_spi_flash_status(struct em100 *em100);
 int read_spi_flash_page(struct em100 *em100, int addr, unsigned char *blk);
diff --git a/firmware.c b/firmware.c
index f5716d8..f5130f8 100644
--- a/firmware.c
+++ b/firmware.c
@@ -78,16 +78,31 @@
 int firmware_dump(struct em100 *em100, const char *filename,
    	int firmware_is_dpfw)
 {
-#define ROM_SIZE (2*1024*1024)
-	unsigned char data[ROM_SIZE];
+	unsigned char *data;
    int i;
+	uint32_t id, rom_size = 0;
    FILE *fw;

+	id = get_spi_flash_id(em100);
+	switch (id) {
+	case 0x202015:
+		rom_size = (2*1024*1024);
+		break;
+	default:
+		printf("Unknown SPI flash id = %06x. Please report\n", id);
+		return 1;
+	}
+	data = malloc(rom_size);
+	if (data == NULL) {
+		perror("Out of memory.\n");
+		exit(1);
+	}
+	memset(data, 0, rom_size);
+
    printf("\nWriting EM100Pro firmware to file %s\n", filename);
-	memset(data, 0, ROM_SIZE);
-	for (i=0; i < ROM_SIZE; i+=256) {
+	for (i=0; i < rom_size; i+=256) {
    	if((i & 0x7fff) == 0)
-			print_progress(i * 100 / ROM_SIZE);
+			print_progress(i * 100 / rom_size);
    	if (!read_spi_flash_page(em100, i, data+i)) {
    		if (!read_spi_flash_page(em100, i, data+i))
    			if (!read_spi_flash_page(em100, i, data+i))
@@ -150,13 +165,13 @@
    	fwrite(data, fpga_size, 1, fw);
    	fwrite(data + 0x100100, mcu_size, 1, fw);
    } else {
-		if (fwrite(data, ROM_SIZE, 1, fw) != 1)
+		if (fwrite(data, rom_size, 1, fw) != 1)
    		printf("ERROR: Couldn't write %s\n", filename);
    }
    fclose(fw);

#if DEBUG
-	hexdump(data, ROM_SIZE);
+	hexdump(data, rom_size);
 #endif
    return 0;
 }
diff --git a/spi.c b/spi.c
index a6496c0..1a225a4 100644
--- a/spi.c
+++ b/spi.c
@@ -22,7 +22,7 @@

/* SPI flash related operations */

-int get_spi_flash_id(struct em100 *em100)
+uint32_t get_spi_flash_id(struct em100 *em100)
 {
    unsigned char cmd[16];
    unsigned char data[512];

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36835 )

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/em100/+/36835/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36835/1/firmware.c@95 
PS1, Line 95: data = malloc(rom_size);
malloc without free

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36835 )

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/em100/+/36835/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36835/1/firmware.c@89 
PS1, Line 89: (2*1024*1024);
no need for the parens here, and since we'll have a bunch of these, maybe #define MB 1024*1024, so it's romsize = 2*MB? (or even #define MB *1024*1024 ;-) )

[Stefan Reinauer (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/em100/+/36835

to look at the new patch set (#2).

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Drop hard coded SPI flash size in firmware.c

Decide the flash size by examining by looking at the SPI flash id.
Yes, the size can be automatically derived from from the ID number,
but it makes sense to me to have people report flash chips with
other IDs instead of adding some automatism. You can guess, why.

Signed-off-by: Stefan Reinauer 
Change-Id: Id6851ab5017dc63ed16b5c289c17ff4ea5323d8f
---
M em100.h
M firmware.c
M spi.c
3 files changed, 32 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/35/36835/2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36835 )

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

https://review.coreboot.org/c/em100/+/36835/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36835/1/firmware.c@89 
PS1, Line 89: (2*1024*1024);
> no need for the parens here, and since we'll have a bunch of these, maybe #define MB 1024*1024, so i […]

Done

https://review.coreboot.org/c/em100/+/36835/1/firmware.c@95 
PS1, Line 95: data = malloc(rom_size);
> malloc without free

Done

[Stefan Reinauer (Code Review)]

Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/36835 )

Change subject: Drop hard coded SPI flash size in firmware.c
......................................................................

Drop hard coded SPI flash size in firmware.c

Decide the flash size by examining by looking at the SPI flash id.
Yes, the size can be automatically derived from from the ID number,
but it makes sense to me to have people report flash chips with
other IDs instead of adding some automatism. You can guess, why.

Signed-off-by: Stefan Reinauer 
Change-Id: Id6851ab5017dc63ed16b5c289c17ff4ea5323d8f
Reviewed-on: https://review.coreboot.org/c/em100/+/36835
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M em100.h
M firmware.c
M spi.c
3 files changed, 32 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/em100.h b/em100.h
index 05a5b24..2159fba 100644
--- a/em100.h
+++ b/em100.h
@@ -62,7 +62,7 @@
    	int length);

/* spi.c */
-int get_spi_flash_id(struct em100 *em100);
+uint32_t get_spi_flash_id(struct em100 *em100);
 int erase_spi_flash(struct em100 *em100);
 int poll_spi_flash_status(struct em100 *em100);
 int read_spi_flash_page(struct em100 *em100, int addr, unsigned char *blk);
@@ -158,4 +158,8 @@
 int read_spi_terminal(struct em100 *em100, int print_counter);
 int init_spi_terminal(struct em100 *em100);

+/* Misc. */
+
+#define MB * 1024 * 1024
+
 #endif
diff --git a/firmware.c b/firmware.c
index f5716d8..c88011c 100644
--- a/firmware.c
+++ b/firmware.c
@@ -78,16 +78,31 @@
 int firmware_dump(struct em100 *em100, const char *filename,
    	int firmware_is_dpfw)
 {
-#define ROM_SIZE (2*1024*1024)
-	unsigned char data[ROM_SIZE];
+	unsigned char *data;
    int i;
+	uint32_t id, rom_size = 0;
    FILE *fw;

+	id = get_spi_flash_id(em100);
+	switch (id) {
+	case 0x202015:
+		rom_size = 2 MB;
+		break;
+	default:
+		printf("Unknown SPI flash id = %06x. Please report\n", id);
+		return 1;
+	}
+	data = malloc(rom_size);
+	if (data == NULL) {
+		perror("Out of memory.\n");
+		exit(1);
+	}
+	memset(data, 0, rom_size);
+
    printf("\nWriting EM100Pro firmware to file %s\n", filename);
-	memset(data, 0, ROM_SIZE);
-	for (i=0; i < ROM_SIZE; i+=256) {
+	for (i=0; i < rom_size; i+=256) {
    	if((i & 0x7fff) == 0)
-			print_progress(i * 100 / ROM_SIZE);
+			print_progress(i * 100 / rom_size);
    	if (!read_spi_flash_page(em100, i, data+i)) {
    		if (!read_spi_flash_page(em100, i, data+i))
    			if (!read_spi_flash_page(em100, i, data+i))
@@ -99,6 +114,7 @@
    fw = fopen(filename, "wb");
    if (!fw) {
    	perror(filename);
+		free(data);
    	return 0;
    }

@@ -117,6 +133,7 @@
    	if (i == 0x100000) {
    		printf("Can't parse device firmware. Please extract"
    				" raw firmware instead.\n");
+			free(data);
    		exit(1);
    	}
    	fpga_size = i;
@@ -128,6 +145,7 @@
    	if (i == 0xfff00) {
    		printf("Can't parse device firmware. Please extract"
    				" raw firmware instead.\n");
+			free(data);
    		exit(1);
    	}
    	mcu_size = i;
@@ -150,14 +168,15 @@
    	fwrite(data, fpga_size, 1, fw);
    	fwrite(data + 0x100100, mcu_size, 1, fw);
    } else {
-		if (fwrite(data, ROM_SIZE, 1, fw) != 1)
+		if (fwrite(data, rom_size, 1, fw) != 1)
    		printf("ERROR: Couldn't write %s\n", filename);
    }
    fclose(fw);

#if DEBUG
-	hexdump(data, ROM_SIZE);
+	hexdump(data, rom_size);
 #endif
+	free(data);
    return 0;
 }

diff --git a/spi.c b/spi.c
index a6496c0..1a225a4 100644
--- a/spi.c
+++ b/spi.c
@@ -22,7 +22,7 @@

/* SPI flash related operations */

-int get_spi_flash_id(struct em100 *em100)
+uint32_t get_spi_flash_id(struct em100 *em100)
 {
    unsigned char cmd[16];
    unsigned char data[512];