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

[Patrick Rudolph (Code Review)]

Hello Patrick Rudolph,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: flashchips: Fix W25Q256.W
......................................................................

flashchips: Fix W25Q256.W

Used the datasheet from here:
https://www.winbond.com/resource-files/w25q256jw%20spi%20revb%2012082017.pdf

The chips supports all native 4BA instructions.

Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Signed-off-by: Patrick Rudolph <[e-mail]>
---
M flashchips.c
1 file changed, 8 insertions(+), 2 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/44810/1

diff --git a/flashchips.c b/flashchips.c
index 8b5b5cc..2c16f89 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -16854,8 +16854,8 @@
 		.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,
+		/* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA,
 		.tested		= TEST_OK_PREW,
 		.probe		= probe_spi_rdid,
 		.probe_timing	= TIMING_ZERO,
@@ -16863,12 +16863,18 @@
 		{
 			{
 				.eraseblocks = { {4 * 1024, 8192} },
+				.block_erase = spi_block_erase_21,
+			}, {
+				.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_dc,
+			}, {
+				.eraseblocks = { {64 * 1024, 512} },
 				.block_erase = spi_block_erase_d8,
 			}, {
 				.eraseblocks = { {32 * 1024 * 1024, 1} },

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newchange

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 1: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 26 Aug 2020 10:50:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/44810/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44810/1//COMMIT_MSG@12 
PS1, Line 12: chips supports
`chip supports` or `chips support`



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 26 Aug 2020 12:09:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[David Hendricks (Code Review)]

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

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Sun, 04 Oct 2020 23:06:07 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Attention is currently required from: Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 2: Code-Review-1

(1 comment)

Patchset:

PS2: 
Sorry, I could not figure out from the history why there is
a wildcard `.` in the name. It seems the original patch was
tested with a JW. But there was a thread on the mailing list

    "Do you have support for W25Q128FW and W25Q256.W?"

where a Winbond employee suggested that an FW once existed.
It seems possible that it did not support the full command
set, just like the FV version (if that is true at all and
not an error in the datasheet).

Without further details, we'd have to split the flashchips
entry into FW and JW. Otherwise, we'd risk a regression for
the former. Only getting hands on a 256FW sample could clear
this up :-/



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 2
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Patrick Rudolph <[e-mail]>
Gerrit-Comment-Date: Tue, 11 May 2021 10:16:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Attention is currently required from: Thomas Heijligen, Patrick Rudolph.
Nico Huber has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................

flashchips: Fix W25Q256.W

The JW is the only known variant. A W25Q256FW may have existed with
less 4BA instructions supported, but it never showed up and no data-
sheet is available.

Used the datasheet from here:
https://www.winbond.com/resource-files/w25q256jw%20spi%20revb%2012082017.pdf

Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Signed-off-by: Patrick Rudolph <[e-mail]>
Signed-off-by: Nico Huber <[e-mail]>
---
M flashchips.c
1 file changed, 8 insertions(+), 3 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/44810/3
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 3
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Felix Singer <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Thomas Heijligen <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Thomas Heijligen <[e-mail]>
Gerrit-Attention: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newpatchset

[Nico Huber (Code Review)]

Attention is currently required from: Thomas Heijligen, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

Patchset:

PS2:
> Sorry, I could not figure out from the history why there is […]
> I went ahead and updated the entry name to W25Q256JW and pushed
> a follow up to split W25Q256.V. If no FW sample shows up, it's
> probably better to do nothing about it.

[Nico Huber (Code Review)]

Attention is currently required from: Thomas Heijligen, Angel Pons, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 3:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/44810/comment/2d940e80_01c2be06 
PS1, Line 12: chips supports
> `chip supports` or `chips support`
> Done

[Arthur Heymans (Code Review)]

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons, Patrick Rudolph.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 3
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Felix Singer <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Thomas Heijligen <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Nico Huber <[e-mail]>
Gerrit-Attention: Thomas Heijligen <[e-mail]>
Gerrit-Attention: Angel Pons <[e-mail]>
Gerrit-Attention: Patrick Rudolph <[e-mail]>
Gerrit-Comment-Date: Mon, 23 May 2022 14:21:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons, Patrick Rudolph.
Nico Huber has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................

flashchips: Fix W25Q256.W

The JW is the only known variant. A W25Q256FW may have existed with
less 4BA instructions supported, but it never showed up and no data-
sheet is available.

Used the datasheet from here:
https://www.winbond.com/resource-files/w25q256jw%20spi%20revb%2012082017.pdf

Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Signed-off-by: Patrick Rudolph <[e-mail]>
Signed-off-by: Nico Huber <[e-mail]>
Ticket: https://ticket.coreboot.org/issues/362
---
M flashchips.c
1 file changed, 8 insertions(+), 3 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/44810/4
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 4
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Felix Singer <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Thomas Heijligen <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Patrick Rudolph <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Nico Huber <[e-mail]>
Gerrit-Attention: Thomas Heijligen <[e-mail]>
Gerrit-Attention: Angel Pons <[e-mail]>
Gerrit-Attention: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newpatchset

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/44810 )

Change subject: flashchips: Fix W25Q256.W
......................................................................

flashchips: Fix W25Q256.W

The JW is the only known variant. A W25Q256FW may have existed with
less 4BA instructions supported, but it never showed up and no data-
sheet is available.

Used the datasheet from here:
https://www.winbond.com/resource-files/w25q256jw%20spi%20revb%2012082017.pdf

Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Signed-off-by: Patrick Rudolph <[e-mail]>
Signed-off-by: Nico Huber <[e-mail]>
Ticket: https://ticket.coreboot.org/issues/362
Reviewed-on: https://review.coreboot.org/c/flashrom/+/44810
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Arthur Heymans <[e-mail]>
---
M flashchips.c
1 file changed, 8 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Arthur Heymans: Looks good to me, approved



diff --git a/flashchips.c b/flashchips.c
index 82bb5b5..a9fd1bf 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -17597,7 +17597,7 @@
 
 	{
 		.vendor		= "Winbond",
-		.name		= "W25Q256.W",
+		.name		= "W25Q256JW",
 		.bustype	= BUS_SPI,
 		.manufacture_id	= WINBOND_NEX_ID,
 		.model_id	= WINBOND_NEX_W25Q256_W,
@@ -17605,8 +17605,7 @@
 		.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,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA,
 		.tested		= TEST_OK_PREW,
 		.probe		= probe_spi_rdid,
 		.probe_timing	= TIMING_ZERO,
@@ -17614,12 +17613,18 @@
 		{
 			{
 				.eraseblocks = { {4 * 1024, 8192} },
+				.block_erase = spi_block_erase_21,
+			}, {
+				.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_dc,
+			}, {
+				.eraseblocks = { {64 * 1024, 512} },
 				.block_erase = spi_block_erase_d8,
 			}, {
 				.eraseblocks = { {32 * 1024 * 1024, 1} },



3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/44810
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a3995c66ad7b74823e17984bf1ffac50b5663e0
Gerrit-Change-Number: 44810
Gerrit-PatchSet: 6
Gerrit-Owner: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Felix Singer <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Thomas Heijligen <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Edward O'Callaghan <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: merged