Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42117
to review the following change.
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
spi/winbond: Add a test for block protection status bits
With refactoring out of the way we can add a test to make sure the region<>bpbits mapping is correct. Indeed we found that a number of configurations are incorrect.
The datasheets say that min/max bp values should protect either all or none of the flash depending on the cmp and tb bits. However we currently always return no protection for these values.
The test merely documents the current behavour, a fix will come in a later commit.
Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c A tests/drivers/Makefile.inc A tests/drivers/winbond-spi-test.c 6 files changed, 344 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42117/1
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index a389bc4..33416b5 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -337,7 +337,7 @@ }; #define IDCODE_LEN 5
-static int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, +int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, const struct spi_flash_vendor_info *vi, const struct spi_flash_part_id *part) { diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index b4d39b3..1ed0ab3 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -24,6 +24,13 @@ /* Common status */ #define STATUS_WIP 0x01
+struct spi_flash_vendor_info; +struct spi_flash_part_id; + +int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, + const struct spi_flash_vendor_info *vi, + const struct spi_flash_part_id *part); + /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len);
diff --git a/src/drivers/spi/spi_winbond.h b/src/drivers/spi/spi_winbond.h index bdf6694..ff799ff 100644 --- a/src/drivers/spi/spi_winbond.h +++ b/src/drivers/spi/spi_winbond.h @@ -21,3 +21,16 @@
/* tw: Maximum time to write a flash cell in milliseconds */ #define WINBOND_FLASH_TIMEOUT 30 + +int winbond_region_to_bpbits( + const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode, + struct spi_flash_bpbits *bits); + +void winbond_bpbits_to_region( + const size_t granularity, + const struct spi_flash_bpbits *bits, + const size_t flash_size, + struct region *out); diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index f68d9d9..d7a750b 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -211,21 +211,14 @@ static int winbond_get_bpbits(const struct spi_flash *flash, struct spi_flash_bpbits *bpbits);
-static int winbond_region_to_bpbits( - const struct spi_flash *flash, - const struct spi_flash_part_id *params, - const struct region *region, - const enum spi_flash_status_reg_lockdown mode, - struct spi_flash_bpbits *bits); - /* * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. */ -static void winbond_bpbits_to_region(const size_t granularity, - const struct spi_flash_bpbits *bits, - const size_t flash_size, - struct region *out) +void winbond_bpbits_to_region(const size_t granularity, + const struct spi_flash_bpbits *bits, + const size_t flash_size, + struct region *out) { size_t protected_size = MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash_size); @@ -514,7 +507,7 @@ return ret; }
-static int winbond_region_to_bpbits( +int winbond_region_to_bpbits( const struct spi_flash *flash, const struct spi_flash_part_id *params, const struct region *region, diff --git a/tests/drivers/Makefile.inc b/tests/drivers/Makefile.inc new file mode 100644 index 0000000..6e5dff3 --- /dev/null +++ b/tests/drivers/Makefile.inc @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0-only + +tests-y += winbond-spi-test + +winbond-spi-test-srcs += tests/drivers/winbond-spi-test.c +winbond-spi-test-srcs += src/drivers/spi/winbond.c +winbond-spi-test-srcs += src/drivers/spi/spi_flash.c +winbond-spi-test-srcs += src/commonlib/region.c +winbond-spi-test-srcs += src/commonlib/mem_pool.c diff --git a/tests/drivers/winbond-spi-test.c b/tests/drivers/winbond-spi-test.c new file mode 100644 index 0000000..da7177b --- /dev/null +++ b/tests/drivers/winbond-spi-test.c @@ -0,0 +1,309 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#include <string.h> + +#include <stdio.h> /* debug */ + +#include <commonlib/helpers.h> +#include <spi_flash.h> +#include <spi-generic.h> +#include "../drivers/spi/spi_flash_internal.h" +#include "../drivers/spi/spi_winbond.h" + +int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, + const struct spi_flash_vendor_info *vi, + const struct spi_flash_part_id *part); + +struct block_region { + /* these are in units of 64k-byte blocks */ + size_t offset; + size_t size; +}; + +struct bp_table { + uint16_t id; + size_t size; + struct block_region wp_blocks[2][2][1<<4]; +} bp_table[] = { + { + /* W25Q256J */ + .id = 0x7019, + .wp_blocks = { + /* [cmp][tb][bp] */ + /* off size */ + + /* cmp=0, tb=x, bp=0b0000: NONE */ + [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=0, tb=x, bp=0b110x|0b1x1x: FIXME: ALL */ + [0][0 ... 1][10 ... 15] = { 0, 0 }, + + /* cmp=1, tb=x, bp=0b110x|0b1x1x: NONE */ + [1][0 ... 1][10 ... 15] = { 0, 0 }, + + [0][0][1] = { 511, 1 }, + [0][0][2] = { 510, 2 }, + [0][0][3] = { 508, 4 }, + [0][0][4] = { 504, 8 }, + [0][0][5] = { 496, 16 }, + [0][0][6] = { 480, 32 }, + [0][0][7] = { 448, 64 }, + [0][0][8] = { 384, 128 }, + [0][0][9] = { 256, 256 }, + + [0][1][1] = { 0, 1 }, + [0][1][2] = { 0, 2 }, + [0][1][3] = { 0, 4 }, + [0][1][4] = { 0, 8 }, + [0][1][5] = { 0, 16 }, + [0][1][6] = { 0, 32 }, + [0][1][7] = { 0, 64 }, + [0][1][8] = { 0, 128 }, + [0][1][9] = { 0, 256 }, + + [1][0][1] = { 0, 512-1 }, + [1][0][2] = { 0, 512-2 }, + [1][0][3] = { 0, 512-4 }, + [1][0][4] = { 0, 512-8 }, + [1][0][5] = { 0, 512-16 }, + [1][0][6] = { 0, 512-32 }, + [1][0][7] = { 0, 512-64 }, + [1][0][8] = { 0, 512-128 }, + [1][0][9] = { 0, 512-256 }, + + [1][1][1] = { 1, 512-1 }, + [1][1][2] = { 2, 512-2 }, + [1][1][3] = { 4, 512-4 }, + [1][1][4] = { 8, 512-8 }, + [1][1][5] = { 16, 512-16 }, + [1][1][6] = { 32, 512-32 }, + [1][1][7] = { 64, 512-64 }, + [1][1][8] = { 128, 512-128 }, + [1][1][9] = { 256, 512-256 }, + } + }, + + { + /* W25Q128FW */ + .id = 0x6018, + .wp_blocks = { + /* [cmp][tb][bp] */ + /* off size */ + [0][0][0] = { 0, 0 }, + + /* cmp=0, tb=x, bp=0b000: NONE */ + [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 }, + + [0][0][1] = { 252, 4 }, + [0][0][2] = { 248, 8 }, + [0][0][3] = { 240, 16 }, + [0][0][4] = { 224, 32 }, + [0][0][5] = { 192, 64 }, + [0][0][6] = { 128, 128 }, + + [0][1][1] = { 0, 4 }, + [0][1][2] = { 0, 8 }, + [0][1][3] = { 0, 16 }, + [0][1][4] = { 0, 32 }, + [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=0b111: NONE */ + [1][0][7] = { 0, 0 }, + [1][1][7] = { 0, 0 }, + + [1][0][1] = { 0, 256-4 }, + [1][0][2] = { 0, 256-8 }, + [1][0][3] = { 0, 256-16 }, + [1][0][4] = { 0, 256-32 }, + [1][0][5] = { 0, 256-64 }, + [1][0][6] = { 0, 256-128 }, + + [1][1][1] = { 4, 256-4 }, + [1][1][2] = { 8, 256-8 }, + [1][1][3] = { 16, 256-16 }, + [1][1][4] = { 32, 256-32 }, + [1][1][5] = { 64, 256-64 }, + [1][1][6] = { 128, 256-128 }, + } + } +}; + +static void test_one_config( + void **state, + struct spi_flash flash, + const struct spi_flash_part_id *part, + const struct bp_table *table, + const struct spi_flash_bpbits *bpbits) +{ + const size_t granularity = + 1<<part->protection_granularity_shift; + + struct region expected, out = {-1, -1}; + struct region roundtripped = {-1, -1}; + + winbond_bpbits_to_region(granularity, bpbits, + flash.size, &out); + + enum spi_flash_status_reg_lockdown mode = + SPI_WRITE_PROTECTION_PIN; + struct spi_flash_bpbits bpbits1; + winbond_region_to_bpbits(&flash, part, &out, mode, + &bpbits1); + + winbond_bpbits_to_region(granularity, &bpbits1, + flash.size, &roundtripped); + + const struct block_region *wp_blocks = + &table->wp_blocks[bpbits->cmp][bpbits->tb][bpbits->bp]; + const size_t block_size = 1<<16; + + expected.offset = wp_blocks->offset * block_size; + expected.size = wp_blocks->size * block_size; + +#if 0 + /* debugging */ + print_message("\nfor part 0x%04x cmp=%d tb=%d bp=%d:\n", + part->id[0], (int)bpbits->cmp, (int)bpbits->tb, + (int)bpbits->bp); + print_message("should: 0x%08zx-0x%08zx\n", + region_offset(&expected), + region_end(&expected)); + print_message("is: 0x%08zx-0x%08zx\n", + region_offset(&out), + region_end(&out)); + print_message("rt'ed: 0x%08zx-0x%08zx\n", + region_offset(&roundtripped), + region_end(&roundtripped)); + print_message("bp1: %d\n", bpbits1.bp); +#endif + + assert_true(expected.size == out.size); + assert_true(expected.offset == out.offset); + + assert_true(expected.size == roundtripped.size); + assert_true(expected.offset == roundtripped.offset); +} + +static void winbond_region_bpbits_roundtrip_test(void **state) +{ + int n_cfgs_tested = 0; + + const struct spi_flash_vendor_info *vi = &spi_flash_winbond_vi; + + for (size_t i = 0; i < vi->nr_part_ids; i++) { + const struct spi_flash_part_id *part = &vi->ids[i]; + + if (!part->protection_granularity_shift) + continue; + + /* FIXME: Add tables for the other parts supporting block + * protection */ + if (part->id[0] != 0x7019 && part->id[0] != 0x6018) + continue; + + struct spi_slave spi; + struct spi_flash flash; + int ret = fill_spi_flash(&spi, &flash, vi, part); + assert_int_equal(ret, 0); + + const struct bp_table *table = NULL; + for (uint16_t j = 0; j < ARRAY_SIZE(bp_table); j++) { + if (bp_table[j].id == part->id[0]) { + table = &bp_table[j]; + break; + } + } + + 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; + else if (part->bp_bits == 4) + bp_max = 9; + 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++) { + struct spi_flash_bpbits bpbits = { + .cmp = tbcmp & (1<<1), + .tb = tbcmp & (1<<0), + .bp = bp, + }; + + test_one_config( + state, flash, part, table, &bpbits); + + n_cfgs_tested++; + } + } + } + + assert_true(n_cfgs_tested == 60); +} + + /* Mocks */ +int do_printk(int msg_level, const char *fmt, ...) +{ + return -1; +} +int spi_claim_bus(const struct spi_slave *slave) +{ + return -1; +} +int spi_xfer_vector(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return -1; +} +void spi_release_bus(const struct spi_slave *slave) +{ +} +unsigned int spi_crop_chunk(const struct spi_slave *slave, unsigned int cmd_len, + unsigned int buf_len) +{ + return 0; +} +struct mono_time; +void timer_monotonic_get(struct mono_time *mt) +{ +} + +const struct spi_flash_ops_descriptor spi_flash_pp_0x20_sector_desc; + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(winbond_region_bpbits_roundtrip_test), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Daniel Gröber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42117
to look at the new patch set (#2).
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
spi/winbond: Add a test for block protection status bits
With refactoring out of the way we can add a test to make sure the region<>bpbits mapping is correct. Indeed we found that a number of configurations are incorrect.
The datasheets say that min/max bp values should protect either all or none of the flash depending on the cmp and tb bits. However we currently always return no protection for these values.
The test merely documents the current behavour, a fix will come in a later commit.
Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c A tests/drivers/Makefile.inc A tests/drivers/winbond-spi-test.c 6 files changed, 348 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42117/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42117 )
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42117/3/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/c/coreboot/+/42117/3/src/drivers/spi/spi_flash.c... PS3, Line 339: int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, Sorry, our test infrastructure is still very new so there are a lot of exciting "firsts" to discover. I think you're the first one who wants to test a static function.
While I agree things need to be testable I always prefer tests to effect real firmware images as little as possible (and something like this may effect what the compiler can inline, for example). How about we create a new macro static_testable or something like that which resolves to 'static' when building firmware normally but to nothing when building for tests?
To tell the two apart you would have to add something like -D__UNIT_TEST__ to TEST_CFLAGS in tests/Makefile.inc. The macro itself could go into src/commonlib/bsd/include/commonlib/bsd/helpers.h (which is where we keep a lot of random utility stuff, it's always included as part of <types.h>).
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... File tests/drivers/winbond-spi-test.c:
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... PS3, Line 274: int do_printk(int msg_level, const char *fmt, ...) See my comment in https://review.coreboot.org/c/coreboot/+/42313/1/tests/lib/b64_decode-test.c... , I think we should be creating reusable stubs for all of these that are likely to be needed by many different tests. Here you could just reuse the console-stub.c I suggested to add on that CL.
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... PS3, Line 296: void timer_monotonic_get(struct mono_time *mt) This would be another prime candidate for a common stub file. I would suggest actually calling clock_gettime() to get a real monotonic timer value here, otherwise code might behave weirdly because the struct mono_time that's passed in may be an uninitialized stack allocation (so it can be completely random, including making the time go backwards and things like that).
Attention is currently required from: Jakub Czapiga.
Hello build bot (Jenkins), Martin L Roth, Daniel Gröber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42117
to look at the new patch set (#26).
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
spi/winbond: Add a test for block protection status bits
With refactoring out of the way we can add a test to make sure the region<>bpbits mapping is correct. Indeed we found that a number of configurations are incorrect.
The datasheets say that min/max bp values should protect either all or none of the flash depending on the cmp and tb bits. However we currently always return no protection for these values.
The test merely documents the current behavour, a fix will come in a later commit.
Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c A tests/drivers/Makefile.inc A tests/drivers/spi-flash-winbond-test.c 5 files changed, 330 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42117/26
Attention is currently required from: Daniel Gröber, Jakub Czapiga.
Hello build bot (Jenkins), Martin L Roth, Jakub Czapiga, Daniel Gröber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42117
to look at the new patch set (#27).
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
spi/winbond: Add a test for block protection status bits
With refactoring out of the way we can add a test to make sure the region<>bpbits mapping is correct. Indeed we found that a number of configurations are incorrect.
The datasheets say that min/max bp values should protect either all or none of the flash depending on the cmp and tb bits. However we currently always return no protection for these values.
The test merely documents the current behavour, a fix will come in a later commit.
Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c A tests/drivers/Makefile.inc A tests/drivers/spi-flash-winbond-test.c 5 files changed, 326 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42117/27
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/+/42117 )
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
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/+/42117 )
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
Removed reviewer Daniel Gröber dxld@darkboxed.org.
Attention is currently required from: Daniel Gröber, Daniel Gröber (dxld), Jakub Czapiga.
Hello Daniel Gröber, Jakub Czapiga, Martin L Roth, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42117?usp=email
to look at the new patch set (#30).
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
spi/winbond: Add a test for block protection status bits
With refactoring out of the way we can add a test to make sure the region<>bpbits mapping is correct. Indeed we found that a number of configurations are incorrect.
The datasheets say that min/max bp values should protect either all or none of the flash depending on the cmp and tb bits. However we currently always return no protection for these values.
The test merely documents the current behavour, a fix will come in a later commit.
Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c M tests/drivers/Makefile.inc A tests/drivers/spi-flash-winbond-test.c 5 files changed, 303 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42117/30
-
Daniel Gröber (Code Review) -
Daniel Gröber (dxld) (Code Review)
-
Julius Werner (Code Review)