Change in flashrom[master]: ichspi.c: Make pprinters parameterics on ich_generation

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parameterics on ich_generation
......................................................................

ichspi.c: Make pprinters parameterics on ich_generation

Make the two prettyprint functions pure by depending on
the ich_generation as a function parameter over a global
variable:

* prettyprint_ich9_reg_hsfs()
 * prettyprint_ich9_reg_hsfc()

Change-Id: I5d4fb012c6b9b843ac30c1fe2ea6fe754c545a43
Signed-off-by: Edward O'Callaghan 
---
M ichspi.c
1 file changed, 17 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/43501/1

diff --git a/ichspi.c b/ichspi.c
index 306b778..b90b864 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -389,13 +389,13 @@
 #define _pprint_reg(bit, mask, off, val, sep) msg_pdbg("%s=%d" sep, #bit, (val & mask) >> off)
 #define pprint_reg(reg, bit, val, sep) _pprint_reg(bit, reg##_##bit, reg##_##bit##_OFF, val, sep)

-static void prettyprint_ich9_reg_hsfs(uint16_t reg_val)
+static void prettyprint_ich9_reg_hsfs(uint16_t reg_val, enum ich_chipset ich_gen)
 {
    msg_pdbg("HSFS: ");
    pprint_reg(HSFS, FDONE, reg_val, ", ");
    pprint_reg(HSFS, FCERR, reg_val, ", ");
    pprint_reg(HSFS, AEL, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -405,7 +405,7 @@
    	break;
    }
    pprint_reg(HSFS, SCIP, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -420,11 +420,11 @@
    pprint_reg(HSFS, FLOCKDN, reg_val, "\n");
 }

-static void prettyprint_ich9_reg_hsfc(uint16_t reg_val)
+static void prettyprint_ich9_reg_hsfc(uint16_t reg_val, enum ich_chipset ich_gen)
 {
    msg_pdbg("HSFC: ");
    pprint_reg(HSFC, FGO, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -1283,7 +1283,8 @@
    Returns 0 if the cycle completes successfully without errors within
    timeout us, 1 on errors. */
 static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
-					     unsigned int len)
+					     unsigned int len,
+					     enum ich_chipset ich_gen)
 {
    uint16_t hsfs;
    uint32_t addr;
@@ -1300,8 +1301,8 @@
    	msg_perr("Timeout error between offset 0x%08x and "
    		 "0x%08x (= 0x%08x + %d)!\n",
    		 addr, addr + len - 1, addr, len - 1);
-		prettyprint_ich9_reg_hsfs(hsfs);
-		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
+		prettyprint_ich9_reg_hsfs(hsfs, ich_gen);
+		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen);
    	return 1;
    }

@@ -1310,8 +1311,8 @@
    	msg_perr("Transaction error between offset 0x%08x and "
    		 "0x%08x (= 0x%08x + %d)!\n",
    		 addr, addr + len - 1, addr, len - 1);
-		prettyprint_ich9_reg_hsfs(hsfs);
-		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
+		prettyprint_ich9_reg_hsfs(hsfs, ich_gen);
+		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen);
    	return 1;
    }
    return 0;
@@ -1415,10 +1416,10 @@
    hsfc |= (0x3 << HSFC_FCYCLE_OFF); /* set erase operation */
    hsfc |= HSFC_FGO; /* start */
    msg_pdbg("HSFC used for block erasing: ");
-	prettyprint_ich9_reg_hsfc(hsfc);
+	prettyprint_ich9_reg_hsfc(hsfc, ich_generation);
    REGWRITE16(ICH9_REG_HSFC, hsfc);

-	if (ich_hwseq_wait_for_cycle_complete(timeout, len))
+	if (ich_hwseq_wait_for_cycle_complete(timeout, len, ich_generation))
    	return -1;
    return 0;
 }
@@ -1455,7 +1456,7 @@
    	hsfc |= HSFC_FGO; /* start */
    	REGWRITE16(ICH9_REG_HSFC, hsfc);

-		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation))
    		return 1;
    	ich_read_data(buf, block_len, ICH9_REG_FDATA0);
    	addr += block_len;
@@ -1497,7 +1498,7 @@
    	hsfc |= HSFC_FGO; /* start */
    	REGWRITE16(ICH9_REG_HSFC, hsfc);

-		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation))
    		return -1;
    	addr += block_len;
    	buf += block_len;
@@ -1840,7 +1841,7 @@

tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
    	msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
-		prettyprint_ich9_reg_hsfs(tmp2);
+		prettyprint_ich9_reg_hsfs(tmp2, ich_gen);
    	if (tmp2 & HSFS_FLOCKDN) {
    		msg_pinfo("SPI Configuration is locked down.\n");
    		ichspi_lock = 1;
@@ -1856,7 +1857,7 @@
    	if (desc_valid) {
    		tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
    		msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
-			prettyprint_ich9_reg_hsfc(tmp2);
+			prettyprint_ich9_reg_hsfc(tmp2, ich_gen);
    	}

tmp = mmio_readl(ich_spibar + ICH9_REG_FADDR);

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parameterics on ich_generation
......................................................................

Patch Set 1: Code-Review+2

(3 comments)

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@7 
PS1, Line 7: parameterics
parametric

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@9 
PS1, Line 9: depending on
taking

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@10 
PS1, Line 10: ich_generation
ich_generation value

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parameterics on ich_generation
......................................................................

Patch Set 1: Code-Review+1

Change in flashrom[master]: ichspi.c: Make pprinters parametric on ich_generation

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Angel Pons,

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

https://review.coreboot.org/c/flashrom/+/43501

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

Change subject: ichspi.c: Make pprinters parametric on ich_generation
......................................................................

ichspi.c: Make pprinters parametric on ich_generation

Make the two prettyprint functions pure by taking the
ich_generation value as a function parameter over a global
variable:

* prettyprint_ich9_reg_hsfs()
 * prettyprint_ich9_reg_hsfc()

Change-Id: I5d4fb012c6b9b843ac30c1fe2ea6fe754c545a43
Signed-off-by: Edward O'Callaghan 
---
M ichspi.c
1 file changed, 17 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/43501/2

Change in flashrom[master]: ichspi.c: Make pprinters parametric on ich_generation

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parametric on ich_generation
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@7 
PS1, Line 7: parameterics
> parametric

Done

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@9 
PS1, Line 9: depending on
> taking

Done

https://review.coreboot.org/c/flashrom/+/43501/1//COMMIT_MSG@10 
PS1, Line 10: ich_generation
> ich_generation value

Done

Change in flashrom[master]: ichspi.c: Make pprinters parametric on ich_generation

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parametric on ich_generation
......................................................................

Patch Set 2: Code-Review+2

Change in flashrom[master]: ichspi.c: Make pprinters parametric on ich_generation

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/43501 )

Change subject: ichspi.c: Make pprinters parametric on ich_generation
......................................................................

ichspi.c: Make pprinters parametric on ich_generation

Make the two prettyprint functions pure by taking the
ich_generation value as a function parameter over a global
variable:

* prettyprint_ich9_reg_hsfs()
 * prettyprint_ich9_reg_hsfc()

Change-Id: I5d4fb012c6b9b843ac30c1fe2ea6fe754c545a43
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/43501
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M ichspi.c
1 file changed, 17 insertions(+), 16 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/ichspi.c b/ichspi.c
index 306b778..b90b864 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -389,13 +389,13 @@
 #define _pprint_reg(bit, mask, off, val, sep) msg_pdbg("%s=%d" sep, #bit, (val & mask) >> off)
 #define pprint_reg(reg, bit, val, sep) _pprint_reg(bit, reg##_##bit, reg##_##bit##_OFF, val, sep)

-static void prettyprint_ich9_reg_hsfs(uint16_t reg_val)
+static void prettyprint_ich9_reg_hsfs(uint16_t reg_val, enum ich_chipset ich_gen)
 {
    msg_pdbg("HSFS: ");
    pprint_reg(HSFS, FDONE, reg_val, ", ");
    pprint_reg(HSFS, FCERR, reg_val, ", ");
    pprint_reg(HSFS, AEL, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -405,7 +405,7 @@
    	break;
    }
    pprint_reg(HSFS, SCIP, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -420,11 +420,11 @@
    pprint_reg(HSFS, FLOCKDN, reg_val, "\n");
 }

-static void prettyprint_ich9_reg_hsfc(uint16_t reg_val)
+static void prettyprint_ich9_reg_hsfc(uint16_t reg_val, enum ich_chipset ich_gen)
 {
    msg_pdbg("HSFC: ");
    pprint_reg(HSFC, FGO, reg_val, ", ");
-	switch (ich_generation) {
+	switch (ich_gen) {
    case CHIPSET_100_SERIES_SUNRISE_POINT:
    case CHIPSET_C620_SERIES_LEWISBURG:
    case CHIPSET_300_SERIES_CANNON_POINT:
@@ -1283,7 +1283,8 @@
    Returns 0 if the cycle completes successfully without errors within
    timeout us, 1 on errors. */
 static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
-					     unsigned int len)
+					     unsigned int len,
+					     enum ich_chipset ich_gen)
 {
    uint16_t hsfs;
    uint32_t addr;
@@ -1300,8 +1301,8 @@
    	msg_perr("Timeout error between offset 0x%08x and "
    		 "0x%08x (= 0x%08x + %d)!\n",
    		 addr, addr + len - 1, addr, len - 1);
-		prettyprint_ich9_reg_hsfs(hsfs);
-		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
+		prettyprint_ich9_reg_hsfs(hsfs, ich_gen);
+		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen);
    	return 1;
    }

@@ -1310,8 +1311,8 @@
    	msg_perr("Transaction error between offset 0x%08x and "
    		 "0x%08x (= 0x%08x + %d)!\n",
    		 addr, addr + len - 1, addr, len - 1);
-		prettyprint_ich9_reg_hsfs(hsfs);
-		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));
+		prettyprint_ich9_reg_hsfs(hsfs, ich_gen);
+		prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC), ich_gen);
    	return 1;
    }
    return 0;
@@ -1415,10 +1416,10 @@
    hsfc |= (0x3 << HSFC_FCYCLE_OFF); /* set erase operation */
    hsfc |= HSFC_FGO; /* start */
    msg_pdbg("HSFC used for block erasing: ");
-	prettyprint_ich9_reg_hsfc(hsfc);
+	prettyprint_ich9_reg_hsfc(hsfc, ich_generation);
    REGWRITE16(ICH9_REG_HSFC, hsfc);

-	if (ich_hwseq_wait_for_cycle_complete(timeout, len))
+	if (ich_hwseq_wait_for_cycle_complete(timeout, len, ich_generation))
    	return -1;
    return 0;
 }
@@ -1455,7 +1456,7 @@
    	hsfc |= HSFC_FGO; /* start */
    	REGWRITE16(ICH9_REG_HSFC, hsfc);

-		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation))
    		return 1;
    	ich_read_data(buf, block_len, ICH9_REG_FDATA0);
    	addr += block_len;
@@ -1497,7 +1498,7 @@
    	hsfc |= HSFC_FGO; /* start */
    	REGWRITE16(ICH9_REG_HSFC, hsfc);

-		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))
+		if (ich_hwseq_wait_for_cycle_complete(timeout, block_len, ich_generation))
    		return -1;
    	addr += block_len;
    	buf += block_len;
@@ -1840,7 +1841,7 @@

tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
    	msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
-		prettyprint_ich9_reg_hsfs(tmp2);
+		prettyprint_ich9_reg_hsfs(tmp2, ich_gen);
    	if (tmp2 & HSFS_FLOCKDN) {
    		msg_pinfo("SPI Configuration is locked down.\n");
    		ichspi_lock = 1;
@@ -1856,7 +1857,7 @@
    	if (desc_valid) {
    		tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
    		msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
-			prettyprint_ich9_reg_hsfc(tmp2);
+			prettyprint_ich9_reg_hsfc(tmp2, ich_gen);
    	}

tmp = mmio_readl(ich_spibar + ICH9_REG_FADDR);