Change in ...flashrom[master]: Add support for security register and SBULK command on Macronix chips

[vl_sk (Code Review)]

vl_sk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/32034

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Add support for security register and SBULK command on Macronix chips

Signed-off-by: Vladimir Skokov 
Change-Id: I6b3d7bd2625dec5fda7b85f421d40c49bbc528a6
---
M chipdrivers.h
M flashchips.c
M spi.h
M spi25_statusreg.c
4 files changed, 78 insertions(+), 24 deletions(-)

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

diff --git a/chipdrivers.h b/chipdrivers.h
index 134ed26..54288cd 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -102,7 +102,9 @@
 int spi_prettyprint_status_register_sst25(struct flashctx *flash);
 int spi_prettyprint_status_register_sst25vf016(struct flashctx *flash);
 int spi_prettyprint_status_register_sst25vf040b(struct flashctx *flash);
+int spi_prettyprint_security_register_mx25(struct flashctx *flash);
 int spi_disable_blockprotect_sst26_global_unprotect(struct flashctx *flash);
+int spi_disable_blockprotect_mx25_global_unprotect(struct flashctx *flash);

/* sfdp.c */
 int probe_spi_sfdp(struct flashctx *flash);
diff --git a/flashchips.c b/flashchips.c
index d85ea74..2c662e9 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -8555,9 +8555,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register and SBLK/SBULK; MX25L12835F: configuration register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) supported */
    	.voltage	= {2700, 3600},
@@ -8604,9 +8603,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register and SBLK/SBULK; MX25L12835F: configuration register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) supported */
    	.voltage	= {2700, 3600},
@@ -8693,9 +8691,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
@@ -8734,9 +8731,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
@@ -8776,9 +8772,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
@@ -8818,9 +8813,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256,
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
@@ -8858,9 +8852,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256, /* Multi I/O supported */
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
@@ -8907,9 +8900,8 @@
    			.block_erase = spi_block_erase_c7,
    		}
    	},
-		/* TODO: security register */
-		.printlock	= spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */
-		.unlock		= spi_disable_blockprotect_bp3_srwd,
+		.printlock	= spi_prettyprint_security_register_mx25,
+		.unlock		= spi_disable_blockprotect_mx25_global_unprotect,
    	.write		= spi_chip_write_256, /* Multi I/O supported */
    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
    	.voltage	= {1650, 2000},
diff --git a/spi.h b/spi.h
index 422e6a0..456d894 100644
--- a/spi.h
+++ b/spi.h
@@ -170,6 +170,15 @@
    From ANY mode (3-bytes or 4-bytes) it works with 4-byte address */
 #define JEDEC_BYTE_PROGRAM_4BA	0x12

+/* Read Security Register */
+#define JEDEC_RDSCUR		0x2B
+#define JEDEC_RDSCUR_OUTSIZE	0x01
+#define JEDEC_RDSCUR_INSIZE		0x01
+
+/* Do GBULK (gang block unlock) */
+#define JEDEC_GBULK		0x98
+#define JEDEC_GBULK_OUTSIZE	0x01
+
 /* Error codes */
 #define SPI_GENERIC_ERROR	-1
 #define SPI_INVALID_OPCODE	-2
diff --git a/spi25_statusreg.c b/spi25_statusreg.c
index 4cf7023..4cdf800 100644
--- a/spi25_statusreg.c
+++ b/spi25_statusreg.c
@@ -237,6 +237,28 @@
    return spi_disable_blockprotect_generic(flash, 0x7C, 1 << 7, 0, 0xFF);
 }

+int spi_disable_blockprotect_mx25_global_unprotect(struct flashctx *flash)
+{
+	static const unsigned char cmd[JEDEC_GBULK_OUTSIZE] = { JEDEC_GBULK };
+	int ret;
+
+	ret = spi_write_enable(flash);
+	if (ret) {
+		msg_cerr("Set WREN failed!\n");
+		return -1;
+	}
+
+	/* Send GBULK cmd to device. */
+	ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
+	if (ret) {
+		msg_cerr("GBULK failed!\n");
+		return -1;
+	}
+
+	/* Disable standard blockprotect */
+	return spi_disable_blockprotect_bp3_srwd(flash);
+}
+
 static void spi_prettyprint_status_register_hex(uint8_t status)
 {
    msg_cdbg("Chip status register is 0x%02x.\n", status);
@@ -751,3 +773,32 @@
    msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]);
    return 0;
 }
+
+/* === Macronix chips with Security Register === */
+
+int spi_read_security_register(struct flashctx *flash)
+{
+	static const unsigned char cmd[JEDEC_RDSCUR_OUTSIZE] = { JEDEC_RDSCUR };
+	unsigned char readarr[JEDEC_RDSCUR_INSIZE];
+	int ret;
+
+	ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd, readarr);
+	if (ret) {
+		msg_cerr("RDSCUR failed!\n");
+		return 0;
+	}
+
+	return readarr[0];
+}
+
+int spi_prettyprint_security_register_mx25(struct flashctx *flash)
+{
+	int security_register = spi_read_security_register(flash);
+	int wpsel = !!(security_register & (1 << 7));
+
+	msg_cdbg("Chip security register: 0x%02X\n", security_register);
+	msg_cdbg("\tWPSEL is %s\n", wpsel ? "ON" : "OFF");
+
+	/* Print standard blockprotect info */
+	return spi_prettyprint_status_register_bp3_srwd(flash);
+}
\ No newline at end of file

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32034 )

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32034/1/spi.h 
File spi.h:

https://review.coreboot.org/#/c/32034/1/spi.h@176 
PS1, Line 176: 		
1 tab ?

[vl_sk (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Add support for security register and SBULK command on Macronix chips

Signed-off-by: Vladimir Skokov 
Change-Id: I6b3d7bd2625dec5fda7b85f421d40c49bbc528a6
---
M chipdrivers.h
M flashchips.c
M spi.h
M spi25_statusreg.c
4 files changed, 78 insertions(+), 24 deletions(-)

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

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32034 )

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

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

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Patch Set 2: Code-Review+2

(3 comments)

Thanks for you patch, and sorry for the delay. It somehow
went under my radar.

I've commented inline on some nits, if you don't mind to
fix them.

https://review.coreboot.org/#/c/32034/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32034/2//COMMIT_MSG@8 
PS2, Line 8: 
Would be nice to have one or two sentences about WPSEL and DPB
here. Not everybody is an expert in macronix chips :)

https://review.coreboot.org/#/c/32034/2/spi25_statusreg.c 
File spi25_statusreg.c:

https://review.coreboot.org/#/c/32034/2/spi25_statusreg.c@242 
PS2, Line 242: 	static const unsigned char cmd[JEDEC_GBULK_OUTSIZE] = { JEDEC_GBULK };
             : 	int ret;
             : 
             : 	ret = spi_write_enable(flash);
             : 	if (ret) {
             : 		msg_cerr("Set WREN failed!\n");
             : 		return -1;
             : 	}
             : 
             : 	/* Send GBULK cmd to device. */
             : 	ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
There is spi_simple_write_cmd() (currently static in spi25.c) that
serves this pattern. I wouldn't mind to export it for this purpose.

https://review.coreboot.org/#/c/32034/2/spi25_statusreg.c@788 
PS2, Line 788: 		return 0;
please return a negative error code and check for it below

Change in flashrom[master]: Add support for security register and SBULK command on Macronix chips

[Nico Huber (Code Review)]

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

Change subject: Add support for security register and SBULK command on Macronix chips
......................................................................

Patch Set 4: Code-Review+1

Needs a careful rebase now that flashchips.c is sorted.