[PATCH v2 0/2] spi-nor: add support for is25wp256

[PATCH v2 0/2] spi-nor: add support for is25wp256

[Palmer Dabbelt]

This adds support for the is25wp256 flash chip, which is on our HiFive
Unleashed board.  Additionally it adds support for ISSI's special
unlocking scheme, which we need to unlock block protection on the whole
chip.

Changes since v1 [<20180804014947.24601-1-palmer@sifive.com>]:
* There are now two patches, as it's logically two separate changes.
* The chip is called is25wp256 instead of is25wp256d, with the
  corresponding commit text also renamed.
* The block size has been changed to match the other similar chips that
  are already supported.
* SPI_NOR_4B_OPCODES is no longer set, to match the other similar chips
  that are are already supported.

[PATCH v2 1/2] spi-nor: add support for ISSI's block unlocking scheme
[PATCH v2 2/2] spi-nor: add support for is25wp256

[PATCH v2 1/2] spi-nor: add support for ISSI's block unlocking scheme

[Palmer Dabbelt]

From: "Wesley W. Terpstra" <wesley@sifive.com>

ISSI uses a non-standard scheme to control block protection, with bit 5
of the status registerr controlling an additional block protection bit.
This patch disables all the block protection bits whenever an ISSI chip
is seen.

We might also want to trigger an error when writing SR_TB to these
chips, as it aliases with this extra protection bit in the status
register.  It looks like that's always conditional on SNOR_F_HAS_SR_TB,
so at least what's there is safe.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d9c368c44194..aab93463a5e7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1515,6 +1515,44 @@ static int macronix_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
+/**
+ * issi_unlock() - clear BP[0123] write-protection.
+ * @nor:	pointer to a 'struct spi_nor'
+ *
+ * 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)
+{
+	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)) {
+		return 0;
+	} else {
+		dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
+		return -EINVAL;
+	}
+}
+
 /*
  * Write status Register and configuration register with 2 bytes
  * The first byte will be written to the status register, while the
@@ -2747,6 +2785,9 @@ static int spi_nor_init(struct spi_nor *nor)
 		spi_nor_wait_till_ready(nor);
 	}
 
+	if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI)
+		issi_unlock(nor);
+
 	if (nor->quad_enable) {
 		err = nor->quad_enable(nor);
 		if (err) {
@@ -2926,7 +2967,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 e60da0d34cc1..da422a37d383 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -23,6 +23,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
 #define SNOR_MFR_MICRON		CFI_MFR_ST /* ST Micro <--> Micron */
 #define SNOR_MFR_MACRONIX	CFI_MFR_MACRONIX
 #define SNOR_MFR_SPANSION	CFI_MFR_AMD
@@ -121,6 +122,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 (on ISSI chips) */
 #define SR_TB			BIT(5)	/* Top/Bottom protect */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
-- 
2.16.4

Re: [PATCH v2 1/2] spi-nor: add support for ISSI's block unlocking scheme

[Tudor.Ambarus]

Hi, Wesley, Palmer,

On 08/07/2018 10:40 PM, Palmer Dabbelt wrote:

First of all, thanks for the patience!

> From: "Wesley W. Terpstra" <wesley@sifive.com>
> 
> ISSI uses a non-standard scheme to control block protection, with bit 5
> of the status registerr controlling an additional block protection bit.

Indeed, issi's block protection (BP3, BP2, BP1, BP0) bits are used in a
different way than how are used in stm_lock/unlock, even with the 4bit block
protection on its way (see [1]).

> This patch disables all the block protection bits whenever an ISSI chip
> is seen.

IS25WP256's datasheet says that "The default value of the BP0, BP1, BP2, BP3,
QE, and SRWD bits were set to “0”
 at factory.", which means that all memory blocks come unprotected (unlocked) by
default.

I see your patch as an extra safety measure for people that somehow
unfortunately set these block protection bits, so that they not end up with
locked blocks. Instead of adding this extra safety check/set, I would suggest to
actually add support for the issi's block protection scheme. In a perfect world,
this would fit in a per-manufacturer hook after the per-manufacturer split up
code will be integrated (see [2]).

> 
> We might also want to trigger an error when writing SR_TB to these
> chips, as it aliases with this extra protection bit in the status
> register.  It looks like that's always conditional on SNOR_F_HAS_SR_TB,
> so at least what's there is safe.

This problem will vanish when you'll have the issi's protection scheme implemented.

Cheers,
ta

[1] https://patchwork.ozlabs.org/patch/1011820/
[2] https://patchwork.ozlabs.org/project/linux-mtd/list/?series=80353

[PATCH v2 2/2] spi-nor: add support for is25wp256

[Palmer Dabbelt]

From: "Wesley W. Terpstra" <wesley@sifive.com>

This is used of the HiFive Unleashed development board, and follows the
pattern of similar ISSI devices already listed.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index aab93463a5e7..f10017b4543d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1072,6 +1072,8 @@ static const struct flash_info spi_nor_ids[] = {
 			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, 512,
+	                SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 
 	/* Macronix */
 	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
-- 
2.16.4

Re: [PATCH v2 2/2] spi-nor: add support for is25wp256

[Tudor.Ambarus]

Hi,

This one looks pretty good. You'll have to start the patch's subject with "mtd:
spi-nor:" instead of just "spi-nor".

When in doubt, run git log --oneline on the file that you are modifying and see
how previous patches are prefixing the subject.

On 08/07/2018 10:40 PM, Palmer Dabbelt wrote:
> From: "Wesley W. Terpstra" <wesley@sifive.com>
> 
> This is used of the HiFive Unleashed development board, and follows the
> pattern of similar ISSI devices already listed.
>
Please add a comment and let us know how you tested the flash, it's always
comfortable to see that new support was actually tested.

> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index aab93463a5e7..f10017b4543d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1072,6 +1072,8 @@ static const struct flash_info spi_nor_ids[] = {
>  			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, 512,
> +	                SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },

You missed the SPI_NOR_4B_OPCODES flag. The flash has 4B opcodes, let's use them.

Cheers,
ta

Re: [PATCH v2 2/2] spi-nor: add support for is25wp256

[Tudor.Ambarus]

On 02/13/2019 07:27 PM, Tudor.Ambarus@microchip.com wrote:
> Hi,
> 
> This one looks pretty good. You'll have to start the patch's subject with "mtd:
> spi-nor:" instead of just "spi-nor".
> 
> When in doubt, run git log --oneline on the file that you are modifying and see
> how previous patches are prefixing the subject.
> 
> On 08/07/2018 10:40 PM, Palmer Dabbelt wrote:
>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>
>> This is used of the HiFive Unleashed development board, and follows the
>> pattern of similar ISSI devices already listed.
>>
> Please add a comment and let us know how you tested the flash, it's always
> comfortable to see that new support was actually tested.
> 
>> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index aab93463a5e7..f10017b4543d 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1072,6 +1072,8 @@ static const struct flash_info spi_nor_ids[] = {
>>  			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, 512,
>> +	                SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },

don't forget to fix the checkpatch warning when resubmitting:
$ ./scripts/checkpatch.pl --strict v2-2-2-spi-nor-add-support-for-is25wp256.patch 
ERROR: code indent should use tabs where possible
#40: FILE: drivers/mtd/spi-nor/spi-nor.c:1076:
+^I                SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },$

total: 1 errors, 0 warnings, 0 checks, 8 lines checked
> 
> You missed the SPI_NOR_4B_OPCODES flag. The flash has 4B opcodes, let's use them.
> 
> Cheers,
> ta
>