Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: vigneshr@ti.com, richard@nod.at, linux-kernel@vger.kernel.org,
	boris.brezillon@collabora.com, linux-mtd@lists.infradead.org,
	miquel.raynal@bootlin.com
Subject: Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile
Date: Wed, 25 Nov 2020 19:52:32 +0100
Message-ID: <f6344b9cb5c61c3bfd075e231b708269@walle.cc> (raw)
In-Reply-To: <e9437b3e-de44-7b17-df2c-6a8502b48ca4@microchip.com>

Am 2020-11-25 13:21, schrieb Tudor.Ambarus@microchip.com:
[..]
>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>> index 49d392c6c8bc..19665095aeab 100644
>> --- a/drivers/mtd/spi-nor/atmel.c
>> +++ b/drivers/mtd/spi-nor/atmel.c
>> @@ -8,23 +8,120 @@
>> 
>>  #include "core.h"
>> 
>> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2)
>> +
>> +/**
>> + * atmel_set_global_protection - Do a Global Protect or Unprotect 
>> command
>> + * @nor:       pointer to 'struct spi_nor'
>> + * @ofs:       offset in bytes
>> + * @len:       len in bytes
>> + * @is_protect:        if true do a Global Protect otherwise it is a 
>> Global Unprotect
>> + *
>> + * Return: 0 on success, -error otherwise.
>> + */
>> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t 
>> ofs,
>> +                                      uint64_t len, bool is_protect)
>> +{
>> +       int ret;
>> +       u8 sr;
>> +
>> +       /* We only support locking the whole flash array */
>> +       if (ofs || len != nor->params->size)
>> +               return -EINVAL;
>> +
>> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> +       if (ret)
>> +               return ret;
>> +       sr = nor->bouncebuf[0];
>> +
>> +       /* SRWD bit needs to be cleared, otherwise the protection 
>> doesn't change */
>> +       if (sr & SR_SRWD) {
>> +               sr &= ~SR_SRWD;
>> +               ret = spi_nor_write_sr_and_check(nor, sr);
>> +               if (ret) {
>> +                       dev_err(nor->dev, "unable to clear SRWD bit, 
>> WP# asserted?\n");
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (is_protect)
>> +               sr |= ATMEL_SR_GLOBAL_PROTECT_MASK;
>> +       else
>> +               sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK;
>> +
>> +       return spi_nor_write_sr_and_check(nor, sr);
>> +}
>> +
>> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       return atmel_set_global_protection(nor, ofs, len, true);
>> +}
>> +
>> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       return atmel_set_global_protection(nor, ofs, len, false);
>> +}
>> +
>> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       int ret;
>> +
>> +       if (ofs >= nor->params->size || (ofs + len) > 
>> nor->params->size)
>> +               return -EINVAL;
>> +
>> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == 
>> ATMEL_SR_GLOBAL_PROTECT_MASK);
>> +}
>> +
>> +static const struct spi_nor_locking_ops atmel_global_protection_ops = 
>> {
>> +       .lock = atmel_global_protect,
>> +       .unlock = atmel_global_unprotect,
>> +       .is_locked = atmel_is_global_protected,
>> +};
> 
> Skimming through my notes in 1/3, I'm guessing this will not work for 
> any
> of the atmel flashes that we currently have.
> 
> Can we instead return -EOPNOTSUPP for .is_locked and .lock and just
> write a 0x00 to SR
> for the .unlock method?

As mentioned in my previous reply to 1/3 I don't think it is enough to
just write 0 because you first have to disable the SPRL.

For the other ops, for what atmel flash do you think this won't work?

All the flashes which uses these ops were marked as "Global
Protect/Unprotect same as in at25df041a." by you. So I think it should
work on all of these. Or am I missing something here?

[..]

>> diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
>> index c93170008118..c2ebf29d95f2 100644
>> --- a/drivers/mtd/spi-nor/esmt.c
>> +++ b/drivers/mtd/spi-nor/esmt.c
>> @@ -11,9 +11,13 @@
>>  static const struct flash_info esmt_parts[] = {
>>         /* ESMT */
>>         { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64,
>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32PA.pdf
> BP GENMASK(4,2), volatile, ok
> 
>>         { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> https://datasheetspdf.com/pdf-file/796196/ESMT/F25L32QA/1
> Datasheet states that "BP0~3, QE and BPL bits are non-volatile."
> At the same time, it says: "After power-up, BP3, BP2, BP1 and BP0 bits
> are set to 0."

Mhh I had this datasheet:
https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32QA.pdf

In that one they are volatile.. but yours is a newer version. So I
guess the flashes with the PA suffix have volatile BP and the QA ones
have the non-volatile version.

> Maybe factory default setting for BPn is 0? Let's treat them as NV, as 
> in
> f25l64qa.

Yes will fix it.

> Do we need BP3?

Rather the top bottom bit. But that is outside of the scope of this 
patch.
And as per your rule, as I don't have this particular flash I cannot 
test
and thus couldn't add the TB bit (technically). But if you like I can do
another patch (outside of this series and after it is applied) which 
will
add the TB bit flag.

> 
>> +       /*
>> +        * According to the datasheet the BPn bits are non-volatile, 
>> whereas
>> +        * they are volatile for the smaller f25l32qa.
>> +        */
>>         { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
>>                            SECT_4K | SPI_NOR_HAS_LOCK) },
> 
> https://datasheetspdf.com/pdf-file/967488/EliteSemiconductor/F25L64QA/1
> BP GENMASK(5, 2), non-volatile.
> 
> BP3?

Same as F25L32QA.

> 
> 
>>  };
>> diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c
>> index d8196f101368..c45ea0ad01f0 100644
>> --- a/drivers/mtd/spi-nor/intel.c
>> +++ b/drivers/mtd/spi-nor/intel.c
>> @@ -10,9 +10,9 @@
>> 
>>  static const struct flash_info intel_parts[] = {
>>         /* Intel/Numonyx -- xxxs33b */
>> -       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
>> -       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
>> -       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
>> +       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> http://www.datasheet.hk/view_download.php?id=1561787&file=0282\qh25f016s33b8_649107.pdf
> BP GENMASK(4,2) all volatile, all set to 1 at power-up.
> 
> You can add SPI_NOR_HAS_LOCK to each flash, and get rid of the 
> manufacturer
> generalization.

ok

> 
>>  };
>> 
>>  static void intel_default_init(struct spi_nor *nor)
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 8b169fa4102a..5e4450877d66 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -11,26 +11,27 @@
>>  static const struct flash_info sst_parts[] = {
>>         /* SST -- large erase sizes are "overlays", "sectors" are 4K 
>> */
>>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> -       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
>> +                             SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> Looks like BP3 is needed here.

https://ww1.microchip.com/downloads/en/DeviceDoc/20005036C.pdf

agreed. But again cannot test it. Would add it as a seperate patch
to this series. (or leave it like it is)

> 
>>         { "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
>>         { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
> 
> These two flashes have just two BP bits located at bit 2 and 3.
> Probably will work.

Mhh? What datasheet were you looking at? There are three BPs:
https://ww1.microchip.com/downloads/en/DeviceDoc/SST25WF040B-4-Mbit-1.8V-SPI-Serial-Flash-Data-Sheet-DS20005193E.pdf

Ahh here are the tables which only inidicate two. But there are three.
https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf

And yes since the rework of the BP bits algorithm this should work
as expected. Its just because the flash is too small to actually fill
up all the BP bits.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-03 15:32 [PATCH v5 0/3] " Michael Walle
2020-10-03 15:32 ` [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag Michael Walle
2020-11-24 19:09   ` Tudor.Ambarus
2020-11-25 18:17     ` Michael Walle
2020-11-26 12:45       ` Tudor.Ambarus
2020-11-26 12:59         ` Tudor.Ambarus
2020-11-26 16:42       ` Tudor.Ambarus
2020-11-26 18:44         ` Michael Walle
2020-10-03 15:32 ` [PATCH v5 2/3] mtd: spi-nor: sst: " Michael Walle
2020-11-24 19:50   ` Tudor.Ambarus
2020-10-03 15:32 ` [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle
2020-11-25 12:21   ` Tudor.Ambarus
2020-11-25 18:52     ` Michael Walle [this message]
2020-11-26 16:47       ` Tudor.Ambarus
2020-11-26 20:46       ` Michael Walle
2020-10-27 22:26 ` [PATCH v5 0/3] " Michael Walle
2020-11-10 13:07   ` Vignesh Raghavendra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f6344b9cb5c61c3bfd075e231b708269@walle.cc \
    --to=michael@walle.cc \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git