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 */
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42120 )
Change subject: spi/winbond: Fix bpbits_to_region edge cases ......................................................................
Patch Set 3: Code-Review+2
Attention is currently required from: Jakub Czapiga.
Daniel Gröber (dxld) has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42120 )
Change subject: spi/winbond: Fix bpbits_to_region edge cases ......................................................................
Removed cc Daniel Gröber dxld@darkboxed.org.
Attention is currently required from: Jakub Czapiga.
Daniel Gröber has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42120 )
Change subject: spi/winbond: Fix bpbits_to_region edge cases ......................................................................
Removed reviewer Daniel Gröber dxld@darkboxed.org.
-
Daniel Gröber (Code Review) -
Daniel Gröber (dxld) (Code Review)
-
Julius Werner (Code Review)