[PATCH v5 0/3] mtd: spi-nor: add support for is25wp256 spi-nor flash

[PATCH v5 0/3] mtd: spi-nor: add support for is25wp256 spi-nor flash

[Sagar Shrikant Kadam]

The patch set is tested on HiFive Unleashed A00 board and is based on mainline
kernel v5.2-rc1. 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.

A unilaterlay block unlocking scheme is added in patch 2.

These patches are based on original work done by Wesley Terpstra and/or Palmer Dabbelt:
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_unlock: Unlock flash memory blocks.Arguments: are offset and number of blocks.
3. flash_lock: 	 Lock flash memory blocks. Arguments: are offset and number of blocks. 

Unlock scheme clears the protection bits of all blocks in the Status register.

Lock scheme:
A basic implementation based on stm_lock scheme and is validated for different number of blocks passed
to flash_lock. ISSI devices have Top/Bottom area selection in "function register" which is OTP memory.
 

Revision history:

V4<->V5:
-Rebased to linux version v5.2-rc1.
-Updated heading of this cover letter with sub-system, instead of just plain "add support for is25wp256..."

V3<->V4:
-Extracted comman code and renamed few stm functions so that it can be reused for issi lock implementation.
-Added function's to read and write FR register, for selecting Top/Bottom area.

V2<->V3:
-Rebased patch to mainline v5.1 from earlier v5.1-rc5.
-Updated commit messages, and cover letter with reference to git URL and author information.
-Deferred flash_lock mechanism and can go as separate patch. 

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 | 348 +++++++++++++++++++++++++++++++++++-------
 include/linux/mtd/spi-nor.h   |   7 +
 2 files changed, 304 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Sagar Shrikant Kadam]

Update spi_nor_id table 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.

Based on code originally written by Wesley Terpstra <wesley@sifive.com>
and/or Palmer Dabbelt <palmer@sifive.com>
https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
---
 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 73172d7..2d5a925 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) },
@@ -3652,6 +3656,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:
@@ -4129,7 +4137,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

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Vignesh Raghavendra]

Hi,

On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
[...]

> @@ -4129,7 +4137,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 */

Hmm, why would you want to ignore addr_width that's read from SFDP table?

Regards
Vignesh


>  	} 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
> 

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Sagar Kadam]

Hello Vignesh,

Thanks for your review comments.

On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi,
>
> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> [...]
>
> > @@ -4129,7 +4137,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 */
>
> Hmm, why would you want to ignore addr_width that's read from SFDP table?

The SFDP table for ISSI device considered here, has addr_width set to
3 byte, and the flash considered
here is 32MB. With 3 byte address width we won't be able to access
flash memories higher address range.
Hence I have ignored the addr width from SFDP.  I have verified that
with 3 byte address width, the
flascp util fails while verifying the written data.  Please let me
know your views on this?

BR,
Sagar Kadam

> Regards
> Vignesh
>
>
> >       } 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
> >

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Vignesh Raghavendra]

Hi,

On 17/06/19 8:48 PM, Sagar Kadam wrote:
> Hello Vignesh,
> 
> Thanks for your review comments.
> 
> On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi,
>>
>> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
>> [...]
>>
>>> @@ -4129,7 +4137,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 */
>>
>> Hmm, why would you want to ignore addr_width that's read from SFDP table?
> 
> The SFDP table for ISSI device considered here, has addr_width set to
> 3 byte, and the flash considered
> here is 32MB. With 3 byte address width we won't be able to access
> flash memories higher address range.

Is it specific to a particular ISSI part as indicated here[1]? If so,
please submit solution agreed there i.e. use spi_nor_fixups callback

[1]https://patchwork.ozlabs.org/patch/1056049/

> Hence I have ignored the addr width from SFDP.  I have verified that
> with 3 byte address width, the
> flascp util fails while verifying the written data.  Please let me
> know your views on this?
> 
If this affects multiple ISSI parts then:
Instead of checking for mfr code, look for SNOR_F_4B_OPCODES flag in
flash_info struct of the device and let it take precedence over SFDP in
case size is over 16MB

Regards
Vignesh

> BR,
> Sagar Kadam
> 
>> Regards
>> Vignesh
>>
>>
>>>       } 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
>>>

-- 
Regards
Vignesh

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Sagar Kadam]

Hello Vignesh,

On Fri, Jun 21, 2019 at 11:33 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi,
>
> On 17/06/19 8:48 PM, Sagar Kadam wrote:
> > Hello Vignesh,
> >
> > Thanks for your review comments.
> >
> > On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> >> [...]
> >>
> >>> @@ -4129,7 +4137,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 */
> >>
> >> Hmm, why would you want to ignore addr_width that's read from SFDP table?
> >
> > The SFDP table for ISSI device considered here, has addr_width set to
> > 3 byte, and the flash considered
> > here is 32MB. With 3 byte address width we won't be able to access
> > flash memories higher address range.
>
> Is it specific to a particular ISSI part as indicated here[1]? If so,
> please submit solution agreed there i.e. use spi_nor_fixups callback
>
> [1]https://patchwork.ozlabs.org/patch/1056049/
>

Thanks for sharing the link.
From what I understand here, it seems that "Address Bytes" of SFDP
table for the device under
consideration (is25lp256) supports 3 byte only Addressing mode
(DWORD1[18:17] = 0b00.
where as that of ISSI device (is25LP/WP 256Mb/512/Mb/1Gb) support 3 or
4 byte Addressing mode DWORD1[18:17] = 0b01.

> > Hence I have ignored the addr width from SFDP.  I have verified that
> > with 3 byte address width, the
> > flascp util fails while verifying the written data.  Please let me
> > know your views on this?
> >
> If this affects multiple ISSI parts then:
> Instead of checking for mfr code, look for SNOR_F_4B_OPCODES flag in
> flash_info struct of the device and let it take precedence over SFDP in
> case size is over 16MB
>

So as per your suggestion I think second approach is a better one.
I will send this in V6.

Thanks & Regards,
Sagar

> Regards
> Vignesh
>
> > BR,
> > Sagar Kadam
> >
> >> Regards
> >> Vignesh
> >>
> >>
> >>>       } 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
> >>>
>
> --
> Regards
> Vignesh

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Vignesh Raghavendra]

Hi,

On 21/06/19 3:58 PM, Sagar Kadam wrote:
> Hello Vignesh,
> 
> On Fri, Jun 21, 2019 at 11:33 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi,
>>
>> On 17/06/19 8:48 PM, Sagar Kadam wrote:
>>> Hello Vignesh,
>>>
>>> Thanks for your review comments.
>>>
>>> On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
>>>> [...]
>>>>
>>>>> @@ -4129,7 +4137,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 */
>>>>
>>>> Hmm, why would you want to ignore addr_width that's read from SFDP table?
>>>
>>> The SFDP table for ISSI device considered here, has addr_width set to
>>> 3 byte, and the flash considered
>>> here is 32MB. With 3 byte address width we won't be able to access
>>> flash memories higher address range.
>>
>> Is it specific to a particular ISSI part as indicated here[1]? If so,
>> please submit solution agreed there i.e. use spi_nor_fixups callback
>>
>> [1]https://patchwork.ozlabs.org/patch/1056049/
>>
> 
> Thanks for sharing the link.
> From what I understand here, it seems that "Address Bytes" of SFDP
> table for the device under
> consideration (is25lp256) supports 3 byte only Addressing mode
> (DWORD1[18:17] = 0b00.
> where as that of ISSI device (is25LP/WP 256Mb/512/Mb/1Gb) support 3 or
> 4 byte Addressing mode DWORD1[18:17] = 0b01.
> 

Okay, so that SFDP table entry is correct. SPI NOR framework should
using 4 byte addressing if WORD1[18:17] = 0b01. Could you see if below 
diff helps:

--->8---
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c0a8837c0575..ebf32aebe5e9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2808,6 +2808,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
                break;
 
        case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+       case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
                nor->addr_width = 4;
                break;
 




-- 
Regards
Vignesh

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Sagar Kadam]

Hello Vignesh,

On Mon, Jun 24, 2019 at 3:04 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi,
>
> On 21/06/19 3:58 PM, Sagar Kadam wrote:
> > Hello Vignesh,
> >
> > On Fri, Jun 21, 2019 at 11:33 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 17/06/19 8:48 PM, Sagar Kadam wrote:
> >>> Hello Vignesh,
> >>>
> >>> Thanks for your review comments.
> >>>
> >>> On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> >>>> [...]
> >>>>
> >>>>> @@ -4129,7 +4137,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 */
> >>>>
> >>>> Hmm, why would you want to ignore addr_width that's read from SFDP table?
> >>>
> >>> The SFDP table for ISSI device considered here, has addr_width set to
> >>> 3 byte, and the flash considered
> >>> here is 32MB. With 3 byte address width we won't be able to access
> >>> flash memories higher address range.
> >>
> >> Is it specific to a particular ISSI part as indicated here[1]? If so,
> >> please submit solution agreed there i.e. use spi_nor_fixups callback
> >>
> >> [1]https://patchwork.ozlabs.org/patch/1056049/
> >>
> >
> > Thanks for sharing the link.
> > From what I understand here, it seems that "Address Bytes" of SFDP
> > table for the device under
> > consideration (is25lp256) supports 3 byte only Addressing mode
> > (DWORD1[18:17] = 0b00.
> > where as that of ISSI device (is25LP/WP 256Mb/512/Mb/1Gb) support 3 or
> > 4 byte Addressing mode DWORD1[18:17] = 0b01.
> >
>
> Okay, so that SFDP table entry is correct. SPI NOR framework should
> using 4 byte addressing if WORD1[18:17] = 0b01. Could you see if below
> diff helps:
>
Thank-you for the suggestion.
I applied it, and observed, that data in SFDP table mentioned in
document received
from ISSI support doesn't match with what is actually present on the
device (I have raised a query with issi support for the same)
The WP device also has the same SFDP entry as the LP device (the one
which you shared).
So, will submit V7 with the solution agreed in the link you shared above.
     https://patchwork.ozlabs.org/patch/1056049/
Apologies for the confusion, so please excuse the v6 which I submitted earlier.

Thanks & BR,
Sagar Kadam

> --->8---
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c0a8837c0575..ebf32aebe5e9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2808,6 +2808,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>                 break;
>
>         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
> +       case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>                 nor->addr_width = 4;
>                 break;


>
>
>
>
> --
> Regards
> Vignesh

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Vignesh Raghavendra]

On 24/06/19 6:10 PM, Sagar Kadam wrote:
> Hello Vignesh,
> 
> On Mon, Jun 24, 2019 at 3:04 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi,
>>
>> On 21/06/19 3:58 PM, Sagar Kadam wrote:
>>> Hello Vignesh,
>>>
>>> On Fri, Jun 21, 2019 at 11:33 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 17/06/19 8:48 PM, Sagar Kadam wrote:
>>>>> Hello Vignesh,
>>>>>
>>>>> Thanks for your review comments.
>>>>>
>>>>> On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
>>>>>> [...]
>>>>>>
>>>>>>> @@ -4129,7 +4137,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 */
>>>>>>
>>>>>> Hmm, why would you want to ignore addr_width that's read from SFDP table?
>>>>>
>>>>> The SFDP table for ISSI device considered here, has addr_width set to
>>>>> 3 byte, and the flash considered
>>>>> here is 32MB. With 3 byte address width we won't be able to access
>>>>> flash memories higher address range.
>>>>
>>>> Is it specific to a particular ISSI part as indicated here[1]? If so,
>>>> please submit solution agreed there i.e. use spi_nor_fixups callback
>>>>
>>>> [1]https://patchwork.ozlabs.org/patch/1056049/
>>>>
>>>
>>> Thanks for sharing the link.
>>> From what I understand here, it seems that "Address Bytes" of SFDP
>>> table for the device under
>>> consideration (is25lp256) supports 3 byte only Addressing mode
>>> (DWORD1[18:17] = 0b00.
>>> where as that of ISSI device (is25LP/WP 256Mb/512/Mb/1Gb) support 3 or
>>> 4 byte Addressing mode DWORD1[18:17] = 0b01.
>>>
>>
>> Okay, so that SFDP table entry is correct. SPI NOR framework should
>> using 4 byte addressing if WORD1[18:17] = 0b01. Could you see if below
>> diff helps:
>>
> Thank-you for the suggestion.
> I applied it, and observed, that data in SFDP table mentioned in
> document received
> from ISSI support doesn't match with what is actually present on the
> device (I have raised a query with issi support for the same)
> The WP device also has the same SFDP entry as the LP device (the one
> which you shared).
> So, will submit V7 with the solution agreed in the link you shared above.
>      https://patchwork.ozlabs.org/patch/1056049/
> Apologies for the confusion, so please excuse the v6 which I submitted earlier.
> 

There is an updated version of the patch:
https://patchwork.ozlabs.org/patch/1071453/

You may want to align with Liu Xiang to avoid duplication of effort

> Thanks & BR,
> Sagar Kadam
> 
>> --->8---
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index c0a8837c0575..ebf32aebe5e9 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -2808,6 +2808,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>                 break;
>>
>>         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
>> +       case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>>                 nor->addr_width = 4;
>>                 break;
> 
> 
>>
>>
>>
>>
>> --
>> Regards
>> Vignesh

-- 
Regards
Vignesh

Re: [PATCH v5 1/3] mtd: spi-nor: add support for is25wp256

[Sagar Kadam]

Hi Vignesh,

On Mon, Jun 24, 2019 at 6:37 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
>
>
> On 24/06/19 6:10 PM, Sagar Kadam wrote:
> > Hello Vignesh,
> >
> > On Mon, Jun 24, 2019 at 3:04 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 21/06/19 3:58 PM, Sagar Kadam wrote:
> >>> Hello Vignesh,
> >>>
> >>> On Fri, Jun 21, 2019 at 11:33 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 17/06/19 8:48 PM, Sagar Kadam wrote:
> >>>>> Hello Vignesh,
> >>>>>
> >>>>> Thanks for your review comments.
> >>>>>
> >>>>> On Sun, Jun 16, 2019 at 6:14 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> >>>>>> [...]
> >>>>>>
> >>>>>>> @@ -4129,7 +4137,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 */
> >>>>>>
> >>>>>> Hmm, why would you want to ignore addr_width that's read from SFDP table?
> >>>>>
> >>>>> The SFDP table for ISSI device considered here, has addr_width set to
> >>>>> 3 byte, and the flash considered
> >>>>> here is 32MB. With 3 byte address width we won't be able to access
> >>>>> flash memories higher address range.
> >>>>
> >>>> Is it specific to a particular ISSI part as indicated here[1]? If so,
> >>>> please submit solution agreed there i.e. use spi_nor_fixups callback
> >>>>
> >>>> [1]https://patchwork.ozlabs.org/patch/1056049/
> >>>>
> >>>
> >>> Thanks for sharing the link.
> >>> From what I understand here, it seems that "Address Bytes" of SFDP
> >>> table for the device under
> >>> consideration (is25lp256) supports 3 byte only Addressing mode
> >>> (DWORD1[18:17] = 0b00.
> >>> where as that of ISSI device (is25LP/WP 256Mb/512/Mb/1Gb) support 3 or
> >>> 4 byte Addressing mode DWORD1[18:17] = 0b01.
> >>>
> >>
> >> Okay, so that SFDP table entry is correct. SPI NOR framework should
> >> using 4 byte addressing if WORD1[18:17] = 0b01. Could you see if below
> >> diff helps:
> >>
> > Thank-you for the suggestion.
> > I applied it, and observed, that data in SFDP table mentioned in
> > document received
> > from ISSI support doesn't match with what is actually present on the
> > device (I have raised a query with issi support for the same)
> > The WP device also has the same SFDP entry as the LP device (the one
> > which you shared).
> > So, will submit V7 with the solution agreed in the link you shared above.
> >      https://patchwork.ozlabs.org/patch/1056049/
> > Apologies for the confusion, so please excuse the v6 which I submitted earlier.
> >
>
> There is an updated version of the patch:
> https://patchwork.ozlabs.org/patch/1071453/
>
> You may want to align with Liu Xiang to avoid duplication of effort
>

Ok. It seems Liu Xiang, is working on supporting is25LP256 device,
while the one considered here is WP
Although both are very similar devices, they differ a bit maybe with
Operating voltage, ID values, die revision etc..
I will sync-up with him about his v4 plan, and update you accordingly.
Thanks for the link.

BR,
Sagar Kadam


> > Thanks & BR,
> > Sagar Kadam
> >
> >> --->8---
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index c0a8837c0575..ebf32aebe5e9 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -2808,6 +2808,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >>                 break;
> >>
> >>         case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
> >> +       case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
> >>                 nor->addr_width = 4;
> >>                 break;
> >
> >
> >>
> >>
> >>
> >>
> >> --
> >> Regards
> >> Vignesh
>
> --
> Regards
> Vignesh

[PATCH v5 2/3] mtd: spi-nor: add support to unlock flash device

[Sagar Shrikant Kadam]

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.

Based on code developed by Wesley Terpstra <wesley@sifive.com>
and/or Palmer Dabbelt <palmer@sifive.com>.
https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |  1 +
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2d5a925..b7c6261 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1461,6 +1461,49 @@ 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);
+		ret = 0;
+	} else {
+		dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+/**
  * spansion_quad_enable() - set QE bit in Configuraiton Register.
  * @nor:	pointer to a 'struct spi_nor'
  *
@@ -1836,7 +1879,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 */
@@ -4080,6 +4123,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

Re: [PATCH v5 2/3] mtd: spi-nor: add support to unlock flash device

[Vignesh Raghavendra]

On 12-Jun-19 4:17 PM, 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.
> 
> Based on code developed by Wesley Terpstra <wesley@sifive.com>
> and/or Palmer Dabbelt <palmer@sifive.com>.
> https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
> 
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/spi-nor.h   |  1 +
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 2d5a925..b7c6261 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1461,6 +1461,49 @@ 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);
> +		ret = 0;
> +	} else {
> +		dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
> +		ret = -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +/**
>   * spansion_quad_enable() - set QE bit in Configuraiton Register.
>   * @nor:	pointer to a 'struct spi_nor'
>   *
> @@ -1836,7 +1879,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 */
> @@ -4080,6 +4123,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) {

This should be:

	if (JEDEC_MFR(info) == SNOR_MFR_ISSI &&
	    info->flags & SPI_NOR_HAS_LOCK) {

Otherwise you would end up overriding nor->flash_unlock function for
other vendors too, right?

> +		nor->flash_unlock = issi_unlock;
> +

No need for blank line here.
Please run ./scripts/checkpatch.pl --strict on all patches and address
all the issues reported by it.



> +	}
>  	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*/

No need to mention ISSI here. I am sure there are devices from other
vendors with BP3

>  #define SR_TB			BIT(5)	/* Top/Bottom protect */
>  #define SR_SRWD			BIT(7)	/* SR write protect */
>  /* Spansion/Cypress specific status bits */
> 

Regards
Vignesh

Re: [PATCH v5 2/3] mtd: spi-nor: add support to unlock flash device

[Sagar Kadam]

Hello Vignesh,

On Sun, Jun 16, 2019 at 6:35 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
>
>
> On 12-Jun-19 4:17 PM, 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.
> >
> > Based on code developed by Wesley Terpstra <wesley@sifive.com>
> > and/or Palmer Dabbelt <palmer@sifive.com>.
> > https://github.com/riscv/riscv-linux/commit/c94e267766d62bc9a669611c3d0c8ed5ea26569b
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/mtd/spi-nor.h   |  1 +
> >  2 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 2d5a925..b7c6261 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1461,6 +1461,49 @@ 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);
> > +             ret = 0;
> > +     } else {
> > +             dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
> > +             ret = -EINVAL;
> > +     }
> > +     return ret;
> > +}
> > +
> > +/**
> >   * spansion_quad_enable() - set QE bit in Configuraiton Register.
> >   * @nor:     pointer to a 'struct spi_nor'
> >   *
> > @@ -1836,7 +1879,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 */
> > @@ -4080,6 +4123,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) {
>
> This should be:
>
>         if (JEDEC_MFR(info) == SNOR_MFR_ISSI &&
>             info->flags & SPI_NOR_HAS_LOCK) {
>
> Otherwise you would end up overriding nor->flash_unlock function for
> other vendors too, right?
>
Yes, right. I will submit a v6 for this.

> > +             nor->flash_unlock = issi_unlock;
> > +
>
> No need for blank line here.
> Please run ./scripts/checkpatch.pl --strict on all patches and address
> all the issues reported by it.
>
>
Ok. The plain checkpatch.pl run didn't catch this.
I will fix this and provide a v6 for this.
>
> > +     }
> >       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*/
>
> No need to mention ISSI here. I am sure there are devices from other
> vendors with BP3
>
Ok, I will drop this in V6 and submit.

> >  #define SR_TB                        BIT(5)  /* Top/Bottom protect */
> >  #define SR_SRWD                      BIT(7)  /* SR write protect */
> >  /* Spansion/Cypress specific status bits */
> >
>
> Regards
> Vignesh

Thanks & BR,
Sagar Kadam

Re: [PATCH v5 2/3] mtd: spi-nor: add support to unlock flash device

[Joe Perches]

On Mon, 2019-06-17 at 21:10 +0530, Sagar Kadam wrote:
> On Sun, Jun 16, 2019 at 6:35 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
[]
> > > +static int issi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > > +{
[]
> > > +     if (ret > 0 && !(ret & mask)) {
> > > +             dev_info(nor->dev,
> > > +                     "ISSI Block Protection Bits cleared SR=0x%x", ret);

Please use '\n' terminations on formats

> > > +             ret = 0;
> > > +     } else {
> > > +             dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");

like this one

> > > +             ret = -EINVAL;
> > > +     }
> > > +     return ret;
> > > +}
> > > +
> > > +/**
> > >   * spansion_quad_enable() - set QE bit in Configuraiton Register.

s/Configuraiton/Configuration/

Re: [PATCH v5 2/3] mtd: spi-nor: add support to unlock flash device

[Sagar Kadam]

Hello Joe,

Thanks for reviewing the patch.

On Tue, Jun 18, 2019 at 5:55 AM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2019-06-17 at 21:10 +0530, Sagar Kadam wrote:
> > On Sun, Jun 16, 2019 at 6:35 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> []
> > > > +static int issi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > > > +{
> []
> > > > +     if (ret > 0 && !(ret & mask)) {
> > > > +             dev_info(nor->dev,
> > > > +                     "ISSI Block Protection Bits cleared SR=0x%x", ret);
>
> Please use '\n' terminations on formats
>
I will include this in v6.

> > > > +             ret = 0;
> > > > +     } else {
> > > > +             dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
>
> like this one
>
> > > > +             ret = -EINVAL;
> > > > +     }
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +/**
> > > >   * spansion_quad_enable() - set QE bit in Configuraiton Register.
>
> s/Configuraiton/Configuration/
>
>
Thanks & BR,
Sagar Kadam

[PATCH v5 3/3] mtd: spi-nor: add locking support for is25xxxxx device

[Sagar Shrikant Kadam]

Implement a locking scheme for ISSI devices based on stm_lock mechanism.
The is25xxxxx  devices have 4 bits for selecting the range of blocks to
be locked/protected from erase/write operations and function register
gives feasibility to select TOP / Bottom area for protection.
Added opcodes to read and write function registers.

The current implementation enables block protection as per the table
defined into datasheet for is25wp256 device having erase size of 0x1000.
ISSI and stm devices differ in terms of TBS (Top/Bottom area protection)
bits. In case of issi this is in Function register and is OTP memory, so
once FR bits are programmed  cannot be modified.

Some common code from stm_lock/unlock implementation is extracted so that
it can be re-used for issi devices. The locking scheme has been tested on
HiFive Unleashed board, having is25wp256 flash memory.

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 291 ++++++++++++++++++++++++++++++++++--------
 include/linux/mtd/spi-nor.h   |   5 +
 2 files changed, 245 insertions(+), 51 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b7c6261..9281ec0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -288,6 +288,45 @@ struct flash_info {
 
 #define JEDEC_MFR(info)	((info)->id[0])
 
+/**
+ * read_fr() -read function register
+ * @nor: pointer to a 'struct spi_nor'.
+ *
+ * ISSI devices have top/bottom area protection bits selection into function
+ * reg.The bits in FR are OTP.So once it's written, it cannot be changed.
+ *
+ * Return: Value in function register or Negative if error.
+ */
+static int read_fr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FR\n", (int) ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/**
+ * write_fr() -Write function register
+ * @nor: pointer to a 'struct spi_nor'.
+ *
+ * ISSI devices have top/bottom area selection protection bits into function
+ * reg whereas other devices have the TBS bit into Status Register.
+ * The bits in FR are OTP.So once it's written, it cannot be changed.
+ *
+ * Return: Negative if error
+ */
+static int write_fr(struct spi_nor *nor, u8 val)
+{
+	nor->cmd_buf[0] = val;
+	return nor->write_reg(nor, SPINOR_OP_WRFR, nor->cmd_buf, 1);
+}
+
 /*
  * Read the status register, returning its value in the location
  * Return the status register value.
@@ -1088,10 +1127,17 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 				 uint64_t *len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	int shift = ffs(mask) - 1;
+	u8 mask = 0;
+	int shift = 0;
 	int pow;
 
+	if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI)
+		mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
+	else
+		mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	shift = ffs(mask) - 1;
+
 	if (!(sr & mask)) {
 		/* No protection */
 		*ofs = 0;
@@ -1099,10 +1145,19 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	} else {
 		pow = ((sr & mask) ^ mask) >> shift;
 		*len = mtd->size >> pow;
-		if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
-			*ofs = 0;
-		else
-			*ofs = mtd->size - *len;
+
+		if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI) {
+			if (nor->flags & SNOR_F_HAS_SR_TB &&
+					(read_fsr(nor) & FR_TB))
+				*ofs = 0;
+			else
+				*ofs = mtd->size - *len;
+		} else {
+			if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
+				*ofs = 0;
+			else
+				*ofs = mtd->size - *len;
+		}
 	}
 }
 
@@ -1129,18 +1184,108 @@ static int stm_check_lock_status_sr(struct spi_nor *nor, loff_t ofs, uint64_t le
 		return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
 }
 
-static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
+/*
+ * check if memory region is locked
+ *
+ * Returns false if region is locked 0 otherwise.
+ */
+static int fl_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 			    u8 sr)
 {
 	return stm_check_lock_status_sr(nor, ofs, len, sr, true);
 }
 
-static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
+/*
+ * check if memory region is unlocked
+ *
+ * Returns false if region is locked 0 otherwise.
+ */
+static int fl_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 			      u8 sr)
 {
 	return stm_check_lock_status_sr(nor, ofs, len, sr, false);
 }
 
+/**
+ * flash_select_zone() - Select TOP area or bottom area to lock/unlock
+ * @nor: pointer to a 'struct spi_nor'.
+ * @ofs: offset from which to lock memory.
+ * @len: number of bytes to unlock.
+ * @sr: status register
+ * @tb: pointer to top/bottom bool used in caller function
+ * @op: zone selection is for lock/unlock operation. 1: lock 0:unlock
+ *
+ * Select the top area / bottom area paattern to protect memory blocks.
+ *
+ * Returns negative on errors, 0 on success.
+ */
+static int fl_select_zone(struct spi_nor *nor, loff_t ofs, uint64_t len,
+				u8 sr, bool *tb, bool op)
+{
+	int retval;
+	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
+
+	if (op) {
+		/* Select for lock zone operation */
+
+		/*
+		 * If nothing in our range is unlocked, we don't need
+		 * to do anything.
+		 */
+		if (fl_is_locked_sr(nor, ofs, len, sr))
+			return 0;
+
+		/*
+		 * If anything below us is unlocked, we can't use 'bottom'
+		 * protection.
+		 */
+		if (!fl_is_locked_sr(nor, 0, ofs, sr))
+			can_be_bottom = false;
+
+		/*
+		 * If anything above us is unlocked, we can't use 'top'
+		 * protection.
+		 */
+		if (!fl_is_locked_sr(nor, ofs + len,
+					nor->mtd.size - (ofs + len), sr))
+			can_be_top = false;
+	} else {
+		/* Select unlock zone */
+
+		/*
+		 * If nothing in our range is locked, we don't need to
+		 * do anything.
+		 */
+		if (fl_is_unlocked_sr(nor, ofs, len, sr))
+			return 0;
+
+		/*
+		 * If anything below us is locked, we can't use 'top'
+		 * protection
+		 */
+		if (!fl_is_unlocked_sr(nor, 0, ofs, sr))
+			can_be_top = false;
+
+		/*
+		 * If anything above us is locked, we can't use 'bottom'
+		 * protection
+		 */
+		if (!fl_is_unlocked_sr(nor, ofs + len,
+					nor->mtd.size - (ofs + len), sr))
+			can_be_bottom = false;
+	}
+
+	if (!can_be_bottom && !can_be_top)
+		retval = -EINVAL;
+	else {
+		/* Prefer top, if both are valid */
+		*tb = can_be_top;
+		retval = 1;
+	}
+
+	return retval;
+}
+
 /*
  * Lock a region of the flash. Compatible with ST Micro and similar flash.
  * Supports the block protection bits BP{0,1,2} in the status register
@@ -1178,33 +1323,19 @@ static int stm_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_BP2 | SR_BP1 | SR_BP0;
-	u8 shift = ffs(mask) - 1, pow, val;
+	u8 shift = ffs(mask) - 1, pow, val, ret;
 	loff_t lock_len;
-	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
 
 	status_old = read_sr(nor);
 	if (status_old < 0)
 		return status_old;
 
-	/* If nothing in our range is unlocked, we don't need to do anything */
-	if (stm_is_locked_sr(nor, ofs, len, status_old))
+	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
+	if (!ret)
 		return 0;
-
-	/* If anything below us is unlocked, we can't use 'bottom' protection */
-	if (!stm_is_locked_sr(nor, 0, ofs, status_old))
-		can_be_bottom = false;
-
-	/* If anything above us is unlocked, we can't use 'top' protection */
-	if (!stm_is_locked_sr(nor, ofs + len, mtd->size - (ofs + len),
-				status_old))
-		can_be_top = false;
-
-	if (!can_be_bottom && !can_be_top)
-		return -EINVAL;
-
-	/* Prefer top, if both are valid */
-	use_top = can_be_top;
+	else if (ret < 0)
+		return ret;
 
 	/* lock_len: length of region that should end up locked */
 	if (use_top)
@@ -1258,35 +1389,21 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	struct mtd_info *mtd = &nor->mtd;
 	int status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 shift = ffs(mask) - 1, pow, val;
+	u8 shift = ffs(mask) - 1, pow, val, ret;
 	loff_t lock_len;
-	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
 
 	status_old = read_sr(nor);
 	if (status_old < 0)
 		return status_old;
 
-	/* If nothing in our range is locked, we don't need to do anything */
-	if (stm_is_unlocked_sr(nor, ofs, len, status_old))
+	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 0);
+	if (!ret)
 		return 0;
+	else if (ret < 0)
+		return ret;
 
-	/* If anything below us is locked, we can't use 'top' protection */
-	if (!stm_is_unlocked_sr(nor, 0, ofs, status_old))
-		can_be_top = false;
-
-	/* If anything above us is locked, we can't use 'bottom' protection */
-	if (!stm_is_unlocked_sr(nor, ofs + len, mtd->size - (ofs + len),
-				status_old))
-		can_be_bottom = false;
-
-	if (!can_be_bottom && !can_be_top)
-		return -EINVAL;
-
-	/* Prefer top, if both are valid */
-	use_top = can_be_top;
-
-	/* lock_len: length of region that should remain locked */
+	/* lock_len: length of region that should end up locked */
 	if (use_top)
 		lock_len = mtd->size - (ofs + len);
 	else
@@ -1338,7 +1455,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
  * Returns 1 if entire region is locked, 0 if any portion is unlocked, and
  * negative on errors.
  */
-static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+static int fl_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	int status;
 
@@ -1346,7 +1463,7 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (status < 0)
 		return status;
 
-	return stm_is_locked_sr(nor, ofs, len, status);
+	return fl_is_locked_sr(nor, ofs, len, status);
 }
 
 static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
@@ -1461,6 +1578,77 @@ static int macronix_quad_enable(struct spi_nor *nor)
 }
 
 /**
+ * issi_lock() - set BP[0123] write-protection.
+ * @nor: pointer to a 'struct spi_nor'.
+ * @ofs: offset from which to lock memory.
+ * @len: number of bytes to unlock.
+ *
+ * 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
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	int status_old, status_new, blk_prot;
+	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
+	u8 shift = ffs(mask) - 1;
+	u8 pow, ret, func_reg;
+	bool use_top;
+	loff_t lock_len;
+
+	status_old = read_sr(nor);
+
+	/* if status reg is Write protected don't update bit protection */
+	if (status_old & SR_SRWD) {
+		dev_err(nor->dev,
+			"SR is Write Protected,can't update BP bits...\n");
+		return -EINVAL;
+	}
+
+	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
+	if (!ret)
+		/* Older protected blocks include the new requested block's */
+		return 0;
+	else if (ret < 0)
+		return ret;
+
+	func_reg = read_fr(nor);
+	/* lock_len: length of region that should end up locked */
+	if (use_top) {
+		/* Update Function register to use TOP area */
+		if ((func_reg >> 1) & 0x1) {
+			/* Currently bootom selected change to top */
+			func_reg ^= FR_TB;
+			write_fr(nor, func_reg);
+		}
+		lock_len = nor->mtd.size - ofs;
+	} else {
+
+		/* Update Function register to use bottom area */
+		if (!((func_reg >> 1) & 0x1)) {
+			/*Currently top is selected, change to bottom */
+			func_reg ^= FR_TB;
+			write_fr(nor, func_reg);
+		}
+		lock_len = ofs + len;
+	}
+
+	pow = order_base_2(lock_len);
+	blk_prot = mask & (((pow+1) & 0xf)<<shift);
+	if (lock_len <= 0) {
+		dev_err(nor->dev, "invalid Length to protect");
+		return -EINVAL;
+	}
+
+	status_new = status_old | blk_prot;
+	if (status_old == status_new)
+		return 0;
+
+	return write_sr_and_check(nor, status_new, mask);
+}
+
+/**
  * issi_unlock() - clear BP[0123] write-protection.
  * @nor: pointer to a 'struct spi_nor'.
  * @ofs: offset from which to unlock memory.
@@ -1879,7 +2067,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_HAS_LOCK)
+			SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
 
 	/* Macronix */
@@ -4120,12 +4308,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	    info->flags & SPI_NOR_HAS_LOCK) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
-		nor->flash_is_locked = stm_is_locked;
+		nor->flash_is_locked = fl_is_locked;
 	}
 
 	/* 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;
 
 	}
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 9a7d719..a15d012 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -40,6 +40,8 @@
 #define SPINOR_OP_RDSR		0x05	/* Read status register */
 #define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
 #define SPINOR_OP_RDSR2		0x3f	/* Read status register 2 */
+#define SPINOR_OP_RDFR		0x48	/* Read Function register */
+#define SPINOR_OP_WRFR		0x42	/* Write Function register 1 byte */
 #define SPINOR_OP_WRSR2		0x3e	/* Write status register 2 */
 #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */
 #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
@@ -139,6 +141,9 @@
 /* Enhanced Volatile Configuration Register bits */
 #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
 
+/*Function register bit */
+#define FR_TB			BIT(1)	/*ISSI: Top/Bottom protect */
+
 /* Flag Status Register bits */
 #define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
 #define FSR_E_ERR		BIT(5)	/* Erase operation status */
-- 
1.9.1

Re: [PATCH v5 3/3] mtd: spi-nor: add locking support for is25xxxxx device

[Vignesh Raghavendra]

+Uwe who had interest in 4bit block protection support

On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> Implement a locking scheme for ISSI devices based on stm_lock mechanism.
> The is25xxxxx  devices have 4 bits for selecting the range of blocks to
> be locked/protected from erase/write operations and function register
> gives feasibility to select TOP / Bottom area for protection.
> Added opcodes to read and write function registers.
> 
> The current implementation enables block protection as per the table
> defined into datasheet for is25wp256 device having erase size of 0x1000.
> ISSI and stm devices differ in terms of TBS (Top/Bottom area protection)
> bits. In case of issi this is in Function register and is OTP memory, so
> once FR bits are programmed  cannot be modified.
> 

I am not a fan of modifying/setting OTP bits are they are irreversible
and change the expectation of other SWs in the system such as
bootloader. See comments further down the patch....

> Some common code from stm_lock/unlock implementation is extracted so that
> it can be re-used for issi devices. The locking scheme has been tested on
> HiFive Unleashed board, having is25wp256 flash memory.
> 

Have you tested lock/unlock on non ISSI device with this series?

> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 291 ++++++++++++++++++++++++++++++++++--------
>  include/linux/mtd/spi-nor.h   |   5 +
>  2 files changed, 245 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index b7c6261..9281ec0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -288,6 +288,45 @@ struct flash_info {
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
>  
> +/**
> + * read_fr() -read function register
> + * @nor: pointer to a 'struct spi_nor'.
> + *
> + * ISSI devices have top/bottom area protection bits selection into function
> + * reg.The bits in FR are OTP.So once it's written, it cannot be changed.
> + *
> + * Return: Value in function register or Negative if error.
> + */
> +static int read_fr(struct spi_nor *nor)

Please prefix spi_nor_ (spi_nor_read_fr()) to all generic functions that
you are adding in this patch

> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = nor->read_reg(nor, SPINOR_OP_RDFR, &val, 1);
> +	if (ret < 0) {
> +		pr_err("error %d reading FR\n", (int) ret);

dev_err() and no need to cast 'ret' to int

> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
> +/**
> + * write_fr() -Write function register
> + * @nor: pointer to a 'struct spi_nor'.
> + *
> + * ISSI devices have top/bottom area selection protection bits into function
> + * reg whereas other devices have the TBS bit into Status Register.
s/into/in

> + * The bits in FR are OTP.So once it's written, it cannot be changed.
> + *
> + * Return: Negative if error
> + */
> +static int write_fr(struct spi_nor *nor, u8 val)
> +{
> +	nor->cmd_buf[0] = val;
> +	return nor->write_reg(nor, SPINOR_OP_WRFR, nor->cmd_buf, 1);
> +}
> +
>  /*
>   * Read the status register, returning its value in the location
>   * Return the status register value.
> @@ -1088,10 +1127,17 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  				 uint64_t *len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	int shift = ffs(mask) - 1;
> +	u8 mask = 0;
> +	int shift = 0;
>  	int pow;
>  
> +	if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI)
> +		mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;

Does all ISSI flashes support SR_BP3?

Irrespective of that this isn't generic enough. There are non ISSI
flashes with BP3. Please add a flag or field to flash_info struct to
identify flashes with BP3 bit and then use combination of the flag and
MFR ID to select suitable lock/unlock mechanism


> +	else
> +		mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	shift = ffs(mask) - 1;
> +
>  	if (!(sr & mask)) {
>  		/* No protection */
>  		*ofs = 0;
> @@ -1099,10 +1145,19 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  	} else {
>  		pow = ((sr & mask) ^ mask) >> shift;
>  		*len = mtd->size >> pow;
> -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> -			*ofs = 0;
> -		else
> -			*ofs = mtd->size - *len;
> +
> +		if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI) {
> +			if (nor->flags & SNOR_F_HAS_SR_TB &&
> +					(read_fsr(nor) & FR_TB))
> +				*ofs = 0;
> +			else
> +				*ofs = mtd->size - *len;
> +		} else {
> +			if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> +				*ofs = 0;
> +			else
> +				*ofs = mtd->size - *len;
> +		}
>  	}
>  }
>  
> @@ -1129,18 +1184,108 @@ static int stm_check_lock_status_sr(struct spi_nor *nor, loff_t ofs, uint64_t le
>  		return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
>  }
>  
> -static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +/*
> + * check if memory region is locked
> + *
> + * Returns false if region is locked 0 otherwise.
> + */
> +static int fl_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  			    u8 sr)
>  {
>  	return stm_check_lock_status_sr(nor, ofs, len, sr, true);
>  }
>  
> -static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +/*
> + * check if memory region is unlocked
> + *
> + * Returns false if region is locked 0 otherwise.
> + */
> +static int fl_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  			      u8 sr)
>  {
>  	return stm_check_lock_status_sr(nor, ofs, len, sr, false);
>  }
>  
> +/**
> + * flash_select_zone() - Select TOP area or bottom area to lock/unlock
> + * @nor: pointer to a 'struct spi_nor'.
> + * @ofs: offset from which to lock memory.
> + * @len: number of bytes to unlock.
> + * @sr: status register
> + * @tb: pointer to top/bottom bool used in caller function
> + * @op: zone selection is for lock/unlock operation. 1: lock 0:unlock
> + *
> + * Select the top area / bottom area paattern to protect memory blocks.

s/paattern/pattern

> + *
> + * Returns negative on errors, 0 on success.
> + */
> +static int fl_select_zone(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +				u8 sr, bool *tb, bool op)
> +{
> +	int retval;
> +	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
> +
> +	if (op) {
> +		/* Select for lock zone operation */
> +
> +		/*
> +		 * If nothing in our range is unlocked, we don't need
> +		 * to do anything.
> +		 */
> +		if (fl_is_locked_sr(nor, ofs, len, sr))
> +			return 0;
> +
> +		/*
> +		 * If anything below us is unlocked, we can't use 'bottom'
> +		 * protection.
> +		 */
> +		if (!fl_is_locked_sr(nor, 0, ofs, sr))
> +			can_be_bottom = false;
> +
> +		/*
> +		 * If anything above us is unlocked, we can't use 'top'
> +		 * protection.
> +		 */
> +		if (!fl_is_locked_sr(nor, ofs + len,
> +					nor->mtd.size - (ofs + len), sr))
> +			can_be_top = false;
> +	} else {
> +		/* Select unlock zone */
> +
> +		/*
> +		 * If nothing in our range is locked, we don't need to
> +		 * do anything.
> +		 */
> +		if (fl_is_unlocked_sr(nor, ofs, len, sr))
> +			return 0;
> +
> +		/*
> +		 * If anything below us is locked, we can't use 'top'
> +		 * protection
> +		 */
> +		if (!fl_is_unlocked_sr(nor, 0, ofs, sr))
> +			can_be_top = false;
> +
> +		/*
> +		 * If anything above us is locked, we can't use 'bottom'
> +		 * protection
> +		 */
> +		if (!fl_is_unlocked_sr(nor, ofs + len,
> +					nor->mtd.size - (ofs + len), sr))
> +			can_be_bottom = false;
> +	}
> +
> +	if (!can_be_bottom && !can_be_top)
> +		retval = -EINVAL;
> +	else {
> +		/* Prefer top, if both are valid */
> +		*tb = can_be_top;
> +		retval = 1;
> +	}
> +
> +	return retval;
> +}
> +
>  /*
>   * Lock a region of the flash. Compatible with ST Micro and similar flash.
>   * Supports the block protection bits BP{0,1,2} in the status register
> @@ -1178,33 +1323,19 @@ static int stm_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_BP2 | SR_BP1 | SR_BP0;
> -	u8 shift = ffs(mask) - 1, pow, val;
> +	u8 shift = ffs(mask) - 1, pow, val, ret;
>  	loff_t lock_len;
> -	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
>  		return status_old;
>  
> -	/* If nothing in our range is unlocked, we don't need to do anything */
> -	if (stm_is_locked_sr(nor, ofs, len, status_old))
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> +	if (!ret)
>  		return 0;
> -
> -	/* If anything below us is unlocked, we can't use 'bottom' protection */
> -	if (!stm_is_locked_sr(nor, 0, ofs, status_old))
> -		can_be_bottom = false;
> -
> -	/* If anything above us is unlocked, we can't use 'top' protection */
> -	if (!stm_is_locked_sr(nor, ofs + len, mtd->size - (ofs + len),
> -				status_old))
> -		can_be_top = false;
> -
> -	if (!can_be_bottom && !can_be_top)
> -		return -EINVAL;
> -
> -	/* Prefer top, if both are valid */
> -	use_top = can_be_top;
> +	else if (ret < 0)
> +		return ret;
>  
>  	/* lock_len: length of region that should end up locked */
>  	if (use_top)
> @@ -1258,35 +1389,21 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	int status_old, status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 shift = ffs(mask) - 1, pow, val;
> +	u8 shift = ffs(mask) - 1, pow, val, ret;
>  	loff_t lock_len;
> -	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
>  		return status_old;
>  
> -	/* If nothing in our range is locked, we don't need to do anything */
> -	if (stm_is_unlocked_sr(nor, ofs, len, status_old))
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 0);
> +	if (!ret)
>  		return 0;
> +	else if (ret < 0)
> +		return ret;
>  
> -	/* If anything below us is locked, we can't use 'top' protection */
> -	if (!stm_is_unlocked_sr(nor, 0, ofs, status_old))
> -		can_be_top = false;
> -
> -	/* If anything above us is locked, we can't use 'bottom' protection */
> -	if (!stm_is_unlocked_sr(nor, ofs + len, mtd->size - (ofs + len),
> -				status_old))
> -		can_be_bottom = false;
> -
> -	if (!can_be_bottom && !can_be_top)
> -		return -EINVAL;
> -
> -	/* Prefer top, if both are valid */
> -	use_top = can_be_top;
> -
> -	/* lock_len: length of region that should remain locked */
> +	/* lock_len: length of region that should end up locked */
>  	if (use_top)
>  		lock_len = mtd->size - (ofs + len);
>  	else
> @@ -1338,7 +1455,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>   * Returns 1 if entire region is locked, 0 if any portion is unlocked, and
>   * negative on errors.
>   */
> -static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +static int fl_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  {
>  	int status;
>  
> @@ -1346,7 +1463,7 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	if (status < 0)
>  		return status;
>  
> -	return stm_is_locked_sr(nor, ofs, len, status);
> +	return fl_is_locked_sr(nor, ofs, len, status);
>  }
>  
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> @@ -1461,6 +1578,77 @@ static int macronix_quad_enable(struct spi_nor *nor)
>  }
>  
>  /**
> + * issi_lock() - set BP[0123] write-protection.
> + * @nor: pointer to a 'struct spi_nor'.
> + * @ofs: offset from which to lock memory.
> + * @len: number of bytes to unlock.
> + *
> + * 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
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	int status_old, status_new, blk_prot;
> +	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 shift = ffs(mask) - 1;
> +	u8 pow, ret, func_reg;
> +	bool use_top;
> +	loff_t lock_len;
> +
> +	status_old = read_sr(nor);
> +
> +	/* if status reg is Write protected don't update bit protection */
> +	if (status_old & SR_SRWD) {
> +		dev_err(nor->dev,
> +			"SR is Write Protected,can't update BP bits...\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> +	if (!ret)
> +		/* Older protected blocks include the new requested block's */
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +
> +	func_reg = read_fr(nor);
> +	/* lock_len: length of region that should end up locked */
> +	if (use_top) {
> +		/* Update Function register to use TOP area */
> +		if ((func_reg >> 1) & 0x1) {
> +			/* Currently bootom selected change to top */
> +			func_reg ^= FR_TB;
> +			write_fr(nor, func_reg);
> +		}

IIUC, since this FR_TB OTP bit is initially 0 and now reads 1, implies
that OTP bit has already been programmed once. So is clearing the bit
possible?

I think this lock/unlock mechanism needs a bit more thought.
One solution would be to not modify OTP bit and return error in all
cases when locking a region requested by user is not possible (for a
default scheme).

Regards
Vignesh

> +		lock_len = nor->mtd.size - ofs;
> +	} else {
> +
> +		/* Update Function register to use bottom area */
> +		if (!((func_reg >> 1) & 0x1)) {
> +			/*Currently top is selected, change to bottom */
> +			func_reg ^= FR_TB;
> +			write_fr(nor, func_reg);
> +		}
> +		lock_len = ofs + len;
> +	}
> +
> +	pow = order_base_2(lock_len);
> +	blk_prot = mask & (((pow+1) & 0xf)<<shift);
> +	if (lock_len <= 0) {
> +		dev_err(nor->dev, "invalid Length to protect");
> +		return -EINVAL;
> +	}
> +
> +	status_new = status_old | blk_prot;
> +	if (status_old == status_new)
> +		return 0;
> +
> +	return write_sr_and_check(nor, status_new, mask);
> +}
> +
> +/**
>   * issi_unlock() - clear BP[0123] write-protection.
>   * @nor: pointer to a 'struct spi_nor'.
>   * @ofs: offset from which to unlock memory.
> @@ -1879,7 +2067,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_HAS_LOCK)
> +			SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>  	},
>  
>  	/* Macronix */
> @@ -4120,12 +4308,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	    info->flags & SPI_NOR_HAS_LOCK) {
>  		nor->flash_lock = stm_lock;
>  		nor->flash_unlock = stm_unlock;
> -		nor->flash_is_locked = stm_is_locked;
> +		nor->flash_is_locked = fl_is_locked;
>  	}
>  
>  	/* 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;
>  
>  	}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 9a7d719..a15d012 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -40,6 +40,8 @@
>  #define SPINOR_OP_RDSR		0x05	/* Read status register */
>  #define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
>  #define SPINOR_OP_RDSR2		0x3f	/* Read status register 2 */
> +#define SPINOR_OP_RDFR		0x48	/* Read Function register */
> +#define SPINOR_OP_WRFR		0x42	/* Write Function register 1 byte */
>  #define SPINOR_OP_WRSR2		0x3e	/* Write status register 2 */
>  #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */
>  #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
> @@ -139,6 +141,9 @@
>  /* Enhanced Volatile Configuration Register bits */
>  #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
>  
> +/*Function register bit */
> +#define FR_TB			BIT(1)	/*ISSI: Top/Bottom protect */
> +
>  /* Flag Status Register bits */
>  #define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
>  #define FSR_E_ERR		BIT(5)	/* Erase operation status */
> 

Re: [PATCH v5 3/3] mtd: spi-nor: add locking support for is25xxxxx device

[Sagar Kadam]

Hello Vignesh,

On Tue, Jun 18, 2019 at 9:55 AM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> +Uwe who had interest in 4bit block protection support
>
> On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> > Implement a locking scheme for ISSI devices based on stm_lock mechanism.
> > The is25xxxxx  devices have 4 bits for selecting the range of blocks to
> > be locked/protected from erase/write operations and function register
> > gives feasibility to select TOP / Bottom area for protection.
> > Added opcodes to read and write function registers.
> >
> > The current implementation enables block protection as per the table
> > defined into datasheet for is25wp256 device having erase size of 0x1000.
> > ISSI and stm devices differ in terms of TBS (Top/Bottom area protection)
> > bits. In case of issi this is in Function register and is OTP memory, so
> > once FR bits are programmed  cannot be modified.
> >
>
> I am not a fan of modifying/setting OTP bits are they are irreversible
> and change the expectation of other SWs in the system such as
> bootloader. See comments further down the patch....
>
> > Some common code from stm_lock/unlock implementation is extracted so that
> > it can be re-used for issi devices. The locking scheme has been tested on
> > HiFive Unleashed board, having is25wp256 flash memory.
> >
>
> Have you tested lock/unlock on non ISSI device with this series?
>
Sorry, I haven't tested this series on non ISSI devices.

> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 291 ++++++++++++++++++++++++++++++++++--------
> >  include/linux/mtd/spi-nor.h   |   5 +
> >  2 files changed, 245 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index b7c6261..9281ec0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -288,6 +288,45 @@ struct flash_info {
> >
> >  #define JEDEC_MFR(info)      ((info)->id[0])
> >
> > +/**
> > + * read_fr() -read function register
> > + * @nor: pointer to a 'struct spi_nor'.
> > + *
> > + * ISSI devices have top/bottom area protection bits selection into function
> > + * reg.The bits in FR are OTP.So once it's written, it cannot be changed.
> > + *
> > + * Return: Value in function register or Negative if error.
> > + */
> > +static int read_fr(struct spi_nor *nor)
>
> Please prefix spi_nor_ (spi_nor_read_fr()) to all generic functions that
> you are adding in this patch
>
Ok. Can do as suggested in v6.

> > +{
> > +     int ret;
> > +     u8 val;
> > +
> > +     ret = nor->read_reg(nor, SPINOR_OP_RDFR, &val, 1);
> > +     if (ret < 0) {
> > +             pr_err("error %d reading FR\n", (int) ret);
>
> dev_err() and no need to cast 'ret' to int
>
> > +             return ret;
> > +     }
> > +
> > +     return val;
> > +}
> > +
> > +/**
> > + * write_fr() -Write function register
> > + * @nor: pointer to a 'struct spi_nor'.
> > + *
> > + * ISSI devices have top/bottom area selection protection bits into function
> > + * reg whereas other devices have the TBS bit into Status Register.
> s/into/in
>
> > + * The bits in FR are OTP.So once it's written, it cannot be changed.
> > + *
> > + * Return: Negative if error
> > + */
> > +static int write_fr(struct spi_nor *nor, u8 val)
> > +{
> > +     nor->cmd_buf[0] = val;
> > +     return nor->write_reg(nor, SPINOR_OP_WRFR, nor->cmd_buf, 1);
> > +}
> > +
> >  /*
> >   * Read the status register, returning its value in the location
> >   * Return the status register value.
> > @@ -1088,10 +1127,17 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
> >                                uint64_t *len)
> >  {
> >       struct mtd_info *mtd = &nor->mtd;
> > -     u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > -     int shift = ffs(mask) - 1;
> > +     u8 mask = 0;
> > +     int shift = 0;
> >       int pow;
> >
> > +     if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI)
> > +             mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
>
> Does all ISSI flashes support SR_BP3?

Yes, I have checked a range of ISSI flash devices
and they do support the fourth block protect bit (BP3).

> Irrespective of that this isn't generic enough. There are non ISSI
> flashes with BP3. Please add a flag or field to flash_info struct to
> identify flashes with BP3 bit and then use combination of the flag and
> MFR ID to select suitable lock/unlock mechanism
>
Ok. To make it more generic I will also introduce a macro into flash_info struct
to indicate that a flash has BP3 bit and then use it accordingly.

>
> > +     else
> > +             mask = SR_BP2 | SR_BP1 | SR_BP0;
> > +
> > +     shift = ffs(mask) - 1;
> > +
> >       if (!(sr & mask)) {
> >               /* No protection */
> >               *ofs = 0;
> > @@ -1099,10 +1145,19 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
> >       } else {
> >               pow = ((sr & mask) ^ mask) >> shift;
> >               *len = mtd->size >> pow;
> > -             if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> > -                     *ofs = 0;
> > -             else
> > -                     *ofs = mtd->size - *len;
> > +
> > +             if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI) {
> > +                     if (nor->flags & SNOR_F_HAS_SR_TB &&
> > +                                     (read_fsr(nor) & FR_TB))
> > +                             *ofs = 0;
> > +                     else
> > +                             *ofs = mtd->size - *len;
> > +             } else {
> > +                     if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> > +                             *ofs = 0;
> > +                     else
> > +                             *ofs = mtd->size - *len;
> > +             }
> >       }
> >  }
> >
> > @@ -1129,18 +1184,108 @@ static int stm_check_lock_status_sr(struct spi_nor *nor, loff_t ofs, uint64_t le
> >               return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
> >  }
> >
> > -static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> > +/*
> > + * check if memory region is locked
> > + *
> > + * Returns false if region is locked 0 otherwise.
> > + */
> > +static int fl_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> >                           u8 sr)
> >  {
> >       return stm_check_lock_status_sr(nor, ofs, len, sr, true);
> >  }
> >
> > -static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> > +/*
> > + * check if memory region is unlocked
> > + *
> > + * Returns false if region is locked 0 otherwise.
> > + */
> > +static int fl_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> >                             u8 sr)
> >  {
> >       return stm_check_lock_status_sr(nor, ofs, len, sr, false);
> >  }
> >
> > +/**
> > + * flash_select_zone() - Select TOP area or bottom area to lock/unlock
> > + * @nor: pointer to a 'struct spi_nor'.
> > + * @ofs: offset from which to lock memory.
> > + * @len: number of bytes to unlock.
> > + * @sr: status register
> > + * @tb: pointer to top/bottom bool used in caller function
> > + * @op: zone selection is for lock/unlock operation. 1: lock 0:unlock
> > + *
> > + * Select the top area / bottom area paattern to protect memory blocks.
>
> s/paattern/pattern
>
Ok

> > + *
> > + * Returns negative on errors, 0 on success.
> > + */
> > +static int fl_select_zone(struct spi_nor *nor, loff_t ofs, uint64_t len,
> > +                             u8 sr, bool *tb, bool op)
> > +{
> > +     int retval;
> > +     bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
> > +
> > +     if (op) {
> > +             /* Select for lock zone operation */
> > +
> > +             /*
> > +              * If nothing in our range is unlocked, we don't need
> > +              * to do anything.
> > +              */
> > +             if (fl_is_locked_sr(nor, ofs, len, sr))
> > +                     return 0;
> > +
> > +             /*
> > +              * If anything below us is unlocked, we can't use 'bottom'
> > +              * protection.
> > +              */
> > +             if (!fl_is_locked_sr(nor, 0, ofs, sr))
> > +                     can_be_bottom = false;
> > +
> > +             /*
> > +              * If anything above us is unlocked, we can't use 'top'
> > +              * protection.
> > +              */
> > +             if (!fl_is_locked_sr(nor, ofs + len,
> > +                                     nor->mtd.size - (ofs + len), sr))
> > +                     can_be_top = false;
> > +     } else {
> > +             /* Select unlock zone */
> > +
> > +             /*
> > +              * If nothing in our range is locked, we don't need to
> > +              * do anything.
> > +              */
> > +             if (fl_is_unlocked_sr(nor, ofs, len, sr))
> > +                     return 0;
> > +
> > +             /*
> > +              * If anything below us is locked, we can't use 'top'
> > +              * protection
> > +              */
> > +             if (!fl_is_unlocked_sr(nor, 0, ofs, sr))
> > +                     can_be_top = false;
> > +
> > +             /*
> > +              * If anything above us is locked, we can't use 'bottom'
> > +              * protection
> > +              */
> > +             if (!fl_is_unlocked_sr(nor, ofs + len,
> > +                                     nor->mtd.size - (ofs + len), sr))
> > +                     can_be_bottom = false;
> > +     }
> > +
> > +     if (!can_be_bottom && !can_be_top)
> > +             retval = -EINVAL;
> > +     else {
> > +             /* Prefer top, if both are valid */
> > +             *tb = can_be_top;
> > +             retval = 1;
> > +     }
> > +
> > +     return retval;
> > +}
> > +
> >  /*
> >   * Lock a region of the flash. Compatible with ST Micro and similar flash.
> >   * Supports the block protection bits BP{0,1,2} in the status register
> > @@ -1178,33 +1323,19 @@ static int stm_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_BP2 | SR_BP1 | SR_BP0;
> > -     u8 shift = ffs(mask) - 1, pow, val;
> > +     u8 shift = ffs(mask) - 1, pow, val, ret;
> >       loff_t lock_len;
> > -     bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
> >       bool use_top;
> >
> >       status_old = read_sr(nor);
> >       if (status_old < 0)
> >               return status_old;
> >
> > -     /* If nothing in our range is unlocked, we don't need to do anything */
> > -     if (stm_is_locked_sr(nor, ofs, len, status_old))
> > +     ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> > +     if (!ret)
> >               return 0;
> > -
> > -     /* If anything below us is unlocked, we can't use 'bottom' protection */
> > -     if (!stm_is_locked_sr(nor, 0, ofs, status_old))
> > -             can_be_bottom = false;
> > -
> > -     /* If anything above us is unlocked, we can't use 'top' protection */
> > -     if (!stm_is_locked_sr(nor, ofs + len, mtd->size - (ofs + len),
> > -                             status_old))
> > -             can_be_top = false;
> > -
> > -     if (!can_be_bottom && !can_be_top)
> > -             return -EINVAL;
> > -
> > -     /* Prefer top, if both are valid */
> > -     use_top = can_be_top;
> > +     else if (ret < 0)
> > +             return ret;
> >
> >       /* lock_len: length of region that should end up locked */
> >       if (use_top)
> > @@ -1258,35 +1389,21 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> >       struct mtd_info *mtd = &nor->mtd;
> >       int status_old, status_new;
> >       u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > -     u8 shift = ffs(mask) - 1, pow, val;
> > +     u8 shift = ffs(mask) - 1, pow, val, ret;
> >       loff_t lock_len;
> > -     bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
> >       bool use_top;
> >
> >       status_old = read_sr(nor);
> >       if (status_old < 0)
> >               return status_old;
> >
> > -     /* If nothing in our range is locked, we don't need to do anything */
> > -     if (stm_is_unlocked_sr(nor, ofs, len, status_old))
> > +     ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 0);
> > +     if (!ret)
> >               return 0;
> > +     else if (ret < 0)
> > +             return ret;
> >
> > -     /* If anything below us is locked, we can't use 'top' protection */
> > -     if (!stm_is_unlocked_sr(nor, 0, ofs, status_old))
> > -             can_be_top = false;
> > -
> > -     /* If anything above us is locked, we can't use 'bottom' protection */
> > -     if (!stm_is_unlocked_sr(nor, ofs + len, mtd->size - (ofs + len),
> > -                             status_old))
> > -             can_be_bottom = false;
> > -
> > -     if (!can_be_bottom && !can_be_top)
> > -             return -EINVAL;
> > -
> > -     /* Prefer top, if both are valid */
> > -     use_top = can_be_top;
> > -
> > -     /* lock_len: length of region that should remain locked */
> > +     /* lock_len: length of region that should end up locked */
> >       if (use_top)
> >               lock_len = mtd->size - (ofs + len);
> >       else
> > @@ -1338,7 +1455,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> >   * Returns 1 if entire region is locked, 0 if any portion is unlocked, and
> >   * negative on errors.
> >   */
> > -static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +static int fl_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> >  {
> >       int status;
> >
> > @@ -1346,7 +1463,7 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> >       if (status < 0)
> >               return status;
> >
> > -     return stm_is_locked_sr(nor, ofs, len, status);
> > +     return fl_is_locked_sr(nor, ofs, len, status);
> >  }
> >
> >  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > @@ -1461,6 +1578,77 @@ static int macronix_quad_enable(struct spi_nor *nor)
> >  }
> >
> >  /**
> > + * issi_lock() - set BP[0123] write-protection.
> > + * @nor: pointer to a 'struct spi_nor'.
> > + * @ofs: offset from which to lock memory.
> > + * @len: number of bytes to unlock.
> > + *
> > + * 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
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > +     int status_old, status_new, blk_prot;
> > +     u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> > +     u8 shift = ffs(mask) - 1;
> > +     u8 pow, ret, func_reg;
> > +     bool use_top;
> > +     loff_t lock_len;
> > +
> > +     status_old = read_sr(nor);
> > +
> > +     /* if status reg is Write protected don't update bit protection */
> > +     if (status_old & SR_SRWD) {
> > +             dev_err(nor->dev,
> > +                     "SR is Write Protected,can't update BP bits...\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> > +     if (!ret)
> > +             /* Older protected blocks include the new requested block's */
> > +             return 0;
> > +     else if (ret < 0)
> > +             return ret;
> > +
> > +     func_reg = read_fr(nor);
> > +     /* lock_len: length of region that should end up locked */
> > +     if (use_top) {
> > +             /* Update Function register to use TOP area */
> > +             if ((func_reg >> 1) & 0x1) {
> > +                     /* Currently bootom selected change to top */
> > +                     func_reg ^= FR_TB;
> > +                     write_fr(nor, func_reg);
> > +             }
>
> IIUC, since this FR_TB OTP bit is initially 0 and now reads 1, implies
> that OTP bit has already been programmed once. So is clearing the bit
> possible?
>
> I think this lock/unlock mechanism needs a bit more thought.
> One solution would be to not modify OTP bit and return error in all
> cases when locking a region requested by user is not possible (for a
> default scheme).

I do agree here, writing the OTP will refrain the user from changing
the lock direction
once it's set. So better not update the OTP bits.
I will redo the approach and submit a v6 for this.
Thanks for your valuable feedback.

> Regards
> Vignesh
>

Regards,
Sagar Kadam

> > +             lock_len = nor->mtd.size - ofs;
> > +     } else {
> > +
> > +             /* Update Function register to use bottom area */
> > +             if (!((func_reg >> 1) & 0x1)) {
> > +                     /*Currently top is selected, change to bottom */
> > +                     func_reg ^= FR_TB;
> > +                     write_fr(nor, func_reg);
> > +             }
> > +             lock_len = ofs + len;
> > +     }
> > +
> > +     pow = order_base_2(lock_len);
> > +     blk_prot = mask & (((pow+1) & 0xf)<<shift);
> > +     if (lock_len <= 0) {
> > +             dev_err(nor->dev, "invalid Length to protect");
> > +             return -EINVAL;
> > +     }
> > +
> > +     status_new = status_old | blk_prot;
> > +     if (status_old == status_new)
> > +             return 0;
> > +
> > +     return write_sr_and_check(nor, status_new, mask);
> > +}
> > +
> > +/**
> >   * issi_unlock() - clear BP[0123] write-protection.
> >   * @nor: pointer to a 'struct spi_nor'.
> >   * @ofs: offset from which to unlock memory.
> > @@ -1879,7 +2067,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_HAS_LOCK)
> > +                     SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> >       },
> >
> >       /* Macronix */
> > @@ -4120,12 +4308,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >           info->flags & SPI_NOR_HAS_LOCK) {
> >               nor->flash_lock = stm_lock;
> >               nor->flash_unlock = stm_unlock;
> > -             nor->flash_is_locked = stm_is_locked;
> > +             nor->flash_is_locked = fl_is_locked;
> >       }
> >
> >       /* 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;
> >
> >       }
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index 9a7d719..a15d012 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -40,6 +40,8 @@
> >  #define SPINOR_OP_RDSR               0x05    /* Read status register */
> >  #define SPINOR_OP_WRSR               0x01    /* Write status register 1 byte */
> >  #define SPINOR_OP_RDSR2              0x3f    /* Read status register 2 */
> > +#define SPINOR_OP_RDFR               0x48    /* Read Function register */
> > +#define SPINOR_OP_WRFR               0x42    /* Write Function register 1 byte */
> >  #define SPINOR_OP_WRSR2              0x3e    /* Write status register 2 */
> >  #define SPINOR_OP_READ               0x03    /* Read data bytes (low frequency) */
> >  #define SPINOR_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
> > @@ -139,6 +141,9 @@
> >  /* Enhanced Volatile Configuration Register bits */
> >  #define EVCR_QUAD_EN_MICRON  BIT(7)  /* Micron Quad I/O */
> >
> > +/*Function register bit */
> > +#define FR_TB                        BIT(1)  /*ISSI: Top/Bottom protect */
> > +
> >  /* Flag Status Register bits */
> >  #define FSR_READY            BIT(7)  /* Device status, 0 = Busy, 1 = Ready */
> >  #define FSR_E_ERR            BIT(5)  /* Erase operation status */
> >