diff mbox series

[v2,3/3] mtd: spi-nor: add locking support for is25xxxxx device

Message ID 1556474956-27786-4-git-send-email-sagar.kadam@sifive.com (mailing list archive)
State New, archived
Headers show
Series add support for is25wp256 spi-nor device. | expand

Commit Message

Sagar Shrikant Kadam April 28, 2019, 6:09 p.m. UTC
The locking scheme for ISSI devices is based on stm_lock mechanism.
The is25xxxxx  devices have 4 bits for selecting the range of blocks to
be locked for write.

The current implementation, blocks entire 512 blocks of flash memory.

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 60 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Paul Walmsley April 30, 2019, 5:13 p.m. UTC | #1
On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:

> The locking scheme for ISSI devices is based on stm_lock mechanism.
> The is25xxxxx  devices have 4 bits for selecting the range of blocks to
> be locked for write.
> 
> The current implementation, blocks entire 512 blocks of flash memory.
> 
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 60 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 81c7b3e..2dba7e9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1459,6 +1459,65 @@ static int macronix_quad_enable(struct spi_nor *nor)
>  
>  	return 0;
>  }
> +/**

The above sequence indicates a kerneldoc-style comment, but the format of 
the comment is not in kerneldoc format.  Please fix this comment to 
conform with

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/kernel-docs.rst


> + * Lock a region of the flash.Implementation is based on stm_lock
> + * Supports the block protection bits BP{0,1,2,3} in the status register
> + * Returns negative on errors, 0 on success.
> + */
> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{

Almost all of this function is copied and pasted from stm_lock().  Please 
don't do this: it adds bloat, makes the code hard to maintain, and 
increases the risk that fixes will only target one function rather than 
both.  Instead please pull the common code out into a separate static 
function.

> +	struct mtd_info *mtd = &nor->mtd;
> +	int status_old, status_new;
> +	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 shift = ffs(mask) - 1, pow, val = 0;
> +	loff_t lock_len;
> +	bool use_top = true;
> +
> +	status_old = read_sr(nor);
> +
> +	if (status_old < 0)
> +		return status_old;
> +
> +	/* lock_len: length of region that should end up locked */
> +	if (use_top)
> +		lock_len = mtd->size - ofs;
> +	else
> +		lock_len = ofs + len;
> +
> +	/*
> +	 * Need smallest pow such that:
> +	 *
> +	 *   1 / (2^pow) <= (len / size)
> +	 *
> +	 * so (assuming power-of-2 size) we do:
> +	 *
> +	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
> +	 */
> +	pow = ilog2(mtd->size) - ilog2(lock_len);
> +	val = mask - (pow << shift);
> +
> +	if (val & ~mask)
> +		return -EINVAL;
> +
> +	/* Don't "lock" with no region! */
> +	if (!(val & mask))
> +		return -EINVAL;
> +
> +	status_new = (status_old & ~mask & ~SR_TB) | val;
> +
> +	/* Disallow further writes if WP pin is asserted */
> +	status_new |= SR_SRWD;
> +
> +	/* Don't bother if they're the same */
> +	if (status_new == status_old)
> +		return 0;
> +
> +	/* Only modify protection if it will not unlock other areas */
> +	if ((status_new & mask) < (status_old & mask))
> +		return -EINVAL;
> +
> +	return write_sr_and_check(nor, status_new, mask);
> +}
>  
>  /**
>   * issi_unlock() - clear BP[0123] write-protection.
> @@ -4121,6 +4180,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	/* NOR protection support for ISSI chips */
>  	if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
>  	    info->flags & SPI_NOR_HAS_LOCK) {
> +		nor->flash_lock = issi_lock;
>  		nor->flash_unlock = issi_unlock;
>  
>  	}
> -- 
> 1.9.1
> 
>
Paul Walmsley April 30, 2019, 5:19 p.m. UTC | #2
On Tue, 30 Apr 2019, Paul Walmsley wrote:

> The above sequence indicates a kerneldoc-style comment, but the format of 
> the comment is not in kerneldoc format.  Please fix this comment to 
> conform with
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/kernel-docs.rst

This, rather:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst#n57


- Paul
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 81c7b3e..2dba7e9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1459,6 +1459,65 @@  static int macronix_quad_enable(struct spi_nor *nor)
 
 	return 0;
 }
+/**
+ * Lock a region of the flash.Implementation is based on stm_lock
+ * Supports the block protection bits BP{0,1,2,3} in the status register
+ * Returns negative on errors, 0 on success.
+ */
+static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	struct mtd_info *mtd = &nor->mtd;
+	int status_old, status_new;
+	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
+	u8 shift = ffs(mask) - 1, pow, val = 0;
+	loff_t lock_len;
+	bool use_top = true;
+
+	status_old = read_sr(nor);
+
+	if (status_old < 0)
+		return status_old;
+
+	/* lock_len: length of region that should end up locked */
+	if (use_top)
+		lock_len = mtd->size - ofs;
+	else
+		lock_len = ofs + len;
+
+	/*
+	 * Need smallest pow such that:
+	 *
+	 *   1 / (2^pow) <= (len / size)
+	 *
+	 * so (assuming power-of-2 size) we do:
+	 *
+	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
+	 */
+	pow = ilog2(mtd->size) - ilog2(lock_len);
+	val = mask - (pow << shift);
+
+	if (val & ~mask)
+		return -EINVAL;
+
+	/* Don't "lock" with no region! */
+	if (!(val & mask))
+		return -EINVAL;
+
+	status_new = (status_old & ~mask & ~SR_TB) | val;
+
+	/* Disallow further writes if WP pin is asserted */
+	status_new |= SR_SRWD;
+
+	/* Don't bother if they're the same */
+	if (status_new == status_old)
+		return 0;
+
+	/* Only modify protection if it will not unlock other areas */
+	if ((status_new & mask) < (status_old & mask))
+		return -EINVAL;
+
+	return write_sr_and_check(nor, status_new, mask);
+}
 
 /**
  * issi_unlock() - clear BP[0123] write-protection.
@@ -4121,6 +4180,7 @@  int spi_nor_scan(struct spi_nor *nor, const char *name,
 	/* NOR protection support for ISSI chips */
 	if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
 	    info->flags & SPI_NOR_HAS_LOCK) {
+		nor->flash_lock = issi_lock;
 		nor->flash_unlock = issi_unlock;
 
 	}