Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41746
to review the following change.
Change subject: spi: Remove non_volatile flag from block protection interface ......................................................................
spi: Remove non_volatile flag from block protection interface
Only Winbond parts seem to support making status register writes volatile. So this flag should not be exposed in the generic interface.
Change-Id: Idadb65ffaff0dd7809b18c53086a466122b37c12 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/boot_device_rw_nommap.c M src/drivers/spi/spi_flash.c M src/drivers/spi/winbond.c M src/include/spi_flash.h 4 files changed, 3 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/41746/1
diff --git a/src/drivers/spi/boot_device_rw_nommap.c b/src/drivers/spi/boot_device_rw_nommap.c index a383f26..ba11d05 100644 --- a/src/drivers/spi/boot_device_rw_nommap.c +++ b/src/drivers/spi/boot_device_rw_nommap.c @@ -97,7 +97,7 @@ if (spi_flash_is_write_protected(boot_dev, region_device_region(rd)) != 1) { return spi_flash_set_write_protected(boot_dev, - region_device_region(rd), true, + region_device_region(rd), SPI_WRITE_PROTECTION_REBOOT); }
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 3ffb87e..a389bc4 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -556,7 +556,6 @@
int spi_flash_set_write_protected(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode) { struct region flash_region = { 0 }; @@ -576,7 +575,7 @@ return -1; }
- ret = flash->prot_ops->set_write(flash, region, non_volatile, mode); + ret = flash->prot_ops->set_write(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/winbond.c b/src/drivers/spi/winbond.c index d8d3cdc..0c8059d 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -422,7 +422,6 @@ * * @param flash: The flash to operate on * @param region: The region to write protect - * @param non_volatile: Make setting permanent * @param mode: Optional status register lock-down mode * * @return 0 on success @@ -430,7 +429,6 @@ static int winbond_set_write_protection(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode) { const struct spi_flash_part_id *params; @@ -527,7 +525,7 @@ mask.reg2.srp1 = 1; }
- ret = winbond_flash_cmd_status(flash, mask.u, val.u, non_volatile); + ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); if (ret) return ret;
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 9d0e3ad..35b02db 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -74,7 +74,6 @@ int (*set_write)(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode);
}; @@ -170,7 +169,6 @@ * * @param flash : A SPI flash device * @param region: A subregion of the device's region - * @param non_volatile: Write status register non-volatile * @param mode: Optional lock-down of status register
* @return 0 on success @@ -178,7 +176,6 @@ int spi_flash_set_write_protected(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode);
/*
Hello build bot (Jenkins), Daniel Gröber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41746
to look at the new patch set (#3).
Change subject: spi: Remove non_volatile flag from block protection interface ......................................................................
spi: Remove non_volatile flag from block protection interface
Only Winbond parts seem to support making status register writes volatile. So this flag should not be exposed in the generic interface.
Change-Id: Idadb65ffaff0dd7809b18c53086a466122b37c12 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/boot_device_rw_nommap.c M src/drivers/spi/spi_flash.c M src/drivers/spi/winbond.c M src/include/spi_flash.h M src/mainboard/opencellular/elgon/bootblock.c 5 files changed, 4 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/41746/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41746 )
Change subject: spi: Remove non_volatile flag from block protection interface ......................................................................
Patch Set 5: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41746 )
Change subject: spi: Remove non_volatile flag from block protection interface ......................................................................
spi: Remove non_volatile flag from block protection interface
Only Winbond parts seem to support making status register writes volatile. So this flag should not be exposed in the generic interface.
Change-Id: Idadb65ffaff0dd7809b18c53086a466122b37c12 Signed-off-by: Daniel Gröber dxld@darkboxed.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41746 Reviewed-by: Patrick Rudolph siro@das-labor.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/spi/boot_device_rw_nommap.c M src/drivers/spi/spi_flash.c M src/drivers/spi/winbond.c M src/include/spi_flash.h M src/mainboard/opencellular/elgon/bootblock.c 5 files changed, 4 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/drivers/spi/boot_device_rw_nommap.c b/src/drivers/spi/boot_device_rw_nommap.c index a383f26..ba11d05 100644 --- a/src/drivers/spi/boot_device_rw_nommap.c +++ b/src/drivers/spi/boot_device_rw_nommap.c @@ -97,7 +97,7 @@ if (spi_flash_is_write_protected(boot_dev, region_device_region(rd)) != 1) { return spi_flash_set_write_protected(boot_dev, - region_device_region(rd), true, + region_device_region(rd), SPI_WRITE_PROTECTION_REBOOT); }
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index da2e868..372575e 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -555,7 +555,6 @@
int spi_flash_set_write_protected(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode) { struct region flash_region = { 0 }; @@ -575,7 +574,7 @@ return -1; }
- ret = flash->prot_ops->set_write(flash, region, non_volatile, mode); + ret = flash->prot_ops->set_write(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/winbond.c b/src/drivers/spi/winbond.c index d8d3cdc..0c8059d 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -422,7 +422,6 @@ * * @param flash: The flash to operate on * @param region: The region to write protect - * @param non_volatile: Make setting permanent * @param mode: Optional status register lock-down mode * * @return 0 on success @@ -430,7 +429,6 @@ static int winbond_set_write_protection(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode) { const struct spi_flash_part_id *params; @@ -527,7 +525,7 @@ mask.reg2.srp1 = 1; }
- ret = winbond_flash_cmd_status(flash, mask.u, val.u, non_volatile); + ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); if (ret) return ret;
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 9d0e3ad..35b02db 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -74,7 +74,6 @@ int (*set_write)(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode);
}; @@ -170,7 +169,6 @@ * * @param flash : A SPI flash device * @param region: A subregion of the device's region - * @param non_volatile: Write status register non-volatile * @param mode: Optional lock-down of status register
* @return 0 on success @@ -178,7 +176,6 @@ int spi_flash_set_write_protected(const struct spi_flash *flash, const struct region *region, - const bool non_volatile, const enum spi_flash_status_reg_lockdown mode);
/* diff --git a/src/mainboard/opencellular/elgon/bootblock.c b/src/mainboard/opencellular/elgon/bootblock.c index 5eaece2..c1b5181 100644 --- a/src/mainboard/opencellular/elgon/bootblock.c +++ b/src/mainboard/opencellular/elgon/bootblock.c @@ -71,7 +71,7 @@ * WP_RO read only and use /WP pin * non-volatile programming */ - if (spi_flash_set_write_protected(flash, &ro_rgn, 1, + if (spi_flash_set_write_protected(flash, &ro_rgn, SPI_WRITE_PROTECTION_PIN) != 0) die("Failed to write-protect WP_RO region!"); }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41746 )
Change subject: spi: Remove non_volatile flag from block protection interface ......................................................................
Patch Set 7:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/5312 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5311 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5310 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/5309
Please note: This test is under development and might not be accurate at all!