Change in coreboot[master]: mb/google/volteer: Switch to fw_config_probe_nodefault when appropriate
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Switch to fw_config_probe_nodefault when appropriate ......................................................................
mb/google/volteer: Switch to fw_config_probe_nodefault when appropriate
In cases when a volteer device is unprovisioned, the safest thing to do for GPIOs that will normally be used for audio codec buses is to leave them disabled (configured as PAD_CFG_NC). The same is true for USB4 support; if it is not know explicitly to be supported, then leave the iTBT PCIe RPs disabled.
Change-Id: I8efd101174f6e3d7233d2bf803b680673cada81a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/mainboard.c M src/mainboard/google/volteer/romstage.c 3 files changed, 26 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47972/1
diff --git a/src/mainboard/google/volteer/fw_config.c b/src/mainboard/google/volteer/fw_config.c index 50e8f93..7be84bf 100644 --- a/src/mainboard/google/volteer/fw_config.c +++ b/src/mainboard/google/volteer/fw_config.c @@ -72,33 +72,43 @@
static void fw_config_handle(void *unused) { + /* + * When FW_CONFIG is undefined (e.g., in an unprovisioned board), we + * don't yet know which audio codec is connected. Our policy here is: + * 1) Configure bus-specific GPIOs which are not yet assigned to a device as NC. + * 2) Expose all of the devices in the ACPI tables, so they can be probed at runtime + * discovery and provisioning. + * Here, that means using fw_config_probe for AUDIO,NONE because it will return true + * for an unprovisioned board, and all GPIOs will get enabled; fw_config_probe_nodefault + * can only return true on provisioned boards. + */ if (fw_config_probe(FW_CONFIG(AUDIO, NONE))) { printk(BIOS_INFO, "Configure GPIOs for no audio.\n"); gpio_configure_pads(i2s_disable_pads, ARRAY_SIZE(i2s_disable_pads)); gpio_configure_pads(dmic_disable_pads, ARRAY_SIZE(dmic_disable_pads)); gpio_configure_pads(sndw_disable_pads, ARRAY_SIZE(sndw_disable_pads)); } - if (fw_config_probe(FW_CONFIG(AUDIO, MAX98373_ALC5682_SNDW))) { + if (fw_config_probe_nodefault(FW_CONFIG(AUDIO, MAX98373_ALC5682_SNDW))) { printk(BIOS_INFO, "Configure GPIOs for SoundWire audio.\n"); gpio_configure_pads(sndw_enable_pads, ARRAY_SIZE(sndw_enable_pads)); gpio_configure_pads(dmic_enable_pads, ARRAY_SIZE(dmic_enable_pads)); gpio_configure_pads(i2s_disable_pads, ARRAY_SIZE(i2s_disable_pads)); } - if (fw_config_probe(FW_CONFIG(AUDIO, MAX98357_ALC5682I_I2S)) || - fw_config_probe(FW_CONFIG(AUDIO, MAX98373_ALC5682I_I2S)) || - fw_config_probe(FW_CONFIG(AUDIO, MAX98360_ALC5682I_I2S))) { + if (fw_config_probe_nodefault(FW_CONFIG(AUDIO, MAX98357_ALC5682I_I2S)) || + fw_config_probe_nodefault(FW_CONFIG(AUDIO, MAX98373_ALC5682I_I2S)) || + fw_config_probe_nodefault(FW_CONFIG(AUDIO, MAX98360_ALC5682I_I2S))) { printk(BIOS_INFO, "Configure GPIOs for I2S audio on UP3.\n"); gpio_configure_pads(i2s_up3_enable_pads, ARRAY_SIZE(i2s_up3_enable_pads)); gpio_configure_pads(dmic_enable_pads, ARRAY_SIZE(dmic_enable_pads)); gpio_configure_pads(sndw_disable_pads, ARRAY_SIZE(sndw_disable_pads)); } - if (fw_config_probe(FW_CONFIG(AUDIO, MAX98373_ALC5682I_I2S_UP4))) { + if (fw_config_probe_nodefault(FW_CONFIG(AUDIO, MAX98373_ALC5682I_I2S_UP4))) { printk(BIOS_INFO, "Configure GPIOs for I2S audio on UP4.\n"); gpio_configure_pads(i2s_up4_enable_pads, ARRAY_SIZE(i2s_up4_enable_pads)); gpio_configure_pads(dmic_enable_pads, ARRAY_SIZE(dmic_enable_pads)); gpio_configure_pads(sndw_disable_pads, ARRAY_SIZE(sndw_disable_pads)); } - if (fw_config_probe(FW_CONFIG(DB_SD, SD_GL9755S))) { + if (fw_config_probe_nodefault(FW_CONFIG(DB_SD, SD_GL9755S))) { printk(BIOS_INFO, "Configure GPIOs for SD GL9755S.\n"); gpio_configure_pads(sd_gl9755s_pads, ARRAY_SIZE(sd_gl9755s_pads)); } diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index ea9e08f..2f016ab 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -53,10 +53,10 @@ if (!conn) return;
- if (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_ACTIVE)) || - fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_NO_A))) { + if (fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN2)) || + fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB3_ACTIVE)) || + fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN3)) || + fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB3_NO_A))) { struct drivers_intel_pmc_mux_conn_config *config = conn->chip_info;
if (config) { @@ -164,8 +164,8 @@ 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))); + has_usb4 = (fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN2)) + || fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN3)));
if (!has_usb4) memset(params->ITbtPcieRootPortEn, 0, diff --git a/src/mainboard/google/volteer/romstage.c b/src/mainboard/google/volteer/romstage.c index 315ec20..a529b75 100644 --- a/src/mainboard/google/volteer/romstage.c +++ b/src/mainboard/google/volteer/romstage.c @@ -22,15 +22,15 @@ }; bool half_populated = gpio_get(GPIO_MEM_CH_SEL);
- /* Disable HDA device if no audio board is present. */ + /* Disable HDA device if no audio board is present (or FW_CONFIG is undefined). */ 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)))) { + /* Disable TBT if no USB4 hardware (or if unknown) */ + if (!(fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN2)) || + fw_config_probe_nodefault(FW_CONFIG(DB_USB, USB4_GEN3)))) { mem_cfg->TcssDma0En = 0; mem_cfg->TcssItbtPcie0En = 0; mem_cfg->TcssItbtPcie1En = 0;
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/+/47972
to look at the new patch set (#2).
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
mb/google/volteer: Make use of fw_config_is_provisioned()
In cases when a volteer device is unprovisioned, the safest thing to do for GPIOs that will normally be used for audio codec buses is to leave them disabled (configured as PAD_CFG_NC). The same is true for USB4 support; if it is not know explicitly to be supported, then leave the iTBT PCIe RPs disabled.
Change-Id: I8efd101174f6e3d7233d2bf803b680673cada81a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/romstage.c 2 files changed, 39 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47972/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 92: gpio_configure_pads(i2s_up3_enable_pads, ARRAY_SIZE(i2s_up3_enable_pads)); line over 96 characters
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 98: gpio_configure_pads(i2s_up4_enable_pads, ARRAY_SIZE(i2s_up4_enable_pads)); line over 96 characters
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 74: { nit: If you handle the unprovisioned case first, then you don't need to align the rest of the code by one additional tab.
if (!fw_config_is_provisioned()) { ... return; }
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/romstage.c:
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 27: mem_cfg->PchHdaEnable = 0; Not for this change, but I think, we should update baseboard/devicetree.cb so that `device ref hda on end` uses probe statements to enable HDA device. And then SoC code can set PchHdaEnable to 1/0 depending upon the state of the device. This will get rid of the checks here.
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 35: mem_cfg->TcssDma0En = 0; : mem_cfg->TcssItbtPcie0En = 0; : mem_cfg->TcssItbtPcie1En = 0; I think we already have devices in devicetree that correspond to each of these UPDs. In fact I see TGL SoC code setting these UPDs already. I think we should check if this code is really required. I am suspecting this is just some old code before we had all the fw_config setup done properly. Same for: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt...
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/+/47972
to look at the new patch set (#3).
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
mb/google/volteer: Make use of fw_config_is_provisioned()
In cases when a volteer device is unprovisioned, the safest thing to do for GPIOs that will normally be used for audio codec buses is to leave them disabled (configured as PAD_CFG_NC). This patch adds support for that.
Change-Id: I8efd101174f6e3d7233d2bf803b680673cada81a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/fw_config.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47972/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 74: {
nit: If you handle the unprovisioned case first, then you don't need to align the rest of the code b […]
Done
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/romstage.c:
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 27: mem_cfg->PchHdaEnable = 0;
Not for this change, but I think, we should update baseboard/devicetree.cb so that […]
Agreed
https://review.coreboot.org/c/coreboot/+/47972/2/src/mainboard/google/voltee... PS2, Line 35: mem_cfg->TcssDma0En = 0; : mem_cfg->TcssItbtPcie0En = 0; : mem_cfg->TcssItbtPcie1En = 0;
I think we already have devices in devicetree that correspond to each of these UPDs. […]
Looks redundant, cleaned up in followup.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47972/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47972/3//COMMIT_MSG@13 PS3, Line 13: Missing BUG and TEST fields.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 3: Code-Review+2
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/+/47972
to look at the new patch set (#4).
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
mb/google/volteer: Make use of fw_config_is_provisioned()
In cases when a volteer device is unprovisioned, the safest thing to do for GPIOs that will normally be used for audio codec buses is to leave them disabled (configured as PAD_CFG_NC). This patch adds support for that.
BUG=none TEST=add debug print to new if branch; remove fw_config from CBI and see print on console
Change-Id: I8efd101174f6e3d7233d2bf803b680673cada81a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/fw_config.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/47972/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47972/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47972/3//COMMIT_MSG@13 PS3, Line 13:
Missing BUG and TEST fields.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 4: Code-Review+2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
Patch Set 4: Code-Review+2
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47972 )
Change subject: mb/google/volteer: Make use of fw_config_is_provisioned() ......................................................................
mb/google/volteer: Make use of fw_config_is_provisioned()
In cases when a volteer device is unprovisioned, the safest thing to do for GPIOs that will normally be used for audio codec buses is to leave them disabled (configured as PAD_CFG_NC). This patch adds support for that.
BUG=none TEST=add debug print to new if branch; remove fw_config from CBI and see print on console
Change-Id: I8efd101174f6e3d7233d2bf803b680673cada81a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/47972 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Nick Vaccaro nvaccaro@google.com Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/volteer/fw_config.c 1 file changed, 7 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Nick Vaccaro: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/fw_config.c b/src/mainboard/google/volteer/fw_config.c index c444b4c..1025119 100644 --- a/src/mainboard/google/volteer/fw_config.c +++ b/src/mainboard/google/volteer/fw_config.c @@ -72,6 +72,13 @@
static void fw_config_handle(void *unused) { + if (!fw_config_is_provisioned()) { + gpio_configure_pads(i2s_disable_pads, ARRAY_SIZE(i2s_disable_pads)); + gpio_configure_pads(dmic_disable_pads, ARRAY_SIZE(dmic_disable_pads)); + gpio_configure_pads(sndw_disable_pads, ARRAY_SIZE(sndw_disable_pads)); + return; + } + if (fw_config_probe(FW_CONFIG(AUDIO, NONE))) { printk(BIOS_INFO, "Configure GPIOs for no audio.\n"); gpio_configure_pads(i2s_disable_pads, ARRAY_SIZE(i2s_disable_pads));
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)