Change in coreboot[master]: spi/winbond: Pull out winbond_set_bpbits function

[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/+/42121

to review the following change.

Change subject: spi/winbond: Pull out winbond_set_bpbits function
......................................................................

spi/winbond: Pull out winbond_set_bpbits function

Split bpbits write code from winbond_set_write_protection into a new
function: winbond_set_bpbits.

Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98
Signed-off-by: Daniel Gröber 
---
M src/drivers/spi/winbond.c
1 file changed, 68 insertions(+), 54 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42121/1

diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c
index afffd4d..b834585 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -210,6 +210,9 @@

static int winbond_get_bpbits(const struct spi_flash *flash,
    		      struct spi_flash_bpbits *bpbits);
+static int winbond_set_bpbits(const struct spi_flash *flash,
+			      const struct spi_flash_part_id *params,
+			      const struct spi_flash_bpbits *bpbits);

/*
  * Convert BPx, TB and CMP to a region.
@@ -432,84 +435,48 @@
    return ret;
 }

-/*
- * Available on all devices.
- * Protect a region starting from start of flash or end of flash.
- * The caller must provide a supported protected region size.
- * SEC isn't supported and set to zero.
- * Write block protect bits to Status/Status2 Reg.
- * Optionally lock the status register if lock_sreg is set with the provided
- * mode.
- *
- * @param flash: The flash to operate on
- * @param region: The region to write protect
- * @param mode: Optional status register lock-down mode
- *
- * @return 0 on success
- */
-static int
-winbond_set_write_protection(const struct spi_flash *flash,
-			     const struct region *region,
-			     const enum spi_flash_status_reg_lockdown mode)
+static int winbond_set_bpbits(const struct spi_flash *flash,
+			      const struct spi_flash_part_id *params,
+			      const struct spi_flash_bpbits *bpbits)
 {
-	const struct spi_flash_part_id *params;
-	struct status_regs mask, val;
-	struct spi_flash_bpbits bpbits;
+	struct status_regs msk, val;
    int ret;

-	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 }
+			.bp3 = { .bp = bpbits->bp, .tb = bpbits->tb, .sec = 0 }
    	};
-		mask.reg1 = (union status_reg1) {
+		msk.reg1 = (union status_reg1) {
    		.bp3 = { .bp = ~0, .tb = 1, .sec = 1 }
    	};
    } else {
    	val.reg1 = (union status_reg1) {
-			.bp4 = { .bp = bpbits.bp, .tb = bpbits.tb }
+			.bp4 = { .bp = bpbits->bp, .tb = bpbits->tb }
    	};
-		mask.reg1 = (union status_reg1) {
+		msk.reg1 = (union status_reg1) {
    		.bp4 = { .bp = ~0, .tb = 1 }
    	};
    }

-	val.reg2 = (union status_reg2) { .cmp = bpbits.cmp };
-	mask.reg2 = (union status_reg2) { .cmp = 1 };
+	val.reg2 = (union status_reg2) { .cmp = bpbits->cmp };
+	msk.reg2 = (union status_reg2) { .cmp = 1 };

if (params->bp_bits == 3) {
-		val.reg1.bp3.srp0 = bpbits.winbond.srp0;
-		mask.reg1.bp3.srp0 = 1;
+		val.reg1.bp3.srp0 = bpbits->winbond.srp0;
+		msk.reg1.bp3.srp0 = 1;
    } else {
-		val.reg1.bp4.srp = bpbits.winbond.srp0;
-		mask.reg1.bp4.srp = 1;
+		val.reg1.bp4.srp = bpbits->winbond.srp;
+		msk.reg1.bp4.srp = 1;
    }

-	val.reg2.srp1 = bpbits.winbond.srp1;
-	mask.reg2.srp1 = 1;
+	val.reg2.srp1 = bpbits->winbond.srp1;
+	msk.reg2.srp1 = 1;

-	ret = winbond_flash_cmd_status(flash, mask.u, val.u, true);
+	ret = winbond_flash_cmd_status(flash, msk.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;
 }

int winbond_region_to_bpbits(
@@ -590,6 +557,53 @@
    return 0;
 }

+/*
+ * Available on all devices.
+ * Protect a region starting from start of flash or end of flash.
+ * The caller must provide a supported protected region size.
+ * SEC isn't supported and set to zero.
+ * Write block protect bits to Status/Status2 Reg.
+ * Optionally lock the status register if lock_sreg is set with the provided
+ * mode.
+ *
+ * @param flash: The flash to operate on
+ * @param region: The region to write protect
+ * @param mode: Optional status register lock-down mode
+ *
+ * @return 0 on success
+ */
+static int
+winbond_set_write_protection(const struct spi_flash *flash,
+			     const struct region *region,
+			     const enum spi_flash_status_reg_lockdown mode)
+{
+	const struct spi_flash_part_id *params;
+	struct spi_flash_bpbits bpbits;
+	int ret;
+
+	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;
+
+
+	ret = winbond_set_bpbits(flash, params, &bpbits);
+	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 const struct spi_flash_protection_ops spi_flash_protection_ops = {
    .get_write = winbond_get_write_protection,
    .set_write = winbond_set_write_protection,

[Julius Werner (Code Review)]

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

Change subject: spi/winbond: Pull out winbond_set_bpbits function
......................................................................

Patch Set 3: Code-Review+2

[M] Change in coreboot[master]: spi/winbond: Pull out winbond_set_bpbits function

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

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

Change subject: spi/winbond: Pull out winbond_set_bpbits function
......................................................................

Removed cc Daniel Gröber .

[M] Change in coreboot[master]: spi/winbond: Pull out winbond_set_bpbits function

[Daniel Gröber (Code Review)]

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

Change subject: spi/winbond: Pull out winbond_set_bpbits function
......................................................................

Removed reviewer Daniel Gröber .