Change in coreboot[master]: mb/google/volteer: Clean up romstage.c

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

mb/google/volteer: Clean up romstage.c

Move the manual calls to fw_config_probe() into the devicetree; the
AUDIO probe is trivial, and the TCSS devices (DMA0, iTBT RP0 & RP1) are
already guarded with probe statements in the baseboard devicetree, so
the code in romstage.c was redundant.

Change-Id: I1d067ff3d181b152c784634ff99202bb2b9202f7
Signed-off-by: Tim Wawrzynczak 
---
M src/mainboard/google/volteer/romstage.c
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
2 files changed, 7 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/48512/1

diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c
index 315ec20..67d3489 100644
--- a/src/mainboard/google/volteer/romstage.c
+++ b/src/mainboard/google/volteer/romstage.c
@@ -22,17 +22,5 @@
    };
    bool half_populated = gpio_get(GPIO_MEM_CH_SEL);

-	/* Disable HDA device if no audio board is present. */
-	if (fw_config_probe(FW_CONFIG(AUDIO, NONE)))
-		mem_cfg->PchHdaEnable = 0;
-
    meminit_ddr(mem_cfg, board_cfg, &spd_info, half_populated);
-
-	/* Disable TBT if no USB4 hardware */
-	if (!(fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) ||
-		    fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)))) {
-		mem_cfg->TcssDma0En = 0;
-		mem_cfg->TcssItbtPcie0En = 0;
-		mem_cfg->TcssItbtPcie1En = 0;
-	}
 }
diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
index 7964885..c61b003 100644
--- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
@@ -501,6 +501,12 @@
    			device pnp 0c09.0 on end
    		end
    	end
-		device ref hda on end
+		device ref hda on
+			probe AUDIO MAX98357_ALC5682I_I2S
+			probe AUDIO MAX98373_ALC5682I_I2S
+			probe AUDIO MAX98373_ALC5682_SNDW
+			probe AUDIO MAX98373_ALC5682I_I2S_UP4
+			probe AUDIO MAX98360_ALC5682I_I2S
+		end
    end
 end

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/48512/1//COMMIT_MSG@13 
PS1, Line 13: 
Missing BUG and TEST fields.

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1:

I think these were here so they got passed to FSP, there might be side effects of removing it.. I suspect the HDA one isn't a big deal since it was more of a problem with early boards when sound wasn't working at all, but I can't remember the details with the TBT UPDs..

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I think these were here so they got passed to FSP, there might be side effects of removing it.. I suspect the HDA one isn't a big deal since it was more of a problem with early boards when sound wasn't working at all, but I can't remember the details with the TBT UPDs..

The earlier change was done as part of b/167983038. The problem at that time was the TBT RPs were kept enabled in devicetree/overridetree even if the board wasn't using them. This caused the UPDs to be enabled as well. I think we need to ensure that the TBT RP and DMA device status in devicetree/overridetree is correct for all variants.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > I think these were here so they got passed to FSP, there might be side effects of removing it.. I suspect the HDA one isn't a big deal since it was more of a problem with early boards when sound wasn't working at all, but I can't remember the details with the TBT UPDs..
>
> The earlier change was done as part of b/167983038. The problem at that time was the TBT RPs were kept enabled in devicetree/overridetree even if the board wasn't using them. This caused the UPDs to be enabled as well. I think we need to ensure that the TBT RP and DMA device status in devicetree/overridetree is correct for all variants.

Good point, I'll go scrub the variants

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > I think these were here so they got passed to FSP, there might be side effects of removing it.. I suspect the HDA one isn't a big deal since it was more of a problem with early boards when sound wasn't working at all, but I can't remember the details with the TBT UPDs..
> >
> > The earlier change was done as part of b/167983038. The problem at that time was the TBT RPs were kept enabled in devicetree/overridetree even if the board wasn't using them. This caused the UPDs to be enabled as well. I think we need to ensure that the TBT RP and DMA device status in devicetree/overridetree is correct for all variants.
>
> Good point, I'll go scrub the variants

voxel is the only one I know so far that's using usb4, and it's overridetree looks correct to me for all the tbt_* devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage.c
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48512/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/romstage.c:

https://review.coreboot.org/c/coreboot/+/48512/1/src/mainboard/google/voltee... 
PS1, Line 31: USB4
This needs update too: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt...

Change in coreboot[master]: mb/google/volteer: Clean up romstage UPDs

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Nick Vaccaro,

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

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

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

Change subject: mb/google/volteer: Clean up romstage UPDs
......................................................................

mb/google/volteer: Clean up romstage UPDs

Move the manual calls to fw_config_probe() into the devicetree; the
AUDIO probe is trivial, and the TCSS devices (DMA0, iTBT RP0 & RP1) are
already guarded with probe statements in the baseboard devicetree, so
the code in romstage.c was redundant. The variants seem to have their
USB4 probe statements correct as well, so the manual UPD setting in
mainboard.c was also unnecessary.

Change-Id: I1d067ff3d181b152c784634ff99202bb2b9202f7
Signed-off-by: Tim Wawrzynczak 
---
M src/mainboard/google/volteer/mainboard.c
M src/mainboard/google/volteer/romstage.c
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
3 files changed, 7 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/48512/2

Change in coreboot[master]: mb/google/volteer: Clean up romstage UPDs

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage UPDs
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: mb/google/volteer: Clean up romstage UPDs

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage UPDs
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/48512/2//COMMIT_MSG@7 
PS2, Line 7: romstage
Both romstage and ramstage

Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Duncan Laurie, Nick Vaccaro,

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

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

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

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

mb/google/volteer: Clean up romstage and ramstage UPDs

Move the manual calls to fw_config_probe() into the devicetree; the
AUDIO probe is trivial, and the TCSS devices (DMA0, iTBT RP0 & RP1) are
already guarded with probe statements in the baseboard devicetree, so
the code in romstage.c was redundant. The variants seem to have their
USB4 probe statements correct as well, so the manual UPD setting in
mainboard.c was also unnecessary.

Change-Id: I1d067ff3d181b152c784634ff99202bb2b9202f7
Signed-off-by: Tim Wawrzynczak 
---
M src/mainboard/google/volteer/mainboard.c
M src/mainboard/google/volteer/romstage.c
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
3 files changed, 7 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/48512/3

Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/48512/2//COMMIT_MSG@7 
PS2, Line 7: romstage
> Both romstage and ramstage

Done

Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Duncan Laurie, Nick Vaccaro,

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

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

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

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

mb/google/volteer: Clean up romstage and ramstage UPDs

Move the manual calls to fw_config_probe() into the devicetree; the
AUDIO probe is trivial, and the TCSS devices (DMA0, iTBT RP0 & RP1) are
already guarded with probe statements in the baseboard devicetree, so
the code in romstage.c was redundant. The variants seem to have their
USB4 probe statements correct as well, so the manual UPD setting in
mainboard.c was also unnecessary.

BUG=none
TEST=abuild google/volteer

Change-Id: I1d067ff3d181b152c784634ff99202bb2b9202f7
Signed-off-by: Tim Wawrzynczak 
---
M src/mainboard/google/volteer/mainboard.c
M src/mainboard/google/volteer/romstage.c
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
3 files changed, 7 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/48512/4

Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

Patch Set 4:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/48512/1//COMMIT_MSG@13 
PS1, Line 13:
> Missing BUG and TEST fields.

Done

https://review.coreboot.org/c/coreboot/+/48512/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/romstage.c:

https://review.coreboot.org/c/coreboot/+/48512/1/src/mainboard/google/voltee... 
PS1, Line 31: USB4
> This needs update too: https://review.coreboot.org/cgit/coreboot. […]

Done

Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48512 )

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

mb/google/volteer: Clean up romstage and ramstage UPDs

Move the manual calls to fw_config_probe() into the devicetree; the
AUDIO probe is trivial, and the TCSS devices (DMA0, iTBT RP0 & RP1) are
already guarded with probe statements in the baseboard devicetree, so
the code in romstage.c was redundant. The variants seem to have their
USB4 probe statements correct as well, so the manual UPD setting in
mainboard.c was also unnecessary.

BUG=none
TEST=abuild google/volteer

Change-Id: I1d067ff3d181b152c784634ff99202bb2b9202f7
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48512
Tested-by: build bot (Jenkins) 
Reviewed-by: Duncan Laurie 
Reviewed-by: Furquan Shaikh 
---
M src/mainboard/google/volteer/mainboard.c
M src/mainboard/google/volteer/romstage.c
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
3 files changed, 7 insertions(+), 27 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c
index 1f20e18..4d65245 100644
--- a/src/mainboard/google/volteer/mainboard.c
+++ b/src/mainboard/google/volteer/mainboard.c
@@ -165,20 +165,6 @@
    }
 }

-void mainboard_silicon_init_params(FSP_S_CONFIG *params)
-{
-	bool has_usb4;
-
-	/* If device doesn't have USB4 hardware, disable tbt */
-	has_usb4 = (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2))
-		    || fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)));
-
-	if (!has_usb4)
-		memset(params->ITbtPcieRootPortEn, 0,
-		       ARRAY_SIZE(params->ITbtPcieRootPortEn)
-			       * sizeof(*params->ITbtPcieRootPortEn));
-}
-
 struct chip_operations mainboard_ops = {
    .init = mainboard_chip_init,
    .enable_dev = mainboard_enable,
diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c
index 315ec20..67d3489 100644
--- a/src/mainboard/google/volteer/romstage.c
+++ b/src/mainboard/google/volteer/romstage.c
@@ -22,17 +22,5 @@
    };
    bool half_populated = gpio_get(GPIO_MEM_CH_SEL);

-	/* Disable HDA device if no audio board is present. */
-	if (fw_config_probe(FW_CONFIG(AUDIO, NONE)))
-		mem_cfg->PchHdaEnable = 0;
-
    meminit_ddr(mem_cfg, board_cfg, &spd_info, half_populated);
-
-	/* Disable TBT if no USB4 hardware */
-	if (!(fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) ||
-		    fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)))) {
-		mem_cfg->TcssDma0En = 0;
-		mem_cfg->TcssItbtPcie0En = 0;
-		mem_cfg->TcssItbtPcie1En = 0;
-	}
 }
diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
index 30625ca..01bbf1e 100644
--- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
@@ -523,6 +523,12 @@
    			device pnp 0c09.0 on end
    		end
    	end
-		device ref hda on end
+		device ref hda on
+			probe AUDIO MAX98357_ALC5682I_I2S
+			probe AUDIO MAX98373_ALC5682I_I2S
+			probe AUDIO MAX98373_ALC5682_SNDW
+			probe AUDIO MAX98373_ALC5682I_I2S_UP4
+			probe AUDIO MAX98360_ALC5682I_I2S
+		end
    end
 end

[M] Change in coreboot[master]: mb/google/volteer: Clean up romstage and ramstage UPDs

[Matt DeVillier (Code Review)]

Attention is currently required from: Tim Wawrzynczak.

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

Change subject: mb/google/volteer: Clean up romstage and ramstage UPDs
......................................................................

Patch Set 5:

(1 comment)

File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/48512/comment/1a82961d_e435eab3 
PS5, Line 526: device ref hda on
             : 			probe AUDIO MAX98357_ALC5682I_I2S
             : 			probe AUDIO MAX98373_ALC5682I_I2S
             : 			probe AUDIO MAX98373_ALC5682_SNDW
             : 			probe AUDIO MAX98373_ALC5682I_I2S_UP4
             : 			probe AUDIO MAX98360_ALC5682I_I2S
             : 		end
you missed the RT1011_ALC5682I_I2S codec, which means all boards using that codec now have their HDA device disabled - whoops! Fixed in CB:71150