Change in flashrom[master]: flashchips: add W25M512JV

[Joel Stanley (Code Review)]

Joel Stanley has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: add W25M512JV
......................................................................

flashchips: add W25M512JV

https://www.winbond.com/resource-files/w25m512jv%20revd%2009052017.pdf

Change-Id: I8d16f0918785795cc49500435a03641b87d706e9
Signed-off-by: Joel Stanley 
---
M flashchips.c
M flashchips.h
2 files changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/34595/1

diff --git a/flashchips.c b/flashchips.c
index 166af6a..304ebe7 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -15902,6 +15902,25 @@

{
    	.vendor		= "Winbond",
+		.name		= "W25Q512JV",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= WINBOND_NEX_ID,
+		.model_id	= WINBOND_NEX_W25Q512JV,
+		.total_size	= 64 * 1024,
+		.page_size	= 1024,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.printlock	= spi_prettyprint_status_register_plain,
+		.unlock		= spi_disable_blockprotect,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {2700, 3600},
+	},
+
+	{
+		.vendor		= "Winbond",
    	.name		= "W25Q256JV_M",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= WINBOND_NEX_ID,
diff --git a/flashchips.h b/flashchips.h
index 006b95e..c606161 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -921,6 +921,7 @@
 #define WINBOND_NEX_W25Q64_V	0x4017	/* W25Q64BV, W25Q64CV; W25Q64FV in SPI mode (default) */
 #define WINBOND_NEX_W25Q128_V	0x4018	/* W25Q128BV; W25Q128FV in SPI mode (default) */
 #define WINBOND_NEX_W25Q256_V	0x4019	/* W25Q256FV or W25Q256JV_Q (QE=1) */
+#define WINBOND_NEX_W25Q512JV	0x4020	/* W25Q512JV */
 #define WINBOND_NEX_W25Q20_W	0x5012	/* W25Q20BW */
 #define WINBOND_NEX_W25Q40BW	0x5013	/* W25Q40BW */
 #define WINBOND_NEX_W25Q80BW	0x5014	/* W25Q80BW */

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: add W25M512JV
......................................................................

Patch Set 1:

(5 comments)

Hi Joel,

this seems like work-in-progress? I've left some
comments anyway. There seems to be a little con-
fusion between W25M512JV and W25Q512JV. Adding
the latter would be easier and seems more likely,
but you should look for the correct datasheet.

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

https://review.coreboot.org/c/flashrom/+/34595/1//COMMIT_MSG@7 
PS1, Line 7: W25M512JV
Code says W25*Q*512JV

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c 
File flashchips.c:

PS1: 
Please keep entries in this file sorted by `.vendor` and `.name`.

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15910 
PS1, Line 15910: 1024
Datasheet says 256 as usual?

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15912 
PS1, Line 15912: 		.tested		= TEST_OK_PREW,
Seems like this needs an update.

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15914 
PS1, Line 15914: 		.probe_timing	= TIMING_ZERO,
You'll have to add some erase functions to `.block_erasers`. They
are mostly the same across flash-chip families, i.e. those for
W25Q256JV should work here, too. Only the block counts / size
for bulk erase need to be adapted.

[Joel Stanley (Code Review)]

Joel Stanley has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: add W25M512JV
......................................................................

Patch Set 1:

(5 comments)

Thanks for the review. I needed to recover a system again today which reminded me to revisit this. I'll upload a v2 that I've tested works.

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

https://review.coreboot.org/c/flashrom/+/34595/1//COMMIT_MSG@7 
PS1, Line 7: W25M512JV
> Code says W25*Q*512JV

Done

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c 
File flashchips.c:

PS1:
> Please keep entries in this file sorted by `.vendor` and `.name`.

Done

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15910 
PS1, Line 15910: 1024
> Datasheet says 256 as usual?

Done

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15912 
PS1, Line 15912: 		.tested		= TEST_OK_PREW,
> Seems like this needs an update.

I don't understand, can you explain what you mean?

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15914 
PS1, Line 15914: 		.probe_timing	= TIMING_ZERO,
> You'll have to add some erase functions to `.block_erasers`. They […]

Done

Change in flashrom[master]: flashchips: Add W25Q512JV

[Joel Stanley (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: flashchips: Add W25Q512JV
......................................................................

flashchips: Add W25Q512JV

https://www.winbond.com/resource-files/W25Q512JV%20DTR%20RevB%2006132019%201...

Tested with dediprog SF100.

Change-Id: I8d16f0918785795cc49500435a03641b87d706e9
Signed-off-by: Joel Stanley 
---
M flashchips.c
M flashchips.h
2 files changed, 45 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/34595/2

Change in flashrom[master]: flashchips: Add W25Q512JV

[Philippe Mathieu-Daudé (Code Review)]

Philippe Mathieu-Daudé has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2: Code-Review+1

Change in flashrom[master]: flashchips: Add W25Q512JV

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15912 
PS1, Line 15912: 		.tested		= TEST_OK_PREW,
> I don't understand, can you explain what you mean?

I'm not sure if I follow either. If you have tested that probe, read, erase and write work, then it's all good.

Change in flashrom[master]: flashchips: Add W25Q512JV

[Joel Stanley (Code Review)]

Joel Stanley has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2:

Thanks for the reviews. Is this good to be merged now?

Change in flashrom[master]: flashchips: Add W25Q512JV

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Thanks for the reviews. Is this good to be merged now?

Have you successfully tested probe/read/erase/write? If so, then it's good to go.

Change in flashrom[master]: flashchips: Add W25Q512JV

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15912 
PS1, Line 15912: 		.tested		= TEST_OK_PREW,
> I don't understand, can you explain what you mean?

Erase was definitely not working with PS1, so I doubted that it was tested
at all. PS2 looks good.

Change in flashrom[master]: flashchips: Add W25Q512JV

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add W25Q512JV
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34595/1/flashchips.c@15912 
PS1, Line 15912: 		.tested		= TEST_OK_PREW,
> > I don't understand, can you explain what you mean? […]

Just checked, looks good as well.

Change in flashrom[master]: flashchips: Add W25Q512JV

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/34595 )

Change subject: flashchips: Add W25Q512JV
......................................................................

flashchips: Add W25Q512JV

https://www.winbond.com/resource-files/W25Q512JV%20DTR%20RevB%2006132019%201...

Tested with dediprog SF100.

Change-Id: I8d16f0918785795cc49500435a03641b87d706e9
Signed-off-by: Joel Stanley 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/34595
Tested-by: build bot (Jenkins) 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Angel Pons 
---
M flashchips.c
M flashchips.h
2 files changed, 45 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philippe Mathieu-Daudé: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/flashchips.c b/flashchips.c
index 648fed4..64fdbb9 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -17006,6 +17006,50 @@

{
    	.vendor		= "Winbond",
+		.name		= "W25Q512JV",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= WINBOND_NEX_ID,
+		.model_id	= WINBOND_NEX_W25Q512JV,
+		.total_size	= 64 * 1024,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 16384} },
+				.block_erase = spi_block_erase_21,
+			}, {
+				.eraseblocks = { {4 * 1024, 16384} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 2048} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 1024} },
+				.block_erase = spi_block_erase_dc,
+			}, {
+				.eraseblocks = { {64 * 1024, 1024} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {64 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {64 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain,
+		.unlock		= spi_disable_blockprotect,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {2700, 3600},
+	},
+
+	{
+		.vendor		= "Winbond",
    	.name		= "W25Q64.V",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= WINBOND_NEX_ID,
diff --git a/flashchips.h b/flashchips.h
index dc04a5a..29294fb 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -931,6 +931,7 @@
 #define WINBOND_NEX_W25Q64_V	0x4017	/* W25Q64BV, W25Q64CV; W25Q64FV in SPI mode (default) */
 #define WINBOND_NEX_W25Q128_V	0x4018	/* W25Q128BV; W25Q128FV in SPI mode (default) */
 #define WINBOND_NEX_W25Q256_V	0x4019	/* W25Q256FV or W25Q256JV_Q (QE=1) */
+#define WINBOND_NEX_W25Q512JV	0x4020	/* W25Q512JV */
 #define WINBOND_NEX_W25Q20_W	0x5012	/* W25Q20BW */
 #define WINBOND_NEX_W25Q40BW	0x5013	/* W25Q40BW */
 #define WINBOND_NEX_W25Q80BW	0x5014	/* W25Q80BW */