Change in flashrom[master]: use JEDEC_SE as the default sector erase opcode for ICH southbridge

[Mayur Panchal (Code Review)]

Hello David Hendricks,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

use JEDEC_SE as the default sector erase opcode for ICH southbridge

Note: We should definitely look into on-the-fly opcode reprogramming
as the comment in ich_spi_send_multicommand() suggests.

Review URL: http://codereview.chromium.org/3239001

Change-Id: I379549e8fa966e75e3d8b7932700df62cf50df64
Signed-off-by: Mayur Panchal 
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/34689/1

diff --git a/ichspi.c b/ichspi.c
index 8b8f0f6..fc61262 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -324,7 +324,7 @@
    {
     {JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Write Byte
     {JEDEC_READ, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0},	// Read Data
-	 {JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Erase Sector
+	 {JEDEC_SE, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Erase Sector
     {JEDEC_RDSR, SPI_OPCODE_TYPE_READ_NO_ADDRESS, 0},	// Read Device Status Reg
     {JEDEC_REMS, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0},	// Read Electronic Manufacturer Signature
     {JEDEC_WRSR, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 0},	// Write Status Register

[Edward O'Callaghan (Code Review)]

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

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

Patch Set 1: Code-Review+2

[Nico Huber (Code Review)]

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

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

TLRD; I don't think this is needed.

Please look into old patches more carefully, especially if their
commit message doesn't state what the change is about. Generally,
I wouldn't reject this change per se. But it would need a commit
message that makes sense in the history of the branch it is pushed
to.

In this case, it seems the original problem was already solved in
different ways upstream. So a commit on either side (upstream/
downstream) should state that its for alignment of the branches.
e.g.

ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

This aligns the upstream master branch with chromium's. Both
    branches support on-the-fly opcode reprogramming by now, so
    it shouldn't matter which opcode is the default.

Or the other way around for a commit to the downstream branch.

NB. In my limited experience with porting things over the years
over from chromium to upstream, it almost never made sense to
take the downstream commits as is because of the loss of context.
Unless the commits introduced completely new code, of course. It
might be better to pick topics, gather the code changes and write
new commits. Though, even that can be slower compared to a clean
reimplementation of the topic for upstream.

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

PS1: 
A commit message should state what is fixed by the change or why the
change should be considered useful.

I know you are just forwarding very old commits. But that this one
doesn't state its purpose obviously makes it very hard to reason
about. In the new history of the upstream master branch it would
be even harder because the commit message would be 9 years out-
dated at the time of the rebase.

There is a hint, however, about the purpose of the change: The
"Note: " below. If you take that together with the state of the
code _before_ the original commit, it's starting to make sense.

[Edward O'Callaghan (Code Review)]

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

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> (1 comment)
>
> TLRD; I don't think this is needed.
>
> Please look into old patches more carefully, especially if their
> commit message doesn't state what the change is about. Generally,
> I wouldn't reject this change per se. But it would need a commit
> message that makes sense in the history of the branch it is pushed
> to.
>
> In this case, it seems the original problem was already solved in
> different ways upstream. So a commit on either side (upstream/
> downstream) should state that its for alignment of the branches.
> e.g.
> ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
>
> This aligns the upstream master branch with chromium's. Both
> branches support on-the-fly opcode reprogramming by now, so
> it shouldn't matter which opcode is the default.
>
> Or the other way around for a commit to the downstream branch.
>
> NB. In my limited experience with porting things over the years
> over from chromium to upstream, it almost never made sense to
> take the downstream commits as is because of the loss of context.
> Unless the commits introduced completely new code, of course. It
> might be better to pick topics, gather the code changes and write
> new commits. Though, even that can be slower compared to a clean
> reimplementation of the topic for upstream.

Hi Nico,

Just so I understand you correctly and I can help Mayur with his patch here.. The two issues here are that the commit message is essentially obsolete for the context of today and this change isn't 'strictly' needed because both trees support on-the-fly opcode reprogramming? However, it should be harmless for the purposes of resync the two trees and making the comment make a bit more sense with the define name? Thus, provided the commit message is something aliken your suggestion then this commit is still alright to go forward with?

Thanks for the review!
Edward.

[Nico Huber (Code Review)]

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

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

Patch Set 1:
> > TLRD; I don't think this is needed.
> >
> > Please look into old patches more carefully, especially if their
> > commit message doesn't state what the change is about. Generally,
> > I wouldn't reject this change per se. But it would need a commit
> > message that makes sense in the history of the branch it is pushed
> > to.
> >
> > In this case, it seems the original problem was already solved in
> > different ways upstream. So a commit on either side (upstream/
> > downstream) should state that its for alignment of the branches.
> > e.g.
> > ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
> >
> > This aligns the upstream master branch with chromium's. Both
> > branches support on-the-fly opcode reprogramming by now, so
> > it shouldn't matter which opcode is the default.
> >
> > Or the other way around for a commit to the downstream branch.
> >
> > NB. In my limited experience with porting things over the years
> > over from chromium to upstream, it almost never made sense to
> > take the downstream commits as is because of the loss of context.
> > Unless the commits introduced completely new code, of course. It
> > might be better to pick topics, gather the code changes and write
> > new commits. Though, even that can be slower compared to a clean
> > reimplementation of the topic for upstream.
>
> Hi Nico,
>
> Just so I understand you correctly and I can help Mayur with his patch here.. The two issues here are that the commit message is essentially obsolete for the context of today and this change isn't 'strictly' needed because both trees support on-the-fly opcode reprogramming? However, it should be harmless for the purposes of resync the two trees and making the comment make a bit more sense with the define name? Thus, provided the commit message is something aliken your suggestion then this commit is still alright to go forward with?

Yes, you can go forward. The change really seems like a no-op to me.

However, people can fail, and upstream has very limited reviewing
capacities. So generally, I would prefer a downstream change in such
cases.

[Mayur Panchal (Code Review)]

Mayur Panchal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34689 )

Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge
......................................................................

Patch Set 1:
> Patch Set 1:
> > > TLRD; I don't think this is needed.
> > >
> > > Please look into old patches more carefully, especially if their
> > > commit message doesn't state what the change is about. Generally,
> > > I wouldn't reject this change per se. But it would need a commit
> > > message that makes sense in the history of the branch it is pushed
> > > to.
> > >
> > > In this case, it seems the original problem was already solved in
> > > different ways upstream. So a commit on either side (upstream/
> > > downstream) should state that its for alignment of the branches.
> > > e.g.
> > > ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
> > >
> > > This aligns the upstream master branch with chromium's. Both
> > > branches support on-the-fly opcode reprogramming by now, so
> > > it shouldn't matter which opcode is the default.
> > >
> > > Or the other way around for a commit to the downstream branch.
> > >
> > > NB. In my limited experience with porting things over the years
> > > over from chromium to upstream, it almost never made sense to
> > > take the downstream commits as is because of the loss of context.
> > > Unless the commits introduced completely new code, of course. It
> > > might be better to pick topics, gather the code changes and write
> > > new commits. Though, even that can be slower compared to a clean
> > > reimplementation of the topic for upstream.
> >
> > Hi Nico,
> >
> > Just so I understand you correctly and I can help Mayur with his patch here.. The two issues here are that the commit message is essentially obsolete for the context of today and this change isn't 'strictly' needed because both trees support on-the-fly opcode reprogramming? However, it should be harmless for the purposes of resync the two trees and making the comment make a bit more sense with the define name? Thus, provided the commit message is something aliken your suggestion then this commit is still alright to go forward with?
>
> Yes, you can go forward. The change really seems like a no-op to me.
>
> However, people can fail, and upstream has very limited reviewing
> capacities. So generally, I would prefer a downstream change in such
> cases.

Understood about upstream review bandwidth. this is my first patch Edward is helping me with so I am just considering this a learning experience on how to get a patch into flashrom.

I shall update the commit description to follow the advice

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Mayur Panchal (Code Review)]

Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

This aligns the upstream master branch with chromium's.
On-the-fly opcode reprogramming is supported by both branches so the default opcode shouldn't matter

Review URL: http://codereview.chromium.org/3239001

Change-Id: I379549e8fa966e75e3d8b7932700df62cf50df64
Signed-off-by: Mayur Panchal 
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/34689/2

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Nico Huber (Code Review)]

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

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

https://review.coreboot.org/c/flashrom/+/34689/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/34689/2//COMMIT_MSG@9 
PS2, Line 9: This aligns the upstream master branch with chromium's.
Nit, don't break lines after every sentence...

https://review.coreboot.org/c/flashrom/+/34689/2//COMMIT_MSG@10 
PS2, Line 10: On-the-fly opcode reprogramming is supported by both branches so the default opcode shouldn't matter
...but break them after 72 chars max.

And always end sentences with a full stop, please.

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Mayur Panchal (Code Review)]

Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

This aligns the upstream master branch with chromium's. On-the-fly
opcode reprogramming is supported by both branches so the default
opcode shouldn't matter.

Review URL: http://codereview.chromium.org/3239001

Change-Id: I379549e8fa966e75e3d8b7932700df62cf50df64
Signed-off-by: Mayur Panchal 
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/34689/3

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Mayur Panchal (Code Review)]

Mayur Panchal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34689 )

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

Patch Set 3:
> Patch Set 2: Code-Review+1
>
> (2 comments)

Thanks for those, updated commit message to follow guidelines.

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Nico Huber (Code Review)]

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

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

Patch Set 4: Code-Review+2

Change in flashrom[master]: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/34689 )

Change subject: ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE
......................................................................

ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE

This aligns the upstream master branch with chromium's. On-the-fly
opcode reprogramming is supported by both branches so the default
opcode shouldn't matter.

Review URL: http://codereview.chromium.org/3239001

Change-Id: I379549e8fa966e75e3d8b7932700df62cf50df64
Signed-off-by: Mayur Panchal 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/34689
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)

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

diff --git a/ichspi.c b/ichspi.c
index 5a86c96..12ee126 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -324,7 +324,7 @@
    {
     {JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Write Byte
     {JEDEC_READ, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0},	// Read Data
-	 {JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Erase Sector
+	 {JEDEC_SE, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0},	// Erase Sector
     {JEDEC_RDSR, SPI_OPCODE_TYPE_READ_NO_ADDRESS, 0},	// Read Device Status Reg
     {JEDEC_REMS, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0},	// Read Electronic Manufacturer Signature
     {JEDEC_WRSR, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 0},	// Write Status Register