coreboot-gerrit August 2020

coreboot-gerrit@coreboot.org

1 participants
2125 discussions

Change in coreboot[master]: spi/winbond: Merge redundant if wrapper into switch
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42118 to review the following change. Change subject: spi/winbond: Merge redundant if wrapper into switch ...................................................................... spi/winbond: Merge redundant if wrapper into switch Change-Id: I9c66fed2e4dcf4a13933c77e828ebc4ac4126ea4 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 22 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/42118/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index d7a750b..49d3c5b 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -553,33 +553,33 @@ return -1; } - if (mode != SPI_WRITE_PROTECTION_PRESERVE) { - switch (mode) { - case SPI_WRITE_PROTECTION_NONE: - bits->winbond.srp1 = 0; - bits->winbond.srp0 = 0; + switch (mode) { + case SPI_WRITE_PROTECTION_PRESERVE: break; - case SPI_WRITE_PROTECTION_PIN: - bits->winbond.srp1 = 0; - bits->winbond.srp0 = 1; + case SPI_WRITE_PROTECTION_NONE: + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 0; break; - case SPI_WRITE_PROTECTION_REBOOT: + case SPI_WRITE_PROTECTION_PIN: + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 1; + break; + case SPI_WRITE_PROTECTION_REBOOT: + bits->winbond.srp1 = 1; + bits->winbond.srp0 = 0; + break; + case SPI_WRITE_PROTECTION_PERMANENT: + if (params->bp_bits == 3) { bits->winbond.srp1 = 1; - bits->winbond.srp0 = 0; - break; - case SPI_WRITE_PROTECTION_PERMANENT: - if (params->bp_bits == 3) { - bits->winbond.srp1 = 1; - bits->winbond.srp0 = 1; - } else { - /* FIXME: special permanent protect write - * sequence not implemented. */ - return -1; - } - break; - default: + bits->winbond.srp0 = 1; + } else { + /* FIXME: special permanent protect write sequence + * not implemented. */ return -1; } + break; + default: + return -1; } return 0; -- To view, visit https://review.coreboot.org/c/coreboot/+/42118 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9c66fed2e4dcf4a13933c77e828ebc4ac4126ea4 Gerrit-Change-Number: 42118 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: spi/winbond: Add a test for block protection status bits
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

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(a)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); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/42117 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb0632221203166288f9608c8492c1d4bd77dd36 Gerrit-Change-Number: 42117 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: spi/winbond: Pull out winbond_set_bpbits function
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42121 to review the following change. Change subject: spi/winbond: Pull out winbond_set_bpbits function ...................................................................... spi/winbond: Pull out winbond_set_bpbits function Split bpbits write code from winbond_set_write_protection into a new function: winbond_set_bpbits. Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 68 insertions(+), 54 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42121/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index afffd4d..b834585 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -210,6 +210,9 @@ static int winbond_get_bpbits(const struct spi_flash *flash, struct spi_flash_bpbits *bpbits); +static int winbond_set_bpbits(const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct spi_flash_bpbits *bpbits); /* * Convert BPx, TB and CMP to a region. @@ -432,84 +435,48 @@ return ret; } -/* - * Available on all devices. - * Protect a region starting from start of flash or end of flash. - * The caller must provide a supported protected region size. - * SEC isn't supported and set to zero. - * Write block protect bits to Status/Status2 Reg. - * Optionally lock the status register if lock_sreg is set with the provided - * mode. - * - * @param flash: The flash to operate on - * @param region: The region to write protect - * @param mode: Optional status register lock-down mode - * - * @return 0 on success - */ -static int -winbond_set_write_protection(const struct spi_flash *flash, - const struct region *region, - const enum spi_flash_status_reg_lockdown mode) +static int winbond_set_bpbits(const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct spi_flash_bpbits *bpbits) { - const struct spi_flash_part_id *params; - struct status_regs mask, val; - struct spi_flash_bpbits bpbits; + struct status_regs msk, val; int ret; - params = flash->part; - - if (!params) - return -1; - - ret = winbond_get_bpbits(flash, &bpbits); - if (!ret) - return ret; - - ret = winbond_region_to_bpbits(flash, params, region, mode, &bpbits); - if (!ret) - return ret; - - /* Write block protection bits */ - if (params->bp_bits == 3) { val.reg1 = (union status_reg1) { - .bp3 = { .bp = bpbits.bp, .tb = bpbits.tb, .sec = 0 } + .bp3 = { .bp = bpbits->bp, .tb = bpbits->tb, .sec = 0 } }; - mask.reg1 = (union status_reg1) { + msk.reg1 = (union status_reg1) { .bp3 = { .bp = ~0, .tb = 1, .sec = 1 } }; } else { val.reg1 = (union status_reg1) { - .bp4 = { .bp = bpbits.bp, .tb = bpbits.tb } + .bp4 = { .bp = bpbits->bp, .tb = bpbits->tb } }; - mask.reg1 = (union status_reg1) { + msk.reg1 = (union status_reg1) { .bp4 = { .bp = ~0, .tb = 1 } }; } - val.reg2 = (union status_reg2) { .cmp = bpbits.cmp }; - mask.reg2 = (union status_reg2) { .cmp = 1 }; + val.reg2 = (union status_reg2) { .cmp = bpbits->cmp }; + msk.reg2 = (union status_reg2) { .cmp = 1 }; if (params->bp_bits == 3) { - val.reg1.bp3.srp0 = bpbits.winbond.srp0; - mask.reg1.bp3.srp0 = 1; + val.reg1.bp3.srp0 = bpbits->winbond.srp0; + msk.reg1.bp3.srp0 = 1; } else { - val.reg1.bp4.srp = bpbits.winbond.srp0; - mask.reg1.bp4.srp = 1; + val.reg1.bp4.srp = bpbits->winbond.srp; + msk.reg1.bp4.srp = 1; } - val.reg2.srp1 = bpbits.winbond.srp1; - mask.reg2.srp1 = 1; + val.reg2.srp1 = bpbits->winbond.srp1; + msk.reg2.srp1 = 1; - ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); + ret = winbond_flash_cmd_status(flash, msk.u, val.u, true); if (ret) return ret; - printk(BIOS_DEBUG, "WINBOND: write-protection set to range " - "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); - - return ret; + return 0; } int winbond_region_to_bpbits( @@ -590,6 +557,53 @@ return 0; } +/* + * Available on all devices. + * Protect a region starting from start of flash or end of flash. + * The caller must provide a supported protected region size. + * SEC isn't supported and set to zero. + * Write block protect bits to Status/Status2 Reg. + * Optionally lock the status register if lock_sreg is set with the provided + * mode. + * + * @param flash: The flash to operate on + * @param region: The region to write protect + * @param mode: Optional status register lock-down mode + * + * @return 0 on success + */ +static int +winbond_set_write_protection(const struct spi_flash *flash, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode) +{ + const struct spi_flash_part_id *params; + struct spi_flash_bpbits bpbits; + int ret; + + params = flash->part; + if (!params) + return -1; + + ret = winbond_get_bpbits(flash, &bpbits); + if (!ret) + return ret; + + ret = winbond_region_to_bpbits(flash, params, region, mode, &bpbits); + if (!ret) + return ret; + + + ret = winbond_set_bpbits(flash, params, &bpbits); + if (!ret) + return ret; + + printk(BIOS_DEBUG, "WINBOND: write-protection set to range " + "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + return ret; +} + static const struct spi_flash_protection_ops spi_flash_protection_ops = { .get_write = winbond_get_write_protection, .set_write = winbond_set_write_protection, -- To view, visit https://review.coreboot.org/c/coreboot/+/42121 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98 Gerrit-Change-Number: 42121 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: spi/winbond: Add 'params' as get_bpbits arg
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42122 to review the following change. Change subject: spi/winbond: Add 'params' as get_bpbits arg ...................................................................... spi/winbond: Add 'params' as get_bpbits arg This lets us avoid repeted checks for flash->part being NULL. Change-Id: I385fd0a73733ce23b3cc22557037f4a9aa3b331d Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 4 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/42122/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index b834585..d127287 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -209,6 +209,7 @@ }; static int winbond_get_bpbits(const struct spi_flash *flash, + const struct spi_flash_part_id *params, struct spi_flash_bpbits *bpbits); static int winbond_set_bpbits(const struct spi_flash *flash, const struct spi_flash_part_id *params, @@ -267,7 +268,7 @@ if (!params) return -1; - ret = winbond_get_bpbits(flash, &bpbits); + ret = winbond_get_bpbits(flash, params, &bpbits); if (ret) return ret; @@ -286,16 +287,13 @@ } static int winbond_get_bpbits(const struct spi_flash *flash, + const struct spi_flash_part_id *params, struct spi_flash_bpbits *bpbits) { union status_reg1 reg1 = { .u = 0 }; union status_reg2 reg2 = { .u = 0 }; int ret; - const struct spi_flash_part_id *params = flash->part; - if (!params) - return -1; - ret = spi_flash_cmd(&flash->spi, flash->status_cmd, &reg1.u, sizeof(reg1.u)); if (ret) @@ -585,7 +583,7 @@ if (!params) return -1; - ret = winbond_get_bpbits(flash, &bpbits); + ret = winbond_get_bpbits(flash, params, &bpbits); if (!ret) return ret; -- To view, visit https://review.coreboot.org/c/coreboot/+/42122 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I385fd0a73733ce23b3cc22557037f4a9aa3b331d Gerrit-Change-Number: 42122 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: spi: Use common code for top-level flash block protection functions
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42123 to review the following change. Change subject: spi: Use common code for top-level flash block protection functions ...................................................................... spi: Use common code for top-level flash block protection functions This turns *_get_write_protection and *_set_write_protection into common code by having the vendor code implement getter/setter functions for bpbits and convertion functions between bpbits and a region. Change-Id: I509a1cedff5de4bee34b8856ce651d100a32fd13 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c M src/include/spi_flash.h 5 files changed, 156 insertions(+), 222 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42123/1 diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 40f4a47..e5a7b98 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -316,64 +316,6 @@ return 0; } - -/* - * Read block protect bits from Status Reg. - * Converts block protection bits to a region. - * - * Returns: - * -1 on error - * 1 if region is covered by write protection - * 0 if a part of region isn't covered by write protection - */ -static int macronix_get_write_protection(const struct spi_flash *flash, - const struct region *region) -{ - const struct spi_flash_part_id *params; - struct region wp_region; - int ret; - - params = flash->part; - - if (!params) - return -1; - - if (!params->protection_granularity_shift) - return -1; - - struct spi_flash_bpbits bpbits = {}; - ret = macronix_get_bpbits(flash, params, &bpbits); - if (!ret) - return ret; - - ret = macronix_bpbits_to_region(flash, params, &bpbits, &wp_region); - if (!ret) - return ret; - - if (!region_sz(&wp_region)) { - printk(BIOS_DEBUG, "MACRONIX: flash isn't protected\n"); - return 0; - } - - printk(BIOS_DEBUG, "MACRONIX: flash protected range 0x%08zx-0x%08zx\n", - region_offset(&wp_region), region_end(&wp_region)); - - return region_is_subregion(&wp_region, region); -} - -/* - * Protect a region starting from start of flash or end of flash. - * The caller must provide a supported protected region size. - * Writes block protect bits to Status Reg. - * Optionally lock the status register if lock_sreg is set with the provided - * mode. - * - * @param flash: The flash to operate on - * @param region: The region to write protect - * @param mode: Optional status register lock-down mode - * - * @return 0 on success - */ static int macronix_set_bpbits(const struct spi_flash *flash, const struct spi_flash_part_id *params, const struct spi_flash_bpbits *bits) @@ -400,52 +342,12 @@ return 0; } - -static int -macronix_set_write_protection(const struct spi_flash *flash, - const struct region *region, - const enum spi_flash_status_reg_lockdown mode) -{ - const struct spi_flash_part_id *params; - int ret; - - printk(BIOS_DEBUG, "MACRONIX: want to set protection for" - " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); - - params = flash->part; - - if (!params) - return -1; - - if (!params->protection_granularity_shift) { - printk(BIOS_ERR, "MACRONIX: ERROR: spi_flash part doesn't support block protection\n"); - return -1; - } - - struct spi_flash_bpbits bpbits = {}; - ret = macronix_get_bpbits(flash, params, &bpbits); - if (!ret) - return ret; - - ret = macronix_region_to_bpbits(flash, params, region, mode, &bpbits); - if (!ret) { - printk(BIOS_ERR, "MACRONIX: ERROR: unsupported block protection" - " region requested: 0x%08zx-0x%08zx\n", - region_offset(region), region_end(region)); - return ret; - } - - ret = macronix_set_bpbits(flash, params, &bpbits); - if (!ret) - return ret; - - return ret; -} - - static const struct spi_flash_protection_ops spi_flash_protection_ops = { - .get_write = macronix_get_write_protection, - .set_write = macronix_set_write_protection, + .get_bpbits = macronix_get_bpbits, + .set_bpbits = macronix_set_bpbits, + + .bpbits_to_region = macronix_bpbits_to_region, + .region_to_bpbits = macronix_region_to_bpbits, }; const struct spi_flash_vendor_info spi_flash_macronix_vi = { diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 33416b5..e57076b 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -532,6 +532,50 @@ return -1; } +/* + * Read block protect bits from Status Reg and convert to a region. + * + * Returns: + * -1 on error + * 1 if region is covered by write protection + * 0 if a part of region isn't covered by write protection + */ +static int spi_flash_get_write_protection(const struct spi_flash *flash, + const struct region *region) +{ + const struct spi_flash_part_id *params; + struct spi_flash_bpbits bpbits = {}; + struct region wp_region; + int ret; + + params = flash->part; + if (!params) + return -1; + + if (!params->protection_granularity_shift) + return -1; + + ret = flash->prot_ops->get_bpbits(flash, params, &bpbits); + if (ret) + return ret; + + ret = flash->prot_ops->bpbits_to_region(flash, params, &bpbits, + &wp_region); + if (ret) + return ret; + + if (!region_sz(&wp_region)) { + printk(BIOS_DEBUG, "SPI: flash isn't write-protected\n"); + return 0; + } + + printk(BIOS_DEBUG, "SPI: flash write protected range:" + " 0x%08zx-0x%08zx\n", region_offset(&wp_region), + region_end(&wp_region)); + + return region_is_subregion(&wp_region, region); +} + int spi_flash_is_write_protected(const struct spi_flash *flash, const struct region *region) { @@ -551,7 +595,64 @@ return -1; } - return flash->prot_ops->get_write(flash, region); + return spi_flash_get_write_protection(flash, region); +} + +/* + * Protect a region starting from start of flash or end of flash. + * The caller must provide a supported protected region size. + * Writes block protect bits to Status Reg. + * Optionally lock the status register if appropriate mode is used. + * + * @param flash: The flash to operate on + * @param region: The region to write protect + * @param mode: Optional status register lock-down mode + * + * @return 0 on success + */ +static int spi_flash_set_write_protection( + const struct spi_flash *flash, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode) +{ + const struct spi_flash_part_id *params; + int ret; + + printk(BIOS_DEBUG, "SPI: want to set protection for" + " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + params = flash->part; + if (!params) + return -1; + + if (!params->protection_granularity_shift) { + printk(BIOS_ERR, "SPI: ERROR: flash part doesn't support block" + " protection\n"); + return -1; + } + + struct spi_flash_bpbits bpbits = {}; + ret = flash->prot_ops->get_bpbits(flash, params, &bpbits); + if (ret) + return ret; + + ret = flash->prot_ops->region_to_bpbits(flash, params, region, mode, + &bpbits); + if (ret) { + printk(BIOS_ERR, "SPI: ERROR: unsupported block protection" + " region requested: 0x%08zx-0x%08zx\n", + region_offset(region), region_end(region)); + return ret; + } + + ret = flash->prot_ops->set_bpbits(flash, params, &bpbits); + if (ret) + return ret; + + printk(BIOS_DEBUG, "SPI: write-protection range set to " + "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + return 0; } int spi_flash_set_write_protected(const struct spi_flash *flash, @@ -575,7 +676,7 @@ return -1; } - ret = flash->prot_ops->set_write(flash, region, mode); + ret = spi_flash_set_write_protection(flash, region, mode); if (ret == 0 && mode != SPI_WRITE_PROTECTION_PRESERVE) { printk(BIOS_INFO, "SPI: SREG lock-down was set to "); diff --git a/src/drivers/spi/spi_winbond.h b/src/drivers/spi/spi_winbond.h index 2a3a4ae..b4f455a8 100644 --- a/src/drivers/spi/spi_winbond.h +++ b/src/drivers/spi/spi_winbond.h @@ -29,7 +29,7 @@ const enum spi_flash_status_reg_lockdown mode, struct spi_flash_bpbits *bits); -void winbond_bpbits_to_region( +int winbond_bpbits_to_region( const struct spi_flash *flash, const struct spi_flash_part_id *params, const struct spi_flash_bpbits *bits, diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index d127287..9605279 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -219,10 +219,10 @@ * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. */ -void winbond_bpbits_to_region(const struct spi_flash *flash, - const struct spi_flash_part_id *params, - const struct spi_flash_bpbits *bits, - struct region *out) +int winbond_bpbits_to_region(const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct spi_flash_bpbits *bits, + struct region *out) { const size_t granularity = 1 << params->protection_granularity_shift; size_t protected_size = @@ -244,46 +244,8 @@ out->offset = tb ? 0 : flash->size - protected_size; out->size = protected_size; } -} -/* - * Available on all devices. - * Read block protect bits from Status/Status2 Reg. - * Converts block protection bits to a region. - * - * Returns: - * -1 on error - * 1 if region is covered by write protection - * 0 if a part of region isn't covered by write protection - */ -static int winbond_get_write_protection(const struct spi_flash *flash, - const struct region *region) -{ - const struct spi_flash_part_id *params; - struct region wp_region; - struct spi_flash_bpbits bpbits; - int ret; - - params = flash->part; - if (!params) - return -1; - - ret = winbond_get_bpbits(flash, params, &bpbits); - if (ret) - return ret; - - winbond_bpbits_to_region(flash, params, &bpbits, &wp_region); - - if (!region_sz(&wp_region)) { - printk(BIOS_DEBUG, "WINBOND: flash isn't protected\n"); - - return 0; - } - - printk(BIOS_DEBUG, "WINBOND: flash protected range 0x%08zx-0x%08zx\n", - region_offset(&wp_region), region_end(&wp_region)); - - return region_is_subregion(&wp_region, region); + return 0; } static int winbond_get_bpbits(const struct spi_flash *flash, @@ -555,56 +517,12 @@ return 0; } -/* - * Available on all devices. - * Protect a region starting from start of flash or end of flash. - * The caller must provide a supported protected region size. - * SEC isn't supported and set to zero. - * Write block protect bits to Status/Status2 Reg. - * Optionally lock the status register if lock_sreg is set with the provided - * mode. - * - * @param flash: The flash to operate on - * @param region: The region to write protect - * @param mode: Optional status register lock-down mode - * - * @return 0 on success - */ -static int -winbond_set_write_protection(const struct spi_flash *flash, - const struct region *region, - const enum spi_flash_status_reg_lockdown mode) -{ - const struct spi_flash_part_id *params; - struct spi_flash_bpbits bpbits; - int ret; - - params = flash->part; - if (!params) - return -1; - - ret = winbond_get_bpbits(flash, params, &bpbits); - if (!ret) - return ret; - - ret = winbond_region_to_bpbits(flash, params, region, mode, &bpbits); - if (!ret) - return ret; - - - ret = winbond_set_bpbits(flash, params, &bpbits); - if (!ret) - return ret; - - printk(BIOS_DEBUG, "WINBOND: write-protection set to range " - "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); - - return ret; -} - static const struct spi_flash_protection_ops spi_flash_protection_ops = { - .get_write = winbond_get_write_protection, - .set_write = winbond_set_write_protection, + .get_bpbits = winbond_get_bpbits, + .set_bpbits = winbond_set_bpbits, + + .bpbits_to_region = winbond_bpbits_to_region, + .region_to_bpbits = winbond_region_to_bpbits, }; const struct spi_flash_vendor_info spi_flash_winbond_vi = { diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index fe62069..bf84ce0 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -85,37 +85,50 @@ }; }; +struct spi_flash_part_id; + /* Current code assumes all callbacks are supplied in this object. */ struct spi_flash_protection_ops { - /* - * Returns 1 if the whole region is software write protected. - * Hardware write protection mechanism aren't accounted. - * If the write protection could be changed, due to unlocked status - * register for example, 0 should be returned. - * Returns 0 on success. - */ - int (*get_write)(const struct spi_flash *flash, - const struct region *region); - /* - * Enable the status register write protection, if supported on the - * requested region, and optionally enable status register lock-down. - * Returns 0 if the whole region was software write protected. - * Hardware write protection mechanism aren't accounted. - * If the status register is locked and the requested configuration - * doesn't match the selected one, return an error. - * Only a single region is supported ! + + /** + * Read current block protection bits from device status registers. * - * @return 0 on success + * Returns non-zero on error, zero on success. */ - int - (*set_write)(const struct spi_flash *flash, + int (*get_bpbits)(const struct spi_flash *flash, + const struct spi_flash_part_id *part, + struct spi_flash_bpbits *bpbits); + /** + * Write block protection bits to non-volatile device status registers. + * + * Returns non-zero on error, zero on success. + */ + int (*set_bpbits)(const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct spi_flash_bpbits *bpbits); + + /** + * Convert block protection bits to write-protected address region. + * + * Returns non-zero on error, zero on success. + */ + int (*bpbits_to_region)(const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct spi_flash_bpbits *bpbits, + struct region *region); + /** + * Convert address region to be write-protected into block + * protection bits. + * + * Returns non-zero on error, zero on success. + */ + int (*region_to_bpbits)(const struct spi_flash *flash, + const struct spi_flash_part_id *part, const struct region *region, - const enum spi_flash_status_reg_lockdown mode); - + const enum spi_flash_status_reg_lockdown mode, + struct spi_flash_bpbits *bpbits); }; -struct spi_flash_part_id; - struct spi_flash { struct spi_slave spi; u8 vendor; -- To view, visit https://review.coreboot.org/c/coreboot/+/42123 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I509a1cedff5de4bee34b8856ce651d100a32fd13 Gerrit-Change-Number: 42123 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: spi/winbond: Pass flash and part info to bpbits_to_region
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42119 to review the following change. Change subject: spi/winbond: Pass flash and part info to bpbits_to_region ...................................................................... spi/winbond: Pass flash and part info to bpbits_to_region This makes bpbits_to_region mirror region_to_bpbits more closely. Change-Id: Ie60e5feaab29f4db80db6ea688453b042397257e Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/spi_winbond.h M src/drivers/spi/winbond.c M tests/drivers/winbond-spi-test.c 3 files changed, 11 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42119/1 diff --git a/src/drivers/spi/spi_winbond.h b/src/drivers/spi/spi_winbond.h index ff799ff..2a3a4ae 100644 --- a/src/drivers/spi/spi_winbond.h +++ b/src/drivers/spi/spi_winbond.h @@ -30,7 +30,7 @@ struct spi_flash_bpbits *bits); void winbond_bpbits_to_region( - const size_t granularity, + const struct spi_flash *flash, + const struct spi_flash_part_id *params, 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 49d3c5b..7d44bfe 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -215,21 +215,22 @@ * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. */ -void winbond_bpbits_to_region(const size_t granularity, +void winbond_bpbits_to_region(const struct spi_flash *flash, + const struct spi_flash_part_id *params, const struct spi_flash_bpbits *bits, - const size_t flash_size, struct region *out) { + const size_t granularity = 1 << params->protection_granularity_shift; size_t protected_size = - MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash_size); + MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash->size); int tb = bits->tb; if (bits->cmp) { - protected_size = flash_size - protected_size; + protected_size = flash->size - protected_size; tb = !tb; } - out->offset = tb ? 0 : flash_size - protected_size; + out->offset = tb ? 0 : flash->size - protected_size; out->size = protected_size; } @@ -252,18 +253,14 @@ int ret; params = flash->part; - if (!params) return -1; - const size_t granularity = (1 << params->protection_granularity_shift); - ret = winbond_get_bpbits(flash, &bpbits); if (ret) return ret; - winbond_bpbits_to_region(granularity, &bpbits, flash->size, - &wp_region); + winbond_bpbits_to_region(flash, params, &bpbits, &wp_region); if (!region_sz(&wp_region)) { printk(BIOS_DEBUG, "WINBOND: flash isn't protected\n"); diff --git a/tests/drivers/winbond-spi-test.c b/tests/drivers/winbond-spi-test.c index da7177b..98c715d 100644 --- a/tests/drivers/winbond-spi-test.c +++ b/tests/drivers/winbond-spi-test.c @@ -161,8 +161,7 @@ struct region expected, out = {-1, -1}; struct region roundtripped = {-1, -1}; - winbond_bpbits_to_region(granularity, bpbits, - flash.size, &out); + winbond_bpbits_to_region(&flash, part, bpbits, &out); enum spi_flash_status_reg_lockdown mode = SPI_WRITE_PROTECTION_PIN; @@ -170,8 +169,7 @@ winbond_region_to_bpbits(&flash, part, &out, mode, &bpbits1); - winbond_bpbits_to_region(granularity, &bpbits1, - flash.size, &roundtripped); + winbond_bpbits_to_region(&flash, part, &bpbits1, &roundtripped); const struct block_region *wp_blocks = &table->wp_blocks[bpbits->cmp][bpbits->tb][bpbits->bp]; -- To view, visit https://review.coreboot.org/c/coreboot/+/42119 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie60e5feaab29f4db80db6ea688453b042397257e Gerrit-Change-Number: 42119 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: spi/winbond: Fix bpbits_to_region edge cases
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

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(a)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(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42113 to review the following change. Change subject: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region ...................................................................... spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 30 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42113/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index e4151de..ccc7ae9 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -213,16 +213,15 @@ * SEC (if available) must be zero. */ static void winbond_bpbits_to_region(const size_t granularity, - const u8 bp, - bool tb, - const bool cmp, + const struct spi_flash_bpbits *bits, const size_t flash_size, struct region *out) { size_t protected_size = - MIN(bp ? granularity << (bp - 1) : 0, flash_size); + MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash_size); - if (cmp) { + int tb = bits->tb; + if (bits->cmp) { protected_size = flash_size - protected_size; tb = !tb; } @@ -246,8 +245,7 @@ { const struct spi_flash_part_id *params; struct region wp_region; - union status_reg2 reg2; - u8 bp, tb; + struct spi_flash_bpbits bpbits; int ret; params = flash->part; @@ -258,34 +256,49 @@ const size_t granularity = (1 << params->protection_granularity_shift); union status_reg1 reg1 = { .u = 0 }; + union status_reg2 reg2 = { .u = 0 }; ret = spi_flash_cmd(&flash->spi, flash->status_cmd, &reg1.u, sizeof(reg1.u)); if (ret) return ret; + ret = spi_flash_cmd(&flash->spi, CMD_W25_RDSR2, &reg2.u, + sizeof(reg2.u)); + if (ret) + return ret; + if (params->bp_bits == 3) { if (reg1.bp3.sec) { // FIXME: not supported return -1; } - bp = reg1.bp3.bp; - tb = reg1.bp3.tb; + bpbits = (struct spi_flash_bpbits){ + .bp = reg1.bp3.bp, + .cmp = reg2.cmp, + .tb = reg1.bp3.tb, + .winbond = { + .srp0 = reg1.bp3.srp0, + .srp1 = reg2.srp1, + }, + }; } else if (params->bp_bits == 4) { - bp = reg1.bp4.bp; - tb = reg1.bp4.tb; + bpbits = (struct spi_flash_bpbits){ + .bp = reg1.bp4.bp, + .cmp = reg2.cmp, + .tb = reg1.bp4.tb, + .winbond = { + .srp = reg1.bp4.srp0, + .srl = reg2.srp1, + }, + }; } else { // FIXME: not supported return -1; } - ret = spi_flash_cmd(&flash->spi, CMD_W25_RDSR2, &reg2.u, - sizeof(reg2.u)); - if (ret) - return ret; - - winbond_bpbits_to_region(granularity, bp, tb, reg2.cmp, flash->size, + winbond_bpbits_to_region(granularity, &bpbits, flash->size, &wp_region); if (!region_sz(&wp_region)) { -- To view, visit https://review.coreboot.org/c/coreboot/+/42113 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Gerrit-Change-Number: 42113 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 2

Change in coreboot[master]: spi: Share common logic of vendor specific bpbits<>region functions
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42124 to review the following change. Change subject: spi: Share common logic of vendor specific bpbits<>region functions ...................................................................... spi: Share common logic of vendor specific bpbits<>region functions Change-Id: Iadedf160575b20ee7fc671642412e6b3d437a20e Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/Makefile.inc M src/drivers/spi/macronix.c A src/drivers/spi/spi_common.c A src/drivers/spi/spi_common.h M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/winbond.c M tests/drivers/Makefile.inc D tests/drivers/macronix-spi-test.c A tests/drivers/spi-test.c D tests/drivers/winbond-spi-test.c 10 files changed, 608 insertions(+), 635 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/42124/1 diff --git a/src/drivers/spi/Makefile.inc b/src/drivers/spi/Makefile.inc index 6dbc43a..e94d14f 100644 --- a/src/drivers/spi/Makefile.inc +++ b/src/drivers/spi/Makefile.inc @@ -14,7 +14,7 @@ $(1)-y += spi-generic.c $(1)-y += bitbang.c $(1)-$(CONFIG_COMMON_CBFS_SPI_WRAPPER) += cbfs_spi.c -$(1)-$(CONFIG_SPI_FLASH) += spi_flash.c +$(1)-$(CONFIG_SPI_FLASH) += spi_flash.c spi_common.c $(1)-$(CONFIG_SPI_SDCARD) += spi_sdcard.c $(1)-$(CONFIG_BOOT_DEVICE_SPI_FLASH_RW_NOMMAP$(2)) += boot_device_rw_nommap.c $(1)-$(CONFIG_CONSOLE_SPI_FLASH) += flashconsole.c diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index e5a7b98..b19f085 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -7,6 +7,7 @@ #include <lib.h> #include "spi_flash_internal.h" +#include "spi_common.h" #include "spi_macronix.h" /* MX25xx-specific commands */ @@ -47,6 +48,7 @@ .id[0] = 0x2015, .nr_sectors_shift = 9, .protection_granularity_shift = 16, + .bp_bits = 4, .bp_lower_th = 10, }, { @@ -54,6 +56,7 @@ .id[0] = 0x2016, .nr_sectors_shift = 10, .protection_granularity_shift = 16, + .bp_bits = 4, .bp_lower_th = 9, }, { @@ -61,6 +64,7 @@ .id[0] = 0x2017, .nr_sectors_shift = 11, .protection_granularity_shift = 17, + .bp_bits = 4, .bp_lower_th = 9, }, { @@ -167,36 +171,7 @@ const struct spi_flash_bpbits *bits, struct region *out) { - if (bits->bp == 0xf) { - out->offset = 0; - out->size = flash->size; - } else if (bits->bp == 0) { - out->offset = 0; - out->size = 0; - } else { - const u16 granularity_shift = - part->protection_granularity_shift; - - if (bits->bp >= part->bp_lower_th) { - /* Lower half is protected */ - const size_t unprotected_size = - MIN(1<<granularity_shift<<(14 - bits->bp), - flash->size); - - out->offset = 0; - out->size = flash->size - unprotected_size; - } else { - /* Upper half is protected */ - const size_t protected_size = - MIN(1<<(granularity_shift + bits->bp - 1), - flash->size); - - out->offset = flash->size - protected_size; - out->size = protected_size; - } - } - - return 0; + return spi_flash_common_bpbits_to_region(flash, part, bits, out); } /** @@ -212,41 +187,11 @@ const enum spi_flash_status_reg_lockdown mode, struct spi_flash_bpbits *bits) { - if (region->offset == 0 && region->size >= flash->size) { - bits->bp = 0xf; - } else if (region->offset == 0 && region->size == 0) { - bits->bp = 0; - } else if (region->offset >= flash->size) { - return -1; - } else if (region->size >= flash->size) { - return -1; - } else { - const size_t gran_shift = params->protection_granularity_shift; - const u16 granularity = 1<<gran_shift; + int ret; - if (region->offset == 0) { /* Lower half should be protected */ - const size_t unprotected_size = - flash->size - region->size; - - const int is_valid = - IS_POWER_OF_2(unprotected_size) && - unprotected_size >= granularity; - if (!is_valid) - return -1; - - bits->bp = 14 - log2(unprotected_size>>gran_shift); - } else { - const size_t protected_size = region->size; - - const int is_valid = - IS_POWER_OF_2(protected_size) && - protected_size >= granularity; - if (!is_valid) - return -1; - - bits->bp = log2(protected_size>>gran_shift) + 1; - } - } + ret = spi_flash_common_region_to_bpbits(flash, params, region, bits); + if (ret) + return ret; switch (mode) { case SPI_WRITE_PROTECTION_PRESERVE: diff --git a/src/drivers/spi/spi_common.c b/src/drivers/spi/spi_common.c new file mode 100644 index 0000000..88cc23a --- /dev/null +++ b/src/drivers/spi/spi_common.c @@ -0,0 +1,133 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <assert.h> +#include <spi_flash.h> +#include <lib.h> + +#include "spi_flash_internal.h" +#include "spi_common.h" + +int spi_flash_common_region_to_bpbits( + const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct region *region, + struct spi_flash_bpbits *bits) +{ + /* Need to touch TOP or BOTTOM */ + if (region_offset(region) != 0 && region_end(region) != flash->size) + return -1; + + if (region_offset(region) == 0 && part->has_tb) + bits->tb = 1; + else + bits->tb = 0; + + struct region wp_region = *region; + if (region_sz(region) > flash->size / 2 && part->has_cmp) { + assert(part->has_tb); + /* When we want to protect more than half the flash, but + * not the whole thing we can use the cmp bit to make the + * normally protected region specified by bp below to be + * what is unprotected instead */ + bits->cmp = 1; + wp_region.offset = bits->tb ? 0 : region_sz(region); + wp_region.size = flash->size - region_sz(region); + bits->tb = !bits->tb; + } else { + bits->cmp = 0; + } + + if (region_sz(&wp_region) == 0) { + bits->bp = 0; + return 0; + } else if (region_offset(&wp_region) == 0 && + region_sz(&wp_region) == flash->size) { + assert(part->bp_bits > 0); + bits->bp = (1<<part->bp_bits) - 1; + return 0; + } + + const size_t gran_shift = part->protection_granularity_shift; + const size_t granularity = 1 << gran_shift; + + if (region_sz(&wp_region) < granularity) + return -1; + + if (part->bp_lower_th && region_offset(region) == 0) { + const size_t complement_size = flash->size - region->size; + + const int is_valid = IS_POWER_OF_2(complement_size) && + complement_size >= granularity; + if (!is_valid) + return -1; + + bits->bp = 14 - log2(complement_size>>gran_shift); + } else { + const size_t size = region_sz(&wp_region); + + const int is_valid = IS_POWER_OF_2(size) && + size >= granularity; + if (!is_valid) + return -1; + + bits->bp = log2(size) - gran_shift + 1; + } + + return 0; +} + +int spi_flash_common_bpbits_to_region( + const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct spi_flash_bpbits *bits, + struct region *out) +{ + int tb = 0; + if (part->has_tb) { + assert(!part->bp_lower_th); + tb = bits->tb; + } else if (part->bp_lower_th && bits->bp >= part->bp_lower_th) { + tb = 1; + } + + const size_t granularity = + 1 << part->protection_granularity_shift; + + size_t protected_size; + if (part->bp_lower_th && bits->bp >= part->bp_lower_th) { + size_t unprotected_size = + MIN(granularity<<(14 - bits->bp), flash->size); + protected_size = flash->size - unprotected_size; + } else{ + protected_size = + MIN(bits->bp ? granularity << (bits->bp - 1) : 0, + flash->size); + } + + if (part->has_cmp && bits->cmp) { + protected_size = flash->size - protected_size; + tb = !tb; + } + + if (protected_size == 0) { + *out = (struct region){ + .offset = 0, + .size = 0, + }; + } else if (protected_size == flash->size) { + *out = (struct region){ + .offset = 0, + .size = flash->size, + }; + } else { + if (tb) { /* tb=1: BOTTOM */ + out->offset = 0; + } else { /* tb=0: TOP */ + out->offset = flash->size - protected_size; + } + + out->size = protected_size; + } + + return 0; +} diff --git a/src/drivers/spi/spi_common.h b/src/drivers/spi/spi_common.h new file mode 100644 index 0000000..a6aab46 --- /dev/null +++ b/src/drivers/spi/spi_common.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +int spi_flash_common_region_to_bpbits( + const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct region *region, + struct spi_flash_bpbits *bits); + +int spi_flash_common_bpbits_to_region( + const struct spi_flash *flash, + const struct spi_flash_part_id *part, + const struct spi_flash_bpbits *bits, + struct region *out); diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index 98837c2..8c8cff9 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -80,9 +80,12 @@ uint16_t _reserved_for_flags: 3; /* Block protection. */ uint16_t protection_granularity_shift : 5; - uint16_t bp_bits : 3; /* Used by Winbond */ + uint16_t bp_bits : 3; /* bp values starting here protect the lower half */ uint16_t bp_lower_th : 5; /* Used by Macronix */ + uint16_t has_cmp : 1; /* region complement available, currently the + * code assumes has_cmp implies has_tb */ + uint16_t has_tb : 1; /* top/bottom select available */ }; struct spi_flash_ops_descriptor { diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 9605279..8b5b29e 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -8,6 +8,7 @@ #include <lib.h> #include "spi_flash_internal.h" +#include "spi_common.h" #include "spi_winbond.h" union status_reg1 { @@ -101,6 +102,7 @@ .id[0] = 0x4014, .nr_sectors_shift = 8, .fast_read_dual_output_support = 1, + .has_tb = 1, }, { /* W25Q16_V */ @@ -109,6 +111,7 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, + .has_tb = 1, }, { /* W25Q16DW */ @@ -117,6 +120,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q32_V */ @@ -125,6 +130,7 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, + .has_tb = 1, }, { /* W25Q32DW */ @@ -133,6 +139,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q64_V */ @@ -141,6 +149,7 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, + .has_tb = 1, }, { /* W25Q64DW */ @@ -149,6 +158,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q64JW */ @@ -157,6 +168,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 17, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q128_V */ @@ -165,6 +178,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q128FW */ @@ -173,6 +188,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q128J */ @@ -181,6 +198,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q128JW */ @@ -189,6 +208,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 18, .bp_bits = 3, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q256_V */ @@ -197,6 +218,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 4, + .has_cmp = 1, + .has_tb = 1, }, { /* W25Q256J */ @@ -205,6 +228,8 @@ .fast_read_dual_output_support = 1, .protection_granularity_shift = 16, .bp_bits = 4, + .has_cmp = 1, + .has_tb = 1, }, }; @@ -224,28 +249,7 @@ const struct spi_flash_bpbits *bits, struct region *out) { - const size_t granularity = 1 << params->protection_granularity_shift; - size_t protected_size = - MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash->size); - - int tb = bits->tb; - if (bits->cmp) { - protected_size = flash->size - protected_size; - tb = !tb; - } - - 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; - } - - return 0; + return spi_flash_common_bpbits_to_region(flash, params, bits, out); } static int winbond_get_bpbits(const struct spi_flash *flash, @@ -446,43 +450,17 @@ const enum spi_flash_status_reg_lockdown mode, struct spi_flash_bpbits *bits) { - struct region wp_region; - - /* Need to touch TOP or BOTTOM */ - if (region_offset(region) != 0 && region_end(region) != flash->size) - return -1; + int ret; if (params->bp_bits != 3 && params->bp_bits != 4) { /* FIXME: not implemented */ return -1; } - wp_region = *region; - - if (region_offset(&wp_region) == 0) - bits->tb = 1; - else - bits->tb = 0; - - if (region_sz(&wp_region) > flash->size / 2) { - bits->cmp = 1; - wp_region.offset = bits->tb ? 0 : region_sz(&wp_region); - wp_region.size = flash->size - region_sz(&wp_region); - bits->tb = !bits->tb; - } else { - bits->cmp = 0; - } - - if (region_sz(&wp_region) == 0) { - bits->bp = 0; - } else if (IS_POWER_OF_2(region_sz(&wp_region)) && - (region_sz(&wp_region) >= - (1 << params->protection_granularity_shift))) { - bits->bp = log2(region_sz(&wp_region)) - - params->protection_granularity_shift + 1; - } else { + ret = spi_flash_common_region_to_bpbits(flash, params, region, bits); + if (ret) { printk(BIOS_ERR, "WINBOND: ERROR: unsupported region size\n"); - return -1; + return ret; } switch (mode) { diff --git a/tests/drivers/Makefile.inc b/tests/drivers/Makefile.inc index d29502f..142dbaa 100644 --- a/tests/drivers/Makefile.inc +++ b/tests/drivers/Makefile.inc @@ -1,17 +1,11 @@ # SPDX-License-Identifier: GPL-2.0-only -tests-y += winbond-spi-test +tests-y += 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 - -tests-y += macronix-spi-test - -macronix-spi-test-srcs += tests/drivers/macronix-spi-test.c -macronix-spi-test-srcs += src/drivers/spi/macronix.c -macronix-spi-test-srcs += src/drivers/spi/spi_flash.c -macronix-spi-test-srcs += src/commonlib/region.c -macronix-spi-test-srcs += src/commonlib/mem_pool.c +spi-test-srcs += tests/drivers/spi-test.c +spi-test-srcs += src/drivers/spi/winbond.c +spi-test-srcs += src/drivers/spi/macronix.c +spi-test-srcs += src/drivers/spi/spi_flash.c +spi-test-srcs += src/drivers/spi/spi_common.c +spi-test-srcs += src/commonlib/region.c +spi-test-srcs += src/commonlib/mem_pool.c diff --git a/tests/drivers/macronix-spi-test.c b/tests/drivers/macronix-spi-test.c deleted file mode 100644 index bacb9d5..0000000 --- a/tests/drivers/macronix-spi-test.c +++ /dev/null @@ -1,202 +0,0 @@ -/* 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_macronix.h" - -struct bp_table { - uint16_t id; - size_t size; - struct block_region { - /* these are in units of 64k-byte blocks */ - size_t offset; - size_t size; - } wp_blocks[1<<4]; -} bp_table[] = { - { - /* MX25L1605D */ - .id = 0x2015, - .size = 0x200000, - .wp_blocks = { - /* off size */ - [0] = { 0, 0 }, - [1] = { 31, 1 }, - [2] = { 30, 2 }, - [3] = { 28, 4 }, - [4] = { 24, 8 }, - [5] = { 16, 16 }, - [6] = { 0, 32 }, - [7] = { 0, 32 }, - [8] = { 0, 32 }, - [9] = { 0, 32 }, - - [10] = { 0, 16 }, - [11] = { 0, 24 }, - [12] = { 0, 28 }, - [13] = { 0, 30 }, - [14] = { 0, 31 }, - [15] = { 0, 32 }, - }, - }, - { - /* MX25L3205D */ - .id = 0x2016, - .size = 0x400000, - .wp_blocks = { - /* off size */ - [0] = { 0, 0 }, - [1] = { 63, 1 }, - [2] = { 62, 2 }, - [3] = { 60, 4 }, - [4] = { 56, 8 }, - [5] = { 48, 16 }, - [6] = { 32, 32 }, - [7] = { 0, 64 }, - [8] = { 0, 64 }, - - [9] = { 0, 32 }, - [10] = { 0, 48 }, - [11] = { 0, 56 }, - [12] = { 0, 60 }, - [13] = { 0, 62 }, - [14] = { 0, 63 }, - [15] = { 0, 64 }, - }, - }, - { - /* MX25L6405D */ - .id = 0x2017, - .size = 0x800000, - .wp_blocks = { - /* off size */ - [0] = { 0, 0 }, - [1] = { 126, 2 }, - [2] = { 124, 4 }, - [3] = { 120, 8 }, - [4] = { 112, 16 }, - [5] = { 96, 32 }, - [6] = { 64, 64 }, - [7] = { 0, 128 }, - [8] = { 0, 128 }, - - [9] = { 0, 64 }, - [10] = { 0, 96 }, - [11] = { 0, 112 }, - [12] = { 0, 120 }, - [13] = { 0, 124 }, - [14] = { 0, 126 }, - [15] = { 0, 128 }, - }, - }, -}; - - -static void macronix_region_bpbits_roundtrip_test(void **state) -{ - int n_cfgs_tested = 0; - - const struct spi_flash_vendor_info *vi = &spi_flash_macronix_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; - - 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); - - for (u8 bp = 0; bp <= 0xf; bp++) { - struct region expected, out = {-1, -1}; - struct region roundtripped = {-1, -1}; - - const struct block_region *wp_blocks = - &table->wp_blocks[bp]; - const size_t block_size = 1<<16; - - expected.offset = wp_blocks->offset * block_size; - expected.size = wp_blocks->size * block_size; - - struct spi_flash_bpbits bpbits = { - .bp = bp, - }; - - macronix_bpbits_to_region(&flash, part, &bpbits, &out); - - enum spi_flash_status_reg_lockdown mode = - SPI_WRITE_PROTECTION_PIN; - struct spi_flash_bpbits bpbits1 = {}; - int ret = macronix_region_to_bpbits(&flash, part, &out, - mode, &bpbits1); - assert_int_equal(ret, 0); - - /* we have to go through bpbits_to_region again as - * the bp<>range mapping isn't unique. For example - * bp=0b1111 always means everything is protected - * but bp values just after the largest upper-half - * protection also mean "all blocks protected" */ - macronix_bpbits_to_region(&flash, part, &bpbits1, - &roundtripped); - -#if 0 - /* debugging */ - print_message("\nfor part 0x%04x bp=%d:\n", - part->id[0], (int)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", bp1); -#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); - - n_cfgs_tested++; - } - } - - assert_true(n_cfgs_tested == 48); -} - -#include "spi-mocks.c" - -int main(void) -{ - const struct CMUnitTest tests[] = { - cmocka_unit_test(macronix_region_bpbits_roundtrip_test), - }; - - return cmocka_run_group_tests(tests, NULL, NULL); -} diff --git a/tests/drivers/spi-test.c b/tests/drivers/spi-test.c new file mode 100644 index 0000000..97b0af1 --- /dev/null +++ b/tests/drivers/spi-test.c @@ -0,0 +1,410 @@ +/* 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 { + const struct spi_flash_vendor_info *vi; + uint16_t id; + size_t size; + struct block_region wp_blocks[2][2][1<<4]; +} bp_table[] = { + { + /* W25Q16_V */ + .vi = &spi_flash_winbond_vi, + .id = 0x4015, + .wp_blocks = { + /* [cmp][tb][bp] */ + /* off size */ + [0][0][0] = { 0, 0 }, + + /* tb=x, bp=0b000: NONE */ + [0][0][0] = { 0, 0 }, + [0][1][0] = { 0, 0 }, + + /* tb=x, bp=0b11x: ALL */ + [0][0][6] = { 0, 32 }, + [0][1][6] = { 0, 32 }, + [0][0][7] = { 0, 32 }, + [0][1][7] = { 0, 32 }, + + [0][0][1] = { 31, 1 }, + [0][0][2] = { 30, 2 }, + [0][0][3] = { 28, 4 }, + [0][0][4] = { 24, 8 }, + [0][0][5] = { 16, 16 }, + + [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 }, + } + }, + { + /* W25Q256J */ + .vi = &spi_flash_winbond_vi, + .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: ALL */ + [1][0][0] = { 0, 512 }, + [1][1][0] = { 0, 512 }, + + /* 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 }, + + [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 */ + .vi = &spi_flash_winbond_vi, + .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: ALL */ + [0][0][7] = { 0, 256 }, + [0][1][7] = { 0, 256 }, + + [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: ALL */ + [1][0][0] = { 0, 256 }, + [1][1][0] = { 0, 256 }, + + /* 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 }, + } + }, + { + /* MX25L1605D */ + .vi = &spi_flash_macronix_vi, + .id = 0x2015, + .wp_blocks = { + /* off size */ + [0][0][0] = { 0, 0 }, + [0][0][1] = { 31, 1 }, + [0][0][2] = { 30, 2 }, + [0][0][3] = { 28, 4 }, + [0][0][4] = { 24, 8 }, + [0][0][5] = { 16, 16 }, + [0][0][6] = { 0, 32 }, + [0][0][7] = { 0, 32 }, + [0][0][8] = { 0, 32 }, + [0][0][9] = { 0, 32 }, + + [0][0][10] = { 0, 16 }, + [0][0][11] = { 0, 24 }, + [0][0][12] = { 0, 28 }, + [0][0][13] = { 0, 30 }, + [0][0][14] = { 0, 31 }, + [0][0][15] = { 0, 32 }, + }, + }, + { + /* MX25L3205D */ + .vi = &spi_flash_macronix_vi, + .id = 0x2016, + .wp_blocks = { + /* off size */ + [0][0][0] = { 0, 0 }, + [0][0][1] = { 63, 1 }, + [0][0][2] = { 62, 2 }, + [0][0][3] = { 60, 4 }, + [0][0][4] = { 56, 8 }, + [0][0][5] = { 48, 16 }, + [0][0][6] = { 32, 32 }, + [0][0][7] = { 0, 64 }, + [0][0][8] = { 0, 64 }, + + [0][0][9] = { 0, 32 }, + [0][0][10] = { 0, 48 }, + [0][0][11] = { 0, 56 }, + [0][0][12] = { 0, 60 }, + [0][0][13] = { 0, 62 }, + [0][0][14] = { 0, 63 }, + [0][0][15] = { 0, 64 }, + }, + }, + { + /* MX25L6405D */ + .vi = &spi_flash_macronix_vi, + .id = 0x2017, + .wp_blocks = { + /* off size */ + [0][0][0] = { 0, 0 }, + [0][0][1] = { 126, 2 }, + [0][0][2] = { 124, 4 }, + [0][0][3] = { 120, 8 }, + [0][0][4] = { 112, 16 }, + [0][0][5] = { 96, 32 }, + [0][0][6] = { 64, 64 }, + [0][0][7] = { 0, 128 }, + [0][0][8] = { 0, 128 }, + + [0][0][9] = { 0, 64 }, + [0][0][10] = { 0, 96 }, + [0][0][11] = { 0, 112 }, + [0][0][12] = { 0, 120 }, + [0][0][13] = { 0, 124 }, + [0][0][14] = { 0, 126 }, + [0][0][15] = { 0, 128 }, + }, + }, + +}; + +static void test_one_config( + void **state, + const 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}; + + flash->prot_ops->bpbits_to_region(flash, part, bpbits, &out); + + enum spi_flash_status_reg_lockdown mode = + SPI_WRITE_PROTECTION_PIN; + struct spi_flash_bpbits bpbits1; + flash->prot_ops->region_to_bpbits(flash, part, &out, mode, &bpbits1); + + flash->prot_ops->bpbits_to_region(flash, part, &bpbits1, &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:\n", part->id[0]); + print_message("in: cmp=%d tb=%d bp=%d\n", + (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: cmp=%d, tb=%d, bp=%d\n", + bpbits1.cmp, bpbits1.tb, bpbits1.bp); + print_message("rt'ed: 0x%08zx-0x%08zx\n", + region_offset(&roundtripped), + region_end(&roundtripped)); +#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 spi_flash_region_bpbits_roundtrip_test(void **state) +{ + int n_cfgs_tested = 0; + + for (size_t i = 0; i < ARRAY_SIZE(bp_table); i++) { + const struct bp_table *table = &bp_table[i]; + const struct spi_flash_vendor_info *vi = table->vi; + const struct spi_flash_part_id *part = NULL; + + for (size_t j = 0; j < vi->nr_part_ids; j++) { + if (vi->ids[j].id[0] == table->id) { + part = &vi->ids[j]; + break; + } + } + + assert_true(part->protection_granularity_shift); + + struct spi_slave spi; + struct spi_flash flash; + int ret = fill_spi_flash(&spi, &flash, vi, part); + assert_int_equal(ret, 0); + + u8 tb_max = part->has_tb + 1; + u8 cmp_max = part->has_cmp + 1; + for (u8 tb = 0; tb < tb_max; tb++) { + for (u8 cmp = 0; cmp < cmp_max; cmp++) { + u8 bp_max + = part->bp_bits + ? 1<<part->bp_bits + : 0x10; + for (u8 bp = 0; bp < bp_max; bp++) { + struct spi_flash_bpbits bpbits = { + .cmp = cmp, + .tb = tb, + .bp = bp, + }; + + test_one_config( + state, + &flash, + part, + table, + &bpbits); + + n_cfgs_tested++; + } + } + } + } + + size_t n_cfgs_tested_expected = 160; + if (n_cfgs_tested != n_cfgs_tested_expected) { + fail_msg("n_cfgs_tested is=%d, expected=%d!\n", + n_cfgs_tested, n_cfgs_tested_expected); + } +} + + /* 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(spi_flash_region_bpbits_roundtrip_test), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/tests/drivers/winbond-spi-test.c b/tests/drivers/winbond-spi-test.c deleted file mode 100644 index 2ed0e90..0000000 --- a/tests/drivers/winbond-spi-test.c +++ /dev/null @@ -1,301 +0,0 @@ -/* 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: ALL */ - [1][0][0] = { 0, 512 }, - [1][1][0] = { 0, 512 }, - - /* 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 }, - - [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: ALL */ - [0][0][7] = { 0, 256 }, - [0][1][7] = { 0, 256 }, - - [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: ALL */ - [1][0][0] = { 0, 256 }, - [1][1][0] = { 0, 256 }, - - /* 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(&flash, part, bpbits, &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(&flash, part, &bpbits1, &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); - - u8 bp_max; - if (part->bp_bits == 3) - bp_max = 7; - else if (part->bp_bits == 4) - bp_max = 0xf; - else - fail_msg("Unrecognized bp_bits"); - - for (u8 tbcmp = 0; tbcmp < 4; tbcmp++) { - for (u8 bp = 0; 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++; - } - } - } - - /* print_message("n_cfgs_tested: %d\n", n_cfgs_tested); */ - assert_true(n_cfgs_tested == 96); -} - - /* 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); -} -- To view, visit https://review.coreboot.org/c/coreboot/+/42124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iadedf160575b20ee7fc671642412e6b3d437a20e Gerrit-Change-Number: 42124 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 8

Change in coreboot[master]: spi/winbond: Pull out winbond_region_to_bpbits function
by Daniel Gröber (dxld) (Code Review) 27 Feb '23

27 Feb '23

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42115 to review the following change. Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ...................................................................... spi/winbond: Pull out winbond_region_to_bpbits function Split region>bpbits conversion related logic out from winbond_set_write_protection into a new function: winbond_region_to_bpbits. Change-Id: I2c7b08cb56772aa620e690077bbbb1fde3d7aae7 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 94 insertions(+), 59 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/42115/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 77191e4..51d43a8 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -211,6 +211,13 @@ 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. @@ -449,19 +456,77 @@ { const struct spi_flash_part_id *params; struct status_regs mask, val; - struct region wp_region; - u8 cmp, bp, tb; + struct spi_flash_bpbits bpbits; int ret; - /* Need to touch TOP or BOTTOM */ - if (region_offset(region) != 0 && region_end(region) != flash->size) - return -1; - params = flash->part; if (!params) return -1; + ret = winbond_get_bpbits(flash, &bpbits); + if (!ret) + return ret; + + ret = winbond_region_to_bpbits(flash, params, region, mode, &bpbits); + if (!ret) + return ret; + + /* Write block protection bits */ + + if (params->bp_bits == 3) { + val.reg1 = (union status_reg1) { + .bp3 = { .bp = bpbits.bp, .tb = bpbits.tb, .sec = 0 } + }; + mask.reg1 = (union status_reg1) { + .bp3 = { .bp = ~0, .tb = 1, .sec = 1 } + }; + } else { + val.reg1 = (union status_reg1) { + .bp4 = { .bp = bpbits.bp, .tb = bpbits.tb } + }; + mask.reg1 = (union status_reg1) { + .bp4 = { .bp = ~0, .tb = 1 } + }; + } + + val.reg2 = (union status_reg2) { .cmp = bpbits.cmp }; + mask.reg2 = (union status_reg2) { .cmp = 1 }; + + if (params->bp_bits == 3) { + val.reg1.bp3.srp0 = bpbits.winbond.srp0; + mask.reg1.bp3.srp0 = 1; + } else { + val.reg1.bp4.srp0 = bpbits.winbond.srp0; + mask.reg1.bp4.srp0 = 1; + } + + val.reg2.srp1 = bpbits.winbond.srp1; + mask.reg2.srp1 = 1; + + ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); + if (ret) + return ret; + + printk(BIOS_DEBUG, "WINBOND: write-protection set to range " + "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + return ret; +} + +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) +{ + struct region wp_region; + + /* Need to touch TOP or BOTTOM */ + if (region_offset(region) != 0 && region_end(region) != flash->size) + return -1; + if (params->bp_bits != 3 && params->bp_bits != 4) { /* FIXME: not implemented */ return -1; @@ -470,91 +535,61 @@ wp_region = *region; if (region_offset(&wp_region) == 0) - tb = 1; + bits->tb = 1; else - tb = 0; + bits->tb = 0; if (region_sz(&wp_region) > flash->size / 2) { - cmp = 1; - wp_region.offset = tb ? 0 : region_sz(&wp_region); + bits->cmp = 1; + wp_region.offset = bits->tb ? 0 : region_sz(&wp_region); wp_region.size = flash->size - region_sz(&wp_region); - tb = !tb; + bits->tb = !bits->tb; } else { - cmp = 0; + bits->cmp = 0; } if (region_sz(&wp_region) == 0) { - bp = 0; + bits->bp = 0; } else if (IS_POWER_OF_2(region_sz(&wp_region)) && (region_sz(&wp_region) >= (1 << params->protection_granularity_shift))) { - bp = log2(region_sz(&wp_region)) - + bits->bp = log2(region_sz(&wp_region)) - params->protection_granularity_shift + 1; } else { printk(BIOS_ERR, "WINBOND: ERROR: unsupported region size\n"); return -1; } - /* Write block protection bits */ - - if (params->bp_bits == 3) { - val.reg1 = (union status_reg1) { - .bp3 = { .bp = bp, .tb = tb, .sec = 0 } - }; - mask.reg1 = (union status_reg1) { - .bp3 = { .bp = ~0, .tb = 1, .sec = 1 } - }; - } else { - val.reg1 = (union status_reg1) { - .bp4 = { .bp = bp, .tb = tb } - }; - mask.reg1 = (union status_reg1) { - .bp4 = { .bp = ~0, .tb = 1 } - }; - } - - val.reg2 = (union status_reg2) { .cmp = cmp }; - mask.reg2 = (union status_reg2) { .cmp = 1 }; - if (mode != SPI_WRITE_PROTECTION_PRESERVE) { - u8 srp; switch (mode) { case SPI_WRITE_PROTECTION_NONE: - srp = 0; + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 0; break; case SPI_WRITE_PROTECTION_PIN: - srp = 1; + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 1; break; case SPI_WRITE_PROTECTION_REBOOT: - srp = 2; + bits->winbond.srp1 = 1; + bits->winbond.srp0 = 0; break; case SPI_WRITE_PROTECTION_PERMANENT: - srp = 3; + if (params->bp_bits == 3) { + bits->winbond.srp1 = 1; + bits->winbond.srp0 = 1; + } else { + /* FIXME: special permanent protect write + * sequence not implemented. */ + return -1; + } break; default: return -1; } - - if (params->bp_bits == 3) { - val.reg1.bp3.srp0 = !!(srp & 1); - mask.reg1.bp3.srp0 = 1; - } else { - val.reg1.bp4.srp0 = !!(srp & 1); - mask.reg1.bp4.srp0 = 1; - } - - val.reg2.srp1 = !!(srp & 2); - mask.reg2.srp1 = 1; } - ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); - if (ret) - return ret; - - printk(BIOS_DEBUG, "WINBOND: write-protection set to range " - "0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); - - return ret; + return 0; } static const struct spi_flash_protection_ops spi_flash_protection_ops = { -- To view, visit https://review.coreboot.org/c/coreboot/+/42115 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2c7b08cb56772aa620e690077bbbb1fde3d7aae7 Gerrit-Change-Number: 42115 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

3 3

Jump to page:

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit August 2020