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; }
/**
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()).
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.
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.
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?
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?).
-
Daniel Gröber (dxld) (Code Review) -
Julius Werner (Code Review)
-
Matt DeVillier (Code Review)