coreboot-gerrit July 2022

coreboot-gerrit@coreboot.org

670 discussions

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: 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/macronix: Add support for boot media protection
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/+/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(a)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, &reg.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(a)darkboxed.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 17

Change in coreboot[master]: spi/winbond: Pull out winbond_get_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/+/42114 to review the following change. Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... spi/winbond: Pull out winbond_get_bpbits function Split logic for retrieving bpbits values from winbond_get_write_protection into a new function: winbond_get_bpbits. Change-Id: Ifc23f0cce695cd8aebf5549a7ca098c08c759f37 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 33 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42114/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index ccc7ae9..77191e4 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -208,6 +208,9 @@ }, }; +static int winbond_get_bpbits(const struct spi_flash *flash, + struct spi_flash_bpbits *bpbits); + /* * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. @@ -255,8 +258,35 @@ 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); + + 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); +} + +static int winbond_get_bpbits(const struct spi_flash *flash, + 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)); @@ -274,7 +304,7 @@ return -1; } - bpbits = (struct spi_flash_bpbits){ + *bpbits = (struct spi_flash_bpbits){ .bp = reg1.bp3.bp, .cmp = reg2.cmp, .tb = reg1.bp3.tb, @@ -284,7 +314,7 @@ }, }; } else if (params->bp_bits == 4) { - bpbits = (struct spi_flash_bpbits){ + *bpbits = (struct spi_flash_bpbits){ .bp = reg1.bp4.bp, .cmp = reg2.cmp, .tb = reg1.bp4.tb, @@ -298,19 +328,7 @@ return -1; } - winbond_bpbits_to_region(granularity, &bpbits, flash->size, - &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; } /** -- To view, visit https://review.coreboot.org/c/coreboot/+/42114 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifc23f0cce695cd8aebf5549a7ca098c08c759f37 Gerrit-Change-Number: 42114 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: [WIP]mb/gigabyte/ga-g41m-combo: Add variant
by Arthur Heymans (Code Review) 21 Feb '23

21 Feb '23

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41868 ) Change subject: [WIP]mb/gigabyte/ga-g41m-combo: Add variant ...................................................................... [WIP]mb/gigabyte/ga-g41m-combo: Add variant Untested. Change-Id: I2752b259ff4a0612a7de1c8a3a9a79a3a070cdfc Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/mainboard/gigabyte/ga-g41m-es2l/Kconfig M src/mainboard/gigabyte/ga-g41m-es2l/Kconfig.name A src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/gpio.c A src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/hda_verb.c A src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/overridetree.cb A src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/superio.c 6 files changed, 247 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/41868/1 diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig b/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig index 97f155a..5137c96 100644 --- a/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig +++ b/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only -if BOARD_GIGABYTE_GA_G41M_ES2L +if BOARD_GIGABYTE_GA_G41M_ES2L || BOARD_GIGABYTE_GA_G41M_COMBO config BOARD_SPECIFIC_OPTIONS def_bool y @@ -8,12 +8,13 @@ select NORTHBRIDGE_INTEL_X4X select SOUTHBRIDGE_INTEL_I82801GX select SUPERIO_ITE_IT8718F if BOARD_GIGABYTE_GA_G41M_ES2L + select SUPERIO_ITE_IT8720F if BOARD_GIGABYTE_GA_G41M_COMBO select HAVE_ACPI_TABLES select BOARD_ROMSIZE_KB_1024 select PCIEXP_ASPM select PCIEXP_CLK_PM select PCIEXP_L1_SUB_STATE - select REALTEK_8168_RESET + select REALTEK_8168_RESET if BOARD_GIGABYTE_GA_G41M_ES2L select HAVE_OPTION_TABLE select HAVE_CMOS_DEFAULT select HAVE_ACPI_RESUME @@ -27,6 +28,7 @@ config VARIANT_DIR string default "ga-g41m-es2l" if BOARD_GIGABYTE_GA_G41M_ES2L + default "ga-g41m-combo" if BOARD_GIGABYTE_GA_G41M_COMBO config OVERRIDE_DEVICETREE string @@ -35,9 +37,14 @@ config MAINBOARD_PART_NUMBER string default "GA-G41M-ES2L" if BOARD_GIGABYTE_GA_G41M_ES2L + default "G41M-Combo" if BOARD_GIGABYTE_GA_G41M_COMBO config MAX_CPUS int default 4 +# Override the default variant behavior, since the data.vbt is the same +config INTEL_GMA_VBT_FILE + default "src/mainboard/$(MAINBOARDDIR)/data.vbt" + endif # BOARD_GIGABYTE_GA_G41M_ES2L diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig.name b/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig.name index e685ce1..efa4fa7 100644 --- a/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig.name +++ b/src/mainboard/gigabyte/ga-g41m-es2l/Kconfig.name @@ -1,2 +1,5 @@ config BOARD_GIGABYTE_GA_G41M_ES2L bool "GA-G41M-ES2L" + +config BOARD_GIGABYTE_GA_G41M_COMBO + bool "GA-G41M-Combo" diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/gpio.c b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/gpio.c new file mode 100644 index 0000000..1f5dfa0 --- /dev/null +++ b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/gpio.c @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <southbridge/intel/common/gpio.h> + +static const struct pch_gpio_set1 pch_gpio_set1_mode = { + .gpio6 = GPIO_MODE_GPIO, + .gpio7 = GPIO_MODE_GPIO, + .gpio8 = GPIO_MODE_GPIO, + .gpio9 = GPIO_MODE_GPIO, + .gpio10 = GPIO_MODE_GPIO, + .gpio12 = GPIO_MODE_GPIO, + .gpio13 = GPIO_MODE_GPIO, + .gpio14 = GPIO_MODE_GPIO, + .gpio15 = GPIO_MODE_GPIO, + .gpio16 = GPIO_MODE_GPIO, + .gpio18 = GPIO_MODE_GPIO, + .gpio20 = GPIO_MODE_GPIO, + .gpio24 = GPIO_MODE_GPIO, + .gpio25 = GPIO_MODE_GPIO, + .gpio26 = GPIO_MODE_GPIO, + .gpio27 = GPIO_MODE_GPIO, + .gpio28 = GPIO_MODE_GPIO, +}; + +static const struct pch_gpio_set1 pch_gpio_set1_direction = { + .gpio6 = GPIO_DIR_INPUT, + .gpio7 = GPIO_DIR_INPUT, + .gpio8 = GPIO_DIR_INPUT, + .gpio9 = GPIO_DIR_INPUT, + .gpio10 = GPIO_DIR_INPUT, + .gpio12 = GPIO_DIR_INPUT, + .gpio13 = GPIO_DIR_INPUT, + .gpio14 = GPIO_DIR_INPUT, + .gpio15 = GPIO_DIR_INPUT, + .gpio16 = GPIO_DIR_INPUT, + .gpio18 = GPIO_DIR_OUTPUT, + .gpio20 = GPIO_DIR_OUTPUT, + .gpio24 = GPIO_DIR_OUTPUT, + .gpio25 = GPIO_DIR_INPUT, + .gpio26 = GPIO_DIR_OUTPUT, + .gpio27 = GPIO_DIR_OUTPUT, + .gpio28 = GPIO_DIR_OUTPUT, +}; + +static const struct pch_gpio_set1 pch_gpio_set1_level = { + .gpio18 = GPIO_LEVEL_HIGH, + .gpio20 = GPIO_LEVEL_HIGH, + .gpio24 = GPIO_LEVEL_LOW, + .gpio26 = GPIO_LEVEL_LOW, + .gpio27 = GPIO_LEVEL_LOW, + .gpio28 = GPIO_LEVEL_LOW, +}; + +static const struct pch_gpio_set1 pch_gpio_set1_invert = { + .gpio6 = GPIO_INVERT, + .gpio7 = GPIO_INVERT, + .gpio8 = GPIO_INVERT, + .gpio12 = GPIO_INVERT, + .gpio13 = GPIO_INVERT, +}; + +static const struct pch_gpio_set1 pch_gpio_set1_blink = { +}; + +static const struct pch_gpio_set2 pch_gpio_set2_mode = { + .gpio32 = GPIO_MODE_GPIO, + .gpio33 = GPIO_MODE_GPIO, + .gpio34 = GPIO_MODE_GPIO, + .gpio38 = GPIO_MODE_GPIO, + .gpio39 = GPIO_MODE_GPIO, +}; + +static const struct pch_gpio_set2 pch_gpio_set2_direction = { + .gpio32 = GPIO_DIR_OUTPUT, + .gpio33 = GPIO_DIR_OUTPUT, + .gpio34 = GPIO_DIR_OUTPUT, + .gpio38 = GPIO_DIR_INPUT, + .gpio39 = GPIO_DIR_INPUT, +}; + +static const struct pch_gpio_set2 pch_gpio_set2_level = { + .gpio32 = GPIO_LEVEL_HIGH, + .gpio33 = GPIO_LEVEL_HIGH, + .gpio34 = GPIO_LEVEL_LOW, +}; + +const struct pch_gpio_map mainboard_gpio_map = { + .set1 = { + .mode = &pch_gpio_set1_mode, + .direction = &pch_gpio_set1_direction, + .level = &pch_gpio_set1_level, + .blink = &pch_gpio_set1_blink, + .invert = &pch_gpio_set1_invert, + }, + .set2 = { + .mode = &pch_gpio_set2_mode, + .direction = &pch_gpio_set2_direction, + .level = &pch_gpio_set2_level, + }, +}; diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/hda_verb.c b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/hda_verb.c new file mode 100644 index 0000000..f4816ee --- /dev/null +++ b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/hda_verb.c @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <device/azalia_device.h> + +const u32 cim_verb_data[] = { + /* coreboot specific header */ + 0x10ec0887, + 0x1458a002, // Subsystem ID + 0x0000000e, // Number of entries + + /* Pin Widget Verb Table */ + + AZALIA_PIN_CFG(2, 0x11, 0x411110f0), + AZALIA_PIN_CFG(2, 0x12, 0x411111f0), + AZALIA_PIN_CFG(2, 0x14, 0x01014410), + AZALIA_PIN_CFG(2, 0x15, 0x411111f0), + AZALIA_PIN_CFG(2, 0x16, 0x411111f0), + AZALIA_PIN_CFG(2, 0x17, 0x411111f0), + AZALIA_PIN_CFG(2, 0x18, 0x01a19c20), + AZALIA_PIN_CFG(2, 0x19, 0x02a19c30), + AZALIA_PIN_CFG(2, 0x1a, 0x0181342f), + AZALIA_PIN_CFG(2, 0x1b, 0x02214c1f), + AZALIA_PIN_CFG(2, 0x1c, 0x411111f0), + AZALIA_PIN_CFG(2, 0x1d, 0x4004c601), + AZALIA_PIN_CFG(2, 0x1e, 0x411111f0), + AZALIA_PIN_CFG(2, 0x1f, 0x411111f0), +}; + +const u32 pc_beep_verbs[0] = {}; + +const u32 pc_beep_verbs_size = ARRAY_SIZE(pc_beep_verbs); +const u32 cim_verb_data_size = ARRAY_SIZE(cim_verb_data); diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/overridetree.cb b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/overridetree.cb new file mode 100644 index 0000000..d7cd61f --- /dev/null +++ b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/overridetree.cb @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +chip northbridge/intel/x4x # Northbridge + device domain 0 on # PCI domain + chip southbridge/intel/i82801gx # Southbridge + device pci 1f.0 on # ISA bridge + subsystemid 0x1458 0x5001 + chip superio/ite/it8720f # Super I/O + device pnp 2e.0 on # Floppy + io 0x60 = 0x3f0 + irq 0x70 = 6 + drq 0x74 = 2 + irq 0xf0 = 0x00 + irq 0xf1 = 0x80 + end + device pnp 2e.1 on # COM1 + io 0x60 = 0x3f8 + irq 0x70 = 4 + end + device pnp 2e.2 off end # COM2 + device pnp 2e.3 on # Parallel port + io 0x60 = 0x378 + io 0x62 = 0x000 + irq 0x70 = 7 + drq 0x74 = 4 + irq 0xf0 = 0x08 + end + device pnp 2e.4 on # Environment controller + io 0x60 = 0x290 + io 0x62 = 0x000 + irq 0x70 = 0x00 + irq 0xf0 = 0x80 + irq 0xf1 = 0x40 + irq 0xf2 = 0x0a + irq 0xf3 = 0x80 + irq 0xf4 = 0x80 + end + device pnp 2e.5 on # Keyboard + io 0x60 = 0x60 + io 0x62 = 0x64 + irq 0x70 = 1 + irq 0xf0 = 0x48 + end + device pnp 2e.6 on # Mouse + irq 0x70 = 12 + irq 0x71 = 2 + irq 0xf0 = 0 + end + device pnp 2e.7 on end # GPIO (done in bootblock) + device pnp 2e.a off end # Consumer IR + end + end + end + end +end diff --git a/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/superio.c b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/superio.c new file mode 100644 index 0000000..7aa4c2c --- /dev/null +++ b/src/mainboard/gigabyte/ga-g41m-es2l/variants/ga-g41m-combo/superio.c @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <mainboard/superio.h> +#include <superio/ite/it8720f/it8720f.h> +#include <superio/ite/common/ite.h> + +#define SERIAL_DEV PNP_DEV(0x2e, IT8720F_SP1) +#define GPIO_DEV PNP_DEV(0x2e, IT8720F_GPIO) +#define EC_DEV PNP_DEV(0x2e, IT8720F_EC) + +void mainboard_early_superio(void) +{ + /* TODO: check this with datasheet */ + /* Set default GPIOs on superio */ + ite_reg_write(GPIO_DEV, 0x25, 0x50); + ite_reg_write(GPIO_DEV, 0x26, 0x0c); + ite_reg_write(GPIO_DEV, 0x27, 0x80); + ite_reg_write(GPIO_DEV, 0x28, 0x43); + ite_reg_write(GPIO_DEV, 0x29, 0x0a); + ite_reg_write(GPIO_DEV, 0x2c, 0x01); + ite_reg_write(GPIO_DEV, 0x62, 0x08); + ite_reg_write(GPIO_DEV, 0x73, 0x00); + ite_reg_write(GPIO_DEV, 0xb8, 0x40); + ite_reg_write(GPIO_DEV, 0xc0, 0x10); + ite_reg_write(GPIO_DEV, 0xc1, 0x0c); + ite_reg_write(GPIO_DEV, 0xc2, 0x80); + ite_reg_write(GPIO_DEV, 0xc3, 0x43); + ite_reg_write(GPIO_DEV, 0xc4, 0x0a); + ite_reg_write(GPIO_DEV, 0xc8, 0x00); + ite_reg_write(GPIO_DEV, 0xc9, 0x00); + ite_reg_write(GPIO_DEV, 0xcb, 0x42); + ite_reg_write(GPIO_DEV, 0xcc, 0x02); + ite_reg_write(GPIO_DEV, 0xf0, 0x10); + ite_reg_write(GPIO_DEV, 0xf1, 0x40); + ite_reg_write(GPIO_DEV, 0xf6, 0x0e); + + ite_reg_write(EC_DEV, 0xf0, 0x80); + ite_reg_write(EC_DEV, 0xf1, 0x40); + ite_reg_write(EC_DEV, 0xf2, 0x0a); + ite_reg_write(EC_DEV, 0xf3, 0x80); + ite_reg_write(EC_DEV, 0x70, 0x00); // Don't use IRQ9 + ite_reg_write(EC_DEV, 0x30, 0x01); // Enable + + ite_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE); + + /* Disable SIO reboot */ + ite_reg_write(GPIO_DEV, 0xEF, 0x7E); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/41868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2752b259ff4a0612a7de1c8a3a9a79a3a070cdfc Gerrit-Change-Number: 41868 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: sb/intel/i82371eb: Leave I/O ports 0x2e-0x2f unclaimed
by Keith Hui (Code Review) 16 Feb '23

16 Feb '23

Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41458 ) Change subject: sb/intel/i82371eb: Leave I/O ports 0x2e-0x2f unclaimed ...................................................................... sb/intel/i82371eb: Leave I/O ports 0x2e-0x2f unclaimed This change reflects vendor setup for asus/p3b-f. Change-Id: I11972b650d95a48e4d372defe201cc9e743d5b30 Signed-off-by: Keith Hui <buurin(a)gmail.com> --- M src/southbridge/intel/i82371eb/acpi/i82371eb.asl 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/41458/1 diff --git a/src/southbridge/intel/i82371eb/acpi/i82371eb.asl b/src/southbridge/intel/i82371eb/acpi/i82371eb.asl index 4a1b385..f0f6a0c 100644 --- a/src/southbridge/intel/i82371eb/acpi/i82371eb.asl +++ b/src/southbridge/intel/i82371eb/acpi/i82371eb.asl @@ -34,7 +34,9 @@ /* Aliased DMA ports */ IO (Decode16, 0x0010, 0x0010, 0x01, 0x10, ) /* Aliased PIC ports */ - IO (Decode16, 0x0022, 0x0022, 0x01, 0x1E, ) + /* Do not claim 0x2e-0x2f, per P3B-F vendor DSDT */ + IO (Decode16, 0x0022, 0x0022, 0x01, 0x0C, ) + IO (Decode16, 0x0030, 0x0030, 0x01, 0x10, ) /* Aliased timer ports */ IO (Decode16, 0x0050, 0x0050, 0x01, 0x04, ) IO (Decode16, 0x0062, 0x0062, 0x01, 0x02, ) -- To view, visit https://review.coreboot.org/c/coreboot/+/41458 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11972b650d95a48e4d372defe201cc9e743d5b30 Gerrit-Change-Number: 41458 Gerrit-PatchSet: 1 Gerrit-Owner: Keith Hui <buurin(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: mb/ocp/deltalake: Add SMM console log level override
by Johnny Lin (Code Review) 16 Feb '23

16 Feb '23

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47006 ) Change subject: mb/ocp/deltalake: Add SMM console log level override ...................................................................... mb/ocp/deltalake: Add SMM console log level override The log level is set to a configuration value, since SMI should not take too much time searching flash VPD. Change-Id: I73afc944fbd6c21e884397f3049bd363e2c1ce2c Signed-off-by: Johnny Lin <johnny_lin(a)wiwynn.com> --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/Makefile.inc M src/mainboard/ocp/deltalake/loglevel_vpd.c 3 files changed, 11 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/47006/1 diff --git a/src/mainboard/ocp/deltalake/Kconfig b/src/mainboard/ocp/deltalake/Kconfig index bb907f1..1d00c29 100644 --- a/src/mainboard/ocp/deltalake/Kconfig +++ b/src/mainboard/ocp/deltalake/Kconfig @@ -57,4 +57,9 @@ bool default y +config SMM_CONSOLE_LOGLEVEL + int + depends on DEBUG_SMI + default 7 + endif # BOARD_OCP_DELTALAKE diff --git a/src/mainboard/ocp/deltalake/Makefile.inc b/src/mainboard/ocp/deltalake/Makefile.inc index e961a34..9718f4c 100644 --- a/src/mainboard/ocp/deltalake/Makefile.inc +++ b/src/mainboard/ocp/deltalake/Makefile.inc @@ -8,5 +8,9 @@ ramstage-y += ramstage.c ipmi.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += fadt.c all-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel_vpd.c +ifeq ($(CONFIG_DEBUG_SMI),y) +smm-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel_vpd.c +endif + CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/include CPPFLAGS_common += -I$(CONFIG_FSP_HEADER_PATH) diff --git a/src/mainboard/ocp/deltalake/loglevel_vpd.c b/src/mainboard/ocp/deltalake/loglevel_vpd.c index 3faf37a..21f3652 100644 --- a/src/mainboard/ocp/deltalake/loglevel_vpd.c +++ b/src/mainboard/ocp/deltalake/loglevel_vpd.c @@ -11,6 +11,8 @@ int log_level = COREBOOT_LOG_LEVEL_DEFAULT; char val_str[VPD_LEN]; + if (ENV_SMM) + return CONFIG_SMM_CONSOLE_LOGLEVEL; if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { log_level = (int)atol(val_str); if (log_level < 0 || log_level >= BIOS_NEVER) -- To view, visit https://review.coreboot.org/c/coreboot/+/47006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I73afc944fbd6c21e884397f3049bd363e2c1ce2c Gerrit-Change-Number: 47006 Gerrit-PatchSet: 1 Gerrit-Owner: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-MessageType: newchange

9 46

Change in ...coreboot[master]: Mistral: Enable USB in romstage
by Name of user not set (Code Review) 16 Jan '23

16 Jan '23

nsekar(a)codeaurora.org has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32063 Change subject: Mistral: Enable USB in romstage ...................................................................... Mistral: Enable USB in romstage Enable USB support for mistral in romstage. TEST=build & run Change-Id: I5c2bbe16aa3601e014a2b77d192565402ed23794 Signed-off-by: Nitheesh Sekar <nsekar(a)codeaurora.org> --- M src/mainboard/google/mistral/Makefile.inc M src/mainboard/google/mistral/mainboard.c A src/mainboard/google/mistral/romstage.c 3 files changed, 48 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/32063/1 diff --git a/src/mainboard/google/mistral/Makefile.inc b/src/mainboard/google/mistral/Makefile.inc index dfb0bbc..2cb9631 100644 --- a/src/mainboard/google/mistral/Makefile.inc +++ b/src/mainboard/google/mistral/Makefile.inc @@ -11,6 +11,7 @@ romstage-y += memlayout.ld romstage-y += chromeos.c romstage-y += reset.c +romstage-y += romstage.c ramstage-y += memlayout.ld ramstage-y += chromeos.c diff --git a/src/mainboard/google/mistral/mainboard.c b/src/mainboard/google/mistral/mainboard.c index b45657f..1d62adb 100644 --- a/src/mainboard/google/mistral/mainboard.c +++ b/src/mainboard/google/mistral/mainboard.c @@ -17,6 +17,20 @@ #include <bootblock_common.h> #include <timestamp.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <soc/usb.h> + +static struct usb_board_data usb1_board_data = { + .parameter_override_x0 = 0x63, + .parameter_override_x1 = 0x03, + .parameter_override_x0 = 0x1d, + .parameter_override_x1 = 0x03, +}; + +static void setup_usb(void) +{ + /* Setting Secondary usb controller */ + setup_usb_host(HSUSB_HS_PORT_1, &usb1_board_data); +} static void mainboard_init(struct device *dev) { @@ -24,6 +38,8 @@ /* Copy WIFI calibration data into CBMEM. */ cbmem_add_vpd_calibration_data(); } + + setup_usb(); } static void mainboard_enable(struct device *dev) diff --git a/src/mainboard/google/mistral/romstage.c b/src/mainboard/google/mistral/romstage.c new file mode 100644 index 0000000..41ee4ed --- /dev/null +++ b/src/mainboard/google/mistral/romstage.c @@ -0,0 +1,31 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2018, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/stages.h> +#include <soc/usb.h> + +static void prepare_usb(void) +{ + /* + * Do DWC3 core and phy reset. Kick these resets off early + * so they get atleast 1msec to settle. + */ + reset_usb(HSUSB_HS_PORT_1); +} + +void platform_romstage_main(void) +{ + prepare_usb(); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/32063 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5c2bbe16aa3601e014a2b77d192565402ed23794 Gerrit-Change-Number: 32063 Gerrit-PatchSet: 1 Gerrit-Owner: nsekar(a)codeaurora.org Gerrit-MessageType: newchange

3 4

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2022