Change in coreboot[master]: mb/google/volteer/halvor: Skip fw_config override on Halvor.

[Frank Wu (Code Review)]

Frank Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

mb/google/volteer/halvor: Skip fw_config override on Halvor.

Volteer and Halvor share the same fw_config MAX98373_ALC5682I_I2S.
However, the GPP_A23 is used as I2S1_SCLK on Volteer but HP_INT_L on Halvor.
The GPP_A23 is overridden as I2S1_SCLK at last, so the detection of headset
jack is not workable.
The audio settings are included in halvor/gpio.c and skip the fw_config on
Halvor to make the gpios correct.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage, then verify
that headset jack detection is fine on Halvor.

Signed-off-by: Frank Wu 
Change-Id: Ie644ed2b95d0e355f91d92f0b3c4ce14cd4afa98
---
M src/mainboard/google/volteer/fw_config.c
M src/mainboard/google/volteer/variants/halvor/include/variant/gpio.h
2 files changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/44455/1

diff --git a/src/mainboard/google/volteer/fw_config.c b/src/mainboard/google/volteer/fw_config.c
index 61e20f4..21eb3e1b 100644
--- a/src/mainboard/google/volteer/fw_config.c
+++ b/src/mainboard/google/volteer/fw_config.c
@@ -4,6 +4,7 @@
 #include <console/console.h>
 #include <fw_config.h>
 #include <gpio.h>
+#include <variant/gpio.h>

static const struct pad_config dmic_enable_pads[] = {
    PAD_CFG_NF(GPP_S4, NONE, DEEP, NF2),	/* DMIC_CLK1 */
@@ -59,6 +60,9 @@

static void fw_config_handle(void *unused)
 {
+#ifdef SKIP_FW_CONFIG_GPIO
+	return;
+#endif
    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));
diff --git a/src/mainboard/google/volteer/variants/halvor/include/variant/gpio.h b/src/mainboard/google/volteer/variants/halvor/include/variant/gpio.h
index fe512d8..ce32b3b 100644
--- a/src/mainboard/google/volteer/variants/halvor/include/variant/gpio.h
+++ b/src/mainboard/google/volteer/variants/halvor/include/variant/gpio.h
@@ -5,6 +5,7 @@

#include <baseboard/gpio.h>

+#define SKIP_FW_CONFIG_GPIO	1
 #undef GPIO_EC_IN_RW
 /* EC in RW */
 #define GPIO_EC_IN_RW		GPP_F17

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1: Code-Review+1

@Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.

FW_CONFIG is already a config option. we probably just need to disable
it on variants where it is not required.

volteer and volteer2 (the actual boards) need it but there are several
variants that do not because they only have a single pre-determined
config for audio, usb, etc.

do you plan to have multiple audio/sd/usb configs on halvor that would
need runtime selection using fw_config?

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1: Code-Review+1
> >
> > @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.
>
> FW_CONFIG is already a config option. we probably just need to disable
> it on variants where it is not required.
>
> volteer and volteer2 (the actual boards) need it but there are several
> variants that do not because they only have a single pre-determined
> config for audio, usb, etc.
>
> do you plan to have multiple audio/sd/usb configs on halvor that would
> need runtime selection using fw_config?

We only have one config but it not in the current setting. Not worth to add one config for this project though. So you mean we disable FW_CONFIG then move all the setting in variant? Will this impact the OS config of audio?

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1: Code-Review+1
> > >
> > > @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.
> >
> > FW_CONFIG is already a config option. we probably just need to disable
> > it on variants where it is not required.
> >
> > volteer and volteer2 (the actual boards) need it but there are several
> > variants that do not because they only have a single pre-determined
> > config for audio, usb, etc.
> >
> > do you plan to have multiple audio/sd/usb configs on halvor that would
> > need runtime selection using fw_config?
>
> We only have one config but it not in the current setting. Not worth to add one config for this project though. So you mean we disable FW_CONFIG then move all the setting in variant? Will this impact the OS config of audio?

if we disable FW_CONFIG for a particular variant, then that variant's
gpio.c needs to have the expected GPIO settings (pretty easy). also,
the device tree for that variant must not have any probe statements
that use FW_CONFIG - that's already (mostly?) the case.
the OS will get the same ACPI tables as before,
they just won't depend on having FW_CONFIG in CBI defined correctly.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > > > Patch Set 1: Code-Review+1
> > > >
> > > > @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.
> > >
> > > FW_CONFIG is already a config option. we probably just need to disable
> > > it on variants where it is not required.
> > >
> > > volteer and volteer2 (the actual boards) need it but there are several
> > > variants that do not because they only have a single pre-determined
> > > config for audio, usb, etc.
> > >
> > > do you plan to have multiple audio/sd/usb configs on halvor that would
> > > need runtime selection using fw_config?
> >
> > We only have one config but it not in the current setting. Not worth to add one config for this project though. So you mean we disable FW_CONFIG then move all the setting in variant? Will this impact the OS config of audio?
>
> if we disable FW_CONFIG for a particular variant, then that variant's
> gpio.c needs to have the expected GPIO settings (pretty easy). also,
> the device tree for that variant must not have any probe statements
> that use FW_CONFIG - that's already (mostly?) the case.
> the OS will get the same ACPI tables as before,
> they just won't depend on having FW_CONFIG in CBI defined correctly.

I am thinking of program.star. But I just saw we not use it in the config.star. We already put the correct audio topologies. I think would be fine. We will try that 👍

[Frank Wu (Code Review)]

Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > > > Patch Set 1: Code-Review+1
> > > >
> > > > @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.
> > >
> > > FW_CONFIG is already a config option. we probably just need to disable
> > > it on variants where it is not required.
> > >
> > > volteer and volteer2 (the actual boards) need it but there are several
> > > variants that do not because they only have a single pre-determined
> > > config for audio, usb, etc.
> > >
> > > do you plan to have multiple audio/sd/usb configs on halvor that would
> > > need runtime selection using fw_config?
> >
> > We only have one config but it not in the current setting. Not worth to add one config for this project though. So you mean we disable FW_CONFIG then move all the setting in variant? Will this impact the OS config of audio?
>
> if we disable FW_CONFIG for a particular variant, then that variant's
> gpio.c needs to have the expected GPIO settings (pretty easy). also,
> the device tree for that variant must not have any probe statements
> that use FW_CONFIG - that's already (mostly?) the case.
> the OS will get the same ACPI tables as before,
> they just won't depend on having FW_CONFIG in CBI defined correctly.

Hi Caveh and Eric,

I appreciate your discussion and explanation very much.
I am going to remove FW_CONFIG on Halvor and re-verify the audio related function.
Thank you again.

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > > > Patch Set 1:
> > > > > Patch Set 1: Code-Review+1
> > > > >
> > > > > @Googler, this project is for demo only. We don't want to compromise the real project. So we don't want add more fw_config or override function on this.
> > > >
> > > > FW_CONFIG is already a config option. we probably just need to disable
> > > > it on variants where it is not required.
> > > >
> > > > volteer and volteer2 (the actual boards) need it but there are several
> > > > variants that do not because they only have a single pre-determined
> > > > config for audio, usb, etc.
> > > >
> > > > do you plan to have multiple audio/sd/usb configs on halvor that would
> > > > need runtime selection using fw_config?
> > >
> > > We only have one config but it not in the current setting. Not worth to add one config for this project though. So you mean we disable FW_CONFIG then move all the setting in variant? Will this impact the OS config of audio?
> >
> > if we disable FW_CONFIG for a particular variant, then that variant's
> > gpio.c needs to have the expected GPIO settings (pretty easy). also,
> > the device tree for that variant must not have any probe statements
> > that use FW_CONFIG - that's already (mostly?) the case.
> > the OS will get the same ACPI tables as before,
> > they just won't depend on having FW_CONFIG in CBI defined correctly.
>
> Hi Caveh and Eric,
>
> I appreciate your discussion and explanation very much.
> I am going to remove FW_CONFIG on Halvor and re-verify the audio related function.
> Thank you again.

also have a look at https://review.coreboot.org/c/coreboot/+/41214 to
see how audio was enabled before the introduction of FW_CONFIG. i just
noticed there's a call to fw_config_probe in volteer/romstage.c that needs
a #ifdef FW_CONFIG around it.

[Angel Pons (Code Review)]

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

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/44455/1//COMMIT_MSG@7 
PS1, Line 7: .
nit: no trailing period in commit summaries

https://review.coreboot.org/c/coreboot/+/44455/1//COMMIT_MSG@10 
PS1, Line 10: However, the GPP_A23 is used as I2S1_SCLK on Volteer but HP_INT_L on Halvor.
Please wrap lines at 72 characters

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1: Code-Review+1

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:

Since the inclusion of fw_config.c is already guarded by FW_CONFIG, maybe it makes more sense to just compile this file out for halvor? Perhaps FW_CONFIG should be selected in Kconfig.name instead for the boards that require it.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:

Under check the probe probe AUDIO MAX98373_ALC5682I_I2S in the override tree as well. If we disable the fw_config, seems like audio driver not be registered to kernel.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Under check the probe probe AUDIO MAX98373_ALC5682I_I2S in the override tree as well. If we disable the fw_config, seems like audio driver not be registered to kernel.

Those are only in the volteer & volteer2 override trees. There are no probe statements in the baseboard devicetree, so it should be safe to move selection of FW_CONFIG to Kconfig.name. Halvor has the Realtek codec and the Maxim amp both defined in its overridetree, without probe statements, should be no problem.

[Duncan Laurie (Code Review)]

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

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:

We should define a new fw_config option for this since it isn't actually the same in the end.

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1: -Code-Review

Removing my code review vote based on the discussions that happened after it.

[Duncan Laurie (Code Review)]

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

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
>
> We should define a new fw_config option for this since it isn't actually the same in the end.

I put a proposal for the volteer program config to add MAX98373_ALC5682I_I2S_UP4 and we can define it in the devicetree.cb here if that gets approved: https://chrome-internal-review.googlesource.com/c/chromeos/program/volteer/+...

[Frank Wu (Code Review)]

Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > We should define a new fw_config option for this since it isn't actually the same in the end.
>
> I put a proposal for the volteer program config to add MAX98373_ALC5682I_I2S_UP4 and we can define it in the devicetree.cb here if that gets approved: https://chrome-internal-review.googlesource.com/c/chromeos/program/volteer/+...

Hi Duncan,

We appreciate the comment and CL for configuration on Halvor.
We are going to add the fw_config for Halvor based on the CL and will update result later.

[Frank Wu (Code Review)]

Frank Wu has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44455 )

Change subject: mb/google/volteer/halvor: Skip fw_config override on Halvor.
......................................................................

Abandoned

Create another CL for Adding new firmware configuration table for halvor. Ref: https://review.coreboot.org/c/coreboot/+/44560