The patch set is tested on HiFive Unleashed board and is based on mainline
kernel v 5.1-rc5. Its intended to add support for 32 MB spi-nor flash
mounted on the board. Memory Device supports 4/32/and 64 KB sectors size.
The device id table is updated accordingly.
Flash parameter table for ISSI device is set to use macronix_quad_enable
procedure to set the QE (quad-enable) bit of Status register.
With issi_lock and unlock schemes support for block protection is added
in patch 2 and 3.
These patches are extended from base work done in a patch at following commit:
https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
Erase/Read/Write operations are verified on HiFive Unleashed board using mtd and flash utils (v1.5.2):
1. mtd_debug :Options available are : erase/read/write.
2. flashcp :Single utility that erases flash, writes a file to flash and verifies the data back.
3. flash_lock :Lock flash memory blocks.
4. flash_unlock: Unlock flash memory blocks.
Unlock scheme clears the bit protection bits of all blocks in the Status register.
Lock schemes in patch 3 is based on stm_lock mechanism. With current implementation entire flash memory
gets protected.
Block protection schemes are tested with flash_lock and unlock utils.
Revision history:
V1<-> V2:
-Incorporated changes suggested by reviewers regarding patch/cover letter versioning, references of patch.
-Updated cover letter with description for flash operations verified with these changes.
-Add support for unlocking is25xxxxxx device
-Add support for locking is25xxxxxx device.
v1:
-Add support for is25wp256 device.
Sagar Shrikant Kadam (3):
mtd: spi-nor: add support for is25wp256
mtd: spi-nor: add support to unlock flash device.
mtd: spi-nor: add locking support for is25xxxxx device
drivers/mtd/spi-nor/spi-nor.c | 116 +++++++++++++++++++++++++++++++++++++++++-
include/linux/mtd/spi-nor.h | 2 +
2 files changed, 117 insertions(+), 1 deletion(-)
--
1.9.1
Update spi_nor_id tablet for is25wp256 (32MB)device from ISSI,
present on HiFive Unleashed dev board (Rev: A00).
Set method to enable quad mode for ISSI device in flash parameters
table.
Signed-off-by: Sagar Shrikant Kadam <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 10 +++++++++-
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index fae1474..c5408ed 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1834,6 +1834,10 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "is25wp128", INFO(0x9d7018, 0, 64 * 1024, 256,
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
+ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+ SPI_NOR_4B_OPCODES)
+ },
/* Macronix */
{ "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1, SECT_4K) },
@@ -3650,6 +3654,10 @@ static int spi_nor_init_params(struct spi_nor *nor,
case SNOR_MFR_MACRONIX:
params->quad_enable = macronix_quad_enable;
break;
+ case SNOR_MFR_ISSI:
+ params->quad_enable = macronix_quad_enable;
+ break;
+
case SNOR_MFR_ST:
case SNOR_MFR_MICRON:
@@ -4127,7 +4135,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (ret)
return ret;
- if (nor->addr_width) {
+ if (nor->addr_width && JEDEC_MFR(info) != SNOR_MFR_ISSI) {
/* already configured from SFDP */
} else if (info->addr_width) {
nor->addr_width = info->addr_width;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index b3d360b..ff13297 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -19,6 +19,7 @@
#define SNOR_MFR_ATMEL CFI_MFR_ATMEL
#define SNOR_MFR_GIGADEVICE 0xc8
#define SNOR_MFR_INTEL CFI_MFR_INTEL
+#define SNOR_MFR_ISSI 0x9d /* ISSI */
#define SNOR_MFR_ST CFI_MFR_ST /* ST Micro */
#define SNOR_MFR_MICRON CFI_MFR_MICRON /* Micron */
#define SNOR_MFR_MACRONIX CFI_MFR_MACRONIX
--
1.9.1
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 <[email protected]>
---
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;
}
+/**
+ * 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;
}
--
1.9.1
Nor device (is25wp256 mounted on HiFive unleashed Rev A00 board) from ISSI
have memory blocks guarded by block protection bits BP[0,1,2,3].
Clearing block protection bits,unlocks the flash memory regions
The unlock scheme is registered during nor scans.
Signed-off-by: Sagar Shrikant Kadam <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c5408ed..81c7b3e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1461,6 +1461,46 @@ static int macronix_quad_enable(struct spi_nor *nor)
}
/**
+ * issi_unlock() - clear BP[0123] write-protection.
+ * @nor: pointer to a 'struct spi_nor'
+ * @ofs: offset from which to unlock memory
+ * @len: number of bytes to unlock
+ * Bits [2345] of the Status Register are BP[0123].
+ * ISSI chips use a different block protection scheme than other chips.
+ * Just disable the write-protect unilaterally.
+ * Return: 0 on success, -errno otherwise.
+ */
+static int issi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+ int ret, val;
+ u8 mask = SR_BP0 | SR_BP1 | SR_BP2 | SR_BP3;
+
+ val = read_sr(nor);
+ if (val < 0)
+ return val;
+ if (!(val & mask))
+ return 0;
+
+ write_enable(nor);
+
+ write_sr(nor, val & ~mask);
+
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret)
+ return ret;
+
+ ret = read_sr(nor);
+ if (ret > 0 && !(ret & mask)) {
+ dev_info(nor->dev,
+ "ISSI Block Protection Bits cleared SR=0x%x", ret);
+ return 0;
+ } else {
+ dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
+ return -EINVAL;
+ }
+}
+
+/**
* spansion_quad_enable() - set QE bit in Configuraiton Register.
* @nor: pointer to a 'struct spi_nor'
*
@@ -1836,7 +1876,7 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
- SPI_NOR_4B_OPCODES)
+ SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK)
},
/* Macronix */
@@ -4078,6 +4118,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
nor->flash_is_locked = stm_is_locked;
}
+ /* NOR protection support for ISSI chips */
+ if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
+ info->flags & SPI_NOR_HAS_LOCK) {
+ nor->flash_unlock = issi_unlock;
+
+ }
if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
mtd->_lock = spi_nor_lock;
mtd->_unlock = spi_nor_unlock;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index ff13297..9a7d719 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -127,6 +127,7 @@
#define SR_BP0 BIT(2) /* Block protect 0 */
#define SR_BP1 BIT(3) /* Block protect 1 */
#define SR_BP2 BIT(4) /* Block protect 2 */
+#define SR_BP3 BIT(5) /* Block protect 3 for ISSI device*/
#define SR_TB BIT(5) /* Top/Bottom protect */
#define SR_SRWD BIT(7) /* SR write protect */
/* Spansion/Cypress specific status bits */
--
1.9.1
On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:
> Update spi_nor_id tablet for is25wp256 (32MB)device from ISSI,
> present on HiFive Unleashed dev board (Rev: A00).
>
> Set method to enable quad mode for ISSI device in flash parameters
> table.
This patch was based on one originally written by Wes and/or Palmer:
https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
The right thing to do is to note this in the commit message.
> Signed-off-by: Sagar Shrikant Kadam <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 10 +++++++++-
> include/linux/mtd/spi-nor.h | 1 +
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index fae1474..c5408ed 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1834,6 +1834,10 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> { "is25wp128", INFO(0x9d7018, 0, 64 * 1024, 256,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
> + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> + SPI_NOR_4B_OPCODES)
> + },
>
> /* Macronix */
> { "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1, SECT_4K) },
> @@ -3650,6 +3654,10 @@ static int spi_nor_init_params(struct spi_nor *nor,
> case SNOR_MFR_MACRONIX:
> params->quad_enable = macronix_quad_enable;
> break;
> + case SNOR_MFR_ISSI:
> + params->quad_enable = macronix_quad_enable;
> + break;
> +
>
> case SNOR_MFR_ST:
> case SNOR_MFR_MICRON:
> @@ -4127,7 +4135,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> if (ret)
> return ret;
>
> - if (nor->addr_width) {
> + if (nor->addr_width && JEDEC_MFR(info) != SNOR_MFR_ISSI) {
> /* already configured from SFDP */
> } else if (info->addr_width) {
> nor->addr_width = info->addr_width;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index b3d360b..ff13297 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -19,6 +19,7 @@
> #define SNOR_MFR_ATMEL CFI_MFR_ATMEL
> #define SNOR_MFR_GIGADEVICE 0xc8
> #define SNOR_MFR_INTEL CFI_MFR_INTEL
> +#define SNOR_MFR_ISSI 0x9d /* ISSI */
> #define SNOR_MFR_ST CFI_MFR_ST /* ST Micro */
> #define SNOR_MFR_MICRON CFI_MFR_MICRON /* Micron */
> #define SNOR_MFR_MACRONIX CFI_MFR_MACRONIX
> --
> 1.9.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 <[email protected]>
> ---
> 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
>
>
On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:
> Nor device (is25wp256 mounted on HiFive unleashed Rev A00 board) from ISSI
> have memory blocks guarded by block protection bits BP[0,1,2,3].
>
> Clearing block protection bits,unlocks the flash memory regions
> The unlock scheme is registered during nor scans.
This also looks like it's partially based on Wes or Palmer's patch from
https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
Please note that in the patch message.
> Signed-off-by: Sagar Shrikant Kadam <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/mtd/spi-nor.h | 1 +
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c5408ed..81c7b3e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1461,6 +1461,46 @@ static int macronix_quad_enable(struct spi_nor *nor)
> }
>
> /**
> + * issi_unlock() - clear BP[0123] write-protection.
> + * @nor: pointer to a 'struct spi_nor'
> + * @ofs: offset from which to unlock memory
> + * @len: number of bytes to unlock
> + * Bits [2345] of the Status Register are BP[0123].
> + * ISSI chips use a different block protection scheme than other chips.
> + * Just disable the write-protect unilaterally.
> + * Return: 0 on success, -errno otherwise.
This is closer to kernel-doc format, but not quite. Please update this to
align to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst#n57
- Paul
> + */
> +static int issi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> + int ret, val;
> + u8 mask = SR_BP0 | SR_BP1 | SR_BP2 | SR_BP3;
> +
> + val = read_sr(nor);
> + if (val < 0)
> + return val;
> + if (!(val & mask))
> + return 0;
> +
> + write_enable(nor);
> +
> + write_sr(nor, val & ~mask);
> +
> + ret = spi_nor_wait_till_ready(nor);
> + if (ret)
> + return ret;
> +
> + ret = read_sr(nor);
> + if (ret > 0 && !(ret & mask)) {
> + dev_info(nor->dev,
> + "ISSI Block Protection Bits cleared SR=0x%x", ret);
> + return 0;
> + } else {
> + dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
> + return -EINVAL;
> + }
> +}
> +
> +/**
> * spansion_quad_enable() - set QE bit in Configuraiton Register.
> * @nor: pointer to a 'struct spi_nor'
> *
> @@ -1836,7 +1876,7 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> - SPI_NOR_4B_OPCODES)
> + SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK)
> },
>
> /* Macronix */
> @@ -4078,6 +4118,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> nor->flash_is_locked = stm_is_locked;
> }
>
> + /* NOR protection support for ISSI chips */
> + if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
> + info->flags & SPI_NOR_HAS_LOCK) {
> + nor->flash_unlock = issi_unlock;
> +
> + }
> if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> mtd->_lock = spi_nor_lock;
> mtd->_unlock = spi_nor_unlock;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index ff13297..9a7d719 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -127,6 +127,7 @@
> #define SR_BP0 BIT(2) /* Block protect 0 */
> #define SR_BP1 BIT(3) /* Block protect 1 */
> #define SR_BP2 BIT(4) /* Block protect 2 */
> +#define SR_BP3 BIT(5) /* Block protect 3 for ISSI device*/
> #define SR_TB BIT(5) /* Top/Bottom protect */
> #define SR_SRWD BIT(7) /* SR write protect */
> /* Spansion/Cypress specific status bits */
> --
> 1.9.1
>
>
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
Thank you Paul, for your review comments.
On Tue, Apr 30, 2019 at 10:33 PM Paul Walmsley <[email protected]> wrote:
>
> On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:
>
> > Update spi_nor_id tablet for is25wp256 (32MB)device from ISSI,
> > present on HiFive Unleashed dev board (Rev: A00).
> >
> > Set method to enable quad mode for ISSI device in flash parameters
> > table.
>
> This patch was based on one originally written by Wes and/or Palmer:
> https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
>
> The right thing to do is to note this in the commit message.
>
Yes true, this is important as well.
I had mentioned this in the covering letter, but I missed to add the
details into the commit message,
I will submit V3 for this.
> > Signed-off-by: Sagar Shrikant Kadam <[email protected]>
> > ---
> > drivers/mtd/spi-nor/spi-nor.c | 10 +++++++++-
> > include/linux/mtd/spi-nor.h | 1 +
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index fae1474..c5408ed 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1834,6 +1834,10 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
> > SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > { "is25wp128", INFO(0x9d7018, 0, 64 * 1024, 256,
> > SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
> > + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> > + SPI_NOR_4B_OPCODES)
> > + },
> >
> > /* Macronix */
> > { "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1, SECT_4K) },
> > @@ -3650,6 +3654,10 @@ static int spi_nor_init_params(struct spi_nor *nor,
> > case SNOR_MFR_MACRONIX:
> > params->quad_enable = macronix_quad_enable;
> > break;
> > + case SNOR_MFR_ISSI:
> > + params->quad_enable = macronix_quad_enable;
> > + break;
> > +
> >
> > case SNOR_MFR_ST:
> > case SNOR_MFR_MICRON:
> > @@ -4127,7 +4135,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > if (ret)
> > return ret;
> >
> > - if (nor->addr_width) {
> > + if (nor->addr_width && JEDEC_MFR(info) != SNOR_MFR_ISSI) {
> > /* already configured from SFDP */
> > } else if (info->addr_width) {
> > nor->addr_width = info->addr_width;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index b3d360b..ff13297 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -19,6 +19,7 @@
> > #define SNOR_MFR_ATMEL CFI_MFR_ATMEL
> > #define SNOR_MFR_GIGADEVICE 0xc8
> > #define SNOR_MFR_INTEL CFI_MFR_INTEL
> > +#define SNOR_MFR_ISSI 0x9d /* ISSI */
> > #define SNOR_MFR_ST CFI_MFR_ST /* ST Micro */
> > #define SNOR_MFR_MICRON CFI_MFR_MICRON /* Micron */
> > #define SNOR_MFR_MACRONIX CFI_MFR_MACRONIX
> > --
> > 1.9.1
> >
> >
On Tue, Apr 30, 2019 at 10:49 PM Paul Walmsley <[email protected]> wrote:
>
> On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:
>
> > Nor device (is25wp256 mounted on HiFive unleashed Rev A00 board) from ISSI
> > have memory blocks guarded by block protection bits BP[0,1,2,3].
> >
> > Clearing block protection bits,unlocks the flash memory regions
> > The unlock scheme is registered during nor scans.
>
> This also looks like it's partially based on Wes or Palmer's patch from
>
> https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
>
> Please note that in the patch message.
Thank you Paul for pointing this out.
Yes, I missed to add it to commit message and will submit a V3
version of the patch.
> > Signed-off-by: Sagar Shrikant Kadam <[email protected]>
> > ---
> > drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
> > include/linux/mtd/spi-nor.h | 1 +
> > 2 files changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index c5408ed..81c7b3e 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1461,6 +1461,46 @@ static int macronix_quad_enable(struct spi_nor *nor)
> > }
> >
> > /**
> > + * issi_unlock() - clear BP[0123] write-protection.
> > + * @nor: pointer to a 'struct spi_nor'
> > + * @ofs: offset from which to unlock memory
> > + * @len: number of bytes to unlock
> > + * Bits [2345] of the Status Register are BP[0123].
> > + * ISSI chips use a different block protection scheme than other chips.
> > + * Just disable the write-protect unilaterally.
> > + * Return: 0 on success, -errno otherwise.
>
> This is closer to kernel-doc format, but not quite. Please update this to
> align to
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst#n57
>
This is a good pointer. I will align the function description
according to kernel-doc format.
>
> - Paul
>
> > + */
> > +static int issi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > + int ret, val;
> > + u8 mask = SR_BP0 | SR_BP1 | SR_BP2 | SR_BP3;
> > +
> > + val = read_sr(nor);
> > + if (val < 0)
> > + return val;
> > + if (!(val & mask))
> > + return 0;
> > +
> > + write_enable(nor);
> > +
> > + write_sr(nor, val & ~mask);
> > +
> > + ret = spi_nor_wait_till_ready(nor);
> > + if (ret)
> > + return ret;
> > +
> > + ret = read_sr(nor);
> > + if (ret > 0 && !(ret & mask)) {
> > + dev_info(nor->dev,
> > + "ISSI Block Protection Bits cleared SR=0x%x", ret);
> > + return 0;
> > + } else {
> > + dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +/**
> > * spansion_quad_enable() - set QE bit in Configuraiton Register.
> > * @nor: pointer to a 'struct spi_nor'
> > *
> > @@ -1836,7 +1876,7 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
> > SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
> > SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> > - SPI_NOR_4B_OPCODES)
> > + SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK)
> > },
> >
> > /* Macronix */
> > @@ -4078,6 +4118,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > nor->flash_is_locked = stm_is_locked;
> > }
> >
> > + /* NOR protection support for ISSI chips */
> > + if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
> > + info->flags & SPI_NOR_HAS_LOCK) {
> > + nor->flash_unlock = issi_unlock;
> > +
> > + }
> > if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> > mtd->_lock = spi_nor_lock;
> > mtd->_unlock = spi_nor_unlock;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index ff13297..9a7d719 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -127,6 +127,7 @@
> > #define SR_BP0 BIT(2) /* Block protect 0 */
> > #define SR_BP1 BIT(3) /* Block protect 1 */
> > #define SR_BP2 BIT(4) /* Block protect 2 */
> > +#define SR_BP3 BIT(5) /* Block protect 3 for ISSI device*/
> > #define SR_TB BIT(5) /* Top/Bottom protect */
> > #define SR_SRWD BIT(7) /* SR write protect */
> > /* Spansion/Cypress specific status bits */
> > --
> > 1.9.1
> >
> >