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@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;