Change in flashrom[master]: WIP: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47276 )


Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................

WIP: flashrom: import chip restore handler from cros flashrom

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/1

diff --git a/flash.h b/flash.h
index 203d32d..a58f1a1 100644
--- a/flash.h
+++ b/flash.h
@@ -349,6 +349,8 @@
 int do_erase(struct flashctx *);
 int do_write(struct flashctx *, const char *const filename, const char *const referencefile);
 int do_verify(struct flashctx *, const char *const filename);
+#define CHIP_RESTORE_CALLBACK	int (*func) (struct flashctx *flash, uint8_t status)
+int register_chip_restore(CHIP_RESTORE_CALLBACK, struct flashctx *flash, uint8_t status);
 
 /* Something happened that shouldn't happen, but we can go on. */
 #define ERROR_NONFATAL 0x100
diff --git a/flashrom.c b/flashrom.c
index 26e2df8..c4b17bb 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -536,6 +536,15 @@
 	{0}, /* This entry corresponds to PROGRAMMER_INVALID. */
 };
 
+#define CHIP_RESTORE_MAXFN 4
+static int chip_restore_fn_count = 0;
+static struct chip_restore_func_data {
+	CHIP_RESTORE_CALLBACK;
+	struct flashctx *flash;
+	uint8_t status;
+} chip_restore_fn[CHIP_RESTORE_MAXFN];
+
+
 #define SHUTDOWN_MAXFN 32
 static int shutdown_fn_count = 0;
 /** @private */
@@ -580,6 +589,22 @@
 	return 0;
 }
 
+int register_chip_restore(CHIP_RESTORE_CALLBACK,
+                          struct flashctx *flash, uint8_t status)
+{
+	if (chip_restore_fn_count >= CHIP_RESTORE_MAXFN) {
+		msg_perr("Tried to register more than %i chip restore"
+		         " functions.\n", CHIP_RESTORE_MAXFN);
+		return 1;
+	}
+	chip_restore_fn[chip_restore_fn_count].func = func;	/* from macro */
+	chip_restore_fn[chip_restore_fn_count].flash = flash;
+	chip_restore_fn[chip_restore_fn_count].status = status;
+	chip_restore_fn_count++;
+
+	return 0;
+}
+
 int programmer_init(enum programmer prog, const char *param)
 {
 	int ret;
@@ -626,6 +651,19 @@
 	return ret;
 }
 
+static int chip_restore()
+{
+	int rc = 0;
+
+	while (chip_restore_fn_count > 0) {
+		int i = --chip_restore_fn_count;
+		rc |= chip_restore_fn[i].func(chip_restore_fn[i].flash,
+		                              chip_restore_fn[i].status);
+	}
+
+	return rc;
+}
+
 /** Calls registered shutdown functions and resets internal programmer-related variables.
  * Calling it is safe even without previous initialization, but further interactions with programmer support
  * require a call to programmer_init() (afterwards).
@@ -2286,6 +2324,7 @@
 void finalize_flash_access(struct flashctx *const flash)
 {
 	unmap_flash(flash);
+	chip_restore();
 }
 
 /**

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-MessageType: newchange

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@2327 
PS1, Line 2327: chip_restore();
This seems like an ok place to call chip_restore(), but it's difficult to determine if it is equivalent to cros since the structure of flashrom.c has diverged so much.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 06 Nov 2020 00:58:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Edward O'Callaghan (Code Review)]

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

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 1:

(5 comments)

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

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@540 
PS1, Line 540: hip_restore_fn_count
this state should be contained within the flashctx structure.


https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@541 
PS1, Line 541: static struct chip_restore_func_data {
             : 	CHIP_RESTORE_CALLBACK;
             : 	struct flashctx *flash;
             : 	uint8_t status;
             : } chip_restore_fn[CHIP_RESTORE_MAXFN];
put the struct definition within the flashctx structure so that you don't need another copy (which is called flash here instead of flashctx - awkward). the flashctx would then carry the state over the required life-time


https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@592 
PS1, Line 592: register_chip_restore
probably a good design is for the chip structure to contain a callback and if it exists then it is dispatch that does the registering. That way this function can be called in the generic core control flow when a chip that requires it is found?


https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@653 
PS1, Line 653: 
             : static int chip_restore()
move this under the register_chip_restore() function and also rename to deregister_chip_restore()


https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@2327 
PS1, Line 2327: chip_restore();
> This seems like an ok place to call chip_restore(), but it's difficult to determine if it is equival […]
> This seems like a reasonable place to me but maybe before the unmap?

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Edward O'Callaghan, 

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

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

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

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................

WIP: flashrom: import chip restore handler from cros flashrom

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 39 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/2
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 2
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 2:

(5 comments)

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

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@540 
PS1, Line 540: hip_restore_fn_count
> this state should be contained within the flashctx structure.
> Done

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@541 
PS1, Line 541: static struct chip_restore_func_data {
             : 	CHIP_RESTORE_CALLBACK;
             : 	struct flashctx *flash;
             : 	uint8_t status;
             : } chip_restore_fn[CHIP_RESTORE_MAXFN];
> put the struct definition within the flashctx structure so that you don't need another copy (which i […]
> Done

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@592 
PS1, Line 592: register_chip_restore
> probably a good design is for the chip structure to contain a callback and if it exists then it is d […]
> We should either store a callback in the flashchip structure or have an array of callbacks in the flashctx. I think the latter is better since this the chip restore function seems to be a driver implementation detail and not something that should be configured per chip.

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@653 
PS1, Line 653: 
             : static int chip_restore()
> move this under the register_chip_restore() function and also rename to deregister_chip_restore()
> Done, but maybe we should chose a name that indicates that it will actually call the chip restore functions before deregistering them?

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@2327 
PS1, Line 2327: chip_restore();
> This seems like a reasonable place to me but maybe before the unmap?
> Done

[Edward O'Callaghan (Code Review)]

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

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 2:

(4 comments)

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

https://review.coreboot.org/c/flashrom/+/47276/2//COMMIT_MSG@7 
PS2, Line 7: WIP: flashrom: import chip restore handler from cros flashrom
Worth writing out a detailed commit message now about what this infrastructure is and why its needed for s25f variant chips with BUG/BRANCH/TEST lines.

Otherwise I think this architecture looks much better now and looks possible to write a unit-test for!


https://review.coreboot.org/c/flashrom/+/47276/2/flash.h 
File flash.h:

https://review.coreboot.org/c/flashrom/+/47276/2/flash.h@282 
PS2, Line 282: int (*func) (struct flashctx *flash, uint8_t status)
typedef this callback signature somewhere sensible above and use that everywhere as the type.

`typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, uint8_t status);`


https://review.coreboot.org/c/flashrom/+/47276/2/flash.h@360 
PS2, Line 360: int (*func) (struct flashctx *flash, uint8_t status),
`chip_restore_fn_cb_t func,`


https://review.coreboot.org/c/flashrom/+/47276/2/flashrom.c 
File flashrom.c:

https://review.coreboot.org/c/flashrom/+/47276/2/flashrom.c@583 
PS2, Line 583: int (*func) (struct flashctx *flash, uint8_t status),
`chip_restore_fn_cb_t func,`



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 2
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Fri, 13 Nov 2020 04:17:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................

flashrom: import chip restore handler from cros flashrom

Allows drivers to register a callback function that gets
called to reset the chip state once programming has finished.

This is used by the s25f driver added in a subsequent patch,
which changes chips with a hybrid (non-uniformly sized) sector
layout to a uniform layout, and registers a restore function
to change the chip back to the hybrid layout at exit.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/3
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 3:

(4 comments)

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

https://review.coreboot.org/c/flashrom/+/47276/2//COMMIT_MSG@7 
PS2, Line 7: WIP: flashrom: import chip restore handler from cros flashrom
> Worth writing out a detailed commit message now about what this infrastructure is and why its needed […]
> Done

https://review.coreboot.org/c/flashrom/+/47276/2/flash.h 
File flash.h:

https://review.coreboot.org/c/flashrom/+/47276/2/flash.h@282 
PS2, Line 282: int (*func) (struct flashctx *flash, uint8_t status)
> typedef this callback signature somewhere sensible above and use that everywhere as the type. […]
> Done

https://review.coreboot.org/c/flashrom/+/47276/2/flash.h@360 
PS2, Line 360: int (*func) (struct flashctx *flash, uint8_t status),
> `chip_restore_fn_cb_t func,`
> Done

https://review.coreboot.org/c/flashrom/+/47276/2/flashrom.c 
File flashrom.c:

https://review.coreboot.org/c/flashrom/+/47276/2/flashrom.c@583 
PS2, Line 583: int (*func) (struct flashctx *flash, uint8_t status),
> `chip_restore_fn_cb_t func,`
> Done

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/47276/3/flash.h 
File flash.h:

https://review.coreboot.org/c/flashrom/+/47276/3/flash.h@362 
PS3, Line 362: int (*func) (struct flashctx *flash, uint8_t status)
missed this



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Fri, 13 Nov 2020 04:54:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................

flashrom: import chip restore handler from cros flashrom

Allows drivers to register a callback function that gets
called to reset the chip state once programming has finished.

This is used by the s25f driver added in a subsequent patch,
which changes chips with a hybrid (non-uniformly sized) sector
layout to a uniform layout, and registers a restore function
to change the chip back to the hybrid layout at exit.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/4
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 4
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/47276/3/flash.h 
File flash.h:

https://review.coreboot.org/c/flashrom/+/47276/3/flash.h@362 
PS3, Line 362: int (*func) (struct flashctx *flash, uint8_t status)
> missed this
> Done

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 4
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Fri, 13 Nov 2020 06:20:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: WIP: flashrom: import chip restore handler from cros flashrom
......................................................................

WIP: flashrom: import chip restore handler from cros flashrom

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/5
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 5
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................

flashrom: import chip restore handler from cros flashrom

Allows drivers to register a callback function that gets
called to reset the chip state once programming has finished.

This is used by the s25f driver added in a subsequent patch,
which changes chips with a hybrid (non-uniformly sized) sector
layout to a uniform layout, and registers a restore function
to change the chip back to the hybrid layout at exit.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/6
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 6
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom: import chip restore handler from cros flashrom

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom: import chip restore handler from cros flashrom
......................................................................


Patch Set 6:

(2 comments)

https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG@7 
PS6, Line 7: import
this isn't true because you re-implemented this for upstream.

 `flashrom.c: Implement chip state restore callback hooking`


https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG@12 
PS6, Line 12: This is used by the s25f driver added in a subsequent patch,
            : which changes chips with a hybrid (non-uniformly sized) sector
            : layout to a uniform layout, and registers a restore function
            : to change the chip back to the hybrid layout at exit.
This is very hard to parse.

 ```
 This is used by Spansion chip support added in a subsequent patch
 as to allows switching between hybrid non-uniformly sized and uniform sector 
 layouts pre-post flashing. The framework allows for a restore function to be 
 registered to change the chip back to the hybrid layout upon tear-down.

 ```



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 6
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Mon, 23 Nov 2020 04:18:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................

flashrom.c: implement chip restore callback registration

Allows drivers to register a function that gets called to
reset the chip state once programming has finished. This is
used by the s25f driver added in a following patch.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/7
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 7
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................

flashrom.c: implement chip restore callback registration

Allows drivers to register a function that gets called to
reset the chip state once programming has finished. This
functionality is used by the s25f driver added in a
following patch.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/8
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 8
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Nikolai Artemiev (Code Review)]

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )

Change subject: flashrom.c: implement chip restore callback registration
......................................................................


Patch Set 8:

(2 comments)

https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG@7 
PS6, Line 7: import
> this isn't true because you re-implemented this for upstream. […]
> Done

https://review.coreboot.org/c/flashrom/+/47276/6//COMMIT_MSG@12 
PS6, Line 12: This is used by the s25f driver added in a subsequent patch,
            : which changes chips with a hybrid (non-uniformly sized) sector
            : layout to a uniform layout, and registers a restore function
            : to change the chip back to the hybrid layout at exit.
> This is very hard to parse. […]
> Done

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................

flashrom.c: implement chip restore callback registration

Allows drivers to register a callback function to reset the
chip state once programming has finished. This is used by
the s25f driver added in a later patch, which needs to change
the chip's sector layout to be able to write to the entire flash.

Adapted from cros flashrom at
`9c4c9a56b6a0370b383df9c75d71b3bd469e672d`.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/9
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 9
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Nikolai Artemiev (Code Review)]

Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, 

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

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

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................

flashrom.c: implement chip restore callback registration

Allows drivers to register a callback function to reset the
chip state once programming has finished. This is used by
the s25f driver added in a later patch, which needs to change
the chip's sector layout to be able to write to the entire flash.

Adapted from cros flashrom at
`9c4c9a56b6a0370b383df9c75d71b3bd469e672d`.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/47276/10
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 10
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................


Patch Set 10: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 10
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Mon, 23 Nov 2020 07:06:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................


Patch Set 11:

(2 comments)

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

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@592 
PS1, Line 592: register_chip_restore
> We should either store a callback in the flashchip structure or have an array of callbacks in the fl […]
> Ack

https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@653 
PS1, Line 653: 
             : static int chip_restore()
> Done, but maybe we should chose a name that indicates that it will actually call the chip restore fu […]
> Ack

Change in flashrom[master]: flashrom.c: implement chip restore callback registration

[Edward O'Callaghan (Code Review)]

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

Change subject: flashrom.c: implement chip restore callback registration
......................................................................

flashrom.c: implement chip restore callback registration

Allows drivers to register a callback function to reset the
chip state once programming has finished. This is used by
the s25f driver added in a later patch, which needs to change
the chip's sector layout to be able to write to the entire flash.

Adapted from cros flashrom at
`9c4c9a56b6a0370b383df9c75d71b3bd469e672d`.

BUG=b:153800073
BRANCH=none
TEST=builds

Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Signed-off-by: Nikolai Artemiev <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/47276
Reviewed-by: Edward O'Callaghan <[e-mail]>
Tested-by: build bot (Jenkins) <[e-mail]>
---
M flash.h
M flashrom.c
2 files changed, 41 insertions(+), 0 deletions(-)

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



diff --git a/flash.h b/flash.h
index a1e7669..883b6f4 100644
--- a/flash.h
+++ b/flash.h
@@ -103,6 +103,8 @@
  */
 #define NUM_ERASEFUNCTIONS 8
 
+#define MAX_CHIP_RESTORE_FUNCTIONS 4
+
 /* Feature bits used for non-SPI only */
 #define FEATURE_REGISTERMAP	(1 << 0)
 #define FEATURE_LONG_RESET	(0 << 4)
@@ -248,6 +250,8 @@
 	struct wp *wp;
 };
 
+typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, uint8_t status);
+
 struct flashrom_flashctx {
 	struct flashchip *chip;
 	/* FIXME: The memory mappings should be saved in a more structured way. */
@@ -275,6 +279,12 @@
 	 */
 	int address_high_byte;
 	bool in_4ba_mode;
+
+	int chip_restore_fn_count;
+	struct chip_restore_func_data {
+		chip_restore_fn_cb_t func;
+		uint8_t status;
+	} chip_restore_fn[MAX_CHIP_RESTORE_FUNCTIONS];
 };
 
 /* Timing used in probe routines. ZERO is -2 to differentiate between an unset
@@ -350,6 +360,7 @@
 int do_erase(struct flashctx *);
 int do_write(struct flashctx *, const char *const filename, const char *const referencefile);
 int do_verify(struct flashctx *, const char *const filename);
+int register_chip_restore(chip_restore_fn_cb_t func, struct flashctx *flash, uint8_t status);
 
 /* Something happened that shouldn't happen, but we can go on. */
 #define ERROR_NONFATAL 0x100
diff --git a/flashrom.c b/flashrom.c
index 7f5f2a8..c89abad 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -580,6 +580,34 @@
 	return 0;
 }
 
+int register_chip_restore(chip_restore_fn_cb_t func,
+			  struct flashctx *flash, uint8_t status)
+{
+	if (flash->chip_restore_fn_count >= MAX_CHIP_RESTORE_FUNCTIONS) {
+		msg_perr("Tried to register more than %i chip restore"
+		         " functions.\n", MAX_CHIP_RESTORE_FUNCTIONS);
+		return 1;
+	}
+	flash->chip_restore_fn[flash->chip_restore_fn_count].func = func;
+	flash->chip_restore_fn[flash->chip_restore_fn_count].status = status;
+	flash->chip_restore_fn_count++;
+
+	return 0;
+}
+
+static int deregister_chip_restore(struct flashctx *flash)
+{
+	int rc = 0;
+
+	while (flash->chip_restore_fn_count > 0) {
+		int i = --flash->chip_restore_fn_count;
+		rc |= flash->chip_restore_fn[i].func(
+			flash, flash->chip_restore_fn[i].status);
+	}
+
+	return rc;
+}
+
 int programmer_init(enum programmer prog, const char *param)
 {
 	int ret;
@@ -2256,6 +2284,7 @@
 
 	flash->address_high_byte = -1;
 	flash->in_4ba_mode = false;
+	flash->chip_restore_fn_count = 0;
 
 	/* Be careful about 4BA chips and broken masters */
 	if (flash->chip->total_size > 16 * 1024 && spi_master_no_4ba_modes(flash)) {
@@ -2285,6 +2314,7 @@
 
 void finalize_flash_access(struct flashctx *const flash)
 {
+	deregister_chip_restore(flash);
 	unmap_flash(flash);
 }
 

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/47276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a522dc1fd3952793fbcad70afc6dd43850fbbc5
Gerrit-Change-Number: 47276
Gerrit-PatchSet: 12
Gerrit-Owner: Nikolai Artemiev <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Andrew McRae <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-MessageType: merged