[PATCH 00/10] mtd: spi-nor: cleanups + block protection support updates

[PATCH 00/10] mtd: spi-nor: cleanups + block protection support updates

[Brian Norris]

[Note: as this is sent out during the merge window, it's based on the
semi-unofficial l2-mtd.git/next branch, which is targeting 4.4, not 4.3]

Hi all,

I've been reviewing various spi-nor drivers as well as working with some
Winbond flash to support new locking features. The former helped point out a
few more things that could use some improvement, and the latter suggested that
we have some glaring oversights in the spi-nor lock/unlock code.

<side note>
Some helpful companion code, for mtd-utils:

 http://lists.infradead.org/pipermail/linux-mtd/2015-August/061526.html

This extends the flash_lock tool so that you can more easily test specific
ranges, using:

  # flash_lock --lock /dev/mtdX <offset> <block-count>
  # flash_lock --unlock /dev/mtdX <offset> <block-count>
  # flash_lock --islocked /dev/mtdX <offset> <block-count>
</side note>

The first half of this series is fairly self-explanatory. The second might take
a bit of thought, as a formulaic approach is a little more subtle than a
table-based approach, so I tried to copy the relevant portions distilled from a
few datasheets and include comments. Please shout if anything deserves more
explanation or looks funny to you.

Highlights:

 * clean up spi-nor.h header
 * spi-nor now supports MEMISLOCKED
 * MEM{LOCK,UNLOCK} support is a little more robust and extendible
 * turn on dual/quad read for Winbond w25q{32,64}dw
 * enable block protection for Winbond flash

Regards,
Brian

Brian Norris (10):
  mtd: spi-nor: make implicit <linux/bitops.h> dependency explicit
  mtd: spi-nor: make bitfield constants more consistent
  mtd: spi-nor: add SPI NOR manufacturer IDs
  mtd: spi-nor: use SNOR_MFR_* instead of CFI_MFR_*
  mtd: spi-nor: fixup kernel-doc for flash lock/unlock function pointers
  mtd: spi-nor: refactor block protection functions
  mtd: spi-nor: add mtd_is_locked() support
  mtd: spi-nor: add DUAL_READ for w25q{32,64}dw
  mtd: spi-nor: support lock/unlock/is_locked for Winbond
  mtd: spi-nor: disable protection for Winbond flash at startup

 drivers/mtd/spi-nor/spi-nor.c | 241 +++++++++++++++++++++++++++++++-----------
 include/linux/mtd/spi-nor.h   |  44 +++++---
 2 files changed, 211 insertions(+), 74 deletions(-)

-- 
2.5.0.457.gab17608

[PATCH 01/10] mtd: spi-nor: make implicit <linux/bitops.h> dependency explicit

[Brian Norris]

We use BIT() in the header. No real problem for now, but it's better to
be accurate.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/spi-nor.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 495433d3f56d..91e165fa3918 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -10,6 +10,8 @@
 #ifndef __LINUX_MTD_SPI_NOR_H
 #define __LINUX_MTD_SPI_NOR_H
 
+#include <linux/bitops.h>
+
 /*
  * Note on opcode nomenclature: some opcodes have a format like
  * SPINOR_OP_FUNCTION{4,}_x_y_z. The numbers x, y, and z stand for the number
-- 
2.5.0.457.gab17608

[PATCH 02/10] mtd: spi-nor: make bitfield constants more consistent

[Brian Norris]

These status bits use different ways of representing similar integer
constants -- some are decimal, some are hex. Make them more consistent.

At the same time, impose my own preference, since IMO it's clearer what
these are when using the BIT() macro.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/spi-nor.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 91e165fa3918..321a055bc266 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -63,24 +63,24 @@
 #define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
 
 /* Status Register bits. */
-#define SR_WIP			1	/* Write in progress */
-#define SR_WEL			2	/* Write enable latch */
+#define SR_WIP			BIT(0)	/* Write in progress */
+#define SR_WEL			BIT(1)	/* Write enable latch */
 /* meaning of other SR_* bits may differ between vendors */
-#define SR_BP0			4	/* Block protect 0 */
-#define SR_BP1			8	/* Block protect 1 */
-#define SR_BP2			0x10	/* Block protect 2 */
-#define SR_SRWD			0x80	/* SR write protect */
+#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_SRWD			BIT(7)	/* SR write protect */
 
-#define SR_QUAD_EN_MX		0x40	/* Macronix Quad I/O */
+#define SR_QUAD_EN_MX		BIT(6)	/* Macronix Quad I/O */
 
 /* Enhanced Volatile Configuration Register bits */
-#define EVCR_QUAD_EN_MICRON    0x80    /* Micron Quad I/O */
+#define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY		0x80
+#define FSR_READY		BIT(7)
 
 /* Configuration Register bits. */
-#define CR_QUAD_EN_SPAN		0x2	/* Spansion Quad I/O */
+#define CR_QUAD_EN_SPAN		BIT(1)	/* Spansion Quad I/O */
 
 enum read_mode {
 	SPI_NOR_NORMAL = 0,
-- 
2.5.0.457.gab17608

[PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Brian Norris]

These are often similar for CFI (parallel NOR) and for SPI NOR, but they
aren't always the same, for various reasons (different namespaces,
company acquisitions and renames, etc.). And some don't have CFI_MFR_*
entries at all.

So let's make a proper place to list the SPI NOR IDs, with all the SPI
NOR specific assumptions and comments.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/spi-nor.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 321a055bc266..8558793cc0f7 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -11,6 +11,21 @@
 #define __LINUX_MTD_SPI_NOR_H
 
 #include <linux/bitops.h>
+#include <linux/mtd/cfi.h>
+
+/*
+ * Manufacturer IDs
+ *
+ * The first byte returned from the flash after sending opcode SPINOR_OP_RDID.
+ * Sometimes these are the same as CFI IDs, but sometimes they aren't.
+ */
+#define SNOR_MFR_ATMEL		CFI_MFR_ATMEL
+#define SNOR_MFR_INTEL		CFI_MFR_INTEL
+#define SNOR_MFR_MICRON		CFI_MFR_ST /* ST Micro <--> Micron */
+#define SNOR_MFR_MACRONIX	CFI_MFR_MACRONIX
+#define SNOR_MFR_SPANSION	CFI_MFR_AMD
+#define SNOR_MFR_SST		CFI_MFR_SST
+#define SNOR_MFR_WINBOND	0xef
 
 /*
  * Note on opcode nomenclature: some opcodes have a format like
-- 
2.5.0.457.gab17608

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Jagan Teki]

On 2 September 2015 at 01:27, Brian Norris <computersforpeace@gmail.com> wrote:
> These are often similar for CFI (parallel NOR) and for SPI NOR, but they
> aren't always the same, for various reasons (different namespaces,
> company acquisitions and renames, etc.). And some don't have CFI_MFR_*
> entries at all.
>
> So let's make a proper place to list the SPI NOR IDs, with all the SPI
> NOR specific assumptions and comments.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  include/linux/mtd/spi-nor.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 321a055bc266..8558793cc0f7 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -11,6 +11,21 @@
>  #define __LINUX_MTD_SPI_NOR_H
>
>  #include <linux/bitops.h>
> +#include <linux/mtd/cfi.h>
> +
> +/*
> + * Manufacturer IDs
> + *
> + * The first byte returned from the flash after sending opcode SPINOR_OP_RDID.
> + * Sometimes these are the same as CFI IDs, but sometimes they aren't.
> + */
> +#define SNOR_MFR_ATMEL         CFI_MFR_ATMEL
> +#define SNOR_MFR_INTEL         CFI_MFR_INTEL
> +#define SNOR_MFR_MICRON                CFI_MFR_ST /* ST Micro <--> Micron */
> +#define SNOR_MFR_MACRONIX      CFI_MFR_MACRONIX
> +#define SNOR_MFR_SPANSION      CFI_MFR_AMD
> +#define SNOR_MFR_SST           CFI_MFR_SST
> +#define SNOR_MFR_WINBOND       0xef

Ideally company names same but looks like winbond is the only diff as
of now, why can't we define WIN and re-use the same from CFI?

>
>  /*
>   * Note on opcode nomenclature: some opcodes have a format like
> --
> 2.5.0.457.gab17608

thanks!
-- 
Jagan | openedev.

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Brian Norris]

On Fri, Sep 25, 2015 at 01:47:18AM +0530, Jagan Teki wrote:
> On 2 September 2015 at 01:27, Brian Norris <computersforpeace@gmail.com> wrote:
> > These are often similar for CFI (parallel NOR) and for SPI NOR, but they
> > aren't always the same, for various reasons (different namespaces,
> > company acquisitions and renames, etc.). And some don't have CFI_MFR_*
> > entries at all.
> >
> > So let's make a proper place to list the SPI NOR IDs, with all the SPI
> > NOR specific assumptions and comments.
> >
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  include/linux/mtd/spi-nor.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index 321a055bc266..8558793cc0f7 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -11,6 +11,21 @@
> >  #define __LINUX_MTD_SPI_NOR_H
> >
> >  #include <linux/bitops.h>
> > +#include <linux/mtd/cfi.h>
> > +
> > +/*
> > + * Manufacturer IDs
> > + *
> > + * The first byte returned from the flash after sending opcode SPINOR_OP_RDID.
> > + * Sometimes these are the same as CFI IDs, but sometimes they aren't.
> > + */
> > +#define SNOR_MFR_ATMEL         CFI_MFR_ATMEL
> > +#define SNOR_MFR_INTEL         CFI_MFR_INTEL
> > +#define SNOR_MFR_MICRON                CFI_MFR_ST /* ST Micro <--> Micron */
> > +#define SNOR_MFR_MACRONIX      CFI_MFR_MACRONIX
> > +#define SNOR_MFR_SPANSION      CFI_MFR_AMD
> > +#define SNOR_MFR_SST           CFI_MFR_SST
> > +#define SNOR_MFR_WINBOND       0xef
> 
> Ideally company names same but looks like winbond is the only diff as
> of now, why can't we define WIN and re-use the same from CFI?

I'm not quite sure what you're suggesting. Are you suggesting we make up
a new Winbond CFI ID? There already is one, and it doesn't match their
SPI NOR, so we can't use it:

#define CFI_MFR_WINBOND         0x00DA

The whole point of this patch is that some mfrs use different IDs for
different classes of flash, so we shouldn't force our programming
patterns into looking like CFI (i.e., parallel NOR [1]) when we're
talking about serial NOR.

If you'd rather, I can just copy the values into this header (e.g.,
0x01, 0x89, etc.) and completely remove all references to CFI.

Brian

[1] Actually, I've seen some serial flash which have CFI parameter
tables. But that's a different subject.

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Jagan Teki]

On 28 September 2015 at 06:16, Brian Norris <computersforpeace@gmail.com> wrote:
> On Fri, Sep 25, 2015 at 01:47:18AM +0530, Jagan Teki wrote:
>> On 2 September 2015 at 01:27, Brian Norris <computersforpeace@gmail.com> wrote:
>> > These are often similar for CFI (parallel NOR) and for SPI NOR, but they
>> > aren't always the same, for various reasons (different namespaces,
>> > company acquisitions and renames, etc.). And some don't have CFI_MFR_*
>> > entries at all.
>> >
>> > So let's make a proper place to list the SPI NOR IDs, with all the SPI
>> > NOR specific assumptions and comments.
>> >
>> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> > ---
>> >  include/linux/mtd/spi-nor.h | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> > index 321a055bc266..8558793cc0f7 100644
>> > --- a/include/linux/mtd/spi-nor.h
>> > +++ b/include/linux/mtd/spi-nor.h
>> > @@ -11,6 +11,21 @@
>> >  #define __LINUX_MTD_SPI_NOR_H
>> >
>> >  #include <linux/bitops.h>
>> > +#include <linux/mtd/cfi.h>
>> > +
>> > +/*
>> > + * Manufacturer IDs
>> > + *
>> > + * The first byte returned from the flash after sending opcode SPINOR_OP_RDID.
>> > + * Sometimes these are the same as CFI IDs, but sometimes they aren't.
>> > + */
>> > +#define SNOR_MFR_ATMEL         CFI_MFR_ATMEL
>> > +#define SNOR_MFR_INTEL         CFI_MFR_INTEL
>> > +#define SNOR_MFR_MICRON                CFI_MFR_ST /* ST Micro <--> Micron */
>> > +#define SNOR_MFR_MACRONIX      CFI_MFR_MACRONIX
>> > +#define SNOR_MFR_SPANSION      CFI_MFR_AMD
>> > +#define SNOR_MFR_SST           CFI_MFR_SST
>> > +#define SNOR_MFR_WINBOND       0xef
>>
>> Ideally company names same but looks like winbond is the only diff as
>> of now, why can't we define WIN and re-use the same from CFI?
>
> I'm not quite sure what you're suggesting. Are you suggesting we make up
> a new Winbond CFI ID? There already is one, and it doesn't match their
> SPI NOR, so we can't use it:
>
> #define CFI_MFR_WINBOND         0x00DA

No - I'm suggesting in a different way, as the only change in winbond
may be you can define this as SPINOR_MFR_WINDOND and rest will reuse.

>
> The whole point of this patch is that some mfrs use different IDs for
> different classes of flash, so we shouldn't force our programming
> patterns into looking like CFI (i.e., parallel NOR [1]) when we're
> talking about serial NOR.
>
> If you'd rather, I can just copy the values into this header (e.g.,
> 0x01, 0x89, etc.) and completely remove all references to CFI.

Understand your intention, but if what are the mfrs id's same then
it's better to use already defined CFI notation because we may get
into impression that the mfrs uses same id for CFI and SPINOR(as cfi
and spinor are NOR complaint flash memories) - IMHO.

>
> Brian
>
> [1] Actually, I've seen some serial flash which have CFI parameter
> tables. But that's a different subject.

--  Jagan.

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Brian Norris]

On Mon, Sep 28, 2015 at 02:42:24PM +0530, Jagan Teki wrote:
> On 28 September 2015 at 06:16, Brian Norris <computersforpeace@gmail.com> wrote:
> > The whole point of this patch is that some mfrs use different IDs for
> > different classes of flash, so we shouldn't force our programming
> > patterns into looking like CFI (i.e., parallel NOR [1]) when we're
> > talking about serial NOR.
> >
> > If you'd rather, I can just copy the values into this header (e.g.,
> > 0x01, 0x89, etc.) and completely remove all references to CFI.
> 
> Understand your intention,

Do you? It really doesn't seem like it.

> but if what are the mfrs id's same then
> it's better to use already defined CFI notation because we may get
> into impression that the mfrs uses same id for CFI and SPINOR

CFI is really unrelated, for the most part. Parallel and serial NOR
evolved quite differently. Why would we want that impression, again?

Really, is it that hard to understand why we'd want two separate MFR ID
lists -- one for CFI and one for SPI NOR -- when it's quite clear that
those lists are NOT the same? Why should you needlessly ask programmers
to jump between using CFI_MFR_* and SNOR_MFR_* in the same framework?
What if someone starts trying to use CFI_MFR_WINBOND (which is NOT
correct for SPI NOR)? I'm trying *clarify* the ID namespace here, not
convolute it...

> (as cfi
> and spinor are NOR complaint flash memories) - IMHO.

That doesn't make any sense. "NOR" is not anything to be "compliant" to;
it's a type of flash technology (i.e., electrical design).

Brian

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Jagan Teki]

On 29 September 2015 at 04:43, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Sep 28, 2015 at 02:42:24PM +0530, Jagan Teki wrote:
>> On 28 September 2015 at 06:16, Brian Norris <computersforpeace@gmail.com> wrote:
>> > The whole point of this patch is that some mfrs use different IDs for
>> > different classes of flash, so we shouldn't force our programming
>> > patterns into looking like CFI (i.e., parallel NOR [1]) when we're
>> > talking about serial NOR.
>> >
>> > If you'd rather, I can just copy the values into this header (e.g.,
>> > 0x01, 0x89, etc.) and completely remove all references to CFI.
>>
>> Understand your intention,
>
> Do you? It really doesn't seem like it.
>
>> but if what are the mfrs id's same then
>> it's better to use already defined CFI notation because we may get
>> into impression that the mfrs uses same id for CFI and SPINOR
>
> CFI is really unrelated, for the most part. Parallel and serial NOR
> evolved quite differently. Why would we want that impression, again?
>
> Really, is it that hard to understand why we'd want two separate MFR ID
> lists -- one for CFI and one for SPI NOR -- when it's quite clear that
> those lists are NOT the same? Why should you needlessly ask programmers
> to jump between using CFI_MFR_* and SNOR_MFR_* in the same framework?
> What if someone starts trying to use CFI_MFR_WINBOND (which is NOT
> correct for SPI NOR)? I'm trying *clarify* the ID namespace here, not
> convolute it...

You're correct if the MFR ID's were different in CFI and SPI-NOR, i'm
referring there are some mfr's have same id's for cfi and spi-nor like
atmel, intel, micron, macronix, sst, spansion, is true right?

For these mfr's I'm suggesting we may reuse the CFI's as it is, do you
see any concerns/issues for this?

>
>> (as cfi
>> and spinor are NOR complaint flash memories) - IMHO.
>
> That doesn't make any sense. "NOR" is not anything to be "compliant" to;
> it's a type of flash technology (i.e., electrical design).

--  Jagan.

Re: [PATCH 03/10] mtd: spi-nor: add SPI NOR manufacturer IDs

[Brian Norris]

On Thu, Oct 01, 2015 at 01:42:05PM +0530, Jagan Teki wrote:
> On 29 September 2015 at 04:43, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Mon, Sep 28, 2015 at 02:42:24PM +0530, Jagan Teki wrote:
> >> On 28 September 2015 at 06:16, Brian Norris <computersforpeace@gmail.com> wrote:
> >> > The whole point of this patch is that some mfrs use different IDs for
> >> > different classes of flash, so we shouldn't force our programming
> >> > patterns into looking like CFI (i.e., parallel NOR [1]) when we're
> >> > talking about serial NOR.
> >> >
> >> > If you'd rather, I can just copy the values into this header (e.g.,
> >> > 0x01, 0x89, etc.) and completely remove all references to CFI.
> >>
> >> Understand your intention,
> >
> > Do you? It really doesn't seem like it.
> >
> >> but if what are the mfrs id's same then
> >> it's better to use already defined CFI notation because we may get
> >> into impression that the mfrs uses same id for CFI and SPINOR
> >
> > CFI is really unrelated, for the most part. Parallel and serial NOR
> > evolved quite differently. Why would we want that impression, again?
> >
> > Really, is it that hard to understand why we'd want two separate MFR ID
> > lists -- one for CFI and one for SPI NOR -- when it's quite clear that
> > those lists are NOT the same? Why should you needlessly ask programmers
> > to jump between using CFI_MFR_* and SNOR_MFR_* in the same framework?
> > What if someone starts trying to use CFI_MFR_WINBOND (which is NOT
> > correct for SPI NOR)? I'm trying *clarify* the ID namespace here, not
> > convolute it...
> 
> You're correct if the MFR ID's were different in CFI and SPI-NOR, i'm
> referring there are some mfr's have same id's for cfi and spi-nor like
> atmel, intel, micron, macronix, sst, spansion, is true right?
> 
> For these mfr's I'm suggesting we may reuse the CFI's as it is, do you
> see any concerns/issues for this?

Yes, I have concerns. I spelled out one in the quoted paragraph. I'll
try to spell it out clearer, but I'm beginning to lose sympathy here...

If there is code that *mostly* uses CFI_MFR_FOO and in only one place
uses SNOR_MFR_WINBOND, then it's very easy for a future programmer to
overlook the SNOR_MFR_WINBOND and start using CFI_MFR_WINBOND for future
code.

A variation of the above: this is a namespacing problem. You can't use
all the same IDs in both parallel and serial NOR contexts (even though
several of them are identical), so it's misleading to pretend that they
are in the same namespace. What's worse, C doesn't provide very strict
type guarantees here (everything's an int, even if we make enums), so
naming and obviousness are the best tools we have to ensure things are
used correctly. So I'm creating two separate namespaces to reflect the
fact at the beginning of this paragraph: that "you can't use all the
same IDs in both parallel and serial NOR contexts." With this patch, it
will be obvious if somebody is doing the wrong thing: we should not see
*any* use of CFI_MFR_ in spi-nor.c. At most, we should see definitions
in spi-nor.h that alias *specific* CFI IDs that are actually carried
over to SPI NOR.

Now, since you've repeatedly ignored (not refuted) my argument, I'll ask
you a question that you still haven't answered sufficiently: why
*shouldn't* we do as I suggest in my patch? I see your answer in two
parts:

  "it's better to use already defined CFI notation because we may get
  into impression that the mfrs uses same id for CFI and SPINOR"

I refuted the "because" part, and you're left only with the highly
subjective (and IMO wrong) statement that "it's better to use ..."

I'm tired of running around in circles on this simple concept. I'm about
ready to stop responding and ignore your thoughts on this matter, so
please take the time to really understand what I'm saying, if you care
to respond further.

Brian

[PATCH 04/10] mtd: spi-nor: use SNOR_MFR_* instead of CFI_MFR_*

[Brian Norris]

No functional change, just cosmetic.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index dd46d5e65846..09183eb51297 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -17,7 +17,6 @@
 #include <linux/mutex.h>
 #include <linux/math64.h>
 
-#include <linux/mtd/cfi.h>
 #include <linux/mtd/mtd.h>
 #include <linux/of_platform.h>
 #include <linux/spi/flash.h>
@@ -179,11 +178,11 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 	u8 cmd;
 
 	switch (JEDEC_MFR(info)) {
-	case CFI_MFR_ST: /* Micron, actually */
+	case SNOR_MFR_MICRON:
 		/* Some Micron need WREN command; all will accept it */
 		need_wren = true;
-	case CFI_MFR_MACRONIX:
-	case 0xEF /* winbond */:
+	case SNOR_MFR_MACRONIX:
+	case SNOR_MFR_WINBOND:
 		if (need_wren)
 			write_enable(nor);
 
@@ -963,14 +962,14 @@ static int set_quad_mode(struct spi_nor *nor, const struct flash_info *info)
 	int status;
 
 	switch (JEDEC_MFR(info)) {
-	case CFI_MFR_MACRONIX:
+	case SNOR_MFR_MACRONIX:
 		status = macronix_quad_enable(nor);
 		if (status) {
 			dev_err(nor->dev, "Macronix quad-read not enabled\n");
 			return -EINVAL;
 		}
 		return status;
-	case CFI_MFR_ST:
+	case SNOR_MFR_MICRON:
 		status = micron_quad_enable(nor);
 		if (status) {
 			dev_err(nor->dev, "Micron quad-read not enabled\n");
@@ -1050,9 +1049,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	 * up with the software protection bits set
 	 */
 
-	if (JEDEC_MFR(info) == CFI_MFR_ATMEL ||
-	    JEDEC_MFR(info) == CFI_MFR_INTEL ||
-	    JEDEC_MFR(info) == CFI_MFR_SST) {
+	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
+	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
+	    JEDEC_MFR(info) == SNOR_MFR_SST) {
 		write_enable(nor);
 		write_sr(nor, 0);
 	}
@@ -1067,8 +1066,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->_erase = spi_nor_erase;
 	mtd->_read = spi_nor_read;
 
-	/* nor protection support for STmicro chips */
-	if (JEDEC_MFR(info) == CFI_MFR_ST) {
+	/* NOR protection support for STmicro/Micron chips */
+	if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
 	}
@@ -1162,7 +1161,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	else if (mtd->size > 0x1000000) {
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info) == CFI_MFR_AMD) {
+		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
 			case SPI_NOR_QUAD:
-- 
2.5.0.457.gab17608

[PATCH 05/10] mtd: spi-nor: fixup kernel-doc for flash lock/unlock function pointers

[Brian Norris]

I got the names of these fields wrong.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 8558793cc0f7..bd8068f6796a 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -174,8 +174,8 @@ struct mtd_info;
  * @write:		[DRIVER-SPECIFIC] write data to the SPI NOR
  * @erase:		[DRIVER-SPECIFIC] erase a sector of the SPI NOR
  *			at the offset @offs
- * @lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
- * @unlock:		[FLASH-SPECIFIC] unlock a region of the SPI NOR
+ * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
+ * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
  * @priv:		the private data
  */
 struct spi_nor {
-- 
2.5.0.457.gab17608

[PATCH 06/10] mtd: spi-nor: refactor block protection functions

[Brian Norris]

This code was a bit sloppy, would produce a lot of copy-and-paste, and
did not always provide a sensible interface:

 * It didn't validate the length for LOCK and the offset for UNLOCK, so
   we were essentially discarding half of the user-supplied data and
   assuming what they wanted to lock/unlock
 * It didn't do very good error checking
 * It didn't make use of the fact that this operation works on
   power-of-two dimensions

So, rewrite this to do proper bit arithmetic rather than a bunch of
hard-coded condition tables. Now we have:

 * More comments on how this was derived
 * Notes on what is (and isn't) supported
 * A more exendible function, so we could add support for other
   protection ranges
 * More accurate locking - e.g., suppose the top quadrant is locked (75%
   to 100%); then in the following cases, case (a) will succeed but (b)
   will not (return -EINVAL):
     (a) user requests lock 3rd quadrant (50% to 75%)
     (b) user requests lock 3rd quadrant, minus a few blocks (e.g., 50%
         to 73%)
   Case (b) *should* fail, since we'd have to lock blocks that weren't
   requested. But the old implementation didn't know the difference and
   would lock the entire second half (50% to 100%)

This refactoring work will also help enable the addition of
mtd_is_locked() support and potentially the support of bottom boot
protection (TB=1).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 171 +++++++++++++++++++++++++++++++-----------
 1 file changed, 126 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 09183eb51297..62fa1b4ff3c0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -370,72 +370,153 @@ erase_err:
 	return ret;
 }
 
+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;
+	int pow;
+
+	if (!(sr & mask)) {
+		/* No protection */
+		*ofs = 0;
+		*len = 0;
+	} else {
+		pow = ((sr & mask) ^ mask) >> shift;
+		*len = mtd->size >> pow;
+		*ofs = mtd->size - *len;
+	}
+}
+
+/*
+ * Return 1 if the entire region is locked, 0 otherwise
+ */
+static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
+			    u8 sr)
+{
+	loff_t lock_offs;
+	uint64_t lock_len;
+
+	stm_get_locked_range(nor, sr, &lock_offs, &lock_len);
+
+	return (ofs + len <= lock_offs + lock_len) && (ofs >= lock_offs);
+}
+
+/*
+ * Lock a region of the flash. Compatible with ST Micro and similar flash.
+ * Supports only the block protection bits BP{0,1,2} in the status register
+ * (SR). Does not support these features found in newer SR bitfields:
+ *   - TB: top/bottom protect - only handle TB=0 (top protect)
+ *   - SEC: sector/block protect - only handle SEC=0 (block protect)
+ *   - CMP: complement protect - only support CMP=0 (range is not complemented)
+ *
+ * Sample table portion for 8MB flash (Winbond w25q64fw):
+ *
+ *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
+ *  --------------------------------------------------------------------------
+ *    X   |   X   |   0   |   0   |   0   |  NONE         | NONE
+ *    0   |   0   |   0   |   0   |   1   |  128 KB       | Upper 1/64
+ *    0   |   0   |   0   |   1   |   0   |  256 KB       | Upper 1/32
+ *    0   |   0   |   0   |   1   |   1   |  512 KB       | Upper 1/16
+ *    0   |   0   |   1   |   0   |   0   |  1 MB         | Upper 1/8
+ *    0   |   0   |   1   |   0   |   1   |  2 MB         | Upper 1/4
+ *    0   |   0   |   1   |   1   |   0   |  4 MB         | Upper 1/2
+ *    X   |   X   |   1   |   1   |   1   |  8 MB         | ALL
+ *
+ * Returns negative on errors, 0 on success.
+ */
 static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	uint32_t offset = ofs;
-	uint8_t status_old, status_new;
-	int ret = 0;
+	u8 status_old, status_new;
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+	u8 shift = ffs(mask) - 1, pow, val;
 
 	status_old = read_sr(nor);
 
-	if (offset < mtd->size - (mtd->size / 2))
-		status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
-	else if (offset < mtd->size - (mtd->size / 4))
-		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
-	else if (offset < mtd->size - (mtd->size / 8))
-		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
-	else if (offset < mtd->size - (mtd->size / 16))
-		status_new = (status_old & ~(SR_BP0 | SR_BP1)) | SR_BP2;
-	else if (offset < mtd->size - (mtd->size / 32))
-		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
-	else if (offset < mtd->size - (mtd->size / 64))
-		status_new = (status_old & ~(SR_BP2 | SR_BP0)) | SR_BP1;
-	else
-		status_new = (status_old & ~(SR_BP2 | SR_BP1)) | SR_BP0;
+	/* SPI NOR always locks to the end */
+	if (ofs + len != mtd->size) {
+		/* Does combined region extend to end? */
+		if (!stm_is_locked_sr(nor, ofs + len, mtd->size - ofs - len,
+				      status_old))
+			return -EINVAL;
+		len = mtd->size - ofs;
+	}
+
+	/*
+	 * Need smallest pow such that:
+	 *
+	 *   1 / (2^pow) <= (len / size)
+	 *
+	 * so (assuming power-of-2 size) we do:
+	 *
+	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
+	 */
+	pow = ilog2(mtd->size) - ilog2(len);
+	val = mask - (pow << shift);
+	if (val & ~mask)
+		return -EINVAL;
+	/* Don't "lock" with no region! */
+	if (!(val & mask))
+		return -EINVAL;
+
+	status_new = (status_old & ~mask) | val;
 
 	/* Only modify protection if it will not unlock other areas */
-	if ((status_new & (SR_BP2 | SR_BP1 | SR_BP0)) >
-				(status_old & (SR_BP2 | SR_BP1 | SR_BP0))) {
-		write_enable(nor);
-		ret = write_sr(nor, status_new);
-	}
+	if ((status_new & mask) <= (status_old & mask))
+		return -EINVAL;
 
-	return ret;
+	write_enable(nor);
+	return write_sr(nor, status_new);
 }
 
+/*
+ * Unlock a region of the flash. See stm_lock() for more info
+ *
+ * Returns negative on errors, 0 on success.
+ */
 static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	uint32_t offset = ofs;
 	uint8_t status_old, status_new;
-	int ret = 0;
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+	u8 shift = ffs(mask) - 1, pow, val;
 
 	status_old = read_sr(nor);
 
-	if (offset+len > mtd->size - (mtd->size / 64))
-		status_new = status_old & ~(SR_BP2 | SR_BP1 | SR_BP0);
-	else if (offset+len > mtd->size - (mtd->size / 32))
-		status_new = (status_old & ~(SR_BP2 | SR_BP1)) | SR_BP0;
-	else if (offset+len > mtd->size - (mtd->size / 16))
-		status_new = (status_old & ~(SR_BP2 | SR_BP0)) | SR_BP1;
-	else if (offset+len > mtd->size - (mtd->size / 8))
-		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
-	else if (offset+len > mtd->size - (mtd->size / 4))
-		status_new = (status_old & ~(SR_BP0 | SR_BP1)) | SR_BP2;
-	else if (offset+len > mtd->size - (mtd->size / 2))
-		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
-	else
-		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
+	/* Cannot unlock; would unlock larger region than requested */
+	if (stm_is_locked_sr(nor, status_old, ofs - mtd->erasesize,
+			     mtd->erasesize))
+		return -EINVAL;
 
-	/* Only modify protection if it will not lock other areas */
-	if ((status_new & (SR_BP2 | SR_BP1 | SR_BP0)) <
-				(status_old & (SR_BP2 | SR_BP1 | SR_BP0))) {
-		write_enable(nor);
-		ret = write_sr(nor, status_new);
+	/*
+	 * Need largest pow such that:
+	 *
+	 *   1 / (2^pow) >= (len / size)
+	 *
+	 * so (assuming power-of-2 size) we do:
+	 *
+	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len))
+	 */
+	pow = ilog2(mtd->size) - order_base_2(mtd->size - (ofs + len));
+	if (ofs + len == mtd->size) {
+		val = 0; /* fully unlocked */
+	} else {
+		val = mask - (pow << shift);
+		/* Some power-of-two sizes are not supported */
+		if (val & ~mask)
+			return -EINVAL;
 	}
 
-	return ret;
+	status_new = (status_old & ~mask) | val;
+
+	/* Only modify protection if it will not lock other areas */
+	if ((status_new & mask) >= (status_old & mask))
+		return -EINVAL;
+
+	write_enable(nor);
+	return write_sr(nor, status_new);
 }
 
 static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
-- 
2.5.0.457.gab17608

[PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Brian Norris]

This enables ioctl(MEMISLOCKED). Status can now be reported in the
mtdinfo or flash_lock utilities found in mtd-utils.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 37 ++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |  3 +++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 62fa1b4ff3c0..c4fb1205f1d3 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -519,6 +519,24 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return write_sr(nor, status_new);
 }
 
+/*
+ * Check if a region of the flash is (completely) locked. See stm_lock() for
+ * more info.
+ *
+ * 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)
+{
+	int status;
+
+	status = read_sr(nor);
+	if (status < 0)
+		return status;
+
+	return stm_is_locked_sr(nor, ofs, len, status);
+}
+
 static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	return ret;
 }
 
+static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	int ret;
+
+	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
+	if (ret)
+		return ret;
+
+	ret = nor->flash_is_locked(nor, ofs, len);
+
+	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
+	return ret;
+}
+
 /* Used when the "_ext_id" is two bytes at most */
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
 		.id = {							\
@@ -1151,11 +1184,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
+		nor->flash_is_locked = stm_is_locked;
 	}
 
-	if (nor->flash_lock && nor->flash_unlock) {
+	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
 		mtd->_lock = spi_nor_lock;
 		mtd->_unlock = spi_nor_unlock;
+		mtd->_is_locked = spi_nor_is_locked;
 	}
 
 	/* sst nor chips use AAI word program */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index bd8068f6796a..8620bfaed261 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -176,6 +176,8 @@ struct mtd_info;
  *			at the offset @offs
  * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
  * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
+ * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
+ *			completely locked
  * @priv:		the private data
  */
 struct spi_nor {
@@ -212,6 +214,7 @@ struct spi_nor {
 
 	int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
+	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 
 	void *priv;
 };
-- 
2.5.0.457.gab17608

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Marek Vasut]

On Tuesday, September 01, 2015 at 09:57:12 PM, Brian Norris wrote:
> This enables ioctl(MEMISLOCKED). Status can now be reported in the
> mtdinfo or flash_lock utilities found in mtd-utils.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 37 ++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/spi-nor.h   |  3 +++
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 62fa1b4ff3c0..c4fb1205f1d3 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -519,6 +519,24 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs,
> uint64_t len) return write_sr(nor, status_new);
>  }
> 
> +/*
> + * Check if a region of the flash is (completely) locked. See stm_lock()
> for + * more info.
> + *
> + * 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)
> +{
> +	int status;
> +
> +	status = read_sr(nor);
> +	if (status < 0)
> +		return status;
> +
> +	return stm_is_locked_sr(nor, ofs, len, status);
> +}
> +
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len) return ret;
>  }
> 
> +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t
> len) +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->flash_is_locked(nor, ofs, len);

Is nor->flash_is_locked () always available or should you check this here ?

> +
> +	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
> +	return ret;
> +}
> +
[...]

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Brian Norris]

On Wed, Sep 02, 2015 at 11:01:49AM +0200, Marek Vasut wrote:
> On Tuesday, September 01, 2015 at 09:57:12 PM, Brian Norris wrote:
> > This enables ioctl(MEMISLOCKED). Status can now be reported in the
> > mtdinfo or flash_lock utilities found in mtd-utils.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 37 ++++++++++++++++++++++++++++++++++++-
> >  include/linux/mtd/spi-nor.h   |  3 +++
> >  2 files changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 62fa1b4ff3c0..c4fb1205f1d3 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
...
> > @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t
> > ofs, uint64_t len) return ret;
> >  }
> > 
> > +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t
> > len) +{
> > +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > +	int ret;
> > +
> > +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nor->flash_is_locked(nor, ofs, len);
> 
> Is nor->flash_is_locked () always available or should you check this here ?

spi_nor_is_locked() is only used when all 3 of
flash_{lock,unlock,is_locked} are available:

	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
		mtd->_lock = spi_nor_lock;
		mtd->_unlock = spi_nor_unlock;
		mtd->_is_locked = spi_nor_is_locked;
	}

I think it's a reasonable condition to enforce, that we only provide
lock/unlock support if your flash also implements is_locked.

> > +
> > +	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
> > +	return ret;
> > +}
> > +
> [...]

Brian

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Marek Vasut]

On Wednesday, September 02, 2015 at 10:30:41 PM, Brian Norris wrote:
> On Wed, Sep 02, 2015 at 11:01:49AM +0200, Marek Vasut wrote:
> > On Tuesday, September 01, 2015 at 09:57:12 PM, Brian Norris wrote:
> > > This enables ioctl(MEMISLOCKED). Status can now be reported in the
> > > mtdinfo or flash_lock utilities found in mtd-utils.
> > > 
> > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > ---
> > > 
> > >  drivers/mtd/spi-nor/spi-nor.c | 37
> > >  ++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h   |
> > >   3 +++
> > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c index 62fa1b4ff3c0..c4fb1205f1d3
> > > 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> 
> ...
> 
> > > @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd,
> > > loff_t ofs, uint64_t len) return ret;
> > > 
> > >  }
> > > 
> > > +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs,
> > > uint64_t len) +{
> > > +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > > +	int ret;
> > > +
> > > +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = nor->flash_is_locked(nor, ofs, len);
> > 
> > Is nor->flash_is_locked () always available or should you check this here
> > ?
> 
> spi_nor_is_locked() is only used when all 3 of
> flash_{lock,unlock,is_locked} are available:
> 
> 	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> 		mtd->_lock = spi_nor_lock;
> 		mtd->_unlock = spi_nor_unlock;
> 		mtd->_is_locked = spi_nor_is_locked;
> 	}
> 
> I think it's a reasonable condition to enforce, that we only provide
> lock/unlock support if your flash also implements is_locked.

Yes, you're right. At least until some sort of flash which can only be locked
and not unlocked (like OTP) comes around. But we shouldn't overengineer things
from the getgo.

Thanks for clarifying.

Best regards,
Marek Vasut

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Brian Norris]

On Thu, Sep 03, 2015 at 11:43:09AM +0200, Marek Vasut wrote:
> On Wednesday, September 02, 2015 at 10:30:41 PM, Brian Norris wrote:
> > On Wed, Sep 02, 2015 at 11:01:49AM +0200, Marek Vasut wrote:
> > > On Tuesday, September 01, 2015 at 09:57:12 PM, Brian Norris wrote:
> > > > This enables ioctl(MEMISLOCKED). Status can now be reported in the
> > > > mtdinfo or flash_lock utilities found in mtd-utils.
> > > > 
> > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/mtd/spi-nor/spi-nor.c | 37
> > > >  ++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h   |
> > > >   3 +++
> > > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > b/drivers/mtd/spi-nor/spi-nor.c index 62fa1b4ff3c0..c4fb1205f1d3
> > > > 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > 
> > ...
> > 
> > > > @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd,
> > > > loff_t ofs, uint64_t len) return ret;
> > > > 
> > > >  }
> > > > 
> > > > +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs,
> > > > uint64_t len) +{
> > > > +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > > > +	int ret;
> > > > +
> > > > +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = nor->flash_is_locked(nor, ofs, len);
> > > 
> > > Is nor->flash_is_locked () always available or should you check this here
> > > ?
> > 
> > spi_nor_is_locked() is only used when all 3 of
> > flash_{lock,unlock,is_locked} are available:
> > 
> > 	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> > 		mtd->_lock = spi_nor_lock;
> > 		mtd->_unlock = spi_nor_unlock;
> > 		mtd->_is_locked = spi_nor_is_locked;
> > 	}
> > 
> > I think it's a reasonable condition to enforce, that we only provide
> > lock/unlock support if your flash also implements is_locked.
> 
> Yes, you're right. At least until some sort of flash which can only be locked
> and not unlocked (like OTP) comes around.

I suppose we could split this up into independent conditions if/when
needed:

	if (nor->flash_lock)
		mtd->_lock = spi_nor_lock;
 	if (nor->flash_unlock)
 		mtd->_unlock = spi_nor_unlock;
 	if (nor->flash_is_locked) {
 		mtd->_is_locked = spi_nor_is_locked;

But I kinda like enforcing all three, for as much as possible.

> But we shouldn't overengineer things
> from the getgo.

Sure.

Thanks for reviewing.

Brian

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Jagan Teki]

On 2 September 2015 at 01:27, Brian Norris <computersforpeace@gmail.com> wrote:
> This enables ioctl(MEMISLOCKED). Status can now be reported in the
> mtdinfo or flash_lock utilities found in mtd-utils.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 37 ++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/spi-nor.h   |  3 +++
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 62fa1b4ff3c0..c4fb1205f1d3 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -519,6 +519,24 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>         return write_sr(nor, status_new);
>  }
>
> +/*
> + * Check if a region of the flash is (completely) locked. See stm_lock() for
> + * more info.
> + *
> + * 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)
> +{
> +       int status;
> +
> +       status = read_sr(nor);
> +       if (status < 0)
> +               return status;
> +
> +       return stm_is_locked_sr(nor, ofs, len, status);
> +}
> +
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>         struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>         return ret;
>  }
>
> +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       int ret;
> +
> +       ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> +       if (ret)
> +               return ret;
> +
> +       ret = nor->flash_is_locked(nor, ofs, len);
> +
> +       spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
> +       return ret;
> +}
> +
>  /* Used when the "_ext_id" is two bytes at most */
>  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)     \
>                 .id = {                                                 \
> @@ -1151,11 +1184,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>         if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
>                 nor->flash_lock = stm_lock;
>                 nor->flash_unlock = stm_unlock;
> +               nor->flash_is_locked = stm_is_locked;
>         }
>
> -       if (nor->flash_lock && nor->flash_unlock) {
> +       if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
>                 mtd->_lock = spi_nor_lock;
>                 mtd->_unlock = spi_nor_unlock;
> +               mtd->_is_locked = spi_nor_is_locked;

Need some understanding here, is this lock status call will be re-used
at the time of _erase and _write calls for skipping locked area or
sectors, if so how it re-used? Idea here is if user erase an area
out-of which some part is locked and other unlocked the erase call
should print the locked area and erase only unlocked area.

>         }
>
>         /* sst nor chips use AAI word program */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index bd8068f6796a..8620bfaed261 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -176,6 +176,8 @@ struct mtd_info;
>   *                     at the offset @offs
>   * @flash_lock:                [FLASH-SPECIFIC] lock a region of the SPI NOR
>   * @flash_unlock:      [FLASH-SPECIFIC] unlock a region of the SPI NOR
> + * @flash_is_locked:   [FLASH-SPECIFIC] check if a region of the SPI NOR is
> + *                     completely locked
>   * @priv:              the private data
>   */
>  struct spi_nor {
> @@ -212,6 +214,7 @@ struct spi_nor {
>
>         int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>         int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> +       int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>
>         void *priv;
>  };
> --
> 2.5.0.457.gab17608
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


-- Jagan.

Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Brian Norris]

On Thu, Oct 01, 2015 at 02:30:40PM +0530, Jagan Teki wrote:
> On 2 September 2015 at 01:27, Brian Norris <computersforpeace@gmail.com> wrote:
> > This enables ioctl(MEMISLOCKED). Status can now be reported in the
> > mtdinfo or flash_lock utilities found in mtd-utils.
> >
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 37 ++++++++++++++++++++++++++++++++++++-
> >  include/linux/mtd/spi-nor.h   |  3 +++
> >  2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 62fa1b4ff3c0..c4fb1205f1d3 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
[...]
> > @@ -1151,11 +1184,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
> >         if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
> >                 nor->flash_lock = stm_lock;
> >                 nor->flash_unlock = stm_unlock;
> > +               nor->flash_is_locked = stm_is_locked;
> >         }
> >
> > -       if (nor->flash_lock && nor->flash_unlock) {
> > +       if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> >                 mtd->_lock = spi_nor_lock;
> >                 mtd->_unlock = spi_nor_unlock;
> > +               mtd->_is_locked = spi_nor_is_locked;
> 
> Need some understanding here, is this lock status call will be re-used
> at the time of _erase and _write calls for skipping locked area or
> sectors, if so how it re-used? Idea here is if user erase an area
> out-of which some part is locked and other unlocked the erase call
> should print the locked area and erase only unlocked area.

You can review the existing lock support in MTD to answer most of
this...

Currently, the lock APIs (mtd_lock(), mtd_unlock(), mtd_is_locked())
aren't automatically used for anything. They are mostly just supported
to provide an ioctl() interface, via MEMLOCK, MEMUNLOCK, MEMISLOCKED.
See mtdchar.c. These ioctls are supported in mtd-utils, so user programs
can handle the locking aspects on their own, if their system design
requires it. (e.g., this allows boot ROMs to be protected, while still
allowing unlock/reflash under specific circumstances.)

As it stands now, no one checks or reports errors if a block is locked
when the user tries to erase it. I guess it's up to the user to figure
that out right now.

I don't have a plan to change this right now, but if you had a good
proposal, then we could discuss. At first glance, I don't like the
suggestion to "print the locked area." That's not the right (primary)
way to report I/O errors. Anyway, that's all orthogonal to this patch.
The interface is already there; it's just not implemented in this
driver.

Brian

[PATCH 08/10] mtd: spi-nor: add DUAL_READ for w25q{32,64}dw

[Brian Norris]

These flash support dual and quad read. Tested dual read on the 32 Mbit
version.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c4fb1205f1d3..c05ee8340e5e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -794,10 +794,10 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "w25x16", INFO(0xef3015, 0, 64 * 1024,  32, SECT_4K) },
 	{ "w25x32", INFO(0xef3016, 0, 64 * 1024,  64, SECT_4K) },
 	{ "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SECT_4K) },
 	{ "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) },
-	{ "w25q64dw", INFO(0xef6017, 0, 64 * 1024, 128, SECT_4K) },
+	{ "w25q64dw", INFO(0xef6017, 0, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SECT_4K) },
 	{ "w25q80", INFO(0xef5014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
-- 
2.5.0.457.gab17608

[PATCH 09/10] mtd: spi-nor: support lock/unlock/is_locked for Winbond

[Brian Norris]

Many other flash share the same features as ST Micro. I've tested some
Winbond flash, so add them.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c05ee8340e5e..6999ef34b597 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1180,8 +1180,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->_erase = spi_nor_erase;
 	mtd->_read = spi_nor_read;
 
-	/* NOR protection support for STmicro/Micron chips */
-	if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
+	/* NOR protection support for STmicro/Micron chips and similar */
+	if (JEDEC_MFR(info) == SNOR_MFR_MICRON ||
+	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
 		nor->flash_is_locked = stm_is_locked;
-- 
2.5.0.457.gab17608

[PATCH 10/10] mtd: spi-nor: disable protection for Winbond flash at startup

[Brian Norris]

In case the flash was locked at boot time.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 6999ef34b597..d69a6c7f11a1 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1159,13 +1159,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mutex_init(&nor->lock);
 
 	/*
-	 * Atmel, SST and Intel/Numonyx serial nor tend to power
-	 * up with the software protection bits set
+	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
+	 * with the software protection bits set
 	 */
 
 	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
 	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
-	    JEDEC_MFR(info) == SNOR_MFR_SST) {
+	    JEDEC_MFR(info) == SNOR_MFR_SST ||
+	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
 		write_enable(nor);
 		write_sr(nor, 0);
 	}
-- 
2.5.0.457.gab17608

Re: [PATCH 00/10] mtd: spi-nor: cleanups + block protection support updates

[Brian Norris]

On Tue, Sep 01, 2015 at 12:57:05PM -0700, Brian Norris wrote:
> [Note: as this is sent out during the merge window, it's based on the
> semi-unofficial l2-mtd.git/next branch, which is targeting 4.4, not 4.3]
> 
> Hi all,
> 
> I've been reviewing various spi-nor drivers as well as working with some
> Winbond flash to support new locking features. The former helped point out a
> few more things that could use some improvement, and the latter suggested that
> we have some glaring oversights in the spi-nor lock/unlock code.
> 
> <side note>
> Some helpful companion code, for mtd-utils:
> 
>  http://lists.infradead.org/pipermail/linux-mtd/2015-August/061526.html
> 
> This extends the flash_lock tool so that you can more easily test specific
> ranges, using:
> 
>   # flash_lock --lock /dev/mtdX <offset> <block-count>
>   # flash_lock --unlock /dev/mtdX <offset> <block-count>
>   # flash_lock --islocked /dev/mtdX <offset> <block-count>
> </side note>
> 
> The first half of this series is fairly self-explanatory. The second might take
> a bit of thought, as a formulaic approach is a little more subtle than a
> table-based approach, so I tried to copy the relevant portions distilled from a
> few datasheets and include comments. Please shout if anything deserves more
> explanation or looks funny to you.
> 
> Highlights:
> 
>  * clean up spi-nor.h header
>  * spi-nor now supports MEMISLOCKED
>  * MEM{LOCK,UNLOCK} support is a little more robust and extendible
>  * turn on dual/quad read for Winbond w25q{32,64}dw
>  * enable block protection for Winbond flash
> 
> Regards,
> Brian

Pushed all to l2-mtd.git, with some minor context fixups for patch 8