Change in em100[master]: em100: Detect emulated flash chip

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

em100: Detect emulated flash chip

It's not clear why uploads (EM100 to host) generate a 64MiB ROM file.
It turns out that the flash chip needs to be specified at the command line, which
isn't intentional.

Detect the currently emulated SPI and use the size as defined in flashchips.

Tested on EM100Pro, will now upload ROM files matching the currently emulated target
even without specifying the flash chip on the command line.

Signed-off-by: Patrick Rudolph 
Change-Id: Ia3fb3034a195313b8d3bb4ce24dc1779058fd08f
---
M em100.c
1 file changed, 56 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/47/35947/1

diff --git a/em100.c b/em100.c
index 1548450..a20dc7d 100644
--- a/em100.c
+++ b/em100.c
@@ -567,6 +567,51 @@
    return !result;
 }

+static int get_chip_init_val(const chipdesc *desc,
+			     const uint8_t reg1,
+			     const uint8_t reg2,
+			     uint16_t *out)
+{
+	int i;
+	for (i = 0; i < desc->init_len; i++) {
+		if (desc->init[i][0] == reg1 && desc->init[i][1] == reg2) {
+			*out = (desc->init[i][2] << 8) | desc->init[i][3];
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static int get_chip_type(struct em100 *em100, const chipdesc **out)
+{
+	uint16_t manu;
+	uint16_t vend;
+
+	/* Read manufacturer and vendor id from FPGA */
+	if (!read_fpga_register(em100, 0x42, &manu))
+		return 1;
+	if (!read_fpga_register(em100, 0x40, &vend))
+		return 1;
+
+	const chipdesc *chip = chips;
+	do {
+		uint16_t comp;
+		if (!get_chip_init_val(chip, 0x23, 0x40, &comp) || vend != comp)
+			continue;
+		if (!get_chip_init_val(chip, 0x23, 0x42, &comp) || manu != comp)
+			continue;
+
+		break;
+	} while ((++chip)->name);
+	if (!chip->name)
+		return 1;
+
+	printf("Configured to emulate %s\n", chip->name);
+	*out = chip;
+
+	return 0;
+}
+
 static const struct option longopts[] = {
    {"set", 1, 0, 'c'},
    {"download", 1, 0, 'd'},
@@ -822,8 +867,17 @@
    }

if (read_filename) {
-		/* largest size - 64MB */
-		int maxlen = desiredchip ? chip->size : 0x4000000;
+		int maxlen = 0x4000000; /* largest size - 64MB */
+
+		if (!desiredchip) {
+			/* Read configured SPI emulation from EM100 */
+			const chipdesc *current;
+			if (!get_chip_type(&em100, &current))
+				maxlen = current->size;
+		} else {
+			maxlen = chip->size;
+		}
+
    	void *data = malloc(maxlen);
    	if (data == NULL) {
    		printf("FATAL: couldn't allocate memory\n");

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 1: Code-Review+1

Does some manual need updating?

[Patrick Rudolph (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 1:

There's no manual and as it does more automatically I don't think it's worth documenting it.

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 1:

(7 comments)

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

https://review.coreboot.org/c/em100/+/35947/1/em100.c@570 
PS1, Line 570: get_chip_init_val
Function comment.

Seems like it compares the init val or searches for it

https://review.coreboot.org/c/em100/+/35947/1/em100.c@585 
PS1, Line 585: get_chip_type
Comment

https://review.coreboot.org/c/em100/+/35947/1/em100.c@596 
PS1, Line 596: chipdesc
Move decl to top of function

https://review.coreboot.org/c/em100/+/35947/1/em100.c@597 
PS1, Line 597: {
Can this be a for loop? The (++chip)->name is a bit ugly

https://review.coreboot.org/c/em100/+/35947/1/em100.c@599 
PS1, Line 599: 0x23, 0x40
What are these values?

https://review.coreboot.org/c/em100/+/35947/1/em100.c@874 
PS1, Line 874: const
blank line after

https://review.coreboot.org/c/em100/+/35947/1/em100.c@875 
PS1, Line 875: 			if (!get_chip_type(&em100, &current))
But this also prints something out so the name is confusing. Can the print go somewhere else?

[Patrick Rudolph (Code Review)]

Hello Aaron Durbin, Simon Glass, Stefan Reinauer, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: em100: Detect emulated flash chip
......................................................................

em100: Detect emulated flash chip

It's not clear why uploads (EM100 to host) generate a 64MiB ROM file.
It turns out that the flash chip needs to be specified at the command line, which
isn't intentional.

Detect the currently emulated SPI and use the size as defined in flashchips.

Tested on EM100Pro, will now upload ROM files matching the currently emulated target
even without specifying the flash chip on the command line.

Signed-off-by: Patrick Rudolph 
Change-Id: Ia3fb3034a195313b8d3bb4ce24dc1779058fd08f
---
M em100.c
M em100.h
2 files changed, 75 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/47/35947/2

[Patrick Rudolph (Code Review)]

Hello Aaron Durbin, Simon Glass, Stefan Reinauer, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: em100: Detect emulated flash chip
......................................................................

em100: Detect emulated flash chip

It's not clear why uploads (EM100 to host) generate a 64MiB ROM file.
It turns out that the flash chip needs to be specified at the command line, which
isn't intentional.

Detect the currently emulated SPI and use the size as defined in flashchips.

Tested on EM100Pro, will now upload ROM files matching the currently emulated target
even without specifying the flash chip on the command line.

Signed-off-by: Patrick Rudolph 
Change-Id: Ia3fb3034a195313b8d3bb4ce24dc1779058fd08f
---
M em100.c
M em100.h
2 files changed, 75 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/47/35947/3

[Patrick Rudolph (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 3:

(7 comments)

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

https://review.coreboot.org/c/em100/+/35947/1/em100.c@570 
PS1, Line 570: get_chip_init_val
> Function comment. […]

Done

https://review.coreboot.org/c/em100/+/35947/1/em100.c@585 
PS1, Line 585: get_chip_type
> Comment

Done

https://review.coreboot.org/c/em100/+/35947/1/em100.c@596 
PS1, Line 596: chipdesc
> Move decl to top of function

Done

https://review.coreboot.org/c/em100/+/35947/1/em100.c@597 
PS1, Line 597: {
> Can this be a for loop? The (++chip)->name is a bit ugly

Done

https://review.coreboot.org/c/em100/+/35947/1/em100.c@599 
PS1, Line 599: 0x23, 0x40
> What are these values?

Added a define for 0x40 and 0x42.

https://review.coreboot.org/c/em100/+/35947/1/em100.c@874 
PS1, Line 874: const
> blank line after

Done

https://review.coreboot.org/c/em100/+/35947/1/em100.c@875 
PS1, Line 875: 			if (!get_chip_type(&em100, &current))
> But this also prints something out so the name is confusing. […]

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 3: Code-Review+1

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 3:

(6 comments)

https://review.coreboot.org/c/em100/+/35947/3/em100.c 
File em100.c:

https://review.coreboot.org/c/em100/+/35947/3/em100.c@573 
PS3, Line 573:  *
This is good but please also document the args

@desc: ...
@reg1: ..
@out: Returns ...

https://review.coreboot.org/c/em100/+/35947/3/em100.c@579 
PS3, Line 579: uint16_t
I feel this could be int or unsigned int. It doesn't need to be a 16-bit int

https://review.coreboot.org/c/em100/+/35947/3/em100.c@581 
PS3, Line 581: int
blank line after decl

https://review.coreboot.org/c/em100/+/35947/3/em100.c@588 
PS3, Line 588: return
blank line before return

https://review.coreboot.org/c/em100/+/35947/3/em100.c@595 
PS3, Line 595:  *
Please document args

https://review.coreboot.org/c/em100/+/35947/3/em100.c@611 
PS3, Line 611: 		uint16_t comp;
blank line after decl

[Stefan Reinauer (Code Review)]

Stefan Reinauer has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

em100: Detect emulated flash chip

It's not clear why uploads (EM100 to host) generate a 64MiB ROM file.
It turns out that the flash chip needs to be specified at the command line, which
isn't intentional.

Detect the currently emulated SPI and use the size as defined in flashchips.

Tested on EM100Pro, will now upload ROM files matching the currently emulated target
even without specifying the flash chip on the command line.

Signed-off-by: Patrick Rudolph 
Change-Id: Ia3fb3034a195313b8d3bb4ce24dc1779058fd08f
---
M em100.c
M em100.h
2 files changed, 82 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/47/35947/4

[Stefan Reinauer (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 4:

(6 comments)

https://review.coreboot.org/c/em100/+/35947/3/em100.c 
File em100.c:

https://review.coreboot.org/c/em100/+/35947/3/em100.c@573 
PS3, Line 573:  *
> This is good but please also document the args […]

Done

https://review.coreboot.org/c/em100/+/35947/3/em100.c@579 
PS3, Line 579: uint16_t
> I feel this could be int or unsigned int. […]

It returns exactly two bytes, so this seems reasonable.

https://review.coreboot.org/c/em100/+/35947/3/em100.c@581 
PS3, Line 581: int
> blank line after decl

Done

https://review.coreboot.org/c/em100/+/35947/3/em100.c@588 
PS3, Line 588: return
> blank line before return

Done

https://review.coreboot.org/c/em100/+/35947/3/em100.c@595 
PS3, Line 595:  *
> Please document args

I'll leave this one to Patrick in a later patch, since this comment is better than what I originally wrote for many of the static functions in the code.

https://review.coreboot.org/c/em100/+/35947/3/em100.c@611 
PS3, Line 611: 		uint16_t comp;
> blank line after decl

Done

[Stefan Reinauer (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 4: Code-Review+2

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 4: Code-Review+1

[Patrick Georgi (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/em100/+/35947 )

Change subject: em100: Detect emulated flash chip
......................................................................

em100: Detect emulated flash chip

It's not clear why uploads (EM100 to host) generate a 64MiB ROM file.
It turns out that the flash chip needs to be specified at the command line, which
isn't intentional.

Detect the currently emulated SPI and use the size as defined in flashchips.

Tested on EM100Pro, will now upload ROM files matching the currently emulated target
even without specifying the flash chip on the command line.

Signed-off-by: Patrick Rudolph 
Change-Id: Ia3fb3034a195313b8d3bb4ce24dc1779058fd08f
Reviewed-on: https://review.coreboot.org/c/em100/+/35947
Tested-by: build bot (Jenkins) 
Reviewed-by: Stefan Reinauer 
Reviewed-by: Simon Glass 
Reviewed-by: Patrick Georgi 
---
M em100.c
M em100.h
2 files changed, 82 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Stefan Reinauer: Looks good to me, approved
  Simon Glass: Looks good to me, but someone else must approve

diff --git a/em100.c b/em100.c
index 215b233..3d0de42 100644
--- a/em100.c
+++ b/em100.c
@@ -567,6 +567,70 @@
    return !result;
 }

+/**
+ * Searches for a specific FPGA register in the chip initialisation
+ * sequence and returns the value in out.
+ *
+ * @reg1: e.g. FPGA write command (0x23)
+ * @reg2: e.g. FPGA register
+ *
+ * Returns 0 on success.
+ */
+static int get_chip_init_val(const chipdesc *desc,
+			     const uint8_t reg1,
+			     const uint8_t reg2,
+			     uint16_t *out)
+{
+	int i;
+
+	for (i = 0; i < desc->init_len; i++) {
+		if (desc->init[i][0] == reg1 && desc->init[i][1] == reg2) {
+			*out = (desc->init[i][2] << 8) | desc->init[i][3];
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
+/**
+ * Tries to identify the currently emulated SPI flash by looking at
+ * known registers in the FPGA and matches those bits with the
+ * chip initialisation sequence.
+ *
+ * Returns 0 on success.
+ */
+static int get_chip_type(struct em100 *em100, const chipdesc **out)
+{
+	const chipdesc *chip;
+	uint16_t venid;
+	uint16_t devid;
+
+	/* Read manufacturer and vendor id from FPGA */
+	if (!read_fpga_register(em100, FPGA_REG_VENDID, &venid))
+		return 1;
+	if (!read_fpga_register(em100, FPGA_REG_DEVID, &devid))
+		return 1;
+
+	for (chip = chips; chip != NULL; chip++) {
+		uint16_t comp;
+
+		if (get_chip_init_val(chip, 0x23, FPGA_REG_DEVID, &comp) || devid != comp)
+			continue;
+		if (get_chip_init_val(chip, 0x23, FPGA_REG_VENDID, &comp) || venid != comp)
+			continue;
+
+		break;
+	}
+
+	if (!chip)
+		return 1;
+
+	*out = chip;
+
+	return 0;
+}
+
 static const struct option longopts[] = {
    {"set", 1, 0, 'c'},
    {"download", 1, 0, 'd'},
@@ -822,8 +886,20 @@
    }

if (read_filename) {
-		/* largest size - 64MB */
-		int maxlen = desiredchip ? chip->size : 0x4000000;
+		int maxlen = 0x4000000; /* largest size - 64MB */
+
+		if (!desiredchip) {
+			/* Read configured SPI emulation from EM100 */
+			const chipdesc *emulated_chip;
+
+			if (!get_chip_type(&em100, &emulated_chip)) {
+				printf("Configured to emulate %s\n", emulated_chip->name);
+				maxlen = emulated_chip->size;
+			}
+		} else {
+			maxlen = chip->size;
+		}
+
    	void *data = malloc(maxlen);
    	if (data == NULL) {
    		printf("FATAL: couldn't allocate memory\n");
diff --git a/em100.h b/em100.h
index 6a58e70..a903baa 100644
--- a/em100.h
+++ b/em100.h
@@ -48,6 +48,10 @@
 int fpga_get_voltage(struct em100 *em100, int *voltage_codep);
 int fpga_reconfigure(struct em100 *em100);

+/* Guessed register names */
+#define FPGA_REG_DEVID		0x40
+#define FPGA_REG_VENDID		0x42
+
 /* hexdump.c */
 void hexdump(const void *memory, size_t length);

[Patrick Georgi (Code Review)]

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

Change subject: em100: Detect emulated flash chip
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/em100/+/35947/5/em100.c 
File em100.c:

https://review.coreboot.org/c/em100/+/35947/5/em100.c@623 
PS5, Line 623: 		break;
Coverity complaining about !chip in line 626 never being true made me look at this again. Why's the break here?