Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42115
to review the following change.
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
spi/winbond: Pull out winbond_region_to_bpbits function
Split region>bpbits conversion related logic out from winbond_set_write_protection into a new function: winbond_region_to_bpbits.
Change-Id: I2c7b08cb56772aa620e690077bbbb1fde3d7aae7 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/winbond.c 1 file changed, 94 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/42115/1
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 77191e4..51d43a8 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -211,6 +211,13 @@ static int winbond_get_bpbits(const struct spi_flash *flash, struct spi_flash_bpbits *bpbits);
+static int winbond_region_to_bpbits( + const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode, + struct spi_flash_bpbits *bits); + /* * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. @@ -449,19 +456,77 @@ { const struct spi_flash_part_id *params; struct status_regs mask, val; - struct region wp_region; - u8 cmp, bp, tb; + struct spi_flash_bpbits bpbits; int ret;
- /* Need to touch TOP or BOTTOM */ - if (region_offset(region) != 0 && region_end(region) != flash->size) - return -1; - params = flash->part;
if (!params) return -1;
+ ret = winbond_get_bpbits(flash, &bpbits); + if (!ret) + return ret; + + ret = winbond_region_to_bpbits(flash, params, region, mode, &bpbits); + if (!ret) + return ret; + + /* Write block protection bits */ + + if (params->bp_bits == 3) { + val.reg1 = (union status_reg1) { + .bp3 = { .bp = bpbits.bp, .tb = bpbits.tb, .sec = 0 } + }; + mask.reg1 = (union status_reg1) { + .bp3 = { .bp = ~0, .tb = 1, .sec = 1 } + }; + } else { + val.reg1 = (union status_reg1) { + .bp4 = { .bp = bpbits.bp, .tb = bpbits.tb } + }; + mask.reg1 = (union status_reg1) { + .bp4 = { .bp = ~0, .tb = 1 } + }; + } + + val.reg2 = (union status_reg2) { .cmp = bpbits.cmp }; + mask.reg2 = (union status_reg2) { .cmp = 1 }; + + if (params->bp_bits == 3) { + val.reg1.bp3.srp0 = bpbits.winbond.srp0; + mask.reg1.bp3.srp0 = 1; + } else { + val.reg1.bp4.srp0 = bpbits.winbond.srp0; + mask.reg1.bp4.srp0 = 1; + } + + val.reg2.srp1 = bpbits.winbond.srp1; + mask.reg2.srp1 = 1; + + ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); + 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 int winbond_region_to_bpbits( + const struct spi_flash *flash, + const struct spi_flash_part_id *params, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode, + struct spi_flash_bpbits *bits) +{ + struct region wp_region; + + /* Need to touch TOP or BOTTOM */ + if (region_offset(region) != 0 && region_end(region) != flash->size) + return -1; + if (params->bp_bits != 3 && params->bp_bits != 4) { /* FIXME: not implemented */ return -1; @@ -470,91 +535,61 @@ wp_region = *region;
if (region_offset(&wp_region) == 0) - tb = 1; + bits->tb = 1; else - tb = 0; + bits->tb = 0;
if (region_sz(&wp_region) > flash->size / 2) { - cmp = 1; - wp_region.offset = tb ? 0 : region_sz(&wp_region); + bits->cmp = 1; + wp_region.offset = bits->tb ? 0 : region_sz(&wp_region); wp_region.size = flash->size - region_sz(&wp_region); - tb = !tb; + bits->tb = !bits->tb; } else { - cmp = 0; + bits->cmp = 0; }
if (region_sz(&wp_region) == 0) { - bp = 0; + bits->bp = 0; } else if (IS_POWER_OF_2(region_sz(&wp_region)) && (region_sz(&wp_region) >= (1 << params->protection_granularity_shift))) { - bp = log2(region_sz(&wp_region)) - + bits->bp = log2(region_sz(&wp_region)) - params->protection_granularity_shift + 1; } else { printk(BIOS_ERR, "WINBOND: ERROR: unsupported region size\n"); return -1; }
- /* Write block protection bits */ - - if (params->bp_bits == 3) { - val.reg1 = (union status_reg1) { - .bp3 = { .bp = bp, .tb = tb, .sec = 0 } - }; - mask.reg1 = (union status_reg1) { - .bp3 = { .bp = ~0, .tb = 1, .sec = 1 } - }; - } else { - val.reg1 = (union status_reg1) { - .bp4 = { .bp = bp, .tb = tb } - }; - mask.reg1 = (union status_reg1) { - .bp4 = { .bp = ~0, .tb = 1 } - }; - } - - val.reg2 = (union status_reg2) { .cmp = cmp }; - mask.reg2 = (union status_reg2) { .cmp = 1 }; - if (mode != SPI_WRITE_PROTECTION_PRESERVE) { - u8 srp; switch (mode) { case SPI_WRITE_PROTECTION_NONE: - srp = 0; + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 0; break; case SPI_WRITE_PROTECTION_PIN: - srp = 1; + bits->winbond.srp1 = 0; + bits->winbond.srp0 = 1; break; case SPI_WRITE_PROTECTION_REBOOT: - srp = 2; + bits->winbond.srp1 = 1; + bits->winbond.srp0 = 0; break; case SPI_WRITE_PROTECTION_PERMANENT: - srp = 3; + if (params->bp_bits == 3) { + bits->winbond.srp1 = 1; + bits->winbond.srp0 = 1; + } else { + /* FIXME: special permanent protect write + * sequence not implemented. */ + return -1; + } break; default: return -1; } - - if (params->bp_bits == 3) { - val.reg1.bp3.srp0 = !!(srp & 1); - mask.reg1.bp3.srp0 = 1; - } else { - val.reg1.bp4.srp0 = !!(srp & 1); - mask.reg1.bp4.srp0 = 1; - } - - val.reg2.srp1 = !!(srp & 2); - mask.reg2.srp1 = 1; }
- ret = winbond_flash_cmd_status(flash, mask.u, val.u, true); - 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; + return 0; }
static const struct spi_flash_protection_ops spi_flash_protection_ops = {
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42115 )
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c:
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@2... PS2, Line 214: static int winbond_region_to_bpbits( Same here.
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 467: ret = winbond_get_bpbits(flash, &bpbits); Why the get_bpbits() here? Aren't you going to overwrite the whole thing anyway?
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 497: val.reg1.bp3.srp0 = bpbits.winbond.srp0; Why is this down here and not with the others above?
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42115 )
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c:
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@2... PS2, Line 214: static int winbond_region_to_bpbits(
Same here.
Not sure what you mean here? The bit about forward-declared static fns? Same answer then :)
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 467: ret = winbond_get_bpbits(flash, &bpbits);
Why the get_bpbits() here? Aren't you going to overwrite the whole thing anyway?
We're doing a read-modify-write cycle on the bpbits because the region_to_bpbits function doesn't necessarily fill-in everything (in the SPI_WRITE_PROTECTION_PRESERVE case).
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 497: val.reg1.bp3.srp0 = bpbits.winbond.srp0;
Why is this down here and not with the others above?
If you look at the removed lines just below, just after the `switch (mode) { block you'll see that it's simply the order it was in before :)
I suppose there's no technical reason to keep it this way, we could move the {val,mask}.reg2 init above the `if` and merge the two `if` blocks. On the other hand there's already a lot of numbers flying around here which is confusing enough so I kind of prefer doing it in this "first reg1, then reg2" sort of way.
Attention is currently required from: Julius Werner.
Daniel Gröber has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42115 )
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
Removed reviewer Daniel Gröber dxld@darkboxed.org.
Attention is currently required from: Julius Werner.
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42115?usp=email )
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
Patch Set 27:
(1 comment)
Patchset:
PS27: This seems to be the first patch in my series that fail in jenkins.
I'm getting
``` arm-eabi-ld.bfd: Verstage exceeded its allotted size! (47K + 768) arm-eabi-ld.bfd: preram_cbfs_cache overlaps the previous region! ```
in various GOOGLE_VEYRON_* configs. I tried to reproduce this locally with `make what-jenkins-does` on top of my whole series but I couldn't reproduce these failures.
Not sure how to fix this.
-
Daniel Gröber (Code Review) -
Daniel Gröber (dxld) (Code Review)
-
Julius Werner (Code Review)