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

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

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 -- 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: 3 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 18 Jun 2020 03:04:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

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>. -- 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: 29 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Jakub Czapiga <jacz@semihalf.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Jakub Czapiga <jacz@semihalf.com> Gerrit-MessageType: deleteReviewer

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>. -- 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: 30 Gerrit-Owner: Daniel Gröber <coreboot@dxld.at> Gerrit-Reviewer: Jakub Czapiga <jacz@semihalf.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Jakub Czapiga <jacz@semihalf.com> Gerrit-MessageType: deleteReviewer
participants (3)
-
Daniel Gröber (Code Review)
-
Daniel Gröber (dxld) (Code Review)
-
Julius Werner (Code Review)