Change in ...coreboot[master]: soc/intel/broadwell/romstage: Clean up unused bist variable

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32757

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

soc/intel/broadwell/romstage: Clean up unused bist variable

Checking BIST is done in the bootblock.

Change-Id: I3ea2eb6a37c038f7348f0abd2056eee5c07bdb9d
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/broadwell/include/soc/romstage.h
M src/soc/intel/broadwell/romstage/romstage.c
2 files changed, 1 insertion(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32757/1

diff --git a/src/soc/intel/broadwell/include/soc/romstage.h b/src/soc/intel/broadwell/include/soc/romstage.h
index 31184f9..86a2c84 100644
--- a/src/soc/intel/broadwell/include/soc/romstage.h
+++ b/src/soc/intel/broadwell/include/soc/romstage.h
@@ -22,7 +22,6 @@
 struct chipset_power_state;
 struct pei_data;
 struct romstage_params {
-	unsigned long bist;
    struct chipset_power_state *power_state;
    struct pei_data *pei_data;
 };
diff --git a/src/soc/intel/broadwell/romstage/romstage.c b/src/soc/intel/broadwell/romstage/romstage.c
index d83ff29..0151023 100644
--- a/src/soc/intel/broadwell/romstage/romstage.c
+++ b/src/soc/intel/broadwell/romstage/romstage.c
@@ -65,10 +65,7 @@
 /* Entry from assembly_entry.S. */
 asmlinkage void car_stage_entry(void)
 {
-	struct romstage_params rp = {
-		.bist = 0, /* checking done in the bootblock */
-		.pei_data = NULL,
-	};
+	struct romstage_params rp;

post_code(0x30);

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph,

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

https://review.coreboot.org/c/coreboot/+/32757

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

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

soc/intel/broadwell/romstage: Clean up unused bist variable

Checking BIST is done in the bootblock.

Change-Id: I3ea2eb6a37c038f7348f0abd2056eee5c07bdb9d
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/broadwell/include/soc/romstage.h
M src/soc/intel/broadwell/romstage/romstage.c
2 files changed, 1 insertion(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32757/2

[Nico Huber (Code Review)]

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

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

Good enough for this weird code flow.

https://review.coreboot.org/#/c/32757/1/src/soc/intel/broadwell/include/soc/... 
File src/soc/intel/broadwell/include/soc/romstage.h:

https://review.coreboot.org/#/c/32757/1/src/soc/intel/broadwell/include/soc/... 
PS1, Line 25: 
So the value was never consumed anyway? ;)

https://review.coreboot.org/#/c/32757/1/src/soc/intel/broadwell/romstage/rom... 
File src/soc/intel/broadwell/romstage/romstage.c:

https://review.coreboot.org/#/c/32757/1/src/soc/intel/broadwell/romstage/rom... 
PS1, Line 66: asmlinkage void car_stage_entry(void)
            : {
            : 	struct romstage_params rp;
            : 
            : 	post_code(0x30);
            : 
            : 	/* Start console drivers */
            : 	console_init();
            : 
            : 	/* Save romstage begin */
            : 	timestamp_add_now(TS_START_ROMSTAGE);
            : 
            : 	/* System Agent Early Initialization */
            : 	systemagent_early_init();
            : 
            : 	/* PCH Early Initialization */
            : 	pch_early_init();
            : 
            : 	/* Get power state */
            : 	rp.power_state = fill_power_state();
            : 
            : 	/* Print useful platform information */
            : 	report_platform_info();
            : 
            : 	/* Set CPU frequency to maximum */
            : 	set_max_freq();
            : 
            : 	/* Call into mainboard. */
            : 	mainboard_romstage_entry(&rp);
            : 
            : 	platform_enter_postcar();
            : }
            : 
            : /* Entry from the mainboard. */
            : void romstage_common(struct romstage_params *params)
            : {
            : 	post_code(0x32);
            : 
            : 	timestamp_add_now(TS_BEFORE_INITRAM);
            : 
            : 	params->pei_data->boot_mode = params->power_state->prev_sleep_state;
            : 
            : #if CONFIG(ELOG_BOOT_COUNT)
            : 	if (params->power_state->prev_sleep_state != ACPI_S3)
            : 		boot_count_increment();
            : #endif
            : 
            : 	/* Print ME state before MRC */
            : 	intel_me_status();
            : 
            : 	/* Save ME HSIO version */
            : 	intel_me_hsio_version(&params->power_state->hsio_version,
            : 			      &params->power_state->hsio_checksum);
            : 
            : 	/* Initialize RAM */
            : 	raminit(params->pei_data);
            : 
            : 	timestamp_add_now(TS_AFTER_INITRAM);
            : 
            : 	romstage_handoff_init(params->power_state->prev_sleep_state == ACPI_S3);
            : }
            : 
It seems these two functions form really just one flow with
one mainboard hook pre and one post raminit. With that in
mind it makes sense to keep the `romstage_params` declaration
here...

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32757 )

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

Patch Set 5:
> Patch Set 2: Code-Review+2
>
> (2 comments)
>
> Good enough for this weird code flow.

Bootflow improved in https://review.coreboot.org/c/coreboot/+/32760/

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32757 )

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

Patch Set 11: Code-Review+1

entire patch train tested on google/lulu

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32757 )

Change subject: soc/intel/broadwell/romstage: Clean up unused bist variable
......................................................................

soc/intel/broadwell/romstage: Clean up unused bist variable

Checking BIST is done in the bootblock.

Change-Id: I3ea2eb6a37c038f7348f0abd2056eee5c07bdb9d
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32757
Reviewed-by: Matt DeVillier 
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/broadwell/include/soc/romstage.h
1 file changed, 0 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Matt DeVillier: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/broadwell/include/soc/romstage.h b/src/soc/intel/broadwell/include/soc/romstage.h
index ece3cd8..ac8265f 100644
--- a/src/soc/intel/broadwell/include/soc/romstage.h
+++ b/src/soc/intel/broadwell/include/soc/romstage.h
@@ -22,7 +22,6 @@

struct chipset_power_state;
 struct romstage_params {
-	unsigned long bist;
    struct chipset_power_state *power_state;
    struct pei_data pei_data;
 };