[PATCH] mtd: m25p80: Flash protection support for STmicro chips

[Brian Norris]

+ Marek, Angus

On Thu, Feb 27, 2014 at 03:34:06PM +0100, Gerlando Falauto wrote:
> Hi,
> 
> it's me again.
> In my opinion (and experience) this introduces a pretty serious bug
> (not to mention the compatibility issues), yet I haven't heard a
> single word or found a patch applied about it in three months.
> Am I the only one having this issue? Or maybe I'm just "seeing things"?

I agree that this doesn't look quite like the best implementation. As
you note, this feature is not available on *ALL* ST SPI flash. I think
it should require yet another device flag in m25p_ids[]...

But I don't see why this is a concern for "certain bootloaders". If your
bootloader doesn't support locked blocks, then don't run ioctl(MEMLOCK)
on the device.

Leaving the following context intact for now, since it's old. But please
trim your replies and bottom-post in the future. Thanks!

Regards,
Brian

> Thank you,
> Gerlando
> 
> P.S. FWIW, the original author of the patch seem to have disappeared.
> 
> On 11/20/2013 09:04 PM, Gerlando Falauto wrote:
> >Hi,
> >
> >On 01/04/2013 01:02 AM, Austin Boyle wrote:
> >>This patch adds generic support for flash protection on STmicro chips.
> >>On chips with less than 3 protection bits, the unused bits are don't
> >>cares
> >>and so can be written anyway.
> >
> >I have two remarks:
> >
> >1) I believe this introduces incompatibilities with existing bootloaders
> >which do not support this feature.
> >Namely, u-boot is not able (to the best of my knowledge) to treat these
> >bits properly. So as soon as you write something to your SPI nor flash
> >from within linux, u-boot is not able to erase/rewrite those blocks
> >anymore.
> >
> >Wouldn't it make more sense to selectively enable this feature, only if
> >explicity configured to do so (e.g. through its device tree node)?
> >Like what was used for the Spansion's PPB, see:
> >
> >http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html
> >
> > > The lock function will only change the
> >>protection bits if it would not unlock other areas. Similarly, the unlock
> >>function will not lock currently unlocked areas. Tested on the m25p64.
> > >
> >>From: Austin Boyle <Austin.Boyle at aviatnet.com>
> >>Signed-off-by: Austin Boyle <Austin.Boyle at aviatnet.com>
> >>---
> >>diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> >>index 4eeeb2d..069e34f 100644
> >>--- a/drivers/mtd/devices/m25p80.c
> >>+++ b/drivers/mtd/devices/m25p80.c
> >>@@ -565,6 +565,96 @@ time_out:
> >>      return ret;
> >>  }
> >>
> >>+static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >>+{
> >>+    struct m25p *flash = mtd_to_m25p(mtd);
> >>+    uint32_t offset = ofs;
> >>+    uint8_t status_old, status_new;
> >>+    int res = 0;
> >>+
> >>+    mutex_lock(&flash->lock);
> >>+    /* Wait until finished previous command */
> >>+    if (wait_till_ready(flash)) {
> >>+        res = 1;
> >>+        goto err;
> >>+    }
> >>+
> >>+    status_old = read_sr(flash);
> >>+
> >>+    if (offset < flash->mtd.size-(flash->mtd.size/2))
> >>+        status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
> >>+    else if (offset < flash->mtd.size-(flash->mtd.size/4))
> >>+        status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> >>+    else if (offset < flash->mtd.size-(flash->mtd.size/8))
> >>+        status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> >>+    else if (offset < flash->mtd.size-(flash->mtd.size/16))
> >>+        status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> >>+    else if (offset < flash->mtd.size-(flash->mtd.size/32))
> >>+        status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> >>+    else if (offset < flash->mtd.size-(flash->mtd.size/64))
> >>+        status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> >>+    else
> >>+        status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> >
> >2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e.
> >flashes with 64 blocks or more), it looks incorrect for smaller chips
> >(namely our m25p80, with just 16 blocks). There, the 1/64 logic scales
> >down to 1/16, e.g.
> >- 000 means protect nothing
> >- 001 means protect 1/16th (=1 blocks)    [m25p64 => 1/64th]
> >- 010 means protect 1/8th  (=2 blocks)    [m25p64 => 1/32th]
> >- ...
> >- 100 means protect 1/2nd  (=8 blocks)
> >- 101,110, 111 mean protect everything
> >
> >and I assume the same goes for chips with fewer sectors.
> >
> >Any comments?
> >
> >Thanks,
> >Gerlando
> >
> >>+
> >>+    /* 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(flash);
> >>+        if (write_sr(flash, status_new) < 0) {
> >>+            res = 1;
> >>+            goto err;
> >>+        }
> >>+    }
> >>+
> >>+err:    mutex_unlock(&flash->lock);
> >>+    return res;
> >>+}
> >>+
> >>+static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >>+{
> >>+    struct m25p *flash = mtd_to_m25p(mtd);
> >>+    uint32_t offset = ofs;
> >>+    uint8_t status_old, status_new;
> >>+    int res = 0;
> >>+
> >>+    mutex_lock(&flash->lock);
> >>+    /* Wait until finished previous command */
> >>+    if (wait_till_ready(flash)) {
> >>+        res = 1;
> >>+        goto err;
> >>+    }
> >>+
> >>+    status_old = read_sr(flash);
> >>+
> >>+    if (offset+len > flash->mtd.size-(flash->mtd.size/64))
> >>+        status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
> >>+    else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
> >>+        status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> >>+    else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
> >>+        status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> >>+    else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
> >>+        status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> >>+    else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
> >>+        status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> >>+    else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
> >>+        status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> >>+    else
> >>+        status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> >>+
> >>+    /* 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(flash);
> >>+        if (write_sr(flash, status_new) < 0) {
> >>+            res = 1;
> >>+            goto err;
> >>+        }
> >>+    }
> >>+
> >>+err:    mutex_unlock(&flash->lock);
> >>+    return res;
> >>+}
> >>+
> >>
> >>/****************************************************************************/
> >>
> >>
> >>  /*
> >>@@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi)
> >>      flash->mtd._erase = m25p80_erase;
> >>      flash->mtd._read = m25p80_read;
> >>
> >>+    /* flash protection support for STmicro chips */
> >>+    if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
> >>+        flash->mtd._lock = m25p80_lock;
> >>+        flash->mtd._unlock = m25p80_unlock;
> >>+    }
> >>+
> >>      /* sst flash chips use AAI word program */
> >>      if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST)
> >>          flash->mtd._write = sst_write;
> >>
> >>