Change in coreboot[master]: spi/winbond: Pull out winbond_get_bpbits function
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@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, ®1.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@darkboxed.org> Gerrit-MessageType: newchange
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42114 ) Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/c/coreboot/+/42114/2/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c: https://review.coreboot.org/c/coreboot/+/42114/2/src/drivers/spi/winbond.c@2... PS2, Line 211: static int winbond_get_bpbits(const struct spi_flash *flash, Please don't forward declare static functions, just reorder the functions until that's unnecessary (i.e. move winbond_get_bpbits() above winbond_get_write_protection()). -- 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: 2 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 18 Jun 2020 03:03:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42114 ) Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/c/coreboot/+/42114/2/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c: https://review.coreboot.org/c/coreboot/+/42114/2/src/drivers/spi/winbond.c@2... PS2, Line 211: static int winbond_get_bpbits(const struct spi_flash *flash,
Please don't forward declare static functions, just reorder the functions until that's unnecessary ( […] I did this quite on purpose because it made the diff significantly smaller and easier to review. If I had reordered the functions we would be getting all sorts of funky interleaving of added/removed blocks from the two functions.
Happy to do just the reordering in another commit though. -- 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: 2 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld@darkboxed.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 18 Jun 2020 10:33:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Gerrit-MessageType: comment
Daniel Gröber (dxld) has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42114 ) Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... Removed reviewer Daniel Gröber <dxld@darkboxed.org>. -- 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: 24 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: deleteReviewer
Attention is currently required from: Daniel Gröber (dxld), Julius Werner. Matt DeVillier has posted comments on this change by Daniel Gröber (dxld). ( https://review.coreboot.org/c/coreboot/+/42114?usp=email ) Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... Patch Set 26: (1 comment) Patchset: PS26: @jwerner@chromium.org mind re-reviewing here if still needed? -- To view, visit https://review.coreboot.org/c/coreboot/+/42114?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifc23f0cce695cd8aebf5549a7ca098c08c759f37 Gerrit-Change-Number: 42114 Gerrit-PatchSet: 26 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Gröber Gerrit-CC: Matt DeVillier <matt.devillier@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Julius Werner <jwerner@chromium.org> Gerrit-Attention: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Comment-Date: Wed, 11 Sep 2024 17:41:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
Attention is currently required from: Daniel Gröber (dxld), David Hendricks, Matt DeVillier. Julius Werner has posted comments on this change by Daniel Gröber (dxld). ( https://review.coreboot.org/c/coreboot/+/42114?usp=email ) Change subject: spi/winbond: Pull out winbond_get_bpbits function ...................................................................... Patch Set 26: (1 comment) Patchset: PS26:
@jwerner@chromium. […] Sorry, I think I may have stopped responding here at some point out of time constraints, and I don't really have much more right now either. It would be good if someone else could review this series if Daniel still wants to work on it (e.g. I think David knows the SPI flash code pretty well?).
-- To view, visit https://review.coreboot.org/c/coreboot/+/42114?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifc23f0cce695cd8aebf5549a7ca098c08c759f37 Gerrit-Change-Number: 42114 Gerrit-PatchSet: 26 Gerrit-Owner: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Gröber Gerrit-CC: Matt DeVillier <matt.devillier@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Attention: David Hendricks <david.hendricks@gmail.com> Gerrit-Attention: Daniel Gröber (dxld) <coreboot@dxld.at> Gerrit-Comment-Date: Mon, 16 Sep 2024 21:22:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier@gmail.com>
participants (3)
-
Daniel Gröber (dxld) (Code Review) -
Julius Werner (Code Review)
-
Matt DeVillier (Code Review)