Change in coreboot[master]: spi/macronix: Add support for boot media protection
Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/41750 to review the following change. Change subject: spi/macronix: Add support for boot media protection ...................................................................... spi/macronix: Add support for boot media protection Tested on a Lenovo T500 with MX25L6405D flash. Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Signed-off-by: Daniel Gröber <dxld@darkboxed.org> --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash_internal.h A src/drivers/spi/spi_macronix.h A tests/drivers/Makefile.inc A tests/drivers/macronix-spi-test.c 5 files changed, 519 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41750/1 diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index f3f7e2d..333276d 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -4,8 +4,10 @@ #include <commonlib/helpers.h> #include <spi_flash.h> #include <spi-generic.h> +#include <lib.h> #include "spi_flash_internal.h" +#include "spi_macronix.h" /* MX25xx-specific commands */ #define CMD_MX25XX_WREN 0x06 /* Write Enable */ @@ -23,6 +25,17 @@ #define MACRONIX_SR_WIP (1 << 0) /* Write-in-Progress */ +union status_reg { + u8 u; + struct { + u8 wip : 1; + u8 wel : 1; + u8 bp : 4; + u8 _res : 1; + u8 srwd : 1; + }; +}; + static const struct spi_flash_part_id flash_table[] = { { /* MX25L8005 */ @@ -33,16 +46,22 @@ /* MX25L1605D */ .id[0] = 0x2015, .nr_sectors_shift = 9, + .protection_granularity_shift = 16, + .bp_lower_th = 10, }, { /* MX25L3205D */ .id[0] = 0x2016, .nr_sectors_shift = 10, + .protection_granularity_shift = 16, + .bp_lower_th = 9, }, { /* MX25L6405D */ .id[0] = 0x2017, .nr_sectors_shift = 11, + .protection_granularity_shift = 17, + .bp_lower_th = 9, }, { /* MX25L12805D */ @@ -121,6 +140,270 @@ }, }; +/** + * Convert block protection bits, to a region. + */ +void macronix_bpbits_to_region(const struct spi_flash_part_id *params, + const u8 bp, + const size_t flash_size, + struct region *out) +{ + if (bp == 0xf) { + out->offset = 0; + out->size = flash_size; + } else if (bp == 0) { + out->offset = 0; + out->size = 0; + } else { + const u16 granularity_shift = + params->protection_granularity_shift; + + if (bp >= params->bp_lower_th) { + /* Lower half is protected */ + const size_t unprotected_size = + MIN(1<<granularity_shift<<(14 - 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 + bp - 1), + flash_size); + + out->offset = flash_size - protected_size; + out->size = protected_size; + } + } +} + +/** + * Convert a region to block protection bits. + * + * rv >= 0: converted bp value + * rv < 0: error + */ +int macronix_region_to_bpbits(const struct spi_flash_part_id *params, + const struct region *region, + const size_t flash_size) +{ + if (region->offset == 0 && region->size >= flash_size) { + return 0xf; + } else if (region->offset == 0 && region->size == 0) { + return 0; + } else if (region->offset >= flash_size || region->size >= flash_size) { + return -1; + } else { + const size_t gran_shift = params->protection_granularity_shift; + const u16 granularity = 1<<gran_shift; + + 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; + + return 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; + + return log2(protected_size>>gran_shift) + 1; + } + } +} + +static int macronix_flash_write_status(const struct spi_flash *flash, + const union status_reg val, + const union status_reg mask) +{ + int ret; + union status_reg status; + union status_reg new_status; + + if (val.u & ~mask.u) { + printk(BIOS_WARNING, "MACRONIX: given status reg value %02x" + " doesn't conform to mask %02x!", val.u, mask.u); + } + + ret = spi_flash_cmd_status(flash, &status.u); + if (ret) + return ret; + + new_status.u = (status.u & ~mask.u) | (val.u & mask.u); + + /* already configured right */ + if (new_status.u == status.u) + return 0; + + if (status.srwd) { + printk(BIOS_WARNING, "MACRONIX: status reg write is disabled!" + " Trying anyway."); + } + + ret = spi_flash_cmd(&flash->spi, flash->wren_cmd, NULL, 0); + if (ret) + return ret; + + ret = spi_flash_cmd(&flash->spi, CMD_MX25XX_WRSR, + &status, sizeof(status)); + if (ret) + return ret; + + ret = spi_flash_cmd_status(flash, &status.u); + if (ret) + return ret; + + if ((status.u & mask.u) != (val.u & mask.u)) { + printk(BIOS_WARNING, "MACRONIX: status reg write didn't take," + " is WP pin at GND?\n"); + return -1; + } + + 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; + + union status_reg reg = { .u = 0 }; + + ret = spi_flash_cmd(&flash->spi, flash->status_cmd, ®.u, + sizeof(reg.u)); + if (ret) + return ret; + + macronix_bpbits_to_region(params, reg.bp, flash->size, &wp_region); + + 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_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; + union status_reg val, mask; + int bp; + 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; // TODO: Warning + } + + bp = macronix_region_to_bpbits(params, region, flash->size); + if (bp < 0) { + printk(BIOS_ERR, "MACRONIX: ERROR: unsupported region size\n"); + return -1; + } + + /* Write block protection bits */ + val = (union status_reg) { .bp = bp, }; + mask = (union status_reg) { .bp = ~0, .srwd = ~0, }; + + if (mode != SPI_WRITE_PROTECTION_PRESERVE) { + switch (mode) { + case SPI_WRITE_PROTECTION_NONE: + val.srwd = 0; + break; + case SPI_WRITE_PROTECTION_PIN: + val.srwd = 1; + break; + default: + printk(BIOS_ERR, "MACRONIX: ERROR: requested write" + " protection duration (%d) is not supported by" + " this chip\n", mode); + return -1; + } + } + + ret = macronix_flash_write_status(flash, val, mask); + if (ret) { + printk(BIOS_DEBUG, "MACRONIX: macronix_flash_write_status" + " failed: ret=%d\n", ret); + return ret; + } + + printk(BIOS_DEBUG, "MACRONIX: write-protection range is" + " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + if (val.srwd) { + printk(BIOS_DEBUG, "MACRONIX: conditional WP# pin" + " hadrware protection is enabled.\n"); + } + + 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, +}; + const struct spi_flash_vendor_info spi_flash_macronix_vi = { .id = VENDOR_ID_MACRONIX, .page_size_shift = 8, @@ -129,4 +412,5 @@ .ids = flash_table, .nr_part_ids = ARRAY_SIZE(flash_table), .desc = &spi_flash_pp_0x20_sector_desc, + .prot_ops = &spi_flash_protection_ops, }; diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index b4d39b3..f23e440 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -71,9 +71,11 @@ uint16_t nr_sectors_shift: 4; uint16_t fast_read_dual_output_support : 1; uint16_t _reserved_for_flags: 3; - /* Block protection. Currently used by Winbond. */ + /* Block protection. */ uint16_t protection_granularity_shift : 5; - uint16_t bp_bits : 3; + uint16_t bp_bits : 3; /* Used by Winbond */ + /* bp values starting here protect the lower half */ + uint16_t bp_lower_th : 5; /* Used by Macronix */ }; struct spi_flash_ops_descriptor { diff --git a/src/drivers/spi/spi_macronix.h b/src/drivers/spi/spi_macronix.h new file mode 100644 index 0000000..da32f81 --- /dev/null +++ b/src/drivers/spi/spi_macronix.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SPI_MACRONIX_H +#define SPI_MACRONIX_H + +void macronix_bpbits_to_region(const struct spi_flash_part_id *params, + const u8 bp, + const size_t flash_size, + struct region *out); + +int macronix_region_to_bpbits(const struct spi_flash_part_id *params, + const struct region *region, + const size_t flash_size); + +#endif /* SPI_MACRONIX_H */ diff --git a/tests/drivers/Makefile.inc b/tests/drivers/Makefile.inc new file mode 100644 index 0000000..691ddc5 --- /dev/null +++ b/tests/drivers/Makefile.inc @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0-only + +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/commonlib/region.c +macronix-spi-test-srcs += src/commonlib/mem_pool.c +macronix-spi-test-srcs-mocks += do_printk region_is_subregion spi_flash_cmd diff --git a/tests/drivers/macronix-spi-test.c b/tests/drivers/macronix-spi-test.c new file mode 100644 index 0000000..232f069 --- /dev/null +++ b/tests/drivers/macronix-spi-test.c @@ -0,0 +1,207 @@ +/* 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 region_bpbits_roundtrip_test(void **state) +{ + int n_parts_tested; + + 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 *id = &vi->ids[i]; + + if (!id->protection_granularity_shift) + continue; + + const struct bp_table *table = NULL; + for (uint16_t j = 0; j < ARRAY_SIZE(bp_table); j++) { + if (bp_table[j].id == id->id[0]) { + table = &bp_table[j]; + break; + } + } + + n_parts_tested++; + + 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; + + macronix_bpbits_to_region(id, bp, table->size, &out); + + int bp1 = macronix_region_to_bpbits(id, &out, + table->size); + + /* 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(id, bp1, table->size, + &roundtripped); + +#if 0 + /* debugging */ + print_message("\nfor part 0x%04x bp=%d:\n", + id->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); + } + } + + /* Just making sure we actually tested something */ + assert_true(n_parts_tested >= 3); +} + + /* Mocks */ +int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, + size_t len) +{ + return -1; +} + +int do_printk(int msg_level, const char *fmt, ...) +{ + return -1; +} + +int spi_flash_cmd_status(const struct spi_flash *flash, u8 *reg) +{ + return -1; +} + +const struct spi_flash_ops_descriptor spi_flash_pp_0x20_sector_desc; + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(region_bpbits_roundtrip_test), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: newchange
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/c/coreboot/+/41750/1/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/1/src/drivers/spi/macronix.c@... PS1, Line 197: } else { else is not generally useful after a break or return https://review.coreboot.org/c/coreboot/+/41750/1/src/drivers/spi/macronix.c@... PS1, Line 212: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 26 May 2020 21:44:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 2: (2 comments) https://review.coreboot.org/c/coreboot/+/41750/2/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/2/src/drivers/spi/macronix.c@... PS2, Line 197: } else { else is not generally useful after a break or return https://review.coreboot.org/c/coreboot/+/41750/2/src/drivers/spi/macronix.c@... PS2, Line 212: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 26 May 2020 21:46:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Daniel Gröber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41750 to look at the new patch set (#3). Change subject: spi/macronix: Add support for boot media protection ...................................................................... spi/macronix: Add support for boot media protection Tested on a Lenovo T500 with MX25L6405D flash. Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Signed-off-by: Daniel Gröber <dxld@darkboxed.org> --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash_internal.h A src/drivers/spi/spi_macronix.h A tests/drivers/Makefile.inc A tests/drivers/macronix-spi-test.c 5 files changed, 519 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41750/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 3: (2 comments) https://review.coreboot.org/c/coreboot/+/41750/3/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/3/src/drivers/spi/macronix.c@... PS3, Line 197: } else { else is not generally useful after a break or return https://review.coreboot.org/c/coreboot/+/41750/3/src/drivers/spi/macronix.c@... PS3, Line 212: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 26 May 2020 22:01:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Daniel Gröber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41750 to look at the new patch set (#4). Change subject: spi/macronix: Add support for boot media protection ...................................................................... spi/macronix: Add support for boot media protection Tested on a Lenovo T500 with MX25L6405D flash. Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Signed-off-by: Daniel Gröber <dxld@darkboxed.org> --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash_internal.h A src/drivers/spi/spi_macronix.h A tests/drivers/Makefile.inc A tests/drivers/macronix-spi-test.c 5 files changed, 519 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41750/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 4: (2 comments) https://review.coreboot.org/c/coreboot/+/41750/4/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/4/src/drivers/spi/macronix.c@... PS4, Line 197: } else { else is not generally useful after a break or return https://review.coreboot.org/c/coreboot/+/41750/4/src/drivers/spi/macronix.c@... PS4, Line 212: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 26 May 2020 22:20:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 197: } else { else is not generally useful after a break or return https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 212: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 28 May 2020 08:57:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 5: Code-Review+1 Even with a unit test. Very nice. -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 29 May 2020 11:46:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 335: macronix_set_write_protection(const struct spi_flash *flash, Not really happy about duplicating so much logic for every vendor. Isn't there some way we can unify this? What are the actual differences between the Macronix and Winbond protection schemes? To me it just looks like they're naming things a bit different (e.g. what Winbond calls the CMP bit is essentially just the highest BP bit in Macronix-lingo) and some chips only support a subset (but that's more between different chips of the same vendor than between vendors... for example, the TB bit you're not implementing here is actually supported on some Macronix chips like MX25V1635F, just not the ones you're adding right now). I think this is going to be a giant mess if we try to keep block protection code completely vendor specific and keep adding it for more and more vendors. We only just spent a lot of effort to merge and unify vendor-specific code recently (e.g. patches ending in CB:38379), I'd hate to add it all back for block protection stuff now. Is there any way you could manage to encode the differences between these in cofiguration structures and factor out code that works for both of them? -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Thu, 04 Jun 2020 01:39:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Daniel Gröber, Paul Menzel, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41750 to look at the new patch set (#6). Change subject: spi/macronix: Add support for boot media protection ...................................................................... spi/macronix: Add support for boot media protection Tested on a Lenovo T500 with MX25L6405D flash. Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Signed-off-by: Daniel Gröber <dxld@darkboxed.org> --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash_internal.h A src/drivers/spi/spi_macronix.h M src/include/spi_flash.h M tests/drivers/Makefile.inc A tests/drivers/macronix-spi-test.c A tests/drivers/spi-mocks.c 7 files changed, 593 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41750/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 6: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/6/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/6/src/drivers/spi/macronix.c@... PS6, Line 223: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Sat, 06 Jun 2020 07:59:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 6: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 335: macronix_set_write_protection(const struct spi_flash *flash,
what Winbond calls the CMP bit is essentially just the highest BP bit in Macronix-lingo
Unfortunately it's not that easy best I can figure it's an arbitrary (non power-of-two) threshold that varies across capacities but in no obvious relation with it.
the TB bit you're not implementing here is actually supported on some Macronix chips like MX25V1635F
Ah, good point.
Is there any way you could manage to encode the differences between these in cofiguration structures and factor out code that works for both of them?
I spent some more time on this and merging the code paths doesn't make things too horrible. See CB:42124 -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Sat, 06 Jun 2020 08:04:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/7/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/7/src/drivers/spi/macronix.c@... PS7, Line 223: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Sat, 06 Jun 2020 09:06:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/8/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/8/src/drivers/spi/macronix.c@... PS8, Line 223: } else { else is not generally useful after a break or return -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 8 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Sun, 07 Jun 2020 21:23:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c: https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 335: macronix_set_write_protection(const struct spi_flash *flash,
what Winbond calls the CMP bit is essentially just the highest BP bit in Macronix-lingo […] Thanks for all the work, this is some good stuff! Sorry for the late review.
I still hope we can get that BP3-is-CMP idea to work and slim the effective differences down a bit further, though. See my comments in CB:42124, I think this should be possible, the only difference really is that for "CMP mode" Winbond starts counting blocks from 0 up and Macronix is counting them from the top down. Also, just for reviewing ease I think it would be nicer if you could put the patches for moving the Winbond calculations to common first, and then put the patch for adding Macronix at the end (so there's not so much code being added and removed right away again, and it's easier to see what Macronix needs to add to the existing algorithm). -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 8 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-Comment-Date: Thu, 18 Jun 2020 03:09:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Comment-In-Reply-To: Daniel Gröber (dxld) Gerrit-MessageType: comment
Attention is currently required from: Daniel Gröber (dxld). Daniel Gröber (dxld) has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Removed cc Daniel Gröber <dxld@darkboxed.org>. -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 36 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh@gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Tim Wawrzynczak <inforichland@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Attention: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-MessageType: deleteReviewer
Attention is currently required from: Daniel Gröber (dxld). Daniel Gröber (dxld) has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/41750 ) Change subject: spi/macronix: Add support for boot media protection ...................................................................... Removed reviewer Daniel Gröber <dxld@darkboxed.org>. -- To view, visit https://review.coreboot.org/c/coreboot/+/41750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Gerrit-Change-Number: 41750 Gerrit-PatchSet: 37 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh@gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Tim Wawrzynczak <inforichland@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Attention: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-MessageType: deleteReviewer
participants (4)
-
build bot (Jenkins) (Code Review) -
Daniel Gröber (dxld) (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)