Change in coreboot[master]: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region

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

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42113 ) Change subject: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/42113 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Gerrit-Change-Number: 42113 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 18 Jun 2020 03:03:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Daniel Gröber has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42113 ) Change subject: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region ...................................................................... Removed reviewer Daniel Gröber <dxld@darkboxed.org>. -- To view, visit https://review.coreboot.org/c/coreboot/+/42113 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Gerrit-Change-Number: 42113 Gerrit-PatchSet: 24 Gerrit-Owner: Daniel Gröber <coreboot@dxld.at> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: deleteReviewer

Attention is currently required from: Daniel Gröber (dxld). Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42113?usp=email ) Change subject: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region ...................................................................... Patch Set 26: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/42113?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Gerrit-Change-Number: 42113 Gerrit-PatchSet: 26 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Jakub Czapiga <jacz@semihalf.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Gröber <dxld@darkboxed.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Attention: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Comment-Date: Tue, 03 Oct 2023 10:39:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42113?usp=email ) Change subject: spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region ...................................................................... spi/winbond: Use spi_flash_bpbits in winbond_bpbits_to_region This consolidates the bp, tb, cmp, srp0 and srp1 variables under the new spi_flash_bpbits struct to allow treating them as one unit in the refactoring to follow. Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Signed-off-by: Daniel Gröber <dxld@darkboxed.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/42113 Reviewed-by: Jakub Czapiga <czapiga@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org> --- M src/drivers/spi/winbond.c 1 file changed, 58 insertions(+), 19 deletions(-) Approvals: Jakub Czapiga: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 7992629..3137bfe 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -19,14 +19,14 @@ uint8_t tb : 1; uint8_t sec : 1; uint8_t srp0 : 1; - } bp3; + } bp3; /* for example: W25Q128FW */ struct { uint8_t busy : 1; uint8_t wel : 1; uint8_t bp : 4; uint8_t tb : 1; uint8_t srp0 : 1; - } bp4; + } bp4; /* for example: W25Q256J */ }; union status_reg2 { @@ -254,16 +254,15 @@ * SEC (if available) must be zero. */ static void winbond_bpbits_to_region(const size_t granularity, - const u8 bp, - bool tb, - const bool cmp, + const struct spi_flash_bpbits *bits, const size_t flash_size, struct region *out) { size_t protected_size = - MIN(bp ? granularity << (bp - 1) : 0, flash_size); + MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash_size); - if (cmp) { + int tb = bits->tb; + if (bits->cmp) { protected_size = flash_size - protected_size; tb = !tb; } @@ -287,8 +286,7 @@ { const struct spi_flash_part_id *params; struct region wp_region; - union status_reg2 reg2; - u8 bp, tb; + struct spi_flash_bpbits bpbits; int ret; params = flash->part; @@ -299,34 +297,75 @@ const size_t granularity = (1 << params->protection_granularity_shift); union status_reg1 reg1 = { .u = 0 }; + union status_reg2 reg2 = { .u = 0 }; ret = spi_flash_cmd(&flash->spi, flash->status_cmd, ®1.u, sizeof(reg1.u)); if (ret) return ret; + ret = spi_flash_cmd(&flash->spi, CMD_W25_RDSR2, ®2.u, + sizeof(reg2.u)); + if (ret) + return ret; + if (params->bp_bits == 3) { if (reg1.bp3.sec) { // FIXME: not supported return -1; } - bp = reg1.bp3.bp; - tb = reg1.bp3.tb; + bpbits = (struct spi_flash_bpbits){ + .bp = reg1.bp3.bp, + .cmp = reg2.cmp, + .tb = reg1.bp3.tb, + /* + * For W25Q*{,F}* parts: + * srp1 srp0 + * 0 0 | writable if WEL==1 + * 0 1 | writable if WEL==1 && #WP==Vcc + * 1 0 | not writable until next power-down + * 1 1 | not writable, permanently + * + * checked datasheets: W25Q128FV, (W25Q80, W25Q16, + * W25Q32) + */ + .winbond = { + .srp0 = reg1.bp3.srp0, + .srp1 = reg2.srp1, + }, + }; } else if (params->bp_bits == 4) { - bp = reg1.bp4.bp; - tb = reg1.bp4.tb; + bpbits = (struct spi_flash_bpbits){ + .bp = reg1.bp4.bp, + .cmp = reg2.cmp, + .tb = reg1.bp4.tb, + /* + * For W25Q*{J,D}* parts: + * + * srp1 srp0 + * 0 0 | writable if WEL==1 + * 0 1 | writable if WEL==1 && #WP==Vcc + * 1 x | not writable until next power-down + * + * checked datasheets: W25Q132JW, W25Q128JW, W25Q256JV. + * W25Q16DW + * + * The srp0/srp1 bits got renamed to srp/srl in the + * datasheets, we retain the prior naming + * convention for the structs though. + */ + .winbond = { + .srp0 = reg1.bp4.srp0, + .srp1 = reg2.srp1, + }, + }; } else { // FIXME: not supported return -1; } - ret = spi_flash_cmd(&flash->spi, CMD_W25_RDSR2, ®2.u, - sizeof(reg2.u)); - if (ret) - return ret; - - winbond_bpbits_to_region(granularity, bp, tb, reg2.cmp, flash->size, + winbond_bpbits_to_region(granularity, &bpbits, flash->size, &wp_region); if (!region_sz(&wp_region)) { -- To view, visit https://review.coreboot.org/c/coreboot/+/42113?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2a1a77fb73047df733498c0fa8b8de1153c3b09e Gerrit-Change-Number: 42113 Gerrit-PatchSet: 27 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Felix Held <felix-coreboot@felixheld.de> Gerrit-Reviewer: Jakub Czapiga <czapiga@google.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Gröber Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-MessageType: merged
participants (5)
-
Daniel Gröber (Code Review)
-
Daniel Gröber (dxld) (Code Review)
-
Felix Held (Code Review)
-
Jakub Czapiga (Code Review)
-
Julius Werner (Code Review)