Change in flashrom[master]: ichspi.c: ich7_run_opcode() fail early on transact error

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46325 )

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

ichspi.c: ich7_run_opcode() fail early on transact error

This patch is adapted from the ChromiumOS fork. While we no longer ship
and support the devices:

- Lenovo S10-t3,
 - EEEPC,
 - Alex and Mario.

I did however want to preserve the support for the community as the
right thing to do.

Downstream commit d2129f1cc036394fcf3c5232647a6e11eb1674fd
http://codereview.chromium.org/6698020

Change-Id: I93198cb8536705e9b71b7fbe43a20e151c2a72a8
Original-author-by: Louis Yung-Chieh Lo 
Signed-off-by: Edward O'Callaghan 
---
M ichspi.c
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/46325/1

diff --git a/ichspi.c b/ichspi.c
index 4209d60..3668a82 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -929,9 +929,11 @@
    while (((REGREAD16(ICH7_REG_SPIS) & (SPIS_CDS | SPIS_FCERR)) == 0) &&
           --timeout) {
    	programmer_delay(10);
+		if (REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR)
+			break;  /* Transaction error */
    }
    if (!timeout) {
-		msg_perr("timeout, ICH7_REG_SPIS=0x%04x\n",
+		msg_perr("CDS timeout, ICH7_REG_SPIS=0x%04x\n",
    		 REGREAD16(ICH7_REG_SPIS));
    	return 1;
    }

[Angel Pons (Code Review)]

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

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Patch Set 1: Code-Review+1

(3 comments)

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

https://review.coreboot.org/c/flashrom/+/46325/1//COMMIT_MSG@7 
PS1, Line 7: ich7_run_opcode() fail early on transact error
Bail early on flash cycle errors in ich7_run_opcode()

https://review.coreboot.org/c/flashrom/+/46325/1//COMMIT_MSG@9 
PS1, Line 9: This patch is adapted from the ChromiumOS fork. While we no longer ship
           : and support the devices:
nit: IMHO, the last sentence parses a bit strange. I'd say:

This patch is adapted from the ChromiumOS fork, and was used on the following, now-obsolete devices:

https://review.coreboot.org/c/flashrom/+/46325/1//COMMIT_MSG@12 
PS1, Line 12: ,
I'd omit the punctuation on the elements of this list

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46325 )

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Patch Set 1: Code-Review-1

I'm not sure that any of the listed devices are relevant for the description of the functionality of this patch.

Looking at the code in Chromium OS mentioned in the commit message, the while loop only reacts to CDS and then another exit condition is defined through the if upon FCERR.

The flashrom  upstream code however alreaady exits the loop on either FCERR or CDS, so the additional if is superfluous, as the exit condition would be reached at the same time as with the patch, just in a tighter loop.
Also, the CDS timeout would be announced through either a an actual timeout or through FCERR aborting the loop. This cosmetic incorrectness is there with and without this patch.

Overall I think the error message should distinguish between those two exit criteria, but (the rest of) this patch should be abandoned.

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46325 )

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Patch Set 1:
> Also, the CDS timeout would be announced through either a an actual timeout or through FCERR aborting the loop. This cosmetic incorrectness is there with and without this patch.
>
> Overall I think the error message should distinguish between those two exit criteria, but (the rest of) this patch should be abandoned.

And I should have kept reading. The code is actually already doing this correctly without any patch. So it should be fine to just abandon this, as it is a no-op as far as I can tell

[Angel Pons (Code Review)]

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

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Patch Set 1: Code-Review-1
> Patch Set 1:
> > Also, the CDS timeout would be announced through either a an actual timeout or through FCERR aborting the loop. This cosmetic incorrectness is there with and without this patch.
> >
> > Overall I think the error message should distinguish between those two exit criteria, but (the rest of) this patch should be abandoned.
>
> And I should have kept reading. The code is actually already doing this correctly without any patch. So it should be fine to just abandon this, as it is a no-op as far as I can tell

Good point, thanks for noticing.

[Edward O'Callaghan (Code Review)]

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

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
> > Patch Set 1:
> > > Also, the CDS timeout would be announced through either a an actual timeout or through FCERR aborting the loop. This cosmetic incorrectness is there with and without this patch.
> > >
> > > Overall I think the error message should distinguish between those two exit criteria, but (the rest of) this patch should be abandoned.
> >
> > And I should have kept reading. The code is actually already doing this correctly without any patch. So it should be fine to just abandon this, as it is a no-op as far as I can tell
>
> Good point, thanks for noticing.

Perfect ! Thanks for the analysis and the background Stefan! I guess that means we should remove it on CrOS side then.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/46325 )

Change subject: ichspi.c: ich7_run_opcode() fail early on transact error
......................................................................

Abandoned

See commentary on code-review