Change in flashrom[master]: flashrom: Added Support for BoyaMicro BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashrom: Added Support for BoyaMicro BY25Q128AS
......................................................................

flashrom: Added Support for BoyaMicro BY25Q128AS

Tested on Buspirate

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

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/1

diff --git a/flashchips.c b/flashchips.c
index 09ac9b4..5c60b2c 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -3442,6 +3442,44 @@
    	.voltage	= {3000, 3600},
    },

+    	{
+		.vendor		= "Boya",
+		.name		= "BY25Q128AS",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= BOYA_ID,
+		.model_id	= BOYA_BY25Q128AS,
+		.total_size	= 16384,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 4096} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 512} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 256} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain,
+		.unlock		= spi_disable_blockprotect_at25fs040,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {2700, 3600},
+	},
+
    {
    	.vendor		= "Bright",
    	.name		= "BM29F040",
diff --git a/flashchips.h b/flashchips.h
index 4ae6c07..9d19ef5 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -207,6 +207,10 @@
 #define ATMEL_AT49F080		0x23
 #define ATMEL_AT49F080T		0x27

+/* Boya Microelectronics Inc.*/
+#define BOYA_ID        		0x68
+#define BOYA_BY25Q128AS    	0x4018
+
 /* Bright Microelectronics has the same manufacturer ID as Hyundai... */
 #define BRIGHT_ID		0xAD	/* Bright Microelectronics */
 #define BRIGHT_BM29F040		0x40

[Angel Pons (Code Review)]

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

Change subject: flashrom: Added Support for BoyaMicro BY25Q128AS
......................................................................

Patch Set 1: Code-Review+1

(7 comments)

Welcome!

Overall, looks good, but with some minor nits about formatting.

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

https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@7 
PS1, Line 7: flashrom: Added Support for BoyaMicro BY25Q128AS
A few nits about the commit summary:

- The `prefix` often refers to the touched files. Here, `flashchips` would be good (both flashchips.c and flashchips.h are touched).
- Commit summaries are written in imperative tense: `Add support for ...`.
- `Support` shouldn't be capitalized.
- The vendor's name seems to be `Boya Microelectronics`.

Instead, I would use:

flashchips: Add support for Boya Microelectronics BY25Q128AS

Note that commit summaries never end with a period.

https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@9 
PS1, Line 9: Tested on Buspirate
missing period `.` at the end.

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

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@211 
PS1, Line 211:         
These spaces should be a tab (tabs are 8 characters wide in flashrom)

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@212 
PS1, Line 212:     
Same here, spaces

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

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3445 
PS1, Line 3445:     
more spaces

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3446 
PS1, Line 3446: Boya
nit: Boya Microelectronics

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3449 
PS1, Line 3449: BOYA_ID
the `BOYA_ID` macro name is fine, no need to change it.

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate

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

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/2

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate.

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

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/3

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate.

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

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/4

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4:

(6 comments)
> Patch Set 1: Code-Review+1
>
> (7 comments)
>
> Welcome!
>
> Overall, looks good, but with some minor nits about formatting.

Thank you for your review, feedback and insights into the standards of this project. I have applied the changes you suggested (I hope, still figuring out some of Gerrit's finer points :). Anyhow let me know if I can do anything else.

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

https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@7 
PS1, Line 7: flashrom: Added Support for BoyaMicro BY25Q128AS
> A few nits about the commit summary: […]

Done

https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@9 
PS1, Line 9: Tested on Buspirate
> missing period `.` at the end.

Done

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

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@211 
PS1, Line 211:
> These spaces should be a tab (tabs are 8 characters wide in flashrom)

Done

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@212 
PS1, Line 212:
> Same here, spaces

Done

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

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3445 
PS1, Line 3445:
> more spaces

Done

https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3446 
PS1, Line 3446: Boya
> nit: Boya Microelectronics

Done

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4: Code-Review+1

(1 comment)
> Patch Set 4:
>
> (6 comments)
> > Patch Set 1: Code-Review+1
> >
> > (7 comments)
> >
> > Welcome!
> >
> > Overall, looks good, but with some minor nits about formatting.
>
> Thank you for your review, feedback and insights into the standards of this project. I have applied the changes you suggested (I hope, still figuring out some of Gerrit's finer points :). Anyhow let me know if I can do anything else.

I'm pleased to help 😊

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c@3477 
PS4, Line 3477: spi_disable_blockprotect_at25fs040
Just to be sure, how have you tested this? I haven't messed with flash chip block protection much (and flashrom is rather dumb about it)

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4:

Yes. I initially had used spi_disable_blockprotect, but was told on GitHub at25fs040 was more appropriate, so I changed it, did another dump, worked just fine.

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c@3477 
PS4, Line 3477: spi_disable_blockprotect_at25fs040
> Just to be sure, how have you tested this? I haven't messed with flash chip block protection much (a […]

Yes. I initially had used spi_disable_blockprotect, but was told on GitHub at25fs040 was more appropriate, so I changed it, did another dump, worked just fine.

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Paul Menzel (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4:

a

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[David Hendricks (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 4: Code-Review-1

(2 comments)

Looks good to me, other than the Signed-off-by line

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG@11 
PS4, Line 11: Change-Id: I881ba86cfaa82e43c73360135d47c74d896cc191
Add your sign-off (https://flashrom.org/Development_Guidelines#Sign-off_Procedure)

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c@3477 
PS4, Line 3477: spi_disable_blockprotect_at25fs040
> Yes. […]

Yes, I had suggested that after looking at the datasheet and comparing it with the unlock functions.

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

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

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

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate.

Signed-off-by: Jack Olsen

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

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/5

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 5:

(1 comment)

Does this work for y'all?

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG@11 
PS4, Line 11: Change-Id: I881ba86cfaa82e43c73360135d47c74d896cc191
> Add your sign-off (https://flashrom. […]

Done

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c 
File flashchips.c:

https://review.coreboot.org/c/flashrom/+/44308/4/flashchips.c@3477 
PS4, Line 3477: spi_disable_blockprotect_at25fs040
> Yes, I had suggested that after looking at the datasheet and comparing it with the unlock functions.

👍 Thank you!

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 5:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/44308/5//COMMIT_MSG@12 
PS5, Line 12: 
nit: No space between the sign-off and the Change-Id

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

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

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

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate.

Signed-off-by: Jack Olsen 
Change-Id: I881ba86cfaa82e43c73360135d47c74d896cc191
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/44308/6

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 7:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/44308/5//COMMIT_MSG@12 
PS5, Line 12:
> nit: No space between the sign-off and the Change-Id

Done

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Jack Olsen (Code Review)]

Jack Olsen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 7:

(1 comment)
> Patch Set 4: Code-Review-1
>
> (2 comments)
>
> Looks good to me, other than the Signed-off-by line

Done! Are you able to remove the code review down vote?

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44308/4//COMMIT_MSG@11 
PS4, Line 11: Change-Id: I881ba86cfaa82e43c73360135d47c74d896cc191
> Done

I have fixed this can you please remove the down vote? Or tell me what else I need to do?

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Angel Pons (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 8: Code-Review+2
> Patch Set 7:
>
> (1 comment)
> > Patch Set 4: Code-Review-1
> >
> > (2 comments)
> >
> > Looks good to me, other than the Signed-off-by line
>
> Done! Are you able to remove the code review down vote?

It's not blocking submit. I'm not sure which timezone David uses, but I'd expect him to reply today.

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 8: Code-Review+2

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[David Hendricks (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

flashchips: Add support for Boya Microelectronics BY25Q128AS

Tested on Buspirate.

Signed-off-by: Jack Olsen 
Change-Id: I881ba86cfaa82e43c73360135d47c74d896cc191
Reviewed-on: https://review.coreboot.org/c/flashrom/+/44308
Reviewed-by: Angel Pons 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M flashchips.c
M flashchips.h
2 files changed, 42 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved

Objections:
  David Hendricks: I would prefer that you didn't submit this

diff --git a/flashchips.c b/flashchips.c
index 48efb85..c4ba4c6 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -3443,6 +3443,44 @@
    },

{
+		.vendor		= "Boya Microelectronics",
+		.name		= "BY25Q128AS",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= BOYA_ID,
+		.model_id	= BOYA_BY25Q128AS,
+		.total_size	= 16384,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 4096} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 512} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 256} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain,
+		.unlock		= spi_disable_blockprotect_at25fs040,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {2700, 3600},
+	},
+
+	{
    	.vendor		= "Bright",
    	.name		= "BM29F040",
    	.bustype	= BUS_PARALLEL,
diff --git a/flashchips.h b/flashchips.h
index 87aedc2..e9c0432 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -207,6 +207,10 @@
 #define ATMEL_AT49F080		0x23
 #define ATMEL_AT49F080T		0x27

+/* Boya Microelectronics Inc.*/
+#define BOYA_ID			0x68
+#define BOYA_BY25Q128AS		0x4018
+
 /* Bright Microelectronics has the same manufacturer ID as Hyundai... */
 #define BRIGHT_ID		0xAD	/* Bright Microelectronics */
 #define BRIGHT_BM29F040		0x40

Change in flashrom[master]: flashchips: Add support for Boya Microelectronics BY25Q128AS

[David Hendricks (Code Review)]

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

Change subject: flashchips: Add support for Boya Microelectronics BY25Q128AS
......................................................................

Patch Set 9: Code-Review+2