Change in flashrom[master]: flashchips: Added W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Added W25Q256.W
......................................................................

flashchips: Added W25Q256.W

Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/1

diff --git a/flashchips.c b/flashchips.c
index 974ba81..f2bd68b 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -16626,6 +16626,47 @@

{
    	.vendor		= "Winbond",
+		.name		= "W25Q256.W",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= WINBOND_NEX_ID,
+		.model_id	= WINBOND_NEX_W25Q256_W,
+		.total_size	= 32768,
+		.page_size	= 256,
+		/* supports SFDP */
+		/* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_ENTER_WREN
+			| FEATURE_4BA_EXT_ADDR | FEATURE_4BA_READ | FEATURE_4BA_FAST_READ,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 8192} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 1024} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 512} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {32 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {32 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain, /* TODO: improve */
+		.unlock		= spi_disable_blockprotect,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {1650, 1950},
+	},
+
+	{
+		.vendor		= "Winbond",
    	.name		= "W25Q128.JW.DTR",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= WINBOND_NEX_ID,
diff --git a/flashchips.h b/flashchips.h
index 0c77d1d..3332974 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -943,6 +943,7 @@
 #define WINBOND_NEX_W25Q32_W	0x6016	/* W25Q32DW; W25Q32FV in QPI mode */
 #define WINBOND_NEX_W25Q64_W	0x6017	/* W25Q64DW; W25Q64FV in QPI mode */
 #define WINBOND_NEX_W25Q128_W	0x6018	/* W25Q128FW; W25Q128FV in QPI mode */
+#define WINBOND_NEX_W25Q256_W	0x6019	/* W25Q256JW */
 #define WINBOND_NEX_W25Q128_V_M	0x7018	/* W25Q128JVSM */
 #define WINBOND_NEX_W25Q256JV_M	0x7019	/* W25Q256JV_M (QE=0) */
 #define WINBOND_NEX_W25Q64JW	0x8017

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#2).

Change subject: flashchips: Added W25Q256.W
......................................................................

flashchips: Added W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that he successfully
run read, write and verify cmd with this changes.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/2

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#3).

Change subject: flashchips: Added W25Q256.W
......................................................................

flashchips: Added W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that he successfully run read, write and verify cmd with this changes.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/3

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Added W25Q256.W
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@7 
PS3, Line 7: Added
Add

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@11 
PS3, Line 11: 
Please only use one blank line.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Added W25Q256.W
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@9 
PS3, Line 9:  successfully run read, write and verify cmd with this changes.
Please put this part of the sentence on the following line

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#4).

Change subject: flashchips: Add W25Q256.W
......................................................................

flashchips: Add W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that
he successfully run read, write and verify cmd with this changes.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/4

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@7 
PS3, Line 7: Added
> Add

Done

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@9 
PS3, Line 9:  successfully run read, write and verify cmd with this changes.
> Please put this part of the sentence on the following line

Done

https://review.coreboot.org/c/flashrom/+/40855/3//COMMIT_MSG@11 
PS3, Line 11:
> Please only use one blank line.

Done

Change in flashrom[master]: flashchips: Add W25Q256.W

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG@10 
PS5, Line 10: read, write and verify
erase too?

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG@10 
PS5, Line 10: this changes
either `this change` or `these changes`

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#6).

Change subject: flashchips: Add W25Q256.W
......................................................................

flashchips: Add W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that
he successfully run read, write and verify cmd.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/6

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 6:

(2 comments)
> Patch Set 5:
>
> (2 comments)

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG@10 
PS5, Line 10: read, write and verify
> erase too?

He did not mention erase. 
As a successful write with random data implies a successful erase, I would accept it.
If you don´t think so, I will change the .tested value.

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG@10 
PS5, Line 10: this changes
> either `this change` or `these changes`

Done

Change in flashrom[master]: flashchips: Add W25Q256.W

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/5//COMMIT_MSG@10 
PS5, Line 10: read, write and verify
> He did not mention erase.  […]

IIRC, chips usually arrive erased from the factory, so the first write might go well even if erase doesn't work. Anyway, fair enough.

https://review.coreboot.org/c/flashrom/+/40855/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/6//COMMIT_MSG@10 
PS6, Line 10: run
he wrote that he _ran_
https://conjugator.reverso.net/conjugation-english-verb-run.html

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#7).

Change subject: flashchips: Add W25Q256.W
......................................................................

flashchips: Add W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that
he successfully ran read, write and verify cmd.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/7

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40855/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40855/6//COMMIT_MSG@10 
PS6, Line 10: run
> he wrote that he _ran_ […]

Done

Change in flashrom[master]: flashchips: Add W25Q256.W

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40855/7/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/40855/7/flashchips.c@16565 
PS7, Line 16565: W25Q256.W
FYI, I recently learned of a newer variant of the W25Q256JW that has a different device ID and put up a patch for it: CB:42386

So I think we'll have essentially duplicate entries for the W25Q256JW and W25Q256JW-IM.

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40855/7/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/40855/7/flashchips.c@16565 
PS7, Line 16565: W25Q256.W
> FYI, I recently learned of a newer variant of the W25Q256JW that has a different device ID and put u […]

Thanks!
Just let me know what do you think is the best way for a clean solution.
I made a suggestion at CB:42386.

Change in flashrom[master]: flashchips: Add W25Q256.W

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 8: Code-Review-1

This looks good to me. Just one request - Can you move the entry in flashchips.c down so it's with the other W25Q256 entries?

Otherwise I'll move my W25Q256JW_DTR entry up so it's closer to where yours is.

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins), David Hendricks,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40855

to look at the new patch set (#9).

Change subject: flashchips: Add W25Q256.W
......................................................................

flashchips: Add W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that
he successfully ran read, write and verify cmd.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/40855/9

Change in flashrom[master]: flashchips: Add W25Q256.W

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 9:
> Patch Set 8: Code-Review-1
>
> This looks good to me. Just one request - Can you move the entry in flashchips.c down so it's with the other W25Q256 entries?
>
> Otherwise I'll move my W25Q256JW_DTR entry up so it's closer to where yours is.

Done

Change in flashrom[master]: flashchips: Add W25Q256.W

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 9: Code-Review+1

Change in flashrom[master]: flashchips: Add W25Q256.W

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

Patch Set 9: Code-Review+2

Change in flashrom[master]: flashchips: Add W25Q256.W

[David Hendricks (Code Review)]

David Hendricks has submitted this change. ( https://review.coreboot.org/c/flashrom/+/40855 )

Change subject: flashchips: Add W25Q256.W
......................................................................

flashchips: Add W25Q256.W

Nicklas Lennert wrote me via the flashrom mailing list that
he successfully ran read, write and verify cmd.

Signed-off-by: Simon Buhrow 
Change-Id: I3608945a38b5ed42b8b2b9b2b388cdea97141d7b
Reviewed-on: https://review.coreboot.org/c/flashrom/+/40855
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: David Hendricks 
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  David Hendricks: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/flashchips.c b/flashchips.c
index b4006df..48405fe 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -16808,6 +16808,47 @@

{
    	.vendor		= "Winbond",
+		.name		= "W25Q256.W",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= WINBOND_NEX_ID,
+		.model_id	= WINBOND_NEX_W25Q256_W,
+		.total_size	= 32768,
+		.page_size	= 256,
+		/* supports SFDP */
+		/* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_ENTER_WREN
+			| FEATURE_4BA_EXT_ADDR | FEATURE_4BA_READ | FEATURE_4BA_FAST_READ,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 8192} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 1024} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 512} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {32 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {32 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain, /* TODO: improve */
+		.unlock		= spi_disable_blockprotect,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {1650, 1950},
+	},
+
+	{
+		.vendor		= "Winbond",
    	.name		= "W25Q32.V",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= WINBOND_NEX_ID,
diff --git a/flashchips.h b/flashchips.h
index e5ef390..c58595d 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -941,6 +941,7 @@
 #define WINBOND_NEX_W25Q32_W	0x6016	/* W25Q32DW; W25Q32FV in QPI mode */
 #define WINBOND_NEX_W25Q64_W	0x6017	/* W25Q64DW; W25Q64FV in QPI mode */
 #define WINBOND_NEX_W25Q128_W	0x6018	/* W25Q128FW; W25Q128FV in QPI mode */
+#define WINBOND_NEX_W25Q256_W	0x6019	/* W25Q256JW */
 #define WINBOND_NEX_W25Q128_V_M	0x7018	/* W25Q128JVSM */
 #define WINBOND_NEX_W25Q256JV_M	0x7019	/* W25Q256JV_M (QE=0) */
 #define WINBOND_NEX_W25Q64JW	0x8017