Change in flashrom[master]: add support to SPI flash MX25L51245G

[Name of user not set (Code Review)]

Name of user not set #1002484 has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: add support to SPI flash MX25L51245G
......................................................................

add support to SPI flash MX25L51245G

Change-Id: I964e630197e33d69b199fdfb8816f18e3112bbb1
Signed-off-by: Hemanth Guruva Reddy 
---
M flashchips.c
M flashchips.h
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/34234/1

diff --git a/flashchips.c b/flashchips.c
index 7f31c30..9aff06f 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -7778,6 +7778,45 @@

{
    	.vendor		= "Macronix",
+		.name		= "MX25L51245G",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= MACRONIX_ID,
+		.model_id	= MACRONIX_MX25L51245G,
+		.total_size	= 65536,
+		.page_size	= 256,
+		/* MX25L51245G supports SFDP */
+		.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_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 2048} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 1024} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {65536 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {65536 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			},
+		},
+		.printlock	= spi_prettyprint_status_register_bp3_srwd,
+		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read, /* Fast read (0x0B) supported, MX25L51245G supports dual I/O */
+		.voltage	= {2700, 3600}, /* 2.35-3.6V for MX25L51245G */
+	},
+
+	{
+		.vendor		= "Macronix",
    	.name		= "MX25L1005(C)/MX25L1006E",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= MACRONIX_ID,
diff --git a/flashchips.h b/flashchips.h
index 1c7ba99..cbc22ed 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -479,6 +479,7 @@
  * Generalplus SPI chips seem to be compatible with Macronix
  * and use the same set of IDs. */
 #define MACRONIX_MX25L512	0x2010	/* Same as MX25L512E, MX25V512, MX25V512C */
+#define MACRONIX_MX25L51245G	0x201A
 #define MACRONIX_MX25L1005	0x2011	/* Same as MX25L1005C, MX25L1006E */
 #define MACRONIX_MX25L2005	0x2012	/* Same as MX25L2005C, MX25L2006E */
 #define MACRONIX_MX25L4005	0x2013	/* Same as MX25L4005A, MX25L4005C, MX25L4006E */

[Hemanth Guruva Reddy (Code Review)]

Hemanth Guruva Reddy has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: add support to SPI flash MX25L51245G
......................................................................

Patch Set 1: Code-Review+1

I see that your one of the maintainer of flashrom, I request for a review for addition of this flash type.

[Hemanth Guruva Reddy (Code Review)]

Hemanth Guruva Reddy has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: add support to SPI flash MX25L51245G
......................................................................

Patch Set 1:

Hi Nico,
Kindly request for a review of the patch to upstream the flash support

[Angel Pons (Code Review)]

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

Change subject: add support to SPI flash MX25L51245G
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

Hi, welcome and thanks for sending this change!

Looks pretty good, just a minor suggestion about the commit summary (which you can easily address with the gerrit web interface).
> Patch Set 1: Code-Review+1
>
> I see that your one of the maintainer of flashrom, I request for a review for addition of this flash type.

Usually, giving code-review scores to one's own changes is discouraged (the idea is that others review and give a score to your change). But as this is your first change in gerrit, I understand it's just a rookie mistake, no big deal :)

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

https://review.coreboot.org/c/flashrom/+/34234/1//COMMIT_MSG@7 
PS1, Line 7: add support to SPI flash MX25L51245G
I would suggest the following commit summary:

flashchips: Add Macronix MX25L51245G

(You can use the "Edit" button on the gerrit change's page, under the commit message)

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Hemanth Guruva Reddy (Code Review)]

Hello Angel Pons, build bot (Jenkins), Nico Huber, Sean Nelson,

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

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

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

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

flashchips: Add Macronix MX25L51245G

Change-Id: I964e630197e33d69b199fdfb8816f18e3112bbb1
Signed-off-by: Hemanth Guruva Reddy 
---
M flashchips.c
M flashchips.h
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/34234/2

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Hemanth Guruva Reddy (Code Review)]

Hemanth Guruva Reddy has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

Patch Set 2:
> Patch Set 1: Code-Review+2
>
> (1 comment)
>
> Hi, welcome and thanks for sending this change!
>
> Looks pretty good, just a minor suggestion about the commit summary (which you can easily address with the gerrit web interface).
> > Patch Set 1: Code-Review+1
> >
> > I see that your one of the maintainer of flashrom, I request for a review for addition of this flash type.
>
> Usually, giving code-review scores to one's own changes is discouraged (the idea is that others review and give a score to your change). But as this is your first change in gerrit, I understand it's just a rookie mistake, no big deal :)

Point noted. Usually some projects request a +1 as a way of communicating the change was tested locally with hardware :)

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

Patch Set 2:

(1 comment)
> Patch Set 2:
> > Patch Set 1: Code-Review+2
> >
> > (1 comment)
> >
> > Hi, welcome and thanks for sending this change!
> >
> > Looks pretty good, just a minor suggestion about the commit summary (which you can easily address with the gerrit web interface).
> > > Patch Set 1: Code-Review+1
> > >
> > > I see that your one of the maintainer of flashrom, I request for a review for addition of this flash type.
> >
> > Usually, giving code-review scores to one's own changes is discouraged (the idea is that others review and give a score to your change). But as this is your first change in gerrit, I understand it's just a rookie mistake, no big deal :)
>
> Point noted. Usually some projects request a +1 as a way of communicating the change was tested locally with hardware :)

Ack, here we usually state that in the commit message. But for flash chips this can be reflected on the code.

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7789 
PS2, Line 7789: 		.tested		= TEST_OK_PREW,
This tells me that probe, read, erase and write are working (or should be)

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Nico Huber (Code Review)]

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

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

Patch Set 2:

(3 comments)

Sorry for being late. There are some rather invisible issues,
hard to get right for the first chip:

* We currently try to align the flashchips.c with the Chromium fork, so entries have to stay 
sorted by .vendor and .name.
* Our 4BA support assumes that 4BA erase functions are listed explicitly and before each regular erase function of the same size.

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7789 
PS2, Line 7789: 		.tested		= TEST_OK_PREW,
> This tells me that probe, read, erase and write are working (or should be)

What was tested should be mentioned in the commit message.

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7796 
PS2, Line 7796: 				.block_erase = spi_block_erase_20,
There should also be native 4-byte address (4BA) functions: 21/5c/dc.
The 4BA should be mentioned first. See MX25L25635F, for instance.

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7815 
PS2, Line 7815: /* 2.35-3.6V for MX25L51245G */
The datasheet says 2.7V, and why is this a comment if the whole entry is for
the MX25L51245G?

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Nico Huber (Code Review)]

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

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.h 
File flashchips.h:

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.h@482 
PS2, Line 482: #define MACRONIX_MX25L51245G	0x201A
Sigh, now I discovered another issue, the same ID is used by MX66L51235F below.

And looking at its entry in flashchips.c, it seems compatible. If it has the
same values everywhere, the two should get merged and we'd only have to change
the .name to "MX66L51235F/MX25L51245G".

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G

[Alan Green (Code Review)]

Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: flashchips: Add Macronix MX25L51245G
......................................................................

Patch Set 2:

Thanks for making sure about the ordering.

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Hemanth Guruva Reddy (Code Review)]

Hello Angel Pons, Alan Green, build bot (Jenkins), Nico Huber, Sean Nelson,

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

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

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

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

flashchips: Add Macronix MX25L51245G as known chip

MX25L51245G is identical to handling of MX66L51235F

Change-Id: I964e630197e33d69b199fdfb8816f18e3112bbb1
Signed-off-by: Hemanth Guruva Reddy 
---
M flashchips.c
M flashchips.h
2 files changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/34234/3

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Hemanth Guruva Reddy (Code Review)]

Hemanth Guruva Reddy has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

Patch Set 3:

(4 comments)

Hi Nico,

Thanks for a quick point out, Didn't realize that from flashrom perspective MX66L51235F == MX25L51245G. Despite the MX25L51245G brings in extended command set for Double-Transfer Read commands. adapted the .name of MX66L51235F as the only change needed.

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.h 
File flashchips.h:

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.h@482 
PS2, Line 482: #define MACRONIX_MX25L51245G	0x201A
> Sigh, now I discovered another issue, the same ID is used by MX66L51235F below. […]

Done

Thanks for a quick point out, Didn't realize that from flashrom perspective MX66L51235F == MX25L51245G. Despite the MX25L51245G brings in extended command set for Double-Transfer Read commands

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7789 
PS2, Line 7789: 		.tested		= TEST_OK_PREW,
> What was tested should be mentioned in the commit message.

Done

This is not necessary anymore

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7796 
PS2, Line 7796: 				.block_erase = spi_block_erase_20,
> There should also be native 4-byte address (4BA) functions: 21/5c/dc. […]

Done

This is not necessary anymore

https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7815 
PS2, Line 7815: /* 2.35-3.6V for MX25L51245G */
> The datasheet says 2.7V, and why is this a comment if the whole entry is for […]

Done

This is not necessary anymore

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Nico Huber (Code Review)]

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

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

Patch Set 3: Code-Review+2

(2 comments)

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

https://review.coreboot.org/c/flashrom/+/34234/3//COMMIT_MSG@9 
PS3, Line 9: MX25L51245G is identical to handling of MX66L51235F
Full stop at the end, please.

https://review.coreboot.org/c/flashrom/+/34234/3/flashchips.h 
File flashchips.h:

https://review.coreboot.org/c/flashrom/+/34234/3/flashchips.h@493 
PS3, Line 493: MX66L51235F
This is the same name as on the left. I had too look twice. I guess the
original comment shouldn't have been there in the first place.

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Hemanth Guruva Reddy (Code Review)]

Hello Angel Pons, build bot (Jenkins), Alan Green, Nico Huber, Sean Nelson,

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

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

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

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

flashchips: Add Macronix MX25L51245G as known chip

MX25L51245G is identical to handling of MX66L51235F.

Change-Id: I964e630197e33d69b199fdfb8816f18e3112bbb1
Signed-off-by: Hemanth Guruva Reddy 
---
M flashchips.c
M flashchips.h
2 files changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/34234/4

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Hemanth Guruva Reddy (Code Review)]

Hemanth Guruva Reddy has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

Patch Set 4:

(2 comments)

DONE

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

https://review.coreboot.org/c/flashrom/+/34234/3//COMMIT_MSG@9 
PS3, Line 9: MX25L51245G is identical to handling of MX66L51235F
> Full stop at the end, please.

Done

https://review.coreboot.org/c/flashrom/+/34234/3/flashchips.h 
File flashchips.h:

https://review.coreboot.org/c/flashrom/+/34234/3/flashchips.h@493 
PS3, Line 493: MX66L51235F
> This is the same name as on the left. I had too look twice. I guess the […]

Done

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Nico Huber (Code Review)]

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

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

Patch Set 4: Code-Review+2

Change in flashrom[master]: flashchips: Add Macronix MX25L51245G as known chip

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/34234 )

Change subject: flashchips: Add Macronix MX25L51245G as known chip
......................................................................

flashchips: Add Macronix MX25L51245G as known chip

MX25L51245G is identical to handling of MX66L51235F.

Change-Id: I964e630197e33d69b199fdfb8816f18e3112bbb1
Signed-off-by: Hemanth Guruva Reddy 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/34234
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M flashchips.c
M flashchips.h
2 files changed, 2 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/flashchips.c b/flashchips.c
index 7f31c30..166af6a 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -9441,7 +9441,7 @@

{
    	.vendor		= "Macronix",
-		.name		= "MX66L51235F",
+		.name		= "MX66L51235F/MX25L51245G",
    	.bustype	= BUS_SPI,
    	.manufacture_id	= MACRONIX_ID,
    	.model_id	= MACRONIX_MX66L51235F,
diff --git a/flashchips.h b/flashchips.h
index 1c7ba99..006b95e 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -490,7 +490,7 @@
 #define MACRONIX_MX25L25635F	0x2019	/* Same as MX25L25639F, but the latter seems to not support REMS */
 #define MACRONIX_MX25L1635D	0x2415
 #define MACRONIX_MX25L1635E	0x2515	/* MX25L1635{E} */
-#define MACRONIX_MX66L51235F	0x201a	/* MX66L51235F */
+#define MACRONIX_MX66L51235F	0x201a	/* MX66L51235F, MX25L51245G */
 #define MACRONIX_MX25U8032E	0x2534
 #define MACRONIX_MX25U1635E	0x2535
 #define MACRONIX_MX25U3235E	0x2536	/* Same as MX25U6435F */