New subject: [M] Change in coreboot[master]: spi/winbond: Fix bpbits_to_region edge cases

June 6, 2020

Hello Daniel Gröber,

I'd like you to do a code review. Please visit

    https://review.coreboot.org/c/coreboot/+/42120

to review the following change.


Change subject: spi/winbond: Fix bpbits_to_region edge cases
......................................................................

spi/winbond: Fix bpbits_to_region edge cases

Currently bpbits_to_region returns the wrong protection range for
min/max bp values. See winbond-spi-test for the list. Here we bring
everything in line with the datasheets.

Change-Id: Ia50dce81937402f1022cf2269e33461d3f605dbf
Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
M src/drivers/spi/winbond.c
M tests/drivers/winbond-spi-test.c
2 files changed, 26 insertions(+), 24 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/42120/1

diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c
index 7d44bfe..afffd4d 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -230,8 +230,16 @@
 		tb = !tb;
 	}
 
-	out->offset = tb ? 0 : flash->size - protected_size;
-	out->size = protected_size;
+	if (protected_size == 0) {
+		out->offset = 0;
+		out->size = 0;
+	} else if (protected_size == flash->size) {
+		out->offset = 0;
+		out->size = flash->size;
+	} else {
+		out->offset = tb ? 0 : flash->size - protected_size;
+		out->size = protected_size;
+	}
 }
 
 /*
diff --git a/tests/drivers/winbond-spi-test.c b/tests/drivers/winbond-spi-test.c
index 98c715d..2ed0e90 100644
--- a/tests/drivers/winbond-spi-test.c
+++ b/tests/drivers/winbond-spi-test.c
@@ -41,12 +41,12 @@
 			[0][0][0]  = {   0,   0 },
 			[0][1][0]  = {   0,   0 },
 
-			/* cmp=1, tb=x, bp=0b0000: FIXME: ALL */
-			[1][0][0]  = {   0,   0 },
-			[1][1][0]  = {   0,   0 },
+			/* cmp=1, tb=x, bp=0b0000: ALL */
+			[1][0][0]  = {   0, 512 },
+			[1][1][0]  = {   0, 512 },
 
-			/* cmp=0, tb=x, bp=0b110x|0b1x1x: FIXME: ALL */
-			[0][0 ... 1][10 ... 15] = {   0,   0 },
+			/* cmp=0, tb=x, bp=0b110x|0b1x1x: ALL */
+			[0][0 ... 1][10 ... 15] = {   0, 512 },
 
 			/* cmp=1, tb=x, bp=0b110x|0b1x1x: NONE */
 			[1][0 ... 1][10 ... 15] = {   0,   0 },
@@ -105,9 +105,9 @@
 			[0][0][0]  = {   0,   0 },
 			[0][1][0]  = {   0,   0 },
 
-			/* cmp=0, tb=x, bp=0b111: FIXME: ALL */
-			[0][0][7]  = {   0,   0 },
-			[0][1][7]  = {   0,   0 },
+			/* cmp=0, tb=x, bp=0b111: ALL */
+			[0][0][7]  = {   0, 256 },
+			[0][1][7]  = {   0, 256 },
 
 			[0][0][1]  = { 252,   4 },
 			[0][0][2]  = { 248,   8 },
@@ -123,9 +123,9 @@
 			[0][1][5]  = {   0,  64 },
 			[0][1][6]  = {   0, 128 },
 
-			/* cmp=1, tb=x, bp=0b000: FIXME: ALL */
-			[1][0][0]  = {   0,   0 },
-			[1][1][0]  = {   0,   0 },
+			/* cmp=1, tb=x, bp=0b000: ALL */
+			[1][0][0]  = {   0, 256 },
+			[1][1][0]  = {   0, 256 },
 
 			/* cmp=1, tb=x, bp=0b111: NONE */
 			[1][0][7]  = {   0, 0   },
@@ -234,23 +234,16 @@
 
 		assert_non_null(table);
 
-		/* FIXME: Max values below are clamped to 6 and 9
-		 * respectively because the implementation currently thinks
-		 * everything above those is fully protected when the
-		 * datasheets say the protection range is empty in those
-		 * cases. */
 		u8 bp_max;
 		if (part->bp_bits == 3)
-			bp_max = 6;
+			bp_max = 7;
 		else if (part->bp_bits == 4)
-			bp_max = 9;
+			bp_max = 0xf;
 		else
 			fail_msg("Unrecognized bp_bits");
 
 		for (u8 tbcmp = 0; tbcmp < 4; tbcmp++) {
-			/* FIXME: bp=0 isn't tested because the
-			 * implementation is currently wrong */
-			for (u8 bp = 1; bp <= bp_max; bp++) {
+			for (u8 bp = 0; bp <= bp_max; bp++) {
 				struct spi_flash_bpbits bpbits = {
 					.cmp = tbcmp & (1<<1),
 					.tb  = tbcmp & (1<<0),
@@ -265,7 +258,8 @@
 		}
 	}
 
-	assert_true(n_cfgs_tested == 60);
+	/* print_message("n_cfgs_tested: %d\n", n_cfgs_tested); */
+	assert_true(n_cfgs_tested == 96);
 }
 
  /* Mocks */

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42120
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia50dce81937402f1022cf2269e33461d3f605dbf
Gerrit-Change-Number: 42120
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org>
Gerrit-MessageType: newchange

    

Change in coreboot[master]: spi/winbond: Fix bpbits_to_region edge cases

Daniel Gröber (dxld) (Code Review)

Julius Werner (Code Review)

Daniel Gröber (dxld) (Code Review)

Daniel Gröber (Code Review)

tags

participants (3)