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 
---
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} },

[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

[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`

[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

[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 :-/

[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 
Signed-off-by: Nico Huber 
---
M flashchips.c
1 file changed, 8 insertions(+), 3 deletions(-)

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

[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

[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 
Signed-off-by: Nico Huber 
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

[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 
Signed-off-by: Nico Huber 
Ticket: https://ticket.coreboot.org/issues/362
Reviewed-on: https://review.coreboot.org/c/flashrom/+/44810
Tested-by: build bot (Jenkins) 
Reviewed-by: Arthur Heymans 
---
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.