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 <[e-mail]>
---
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,

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42121
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98
Gerrit-Change-Number: 42121
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-MessageType: newchange

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42121
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98
Gerrit-Change-Number: 42121
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 18 Jun 2020 03:04:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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 <[e-mail]>.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42121
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98
Gerrit-Change-Number: 42121
Gerrit-PatchSet: 29
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: deleteReviewer

[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 <[e-mail]>.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42121
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If3ed24cd5bf31272bad5c3a404503cc0b3889e98
Gerrit-Change-Number: 42121
Gerrit-PatchSet: 30
Gerrit-Owner: Daniel Gröber <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: deleteReviewer