Change in coreboot[master]: nb/intel/haswell: Add `mb_post_raminit_setup` function

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

nb/intel/haswell: Add `mb_post_raminit_setup` function

This function is called at the end of `romstage_common`. Only one board
makes use of it, the Lenovo ThinkPad T440p. To preserve behavior, call
it after `romstage_common` has done nearly everything.

Change-Id: I35742879e737be4f383a0e36aecc6682fc9df058
Signed-off-by: Angel Pons 
---
M src/mainboard/lenovo/t440p/romstage.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/romstage.c
3 files changed, 24 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43094/1

diff --git a/src/mainboard/lenovo/t440p/romstage.c b/src/mainboard/lenovo/t440p/romstage.c
index ea1193f..5ddbd03 100644
--- a/src/mainboard/lenovo/t440p/romstage.c
+++ b/src/mainboard/lenovo/t440p/romstage.c
@@ -26,8 +26,24 @@
    RCBA_END_CONFIG,
 };

-void mainboard_config_superio(void)
+void mb_post_raminit_setup(void)
 {
+	u8 enable_peg;
+	if (get_option(&enable_peg, "enable_dual_graphics") != CB_SUCCESS)
+		enable_peg = 0;
+
+	bool power_en = pmh7_dgpu_power_state();
+
+	if (enable_peg != power_en)
+		pmh7_dgpu_power_enable(!power_en);
+
+	if (!enable_peg) {
+		// Hide disabled dGPU device
+		u32 reg32 = pci_read_config32(PCI_DEV(0, 0, 0), DEVEN);
+		reg32 &= ~DEVEN_D1F0EN;
+
+		pci_write_config32(PCI_DEV(0, 0, 0), DEVEN, reg32);
+	}
 }

void mainboard_romstage_entry(void)
@@ -86,21 +102,4 @@
    };

romstage_common(&romstage_params);
-
-	u8 enable_peg;
-	if (get_option(&enable_peg, "enable_dual_graphics") != CB_SUCCESS)
-		enable_peg = 0;
-
-	bool power_en = pmh7_dgpu_power_state();
-
-	if (enable_peg != power_en)
-		pmh7_dgpu_power_enable(!power_en);
-
-	if (!enable_peg) {
-		// Hide disabled dGPU device
-		u32 reg32 = pci_read_config32(PCI_DEV(0, 0, 0), DEVEN);
-		reg32 &= ~DEVEN_D1F0EN;
-
-		pci_write_config32(PCI_DEV(0, 0, 0), DEVEN, reg32);
-	}
 }
diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index b98d880..0b35acb 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -198,6 +198,7 @@
    void (*copy_spd)(struct pei_data *peid);
 };
 void romstage_common(const struct romstage_params *params);
+void mb_post_raminit_setup(void); /* optional */

void haswell_early_initialization(void);
 void haswell_late_initialization(void);
diff --git a/src/northbridge/intel/haswell/romstage.c b/src/northbridge/intel/haswell/romstage.c
index 579eca7..e5e29c1 100644
--- a/src/northbridge/intel/haswell/romstage.c
+++ b/src/northbridge/intel/haswell/romstage.c
@@ -13,6 +13,10 @@
 #include <southbridge/intel/lynxpoint/pch.h>
 #include <southbridge/intel/lynxpoint/me.h>

+void __weak mb_post_raminit_setup(void)
+{
+}
+
 void romstage_common(const struct romstage_params *params)
 {
    int wake_from_s3;
@@ -77,5 +81,7 @@

romstage_handoff_init(wake_from_s3);

+	mb_post_raminit_setup();
+
    post_code(0x3f);
 }

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

Perhaps a name such as `mb_romstage_final` would be more appropriate? If
the T440p PEG code runs after raminit, it will have to come after
`haswell_unhide_peg` too.

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
File src/mainboard/lenovo/t440p/romstage.c:

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
PS1, Line 29: mainboard_config_superio
It would be better to drop this function in a separate commit.

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
File src/mainboard/lenovo/t440p/romstage.c:

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
PS1, Line 29: mainboard_config_superio
> It would be better to drop this function in a separate commit.

*sigh* I'll have to rebase the two dozen commits that go atop this one, or convert this train into a cactus. I'll try to do it

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Tristan Corrick, Alexander Couzens, Patrick Rudolph,

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

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

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

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

nb/intel/haswell: Add `mb_post_raminit_setup` function

This function is called at the end of `romstage_common`. Only one board
makes use of it, the Lenovo ThinkPad T440p. To preserve behavior, call
it after `romstage_common` has done nearly everything.

Change-Id: I35742879e737be4f383a0e36aecc6682fc9df058
Signed-off-by: Angel Pons 
---
M src/mainboard/lenovo/t440p/romstage.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/romstage.c
3 files changed, 27 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43094/2

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
File src/mainboard/lenovo/t440p/romstage.c:

https://review.coreboot.org/c/coreboot/+/43094/1/src/mainboard/lenovo/t440p/... 
PS1, Line 29: mainboard_config_superio
> *sigh* I'll have to rebase the two dozen commits that go atop this one, or convert this train into a […]

CB:43297

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 2: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 2:
> Patch Set 1: Code-Review+2
>
> (1 comment)
>
> Perhaps a name such as `mb_romstage_final` would be more appropriate? If
> the T440p PEG code runs after raminit, it will have to come after
> `haswell_unhide_peg` too.

I used the same name as in x4x, and to preserve the original behavior (which may be wrong) I placed the code so that it runs at the same point as before (only difference being the postcode call, which doesn't really matter). In any case, once I turn romstage_common inside-out in CB:43108 it will be easier to move the function call to a better place.

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 1: Code-Review+2
> >
> > (1 comment)
> >
> > Perhaps a name such as `mb_romstage_final` would be more appropriate? If
> > the T440p PEG code runs after raminit, it will have to come after
> > `haswell_unhide_peg` too.
>
> I used the same name as in x4x, and to preserve the original behavior (which may be wrong) I placed the code so that it runs at the same point as before (only difference being the postcode call, which doesn't really matter). In any case, once I turn romstage_common inside-out in CB:43108 it will be easier to move the function call to a better place.

Yes, the behaviour is preserved. My thought was that the T440p code
relies on more than just coming after raminit, since the PEG devices
need to be unhidden first. The name `mb_post_raminit_setup` doesn't
suggest that one could always rely on it being executed after the PEG
devices are unhidden. However, I think a name such as `mb_romstage_final`
shows that one can rely on it running after all the common romstage code,
including the PEG devices being unhidden.

When thinking of future boards that might use it, I think it's much more
likely to have use cases that depend on running after all the common
romstage code than immediately after raminit and before the end of
common romstage.

What I could see potentially happening in the future is it being decided
to move `mb_post_raminit_setup` immediately after raminit, for whatever
reason, which would break the T440p PEG code as I understand it. Unlikely?
Perhaps. But I don't see a downside to changing the name.

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Add `mb_post_raminit_setup` function
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 1: Code-Review+2
> > >
> > > (1 comment)
> > >
> > > Perhaps a name such as `mb_romstage_final` would be more appropriate? If
> > > the T440p PEG code runs after raminit, it will have to come after
> > > `haswell_unhide_peg` too.
> >
> > I used the same name as in x4x, and to preserve the original behavior (which may be wrong) I placed the code so that it runs at the same point as before (only difference being the postcode call, which doesn't really matter). In any case, once I turn romstage_common inside-out in CB:43108 it will be easier to move the function call to a better place.
>
> Yes, the behaviour is preserved. My thought was that the T440p code
> relies on more than just coming after raminit, since the PEG devices
> need to be unhidden first. The name `mb_post_raminit_setup` doesn't
> suggest that one could always rely on it being executed after the PEG
> devices are unhidden. However, I think a name such as `mb_romstage_final`
> shows that one can rely on it running after all the common romstage code,
> including the PEG devices being unhidden.
>
> When thinking of future boards that might use it, I think it's much more
> likely to have use cases that depend on running after all the common
> romstage code than immediately after raminit and before the end of
> common romstage.
>
> What I could see potentially happening in the future is it being decided
> to move `mb_post_raminit_setup` immediately after raminit, for whatever
> reason, which would break the T440p PEG code as I understand it. Unlikely?
> Perhaps. But I don't see a downside to changing the name.

Sorry, I said x4x but `mb_post_raminit_setup` is on gm45. And it's called right after raminit, hence the name.

Change in coreboot[master]: inb/intel/haswell: Add `mb_late_romstage_setup` function

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Tristan Corrick, Alexander Couzens, Patrick Rudolph,

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

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

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

Change subject: inb/intel/haswell: Add `mb_late_romstage_setup` function
......................................................................

inb/intel/haswell: Add `mb_late_romstage_setup` function

This function is called at the end of `romstage_common`. Only one board
makes use of it, the Lenovo ThinkPad T440p. To preserve behavior, call
it after `romstage_common` has done nearly everything.

Change-Id: I35742879e737be4f383a0e36aecc6682fc9df058
Signed-off-by: Angel Pons 
---
M src/mainboard/lenovo/t440p/romstage.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/romstage.c
3 files changed, 27 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43094/3

Change in coreboot[master]: nb/intel/haswell: Add `mb_late_romstage_setup` function

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Tristan Corrick, Alexander Couzens, Patrick Rudolph,

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

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

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

Change subject: nb/intel/haswell: Add `mb_late_romstage_setup` function
......................................................................

nb/intel/haswell: Add `mb_late_romstage_setup` function

This function is called at the end of `romstage_common`. Only one board
makes use of it, the Lenovo ThinkPad T440p. To preserve behavior, call
it after `romstage_common` has done nearly everything.

Change-Id: I35742879e737be4f383a0e36aecc6682fc9df058
Signed-off-by: Angel Pons 
---
M src/mainboard/lenovo/t440p/romstage.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/romstage.c
3 files changed, 27 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43094/4

Change in coreboot[master]: nb/intel/haswell: Add `mb_late_romstage_setup` function

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Add `mb_late_romstage_setup` function
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 1: Code-Review+2
> > > >
> > > > (1 comment)
> > > >
> > > > Perhaps a name such as `mb_romstage_final` would be more appropriate? If
> > > > the T440p PEG code runs after raminit, it will have to come after
> > > > `haswell_unhide_peg` too.
> > >
> > > I used the same name as in x4x, and to preserve the original behavior (which may be wrong) I placed the code so that it runs at the same point as before (only difference being the postcode call, which doesn't really matter). In any case, once I turn romstage_common inside-out in CB:43108 it will be easier to move the function call to a better place.
> >
> > Yes, the behaviour is preserved. My thought was that the T440p code
> > relies on more than just coming after raminit, since the PEG devices
> > need to be unhidden first. The name `mb_post_raminit_setup` doesn't
> > suggest that one could always rely on it being executed after the PEG
> > devices are unhidden. However, I think a name such as `mb_romstage_final`
> > shows that one can rely on it running after all the common romstage code,
> > including the PEG devices being unhidden.
> >
> > When thinking of future boards that might use it, I think it's much more
> > likely to have use cases that depend on running after all the common
> > romstage code than immediately after raminit and before the end of
> > common romstage.
> >
> > What I could see potentially happening in the future is it being decided
> > to move `mb_post_raminit_setup` immediately after raminit, for whatever
> > reason, which would break the T440p PEG code as I understand it. Unlikely?
> > Perhaps. But I don't see a downside to changing the name.
>
> Sorry, I said x4x but `mb_post_raminit_setup` is on gm45. And it's called right after raminit, hence the name.

I went with `mb_late_romstage_setup`.

Change in coreboot[master]: nb/intel/haswell: Add `mb_late_romstage_setup` function

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_late_romstage_setup` function
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: nb/intel/haswell: Add `mb_late_romstage_setup` function

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43094 )

Change subject: nb/intel/haswell: Add `mb_late_romstage_setup` function
......................................................................

nb/intel/haswell: Add `mb_late_romstage_setup` function

This function is called at the end of `romstage_common`. Only one board
makes use of it, the Lenovo ThinkPad T440p. To preserve behavior, call
it after `romstage_common` has done nearly everything.

Change-Id: I35742879e737be4f383a0e36aecc6682fc9df058
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43094
Reviewed-by: Tristan Corrick 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/lenovo/t440p/romstage.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/romstage.c
3 files changed, 27 insertions(+), 17 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tristan Corrick: Looks good to me, approved

diff --git a/src/mainboard/lenovo/t440p/romstage.c b/src/mainboard/lenovo/t440p/romstage.c
index bd1020d..900bef5 100644
--- a/src/mainboard/lenovo/t440p/romstage.c
+++ b/src/mainboard/lenovo/t440p/romstage.c
@@ -23,6 +23,26 @@
    RCBA16(D20IR) = DIR_ROUTE(PIRQA, PIRQB, PIRQC, PIRQD);
 }

+void mb_late_romstage_setup(void)
+{
+	u8 enable_peg;
+	if (get_option(&enable_peg, "enable_dual_graphics") != CB_SUCCESS)
+		enable_peg = 0;
+
+	bool power_en = pmh7_dgpu_power_state();
+
+	if (enable_peg != power_en)
+		pmh7_dgpu_power_enable(!power_en);
+
+	if (!enable_peg) {
+		// Hide disabled dGPU device
+		u32 reg32 = pci_read_config32(PCI_DEV(0, 0, 0), DEVEN);
+		reg32 &= ~DEVEN_D1F0EN;
+
+		pci_write_config32(PCI_DEV(0, 0, 0), DEVEN, reg32);
+	}
+}
+
 void mainboard_romstage_entry(void)
 {
    struct pei_data pei_data = {
@@ -77,21 +97,4 @@
    };

romstage_common(&romstage_params);
-
-	u8 enable_peg;
-	if (get_option(&enable_peg, "enable_dual_graphics") != CB_SUCCESS)
-		enable_peg = 0;
-
-	bool power_en = pmh7_dgpu_power_state();
-
-	if (enable_peg != power_en)
-		pmh7_dgpu_power_enable(!power_en);
-
-	if (!enable_peg) {
-		// Hide disabled dGPU device
-		u32 reg32 = pci_read_config32(PCI_DEV(0, 0, 0), DEVEN);
-		reg32 &= ~DEVEN_D1F0EN;
-
-		pci_write_config32(PCI_DEV(0, 0, 0), DEVEN, reg32);
-	}
 }
diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index fa32eca..7fb24c8 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -195,6 +195,7 @@
    void (*copy_spd)(struct pei_data *peid);
 };
 void romstage_common(const struct romstage_params *params);
+void mb_late_romstage_setup(void); /* optional */

void haswell_early_initialization(void);
 void haswell_late_initialization(void);
diff --git a/src/northbridge/intel/haswell/romstage.c b/src/northbridge/intel/haswell/romstage.c
index 8cf2e7c..ae9d707 100644
--- a/src/northbridge/intel/haswell/romstage.c
+++ b/src/northbridge/intel/haswell/romstage.c
@@ -13,6 +13,10 @@
 #include <southbridge/intel/lynxpoint/pch.h>
 #include <southbridge/intel/lynxpoint/me.h>

+void __weak mb_late_romstage_setup(void)
+{
+}
+
 void romstage_common(const struct romstage_params *params)
 {
    int wake_from_s3;
@@ -77,5 +81,7 @@

romstage_handoff_init(wake_from_s3);

+	mb_late_romstage_setup();
+
    post_code(0x3f);
 }