Patchwork Add support for SST26VF064B

login
register
about
Submitter Stefan Tauner
Date 2014-05-04 20:06:09
Message ID <201405042006.s44K69lV024786@mail2.student.tuwien.ac.at>
Download mbox | patch
Permalink /patch/4139/
State New
Headers show

Comments

Stefan Tauner - 2014-05-04 20:06:09
On Mon, 31 Mar 2014 22:18:48 -0700
Wei Hu <wei@aristanetworks.com> wrote:

> Datasheet available at
> http://ww1.microchip.com/downloads/en/DeviceDoc/20005119D.pdf

Hi,

thank you for your patch! I have reviewed it and I think you got a few
things wrong:
- Most importantly, I think the block erase sizes are non-uniform
  (for opcode 0xD8). See note 11 of Table 5.1 and Section 3.0 etc.
- Also, the ULBPR requires a WREN opcode sent before it. OTOH your
  spi_disable_blockprotect_generic() calls seems useless (all bits of
  the status register are read-only).
- The spi_prettyprint_status_register_sst25 function is completely
  wrong for these chips.
- The OTP memory, and other advanced features were not documented at
  all.
- Debatably the name of the chip should include the optional A that
  indicates the disable default state of the WP/Hold feature of this
  chip variant.

I have attached my counterproposal that also adds support for the
SST26VF032B(A). Please take a look and tell me if you disagree with
anything I have changed, thanks.
Wei Hu - 2014-05-05 21:17:05
Stefan,

Thanks for your review and your new patch. Much appreciated.

On Sun, May 4, 2014 at 1:06 PM, Stefan Tauner
<stefan.tauner@alumni.tuwien.ac.at> wrote:
> On Mon, 31 Mar 2014 22:18:48 -0700
> Wei Hu <wei@aristanetworks.com> wrote:
>
>> Datasheet available at
>> http://ww1.microchip.com/downloads/en/DeviceDoc/20005119D.pdf
>
> Hi,
>
> thank you for your patch! I have reviewed it and I think you got a few
> things wrong:
> - Most importantly, I think the block erase sizes are non-uniform
>   (for opcode 0xD8). See note 11 of Table 5.1 and Section 3.0 etc.

Thanks for fixing the non-uniform layout! Learned something new today.

> - Also, the ULBPR requires a WREN opcode sent before it. OTOH your
>   spi_disable_blockprotect_generic() calls seems useless (all bits of
>   the status register are read-only).

I know spi_disable_blockprotect_generic() is useless here, but I
thought it was a good idea to call the generic function first in
general? As a side effect of calling this function I'm sending WREN,
so I didn't send on explicitly.

> - The spi_prettyprint_status_register_sst25 function is completely
>   wrong for these chips.
> - The OTP memory, and other advanced features were not documented at
>   all.

What's OTP? I didn't see it in the datasheet. Why do you want that?

> - Debatably the name of the chip should include the optional A that
>   indicates the disable default state of the WP/Hold feature of this
>   chip variant.
>
> I have attached my counterproposal that also adds support for the
> SST26VF032B(A). Please take a look and tell me if you disagree with
> anything I have changed, thanks.
>
> --
> Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Stefan Tauner - 2014-05-06 20:55:32
On Mon, 5 May 2014 14:17:05 -0700
Wei Hu <wei@arista.com> wrote:

> On Sun, May 4, 2014 at 1:06 PM, Stefan Tauner
> <stefan.tauner@alumni.tuwien.ac.at> wrote:
> > On Mon, 31 Mar 2014 22:18:48 -0700
> > Wei Hu <wei@aristanetworks.com> wrote:
> >
> >> Datasheet available at
> >> http://ww1.microchip.com/downloads/en/DeviceDoc/20005119D.pdf
> >
> > Hi,
> >
> > thank you for your patch! I have reviewed it and I think you got a few
> > things wrong:
> > - Most importantly, I think the block erase sizes are non-uniform
> >   (for opcode 0xD8). See note 11 of Table 5.1 and Section 3.0 etc.
> 
> Thanks for fixing the non-uniform layout! Learned something new today.

This became rather uncommon with SPI chips but still there are even
new chips with this feature. It was very common in older flash chips. We
currently support over 100 chips with such a layout (look for
".eraseblocks = {\n" in flashchips.c).

> > - Also, the ULBPR requires a WREN opcode sent before it. OTOH your
> >   spi_disable_blockprotect_generic() calls seems useless (all bits of
> >   the status register are read-only).
> 
> I know spi_disable_blockprotect_generic() is useless here, but I
> thought it was a good idea to call the generic function first in
> general? As a side effect of calling this function I'm sending WREN,
> so I didn't send on explicitly.

There is no "generic" function that always should be called. The
spi_disable_blockprotect_generic() function is just generic in the
sense that most unprotect schemes can be implemented with it by
supplying the respective parameters.
Also, the WREN command is only active for one matching operation, and
in this case this is the WRSR command that immediately follows it
within spi_write_status_register_flag(). So your version would not do
what you expect.

> > - The spi_prettyprint_status_register_sst25 function is completely
> >   wrong for these chips.
> > - The OTP memory, and other advanced features were not documented at
> >   all.
> 
> What's OTP? I didn't see it in the datasheet. Why do you want that?

One-time-programmable memory, it is noted on the front page of the
datahsset and is explained in more detail in 5.27 "Program Security ID".
If the flag FEATURE_OTP is set for a chip, we produce the following
message:
"This chip may contain one-time programmable memory. flashrom cannot
read and may never be able to write it, hence it may not be able to
completely clone the contents of this chip (see man page for details)."

Most users seem to not understand that, but I think nevertheless that
it is useful for those that do :)
Wei Hu - 2014-05-06 21:24:43
Stefan,

Thanks for your reply. Your patch looks good to me.

On Tue, May 6, 2014 at 1:55 PM, Stefan Tauner
<stefan.tauner@alumni.tuwien.ac.at> wrote:
>>
>> I know spi_disable_blockprotect_generic() is useless here, but I
>> thought it was a good idea to call the generic function first in
>> general? As a side effect of calling this function I'm sending WREN,
>> so I didn't send on explicitly.
>
> There is no "generic" function that always should be called. The
> spi_disable_blockprotect_generic() function is just generic in the
> sense that most unprotect schemes can be implemented with it by
> supplying the respective parameters.
> Also, the WREN command is only active for one matching operation, and
> in this case this is the WRSR command that immediately follows it
> within spi_write_status_register_flag(). So your version would not do
> what you expect.

Maybe you're right, but I was able to erase/write the chip with my patch.

>> What's OTP? I didn't see it in the datasheet. Why do you want that?
>
> One-time-programmable memory, it is noted on the front page of the
> datahsset and is explained in more detail in 5.27 "Program Security ID".
> If the flag FEATURE_OTP is set for a chip, we produce the following
> message:

Sorry for not seeing OTP. I opened the file in a PDF reader and was
able to find the keyword. I wasn't able to search for it in Firefox
for some reason.
Stefan Tauner - 2014-05-06 21:56:31
On Tue, 6 May 2014 14:24:43 -0700
Wei Hu <wei@arista.com> wrote:

> Maybe you're right, but I was able to erase/write the chip with my patch.

Interesting, thank you. IIRC the protection is on by default... you
successful modification contradicts my hypothesis that your patch is
not able to disable the protection. I'll re-read the datasheet before
including the patch. Another write test with my version would be much
appreciated, just to make sure my version is no regression to yours.

Patch

From 6bbce3a432a1719d7bcf63178bc9becb47e55215 Mon Sep 17 00:00:00 2001
From: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
Date: Sun, 4 May 2014 21:44:36 +0200
Subject: [PATCH] Add support for SST26VF032B(A) and SST26VF064B(A).

Differences between SST25 and these:
1. The WREN instruction must be executed prior to WRSR [Section 5.31].
   There is no EWSR.
2. Block protection bits are no longer in the status register. There
   is a dedicated 144-bit register [Table 5-6]. The device is
   write-protected by default. A Global Block-Protection Unlock
   command unlocks the entire memory [Section 4.1].
3. Non-uniform erase block sizes [Section 3.0, Figure 3.1].

Signed-off-by: Wei Hu <wei@aristanetworks.com>
Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
---
 chipdrivers.h     |  1 +
 flashchips.c      | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 flashchips.h      |  1 +
 spi25_statusreg.c | 13 ++++++++++
 4 files changed, 92 insertions(+)

diff --git a/chipdrivers.h b/chipdrivers.h
index 851e90a..73b727b 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -100,6 +100,7 @@  int spi_disable_blockprotect_bp2_ep_srwd(struct flashctx *flash);
 int spi_prettyprint_status_register_sst25(struct flashctx *flash);
 int spi_prettyprint_status_register_sst25vf016(struct flashctx *flash);
 int spi_prettyprint_status_register_sst25vf040b(struct flashctx *flash);
+int spi_disable_blockprotect_sst26_global_unprotect(struct flashctx *flash);
 
 /* sfdp.c */
 int probe_spi_sfdp(struct flashctx *flash);
diff --git a/flashchips.c b/flashchips.c
index 9263e15..9e6f677 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -10014,6 +10014,83 @@  const struct flashchip flashchips[] = {
 
 	{
 		.vendor		= "SST",
+		.name		= "SST26VF032B(A)",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= SST_ID,
+		.model_id	= SST_SST26VF032B,
+		.total_size	= 4096,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
+		.tested		= TEST_UNTESTED,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 1024} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = {
+					{8 * 1024, 4},
+					{32 * 1024, 1},
+					{64 * 1024, 62},
+					{32 * 1024, 1},
+					{8 * 1024, 4},
+				},
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {4 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			},
+		},
+		.printlock	= spi_prettyprint_status_register_plain, /* TODO: improve */
+		.unlock		= spi_disable_blockprotect_sst26_global_unprotect,
+		.write		= spi_chip_write_256, /* Multi I/O supported */
+		.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
+		.voltage	= {2700, 3600},
+	},
+
+
+	{
+		.vendor		= "SST",
+		.name		= "SST26VF064B(A)",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= SST_ID,
+		.model_id	= SST_SST26VF064B,
+		.total_size	= 8192,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 2048} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = {
+					{8 * 1024, 4},
+					{32 * 1024, 1},
+					{64 * 1024, 126},
+					{32 * 1024, 1},
+					{8 * 1024, 4},
+				},
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {8 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			},
+		},
+		.printlock	= spi_prettyprint_status_register_plain, /* TODO: improve */
+		.unlock		= spi_disable_blockprotect_sst26_global_unprotect,
+		.write		= spi_chip_write_256, /* Multi I/O supported */
+		.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
+		.voltage	= {2700, 3600},
+	},
+
+	{
+		.vendor		= "SST",
 		.name		= "SST25WF512",
 		.bustype	= BUS_SPI,
 		.manufacture_id	= SST_ID,
diff --git a/flashchips.h b/flashchips.h
index af0648a..05b0492 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -592,6 +592,7 @@ 
 #define SST_SST25VF064C		0x254B
 #define SST_SST26VF016		0x2601
 #define SST_SST26VF032		0x2602
+#define SST_SST26VF032B		0x2642
 #define SST_SST26VF064B		0x2643
 #define SST_SST27SF512		0xA4
 #define SST_SST27SF010		0xA5
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index f96d4e8..7479b52 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -196,6 +196,19 @@  int spi_disable_blockprotect(struct flashctx *flash)
 	return spi_disable_blockprotect_generic(flash, 0x3C, 0, 0, 0xFF);
 }
 
+int spi_disable_blockprotect_sst26_global_unprotect(struct flashctx *flash)
+{
+	int result = spi_write_enable(flash);
+	if (result)
+		return result;
+
+	static const unsigned char cmd[] = { 0x98 }; /* ULBPR */
+	result = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
+	if (result)
+		msg_cerr("ULBPR failed\n");
+	return result;
+}
+
 /* A common block protection disable that tries to unset the status register bits masked by 0x1C (BP0-2) and
  * protected/locked by bit #7. Useful when bit #5 is neither a protection bit nor reserved (and hence possibly
  * non-0). */
-- 
Kind regards, Stefan Tauner