Change in coreboot[master]: spi: Use common code for top-level flash block protection functions

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

to review the following change.

Change subject: spi: Use common code for top-level flash block protection functions
......................................................................

spi: Use common code for top-level flash block protection functions

This turns *_get_write_protection and *_set_write_protection into common
code by having the vendor code implement getter/setter functions for
bpbits and convertion functions between bpbits and a region.

Change-Id: I509a1cedff5de4bee34b8856ce651d100a32fd13
Signed-off-by: Daniel Gröber 
---
M src/drivers/spi/macronix.c
M src/drivers/spi/spi_flash.c
M src/drivers/spi/spi_winbond.h
M src/drivers/spi/winbond.c
M src/include/spi_flash.h
5 files changed, 156 insertions(+), 222 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42123/1

diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c
index 40f4a47..e5a7b98 100644
--- a/src/drivers/spi/macronix.c
+++ b/src/drivers/spi/macronix.c
@@ -316,64 +316,6 @@
    return 0;
 }

-
-/*
- * Read block protect bits from Status Reg.
- * Converts block protection bits to a region.
- *
- * Returns:
- * -1    on error
- *  1    if region is covered by write protection
- *  0    if a part of region isn't covered by write protection
- */
-static int macronix_get_write_protection(const struct spi_flash *flash,
-					const struct region *region)
-{
-	const struct spi_flash_part_id *params;
-	struct region wp_region;
-	int ret;
-
-	params = flash->part;
-
-	if (!params)
-		return -1;
-
-	if (!params->protection_granularity_shift)
-		return -1;
-
-	struct spi_flash_bpbits bpbits = {};
-	ret = macronix_get_bpbits(flash, params, &bpbits);
-	if (!ret)
-		return ret;
-
-	ret = macronix_bpbits_to_region(flash, params, &bpbits, &wp_region);
-	if (!ret)
-		return ret;
-
-	if (!region_sz(&wp_region)) {
-		printk(BIOS_DEBUG, "MACRONIX: flash isn't protected\n");
-		return 0;
-	}
-
-	printk(BIOS_DEBUG, "MACRONIX: flash protected range 0x%08zx-0x%08zx\n",
-	       region_offset(&wp_region), region_end(&wp_region));
-
-	return region_is_subregion(&wp_region, region);
-}
-
-/*
- * Protect a region starting from start of flash or end of flash.
- * The caller must provide a supported protected region size.
- * Writes block protect bits to Status 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 macronix_set_bpbits(const struct spi_flash *flash,
    		       const struct spi_flash_part_id *params,
    		       const struct spi_flash_bpbits *bits)
@@ -400,52 +342,12 @@
    return 0;
 }

-
-static int
-macronix_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;
-	int ret;
-
-	printk(BIOS_DEBUG, "MACRONIX: want to set protection for"
-	       " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region));
-
-	params = flash->part;
-
-	if (!params)
-		return -1;
-
-	if (!params->protection_granularity_shift) {
-		printk(BIOS_ERR, "MACRONIX: ERROR: spi_flash part doesn't support block protection\n");
-		return -1;
-	}
-
-	struct spi_flash_bpbits bpbits = {};
-	ret = macronix_get_bpbits(flash, params, &bpbits);
-	if (!ret)
-		return ret;
-
-	ret = macronix_region_to_bpbits(flash, params, region, mode, &bpbits);
-	if (!ret) {
-		printk(BIOS_ERR, "MACRONIX: ERROR: unsupported block protection"
-		       " region requested: 0x%08zx-0x%08zx\n",
-		       region_offset(region), region_end(region));
-		return ret;
-	}
-
-	ret = macronix_set_bpbits(flash, params, &bpbits);
-	if (!ret)
-		return ret;
-
-	return ret;
-}
-
-
 static const struct spi_flash_protection_ops spi_flash_protection_ops = {
-	.get_write = macronix_get_write_protection,
-	.set_write = macronix_set_write_protection,
+	.get_bpbits = macronix_get_bpbits,
+	.set_bpbits = macronix_set_bpbits,
+
+	.bpbits_to_region = macronix_bpbits_to_region,
+	.region_to_bpbits = macronix_region_to_bpbits,
 };

const struct spi_flash_vendor_info spi_flash_macronix_vi = {
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c
index 33416b5..e57076b 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -532,6 +532,50 @@
    return -1;
 }

+/*
+ * Read block protect bits from Status Reg and convert to a region.
+ *
+ * Returns:
+ * -1    on error
+ *  1    if region is covered by write protection
+ *  0    if a part of region isn't covered by write protection
+ */
+static int spi_flash_get_write_protection(const struct spi_flash *flash,
+					  const struct region *region)
+{
+	const struct spi_flash_part_id *params;
+	struct spi_flash_bpbits bpbits = {};
+	struct region wp_region;
+	int ret;
+
+	params = flash->part;
+	if (!params)
+		return -1;
+
+	if (!params->protection_granularity_shift)
+		return -1;
+
+	ret = flash->prot_ops->get_bpbits(flash, params, &bpbits);
+	if (ret)
+		return ret;
+
+	ret = flash->prot_ops->bpbits_to_region(flash, params, &bpbits,
+						&wp_region);
+	if (ret)
+		return ret;
+
+	if (!region_sz(&wp_region)) {
+		printk(BIOS_DEBUG, "SPI: flash isn't write-protected\n");
+		return 0;
+	}
+
+	printk(BIOS_DEBUG, "SPI: flash write protected range:"
+	       " 0x%08zx-0x%08zx\n", region_offset(&wp_region),
+	       region_end(&wp_region));
+
+	return region_is_subregion(&wp_region, region);
+}
+
 int spi_flash_is_write_protected(const struct spi_flash *flash,
    			 const struct region *region)
 {
@@ -551,7 +595,64 @@
    	return -1;
    }

-	return flash->prot_ops->get_write(flash, region);
+	return spi_flash_get_write_protection(flash, region);
+}
+
+/*
+ * Protect a region starting from start of flash or end of flash.
+ * The caller must provide a supported protected region size.
+ * Writes block protect bits to Status Reg.
+ * Optionally lock the status register if appropriate mode is used.
+ *
+ * @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 spi_flash_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;
+	int ret;
+
+	printk(BIOS_DEBUG, "SPI: want to set protection for"
+	       " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region));
+
+	params = flash->part;
+	if (!params)
+		return -1;
+
+	if (!params->protection_granularity_shift) {
+		printk(BIOS_ERR, "SPI: ERROR: flash part doesn't support block"
+		       " protection\n");
+		return -1;
+	}
+
+	struct spi_flash_bpbits bpbits = {};
+	ret = flash->prot_ops->get_bpbits(flash, params, &bpbits);
+	if (ret)
+		return ret;
+
+	ret = flash->prot_ops->region_to_bpbits(flash, params, region, mode,
+						&bpbits);
+	if (ret) {
+		printk(BIOS_ERR, "SPI: ERROR: unsupported block protection"
+		       " region requested: 0x%08zx-0x%08zx\n",
+		       region_offset(region), region_end(region));
+		return ret;
+	}
+
+	ret = flash->prot_ops->set_bpbits(flash, params, &bpbits);
+	if (ret)
+		return ret;
+
+	printk(BIOS_DEBUG, "SPI: write-protection range set to "
+	       "0x%08zx-0x%08zx\n", region_offset(region), region_end(region));
+
+	return 0;
 }

int spi_flash_set_write_protected(const struct spi_flash *flash,
@@ -575,7 +676,7 @@
    	return -1;
    }

-	ret = flash->prot_ops->set_write(flash, region, mode);
+	ret = spi_flash_set_write_protection(flash, region, mode);

if (ret == 0 && mode != SPI_WRITE_PROTECTION_PRESERVE) {
    	printk(BIOS_INFO, "SPI: SREG lock-down was set to ");
diff --git a/src/drivers/spi/spi_winbond.h b/src/drivers/spi/spi_winbond.h
index 2a3a4ae..b4f455a8 100644
--- a/src/drivers/spi/spi_winbond.h
+++ b/src/drivers/spi/spi_winbond.h
@@ -29,7 +29,7 @@
    const enum spi_flash_status_reg_lockdown mode,
    struct spi_flash_bpbits *bits);

-void winbond_bpbits_to_region(
+int winbond_bpbits_to_region(
    const struct spi_flash *flash,
    const struct spi_flash_part_id *params,
    const struct spi_flash_bpbits *bits,
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c
index d127287..9605279 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -219,10 +219,10 @@
  * Convert BPx, TB and CMP to a region.
  * SEC (if available) must be zero.
  */
-void winbond_bpbits_to_region(const struct spi_flash *flash,
-			      const struct spi_flash_part_id *params,
-			      const struct spi_flash_bpbits *bits,
-			      struct region *out)
+int winbond_bpbits_to_region(const struct spi_flash *flash,
+			     const struct spi_flash_part_id *params,
+			     const struct spi_flash_bpbits *bits,
+			     struct region *out)
 {
    const size_t granularity = 1 << params->protection_granularity_shift;
    size_t protected_size =
@@ -244,46 +244,8 @@
    	out->offset = tb ? 0 : flash->size - protected_size;
    	out->size = protected_size;
    }
-}

-/*
- * Available on all devices.
- * Read block protect bits from Status/Status2 Reg.
- * Converts block protection bits to a region.
- *
- * Returns:
- * -1    on error
- *  1    if region is covered by write protection
- *  0    if a part of region isn't covered by write protection
- */
-static int winbond_get_write_protection(const struct spi_flash *flash,
-					const struct region *region)
-{
-	const struct spi_flash_part_id *params;
-	struct region wp_region;
-	struct spi_flash_bpbits bpbits;
-	int ret;
-
-	params = flash->part;
-	if (!params)
-		return -1;
-
-	ret = winbond_get_bpbits(flash, params, &bpbits);
-	if (ret)
-		return ret;
-
-	winbond_bpbits_to_region(flash, params, &bpbits, &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;
 }

static int winbond_get_bpbits(const struct spi_flash *flash,
@@ -555,56 +517,12 @@
    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, params, &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,
+	.get_bpbits = winbond_get_bpbits,
+	.set_bpbits = winbond_set_bpbits,
+
+	.bpbits_to_region = winbond_bpbits_to_region,
+	.region_to_bpbits = winbond_region_to_bpbits,
 };

const struct spi_flash_vendor_info spi_flash_winbond_vi = {
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h
index fe62069..bf84ce0 100644
--- a/src/include/spi_flash.h
+++ b/src/include/spi_flash.h
@@ -85,37 +85,50 @@
    };
 };

+struct spi_flash_part_id;
+
 /* Current code assumes all callbacks are supplied in this object. */
 struct spi_flash_protection_ops {
-	/*
-	 * Returns 1 if the whole region is software write protected.
-	 * Hardware write protection mechanism aren't accounted.
-	 * If the write protection could be changed, due to unlocked status
-	 * register for example, 0 should be returned.
-	 * Returns 0 on success.
-	 */
-	int (*get_write)(const struct spi_flash *flash,
-				    const struct region *region);
-	/*
-	 * Enable the status register write protection, if supported on the
-	 * requested region, and optionally enable status register lock-down.
-	 * Returns 0 if the whole region was software write protected.
-	 * Hardware write protection mechanism aren't accounted.
-	 * If the status register is locked and the requested configuration
-	 * doesn't match the selected one, return an error.
-	 * Only a single region is supported !
+
+	/**
+	 * Read current block protection bits from device status registers.
     *
-	 * @return 0 on success
+	 * Returns non-zero on error, zero on success.
     */
-	int
-	(*set_write)(const struct spi_flash *flash,
+	int (*get_bpbits)(const struct spi_flash *flash,
+			  const struct spi_flash_part_id *part,
+			  struct spi_flash_bpbits *bpbits);
+	/**
+	 * Write block protection bits to non-volatile device status registers.
+	 *
+	 * Returns non-zero on error, zero on success.
+	 */
+	int (*set_bpbits)(const struct spi_flash *flash,
+			  const struct spi_flash_part_id *part,
+			  const struct spi_flash_bpbits *bpbits);
+
+	/**
+	 * Convert block protection bits to write-protected address region.
+	 *
+	 * Returns non-zero on error, zero on success.
+	 */
+	int (*bpbits_to_region)(const struct spi_flash *flash,
+				const struct spi_flash_part_id *part,
+				const struct spi_flash_bpbits *bpbits,
+				struct region *region);
+	/**
+	 * Convert address region to be write-protected into block
+	 * protection bits.
+	 *
+	 * Returns non-zero on error, zero on success.
+	 */
+	int (*region_to_bpbits)(const struct spi_flash *flash,
+				const struct spi_flash_part_id *part,
    			const struct region *region,
-				const enum spi_flash_status_reg_lockdown mode);
-
+				const enum spi_flash_status_reg_lockdown mode,
+				struct spi_flash_bpbits *bpbits);
 };

-struct spi_flash_part_id;
-
 struct spi_flash {
    struct spi_slave spi;
    u8 vendor;

[L] Change in coreboot[master]: spi: Use common code for top-level flash block protection functions

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

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

Change subject: spi: Use common code for top-level flash block protection functions
......................................................................

Removed cc Daniel Gröber .

[L] Change in coreboot[master]: spi: Use common code for top-level flash block protection functions

[Daniel Gröber (Code Review)]

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

Change subject: spi: Use common code for top-level flash block protection functions
......................................................................

Removed reviewer Daniel Gröber .