Patchwork Return code for spi_read_status_register

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2012-06-03 14:51:18
Message ID <4FCB79E6.2030608@gmx.net>
Download mbox | patch
Permalink /patch/3646/
State Bitrotted
Headers show

Comments

Carl-Daniel Hailfinger - 2012-06-03 14:51:18
spi_read_status_register now returns success/failure and stores the
status register value via call-by-reference. That way, status register
reading failure (only possible if the programmer couldn't run RDSR) can
be detected and acted upon in the future.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Stefan Tauner - 2012-08-14 02:03:06
On Sun, 03 Jun 2012 16:51:18 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> spi_read_status_register now returns success/failure and stores the
> status register value via call-by-reference. That way, status register
> reading failure (only possible if the programmer couldn't run RDSR) can
> be detected and acted upon in the future.

with this patch this failure is largely ignored and i am not sure that
this does not change behavior due to the 0xff status value that is
returned on errors.

> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
> 
> Index: flashrom-spi_rdsr_errorcheck/it87spi.c
> ===================================================================
> --- flashrom-spi_rdsr_errorcheck/it87spi.c	(Revision 1539)
> +++ flashrom-spi_rdsr_errorcheck/it87spi.c	(Arbeitskopie)
> @@ -321,6 +321,7 @@
>  {
>  	unsigned int i;
>  	int result;
> +	uint8_t status;
>  	chipaddr bios = flash->virtual_memory;
>  
>  	result = spi_write_enable(flash);
> @@ -335,7 +336,7 @@
>  	/* Wait until the Write-In-Progress bit is cleared.
>  	 * This usually takes 1-10 ms, so wait in 1 ms steps.
>  	 */
> -	while (spi_read_status_register(flash) & SPI_SR_WIP)
> +	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
>  		programmer_delay(1000);
>  	return 0;

for example here it would be easy to store the return value of
spi_read_status_register and return it instead of the fixed 0

>  }
> Index: flashrom-spi_rdsr_errorcheck/a25.c
> ===================================================================
> --- flashrom-spi_rdsr_errorcheck/a25.c	(Revision 1539)
> +++ flashrom-spi_rdsr_errorcheck/a25.c	(Arbeitskopie)
> @@ -33,7 +33,7 @@
>  {
>  	uint8_t status;
>  
> -	status = spi_read_status_register(flash);
> +	spi_read_status_register(flash, &status);
>  	msg_cdbg("Chip status register is %02x\n", status);
here the output might change depending on what the programmers'
send_spi_command routine "return" in case of errors etc.

in general i like the idea very much, but i would rather see a
"complete" patch, that does acts appropriately on errors... try to
convince me if you disagree :)
Stefan Tauner - 2012-08-28 02:31:45
On Sun, 03 Jun 2012 16:51:18 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> only possible if the programmer couldn't run RDSR

or whatever read status register opcode it is... i miss this feature atm
while working on at45db ;)
Stefan Tauner - 2012-08-28 02:33:57
On Tue, 28 Aug 2012 04:31:45 +0200
Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote:

> On Sun, 03 Jun 2012 16:51:18 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
> 
> > only possible if the programmer couldn't run RDSR
> 
> or whatever read status register opcode it is... i miss this feature atm
> while working on at45db ;)
> 

hm no. what i actually miss is returning the status register value by
the pretty print/"unlock" function. *sigh*

Patch

Index: flashrom-spi_rdsr_errorcheck/it87spi.c
===================================================================
--- flashrom-spi_rdsr_errorcheck/it87spi.c	(Revision 1539)
+++ flashrom-spi_rdsr_errorcheck/it87spi.c	(Arbeitskopie)
@@ -321,6 +321,7 @@ 
 {
 	unsigned int i;
 	int result;
+	uint8_t status;
 	chipaddr bios = flash->virtual_memory;
 
 	result = spi_write_enable(flash);
@@ -335,7 +336,7 @@ 
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 1-10 ms, so wait in 1 ms steps.
 	 */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(1000);
 	return 0;
 }
Index: flashrom-spi_rdsr_errorcheck/a25.c
===================================================================
--- flashrom-spi_rdsr_errorcheck/a25.c	(Revision 1539)
+++ flashrom-spi_rdsr_errorcheck/a25.c	(Arbeitskopie)
@@ -33,7 +33,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_amic_a25_srwd(status);
@@ -49,7 +49,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_amic_a25_srwd(status);
@@ -64,7 +64,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_amic_a25_srwd(status);
@@ -82,7 +82,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_amic_a25_srwd(status);
Index: flashrom-spi_rdsr_errorcheck/at25.c
===================================================================
--- flashrom-spi_rdsr_errorcheck/at25.c	(Revision 1539)
+++ flashrom-spi_rdsr_errorcheck/at25.c	(Arbeitskopie)
@@ -61,7 +61,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -84,7 +84,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -103,7 +103,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	msg_cdbg("Chip status register: Status Register Write Protect (WPEN) "
@@ -127,7 +127,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	msg_cdbg("Chip status register: Status Register Write Protect (WPEN) "
@@ -151,7 +151,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 
 	spi_prettyprint_status_register_atmel_at25_srpl(status);
@@ -168,7 +168,7 @@ 
 	uint8_t status;
 	int result;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	/* If block protection is disabled, stop here. */
 	if ((status & (3 << 2)) == 0)
 		return 0;
@@ -195,7 +195,7 @@ 
 		msg_cerr("spi_write_status_register failed\n");
 		return result;
 	}
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	if ((status & (3 << 2)) != 0) {
 		msg_cerr("Block protection could not be disabled!\n");
 		return 1;
@@ -223,7 +223,7 @@ 
 	uint8_t status;
 	int result;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	/* If block protection is disabled, stop here. */
 	if ((status & 0x6c) == 0)
 		return 0;
@@ -244,7 +244,7 @@ 
 		msg_cerr("spi_write_status_register failed\n");
 		return result;
 	}
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	if ((status & 0x6c) != 0) {
 		msg_cerr("Block protection could not be disabled!\n");
 		return 1;
@@ -257,7 +257,7 @@ 
 	uint8_t status;
 	int result;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	/* If block protection is disabled, stop here. */
 	if ((status & 0x7c) == 0)
 		return 0;
@@ -278,7 +278,7 @@ 
 		msg_cerr("spi_write_status_register failed\n");
 		return result;
 	}
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	if ((status & 0x7c) != 0) {
 		msg_cerr("Block protection could not be disabled!\n");
 		return 1;
Index: flashrom-spi_rdsr_errorcheck/spi25.c
===================================================================
--- flashrom-spi_rdsr_errorcheck/spi25.c	(Revision 1539)
+++ flashrom-spi_rdsr_errorcheck/spi25.c	(Arbeitskopie)
@@ -301,7 +301,7 @@ 
 	return 1;
 }
 
-uint8_t spi_read_status_register(struct flashctx *flash)
+int spi_read_status_register(struct flashctx *flash, uint8_t *status)
 {
 	static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
 	/* FIXME: No workarounds for driver/hardware bugs in generic code. */
@@ -311,10 +311,14 @@ 
 	/* Read Status Register */
 	ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd,
 			       readarr);
-	if (ret)
+	if (ret) {
 		msg_cerr("RDSR failed!\n");
+		*status = 0xff;
+		return ret;
+	}
 
-	return readarr[0];
+	*status = readarr[0];
+	return 0;
 }
 
 /* Prettyprint the status register. Common definitions. */
@@ -418,7 +422,7 @@ 
 {
 	uint8_t status;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	msg_cdbg("Chip status register is %02x\n", status);
 	switch (flash->manufacture_id) {
 	case ST_ID:
@@ -451,6 +455,7 @@ 
 int spi_chip_erase_60(struct flashctx *flash)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -479,7 +484,7 @@ 
 	 * This usually takes 1-85 s, so wait in 1 s steps.
 	 */
 	/* FIXME: We assume spi_read_status_register will never fail. */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(1000 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -488,6 +493,7 @@ 
 int spi_chip_erase_c7(struct flashctx *flash)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -515,7 +521,7 @@ 
 	 * This usually takes 1-85 s, so wait in 1 s steps.
 	 */
 	/* FIXME: We assume spi_read_status_register will never fail. */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(1000 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -525,6 +531,7 @@ 
 		       unsigned int blocklen)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -557,7 +564,7 @@ 
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 100-4000 ms, so wait in 100 ms steps.
 	 */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(100 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -572,6 +579,7 @@ 
 		       unsigned int blocklen)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -604,7 +612,7 @@ 
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 100-4000 ms, so wait in 100 ms steps.
 	 */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(100 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -617,6 +625,7 @@ 
 		       unsigned int blocklen)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -649,7 +658,7 @@ 
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 100-4000 ms, so wait in 100 ms steps.
 	 */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(100 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -660,6 +669,7 @@ 
 		       unsigned int blocklen)
 {
 	int result;
+	uint8_t status;
 	struct spi_command cmds[] = {
 	{
 		.writecnt	= JEDEC_WREN_OUTSIZE,
@@ -692,7 +702,7 @@ 
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 15-800 ms, so wait in 10 ms steps.
 	 */
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(10 * 1000);
 	/* FIXME: Check the status register for errors. */
 	return 0;
@@ -764,7 +774,8 @@ 
  * This is according the SST25VF016 datasheet, who knows it is more
  * generic that this...
  */
-static int spi_write_status_register_flag(struct flashctx *flash, int status, const unsigned char enable_opcode)
+static int spi_write_status_register_flag(struct flashctx *flash, uint8_t status,
+					  const unsigned char enable_opcode)
 {
 	int result;
 	int i = 0;
@@ -781,7 +792,7 @@ 
 		.readarr	= NULL,
 	}, {
 		.writecnt	= JEDEC_WRSR_OUTSIZE,
-		.writearr	= (const unsigned char[]){ JEDEC_WRSR, (unsigned char) status },
+		.writearr	= (const unsigned char[]){ JEDEC_WRSR, status },
 		.readcnt	= 0,
 		.readarr	= NULL,
 	}, {
@@ -805,7 +816,7 @@ 
 	 * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
 	 */
 	programmer_delay(100 * 1000);
-	while (spi_read_status_register(flash) & SPI_SR_WIP) {
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP) {
 		if (++i > 490) {
 			msg_cerr("Error: WIP bit after WRSR never cleared\n");
 			return TIMEOUT_ERROR;
@@ -925,7 +936,7 @@ 
 	uint8_t status;
 	int result;
 
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	/* If block protection is disabled, stop here. */
 	if ((status & 0x3c) == 0)
 		return 0;
@@ -936,7 +947,7 @@ 
 		msg_cerr("spi_write_status_register failed\n");
 		return result;
 	}
-	status = spi_read_status_register(flash);
+	spi_read_status_register(flash, &status);
 	if ((status & 0x3c) != 0) {
 		msg_cerr("Block protection could not be disabled!\n");
 		return 1;
@@ -1007,6 +1018,7 @@ 
 		      unsigned int len, unsigned int chunksize)
 {
 	int rc = 0;
+	uint8_t status;
 	unsigned int i, j, starthere, lenhere, towrite;
 	/* FIXME: page_size is the wrong variable. We need max_writechunk_size
 	 * in struct flashctx to do this properly. All chips using
@@ -1035,7 +1047,7 @@ 
 			rc = spi_nbyte_program(flash, starthere + j, buf + starthere - start + j, towrite);
 			if (rc)
 				break;
-			while (spi_read_status_register(flash) & SPI_SR_WIP)
+			while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 				programmer_delay(10);
 		}
 		if (rc)
@@ -1056,13 +1068,14 @@ 
 		     unsigned int len)
 {
 	unsigned int i;
+	uint8_t status;
 	int result = 0;
 
 	for (i = start; i < start + len; i++) {
 		result = spi_byte_program(flash, i, buf[i - start]);
 		if (result)
 			return 1;
-		while (spi_read_status_register(flash) & SPI_SR_WIP)
+		while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 			programmer_delay(10);
 	}
 
@@ -1074,6 +1087,7 @@ 
 {
 	uint32_t pos = start;
 	int result;
+	uint8_t status;
 	unsigned char cmd[JEDEC_AAI_WORD_PROGRAM_CONT_OUTSIZE] = {
 		JEDEC_AAI_WORD_PROGRAM,
 	};
@@ -1157,7 +1171,7 @@ 
 		 */
 		return result;
 	}
-	while (spi_read_status_register(flash) & SPI_SR_WIP)
+	while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 		programmer_delay(10);
 
 	/* We already wrote 2 bytes in the multicommand step. */
@@ -1169,7 +1183,7 @@ 
 		cmd[2] = buf[pos++ - start];
 		spi_send_command(flash, JEDEC_AAI_WORD_PROGRAM_CONT_OUTSIZE, 0,
 				 cmd, NULL);
-		while (spi_read_status_register(flash) & SPI_SR_WIP)
+		while (spi_read_status_register(flash, &status), status & SPI_SR_WIP)
 			programmer_delay(10);
 	}
 
Index: flashrom-spi_rdsr_errorcheck/chipdrivers.h
===================================================================
--- flashrom-spi_rdsr_errorcheck/chipdrivers.h	(Revision 1539)
+++ flashrom-spi_rdsr_errorcheck/chipdrivers.h	(Arbeitskopie)
@@ -45,7 +45,7 @@ 
 int spi_chip_write_1(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
 int spi_chip_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
 int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, int unsigned len);
-uint8_t spi_read_status_register(struct flashctx *flash);
+int spi_read_status_register(struct flashctx *flash, uint8_t *status);
 int spi_write_status_register(struct flashctx *flash, int status);
 void spi_prettyprint_status_register_bit(uint8_t status, int bit);
 void spi_prettyprint_status_register_bp3210(uint8_t status, int bp);