Change in coreboot[master]: spi/winbond: Pass flash and part info to bpbits_to_region

[Daniel Gröber (dxld) (Code Review)]

Hello Daniel Gröber,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/42119

to review the following change.

Change subject: spi/winbond: Pass flash and part info to bpbits_to_region
......................................................................

spi/winbond: Pass flash and part info to bpbits_to_region

This makes bpbits_to_region mirror region_to_bpbits more closely.

Change-Id: Ie60e5feaab29f4db80db6ea688453b042397257e
Signed-off-by: Daniel Gröber 
---
M src/drivers/spi/spi_winbond.h
M src/drivers/spi/winbond.c
M tests/drivers/winbond-spi-test.c
3 files changed, 11 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42119/1

diff --git a/src/drivers/spi/spi_winbond.h b/src/drivers/spi/spi_winbond.h
index ff799ff..2a3a4ae 100644
--- a/src/drivers/spi/spi_winbond.h
+++ b/src/drivers/spi/spi_winbond.h
@@ -30,7 +30,7 @@
    struct spi_flash_bpbits *bits);

void winbond_bpbits_to_region(
-	const size_t granularity,
+	const struct spi_flash *flash,
+	const struct spi_flash_part_id *params,
    const struct spi_flash_bpbits *bits,
-	const size_t flash_size,
    struct region *out);
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c
index 49d3c5b..7d44bfe 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -215,21 +215,22 @@
  * Convert BPx, TB and CMP to a region.
  * SEC (if available) must be zero.
  */
-void winbond_bpbits_to_region(const size_t granularity,
+void winbond_bpbits_to_region(const struct spi_flash *flash,
+			      const struct spi_flash_part_id *params,
    		      const struct spi_flash_bpbits *bits,
-			      const size_t flash_size,
    		      struct region *out)
 {
+	const size_t granularity = 1 << params->protection_granularity_shift;
    size_t protected_size =
-		MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash_size);
+		MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash->size);

int tb = bits->tb;
    if (bits->cmp) {
-		protected_size = flash_size - protected_size;
+		protected_size = flash->size - protected_size;
    	tb = !tb;
    }

-	out->offset = tb ? 0 : flash_size - protected_size;
+	out->offset = tb ? 0 : flash->size - protected_size;
    out->size = protected_size;
 }

@@ -252,18 +253,14 @@
    int ret;

params = flash->part;
-
    if (!params)
    	return -1;

-	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);
+	winbond_bpbits_to_region(flash, params, &bpbits, &wp_region);

if (!region_sz(&wp_region)) {
    	printk(BIOS_DEBUG, "WINBOND: flash isn't protected\n");
diff --git a/tests/drivers/winbond-spi-test.c b/tests/drivers/winbond-spi-test.c
index da7177b..98c715d 100644
--- a/tests/drivers/winbond-spi-test.c
+++ b/tests/drivers/winbond-spi-test.c
@@ -161,8 +161,7 @@
    struct region expected, out = {-1, -1};
    struct region roundtripped = {-1, -1};

-	winbond_bpbits_to_region(granularity, bpbits,
-				 flash.size, &out);
+	winbond_bpbits_to_region(&flash, part, bpbits, &out);

enum spi_flash_status_reg_lockdown mode =
    	SPI_WRITE_PROTECTION_PIN;
@@ -170,8 +169,7 @@
    winbond_region_to_bpbits(&flash, part, &out, mode,
    			 &bpbits1);

-	winbond_bpbits_to_region(granularity, &bpbits1,
-				 flash.size, &roundtripped);
+	winbond_bpbits_to_region(&flash, part, &bpbits1, &roundtripped);

const struct block_region *wp_blocks =
    	&table->wp_blocks[bpbits->cmp][bpbits->tb][bpbits->bp];

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42119 )

Change subject: spi/winbond: Pass flash and part info to bpbits_to_region
......................................................................

Patch Set 3: Code-Review+2

[S] Change in coreboot[master]: spi/winbond: Pass flash and part info to bpbits_to_region

[Daniel Gröber (dxld) (Code Review)]

Attention is currently required from: Jakub Czapiga.

Daniel Gröber (dxld) has removed Daniel Gröber from this change.  ( https://review.coreboot.org/c/coreboot/+/42119 )

Change subject: spi/winbond: Pass flash and part info to bpbits_to_region
......................................................................

Removed cc Daniel Gröber .

[S] Change in coreboot[master]: spi/winbond: Pass flash and part info to bpbits_to_region

[Daniel Gröber (Code Review)]

Attention is currently required from: Jakub Czapiga.

Daniel Gröber has removed Daniel Gröber from this change.  ( https://review.coreboot.org/c/coreboot/+/42119 )

Change subject: spi/winbond: Pass flash and part info to bpbits_to_region
......................................................................

Removed reviewer Daniel Gröber .