Change in coreboot[master]: mb/google/volteer: Add firmware configuration table for Halvor
Frank Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration table for Halvor ......................................................................
mb/google/volteer: Add firmware configuration table for Halvor
According to chrome-internal:3217187, add MAX98373_ALC5682I_I2S_UP4 firmware configuration table for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/1
diff --git a/src/mainboard/google/volteer/fw_config.c b/src/mainboard/google/volteer/fw_config.c index 61e20f4..e930d9f 100644 --- a/src/mainboard/google/volteer/fw_config.c +++ b/src/mainboard/google/volteer/fw_config.c @@ -45,6 +45,18 @@ PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM */ };
+static const struct pad_config i2s_up4_enable_pads[] = { + PAD_CFG_NF(GPP_A7, NONE, DEEP, NF1), /* I2S1_SCLK */ + PAD_CFG_NF(GPP_D19, NONE, DEEP, NF1), /* I2S_MCLK1 */ + PAD_CFG_NF(GPP_R0, NONE, DEEP, NF2), /* I2S0_SCLK */ + PAD_CFG_NF(GPP_R1, NONE, DEEP, NF2), /* I2S0_SFRM */ + PAD_CFG_NF(GPP_R2, DN_20K, DEEP, NF2), /* I2S0_TXD */ + PAD_CFG_NF(GPP_R3, NONE, DEEP, NF2), /* I2S0_RXD */ + PAD_CFG_NF(GPP_R5, NONE, DEEP, NF2), /* I2S1_RXD */ + PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ + PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM */ +}; + static const struct pad_config i2s_disable_pads[] = { PAD_NC(GPP_A23, NONE), PAD_NC(GPP_D19, NONE), @@ -78,5 +90,11 @@ 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))) { + 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)); + } } BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, fw_config_handle, NULL); diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index ffae2f0..c8245c0 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -13,6 +13,7 @@ option MAX98357_ALC5682I_I2S 1 option MAX98373_ALC5682I_I2S 2 option MAX98373_ALC5682_SNDW 3 + option MAX98373_ALC5682I_I2S_UP4 4 end field TABLETMODE 11 option TABLETMODE_DISABLED 0
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration table for Halvor ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44560/1//COMMIT_MSG@9 PS1, Line 9: According to chrome-internal:3217187, add MAX98373_ALC5682I_I2S_UP4 firmware : configuration table for Halvor. Perhaps a bit more descriptive, something like "Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design."
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration table for Halvor ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 36: i2s_enable_pads i'd suggest renaming this to i2s_up3_enable_pads.
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 49: I2S1_SCLK I2S2_SCLK
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 55: GPP_R5 is this a NC on the schematics?
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 56: PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ : PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM */ i don't see these on the schematics.
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 74: NONE a note about having CBI programmed before you need this. otherwise, you'll hit this case and and loose GPP_A23 HP_INT_L functionality.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Duncan Laurie, Tim Wawrzynczak, Paul Fagerburg, Angel Pons, Nick Vaccaro, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44560
to look at the new patch set (#2).
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 22 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/2
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44560/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44560/1//COMMIT_MSG@9 PS1, Line 9: According to chrome-internal:3217187, add MAX98373_ALC5682I_I2S_UP4 firmware : configuration table for Halvor.
Perhaps a bit more descriptive, something like "Add MAX98373_ALC5682I_I2S_UP4 firmware configuration […]
Done
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 36: i2s_enable_pads
i'd suggest renaming this to i2s_up3_enable_pads.
Done
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 49: I2S1_SCLK
I2S2_SCLK
Done
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 55: GPP_R5
is this a NC on the schematics?
I set R5,R6,R7 as NC pin at first. However, the audio cannot work. The audio function is enable after configuring them as NF2 like i2s_up3_enable. Therefore, I put them as NF2 even they are not used in Halvor.
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 56: PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ : PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM */
i don't see these on the schematics.
I set R5,R6,R7 as NC pin at first. However, the audio cannot work. The audio function is enable after configuring them as NF2 like i2s_up3_enable. Therefore, I put them as NF2 even they are not used in Halvor.
https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... PS1, Line 74: NONE
a note about having CBI programmed before you need this. […]
Thanks for reminding. I will set the fw_config correct before testing the Halvor.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Duncan Laurie, Tim Wawrzynczak, Paul Fagerburg, Angel Pons, Nick Vaccaro, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44560
to look at the new patch set (#3).
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 22 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/3
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Duncan Laurie, Tim Wawrzynczak, Paul Fagerburg, Angel Pons, Nick Vaccaro, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44560
to look at the new patch set (#4).
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 22 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/4
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4: Code-Review+1
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 96: dmic_enable_pads Are dmic gpios the same for UP3 and UP4?
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 96: dmic_enable_pads
Are dmic gpios the same for UP3 and UP4?
In Halvor: The GPP_S4(SOC_DMIC_CLK1) and GPP_S5(SOC_DMIC_DATA1) are preserved for DMIC1. The GPP_S6(SOC_DMIC_CLK0) and GPP_S7(SOC_DMIC_DATA0) are used for DMIC0. All of them are NF2.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 49: I2S2_SCLK could someone point me at the relevant intel docid? i was trying to determine if you can use I2S2_SCLK in combination with other signals coming from I2S1.
also, the kernel driver would probably need to know this. is this part of the "topology" configuration?
Sathyanarayana Nujella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 49: I2S2_SCLK
could someone point me at the relevant intel docid? […]
For 2nd part of question, I would like to reply: yes kernel driver needs to know for proper topology loading.. currently we are relying on DMI information. Duncan recently told, he will add DMI:OEM_String for UP4. That would be sufficient for kernel driver and in OS side.
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
Hi Google/Intel,
Is there suggestion for us to fix the CL? Thank you very much.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 49: I2S2_SCLK
For 2nd part of question, I would like to reply: […]
CB:44783 adds the DMI quirk strings, so we should be good there w/r/t topology.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4: Code-Review+2
Kevin Cheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 49: I2S2_SCLK
CB:44783 adds the DMI quirk strings, so we should be good there w/r/t topology.
It also required to add "probe AUDIO" in overridetree.cb to enable CB:44783
device i2c 31 on + probe AUDIO MAX98373_ALC5682I_I2S_UP4 end
device i2c 32 on + probe AUDIO MAX98373_ALC5682I_I2S_UP4 end
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 55: PAD_CFG_NF(GPP_R5, NONE, DEEP, NF2), /* I2S1_RXD */ : PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ : PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM * Can we root cause why I2S1 needs here? Some pin are configure for other function in other projects. Change it NF here may break functions in other project.
Hello Sathyanarayana Nujella, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Caveh Jalali, Tim Wawrzynczak, Duncan Laurie, Paul Fagerburg, Angel Pons, Nick Vaccaro, Srinidhi N Kaushik, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44560
to look at the new patch set (#5).
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/mainboard/google/volteer/variants/halvor/overridetree.cb 3 files changed, 28 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/5
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 5:
(2 comments)
Hi Google,
Would help review the CL again after updating the overridetree.cb? Thank you.
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 49: I2S2_SCLK
It also required to add "probe AUDIO" in overridetree.cb to enable CB:44783 […]
Done
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 55: PAD_CFG_NF(GPP_R5, NONE, DEEP, NF2), /* I2S1_RXD */ : PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ : PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM *
Can we root cause why I2S1 needs here? Some pin are configure for other function in other projects. […]
I set R5,R6,R7 as NC pin at first. However, the audio cannot work. The audio function is enable after configuring them as NF2 like i2s_up3_enable. Therefore, I put them as NF2 even they are not used in Halvor.
as reply in the past: https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/halvor/overridetree.cb:
PS5: I would prefer to see this change in a separate CL on top of this one, specific to Halvor; the rest of this CL is not board-specific.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/halvor/overridetree.cb:
PS5:
I would prefer to see this change in a separate CL on top of this one, specific to Halvor; the rest […]
apologies for going a bit beyond the scope of this patch.
i think we should update volteer/fw_config.c::fw_config_handle() to also update update lib/fw_config.c::cached_configs[]. not directly, but by making fw_config_probe() a caching call.
that will remove the somewhat artificial requirement to have these probe statements dangling off of i2c devices. it is not at all obvious that a device probe statement here would also inform the kernel of the audio configuration.
Kevin Cheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/4/src/mainboard/google/voltee... PS4, Line 55: PAD_CFG_NF(GPP_R5, NONE, DEEP, NF2), /* I2S1_RXD */ : PAD_CFG_NF(GPP_R6, NONE, DEEP, NF2), /* I2S1_TXD */ : PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM *
I set R5,R6,R7 as NC pin at first. […]
The up4 design only use I2S0 and I2S2. The I2S1 pins are define for different function in project. It will impact other project when it configure here as a common path for all up4 design. I saw you also program the R5, R6, R7 in cb:44409 gpio.c. Can we leave these pins there? and why other I2S2 pins didn't need here and only in gpio.c?
Hello Sathyanarayana Nujella, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Caveh Jalali, Tim Wawrzynczak, Duncan Laurie, Paul Fagerburg, Angel Pons, Nick Vaccaro, Srinidhi N Kaushik, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44560
to look at the new patch set (#6).
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 19 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/44560/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/halvor/overridetree.cb:
PS5:
apologies for going a bit beyond the scope of this patch. […]
We went back and forth on that change specifically about making the manual `probe` calls cache as well, and we decided against it. The main point of the fw_config probe statements is to enable/disable devices in the devicetree, so we generate the correct ACPI tables, etc.
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 7:
(1 comment)
Would you help review the CL again? Thank you very much.
https://review.coreboot.org/c/coreboot/+/44560/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/halvor/overridetree.cb:
PS5:
We went back and forth on that change specifically about making the manual `probe` calls cache as we […]
Ack
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 7: Code-Review+1
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/7/src/mainboard/google/voltee... PS7, Line 85: on UP3 How do you know this is UP3? Is the only UP4 supported configuration MAX98373_ALC5682I_I2S_UP4?
Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44560/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44560/7/src/mainboard/google/voltee... PS7, Line 85: on UP3
How do you know this is UP3? Is the only UP4 supported configuration MAX98373_ALC5682I_I2S_UP4?
I took the suggestion here. https://review.coreboot.org/c/coreboot/+/44560/1/src/mainboard/google/voltee... => Caveh Jalali => Aug 19 => i'd suggest renaming this to i2s_up3_enable_pads.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44560 )
Change subject: mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4 ......................................................................
mb/google/volteer: Add firmware configuration for MAX98373_ALC5682I_I2S_UP4
Add MAX98373_ALC5682I_I2S_UP4 firmware configuration option and configure GPIOs properly for UP4 design. The design is also for Halvor.
BUG=b:153680359, b:163382106 TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, fw_config value in Halvor:
AUDIO=MAX98373_ALC5682I_I2S_UP4
ectool cbi set 6 0x00000400 4 2
Signed-off-by: Frank Wu frank_wu@compal.corp-partner.google.com Change-Id: Ie25f278dfbdc2f41a36b70403699a2e3c2234600 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44560 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Caveh Jalali caveh@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/fw_config.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 2 files changed, 19 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Caveh Jalali: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/fw_config.c b/src/mainboard/google/volteer/fw_config.c index 61e20f4..0538e74 100644 --- a/src/mainboard/google/volteer/fw_config.c +++ b/src/mainboard/google/volteer/fw_config.c @@ -33,7 +33,7 @@ PAD_NC(GPP_S3, NONE), };
-static const struct pad_config i2s_enable_pads[] = { +static const struct pad_config i2s_up3_enable_pads[] = { PAD_CFG_NF(GPP_A23, NONE, DEEP, NF1), /* I2S1_SCLK */ PAD_CFG_NF(GPP_D19, NONE, DEEP, NF1), /* I2S_MCLK1 */ PAD_CFG_NF(GPP_R0, NONE, DEEP, NF2), /* I2S0_SCLK */ @@ -45,6 +45,15 @@ PAD_CFG_NF(GPP_R7, NONE, DEEP, NF2), /* I2S1_SFRM */ };
+static const struct pad_config i2s_up4_enable_pads[] = { + PAD_CFG_NF(GPP_A7, NONE, DEEP, NF1), /* I2S2_SCLK */ + PAD_CFG_NF(GPP_D19, NONE, DEEP, NF1), /* I2S_MCLK1 */ + PAD_CFG_NF(GPP_R0, NONE, DEEP, NF2), /* I2S0_SCLK */ + PAD_CFG_NF(GPP_R1, NONE, DEEP, NF2), /* I2S0_SFRM */ + PAD_CFG_NF(GPP_R2, DN_20K, DEEP, NF2), /* I2S0_TXD */ + PAD_CFG_NF(GPP_R3, NONE, DEEP, NF2), /* I2S0_RXD */ +}; + static const struct pad_config i2s_disable_pads[] = { PAD_NC(GPP_A23, NONE), PAD_NC(GPP_D19, NONE), @@ -73,8 +82,14 @@ } if (fw_config_probe(FW_CONFIG(AUDIO, MAX98357_ALC5682I_I2S)) || fw_config_probe(FW_CONFIG(AUDIO, MAX98373_ALC5682I_I2S))) { - printk(BIOS_INFO, "Configure GPIOs for I2S audio.\n"); - gpio_configure_pads(i2s_enable_pads, ARRAY_SIZE(i2s_enable_pads)); + 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))) { + 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)); } diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index 7b018b2..3c1c5f0 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -13,6 +13,7 @@ option MAX98357_ALC5682I_I2S 1 option MAX98373_ALC5682I_I2S 2 option MAX98373_ALC5682_SNDW 3 + option MAX98373_ALC5682I_I2S_UP4 4 end field TABLETMODE 11 option TABLETMODE_DISABLED 0
-
Caveh Jalali (Code Review) -
Duncan Laurie (Code Review)
-
Frank Wu (Code Review)
-
Kevin Cheng (Code Review)
-
Nick Vaccaro (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Sathyanarayana Nujella (Code Review)
-
Tim Wawrzynczak (Code Review)