Change in coreboot[master]: mb/google/volteer/halvor: Update settings for audio function

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure gpio/overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/gpio.c
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
2 files changed, 9 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/1

diff --git a/src/mainboard/google/volteer/variants/halvor/gpio.c b/src/mainboard/google/volteer/variants/halvor/gpio.c
index 2a98688..92e33ca 100644
--- a/src/mainboard/google/volteer/variants/halvor/gpio.c
+++ b/src/mainboard/google/volteer/variants/halvor/gpio.c
@@ -53,6 +53,10 @@
    PAD_NC(GPP_C11, NONE),
    /* C13 : UART1_TXD ==> NC */
    PAD_NC(GPP_C13, NONE),
+	/* C16 : I2C0_SDA ==> PCH_I2C0_1V8_AUDIO_SDA */
+	PAD_CFG_NF(GPP_C16, NONE, DEEP, NF1),
+	/* C17 : I2C0_SCL ==> PCH_I2C0_1V8_AUDIO_SCL */
+	PAD_CFG_NF(GPP_C17, NONE, DEEP, NF1),

/* D7  : SRCCLKREQ2# ==> NC */
    PAD_NC(GPP_D7, NONE),
@@ -141,6 +145,10 @@
    PAD_CFG_NF(GPP_S4, NONE, DEEP, NF2),
    /* S5 : SNDW2_DATA ==> SOC_DMIC_DATA1 */
    PAD_CFG_NF(GPP_S5, NONE, DEEP, NF2),
+	/* S6 : SNDW3_CLK ==> DMIC_CLK0 */
+	PAD_CFG_NF(GPP_S6, NONE, DEEP, NF2),
+	/* S7 : SNDW3_DATA ==> DMIC_DATA0 */
+	PAD_CFG_NF(GPP_S7, NONE, DEEP, NF2),

/* GPD11: LANPHYC ==> NC */
    PAD_NC(GPD11, NONE),
diff --git a/src/mainboard/google/volteer/variants/halvor/overridetree.cb b/src/mainboard/google/volteer/variants/halvor/overridetree.cb
index 12e059c..268b1a5 100644
--- a/src/mainboard/google/volteer/variants/halvor/overridetree.cb
+++ b/src/mainboard/google/volteer/variants/halvor/overridetree.cb
@@ -121,13 +121,6 @@
    			device i2c 15 on end
    		end
    	end # I2C5				0xA0C6
-		device pci 1f.3 on
-			chip drivers/generic/max98357a
-				register "hid" = ""MX98357A""
-				register "sdmode_gpio" =  "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_F18)"
-				register "sdmode_delay" = "5"
-				device generic 0 on end
-			end
-		end # Intel HD audio		0xA0C8-A0CF
+		device pci 1f.3 on end # Intel HD audio		0xA0C8-A0CF
    end
 end

[Frank Wu (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure gpio/overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/gpio.c
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
2 files changed, 23 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/2

[Frank Wu (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure gpio/overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/gpio.c
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
2 files changed, 23 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/3

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 4:

Hi Google,

Would you help review the CL?
Thank you very much.

[EricR Lai (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 4:

Please remove the gpio.c change. We left that in the fw_config.c already.

[EricR Lai (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44409/4/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/variants/halvor/gpio.c:

https://review.coreboot.org/c/coreboot/+/44409/4/src/mainboard/google/voltee... 
PS4, Line 58: 	/* C16 : I2C0_SDA ==> PCH_I2C0_1V8_AUDIO_SDA */
Maybe just leave I2C setting here.

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 4:
> Patch Set 4:
>
> Please remove the gpio.c change. We left that in the fw_config.c already.

[Frank Wu (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, 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/+/44409

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure gpio/overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/gpio.c
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
2 files changed, 23 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/5

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 4:
>
> Please remove the gpio.c change. We left that in the fw_config.c already.

The gpio pins about audio are both configured in volteer/gpio.c and fw_config.c.
In my point of view, they are fine to configure in halvor/gpio.c.

Ref about GPP_A23 in volteer:
volteer/gpio.c:
https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/voltee...
fw_config.c:
https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/voltee...

https://review.coreboot.org/c/coreboot/+/44409/4/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/variants/halvor/gpio.c:

https://review.coreboot.org/c/coreboot/+/44409/4/src/mainboard/google/voltee... 
PS4, Line 58: 	/* C16 : I2C0_SDA ==> PCH_I2C0_1V8_AUDIO_SDA */
> Maybe just leave I2C setting here.

Done

[Frank Wu (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, 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/+/44409

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure gpio/overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/gpio.c
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
2 files changed, 23 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/6

[Caveh Jalali (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 6:

please have a look at https://review.coreboot.org/c/coreboot/+/44783.

it proposes to pass the audio config hint up to the kernel based on
the audio config specified in CBI.FW_CONFIG. this means we'll need
to keep FW_CONFIG enabled. when FW_CONFIG is enabled, 
volteer/fw_config.c:fw_config_handle() is going to run and as
soon as someone adds a different audio config for UP4, we'll
have to handle both cases. right now, fw_config_handle() does
nothing for halvor, so the settings in gpio.c are sufficient,
but i think this will break if we have another UP4 board that
supports multiple audio configs.

perhaps one way out is allow a compile-time (kconfig?) over-ride
for the FW_CONFIG audio DB value.

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 6:
> Patch Set 6:
>
> please have a look at https://review.coreboot.org/c/coreboot/+/44783.
>
> it proposes to pass the audio config hint up to the kernel based on
> the audio config specified in CBI.FW_CONFIG. this means we'll need
> to keep FW_CONFIG enabled. when FW_CONFIG is enabled, 
> volteer/fw_config.c:fw_config_handle() is going to run and as
> soon as someone adds a different audio config for UP4, we'll
> have to handle both cases. right now, fw_config_handle() does
> nothing for halvor, so the settings in gpio.c are sufficient,
> but i think this will break if we have another UP4 board that
> supports multiple audio configs.
>
> perhaps one way out is allow a compile-time (kconfig?) over-ride
> for the FW_CONFIG audio DB value.

Hi Caveh and Google,

We have another CL for fw_config MAX98373_ALC5682I_I2S_UP4.
https://review.coreboot.org/c/coreboot/+/44560

Could you give us advise to merge the CL?
Thank you.

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> >
> > please have a look at https://review.coreboot.org/c/coreboot/+/44783.
> >
> > it proposes to pass the audio config hint up to the kernel based on
> > the audio config specified in CBI.FW_CONFIG. this means we'll need
> > to keep FW_CONFIG enabled. when FW_CONFIG is enabled, 
> > volteer/fw_config.c:fw_config_handle() is going to run and as
> > soon as someone adds a different audio config for UP4, we'll
> > have to handle both cases. right now, fw_config_handle() does
> > nothing for halvor, so the settings in gpio.c are sufficient,
> > but i think this will break if we have another UP4 board that
> > supports multiple audio configs.
> >
> > perhaps one way out is allow a compile-time (kconfig?) over-ride
> > for the FW_CONFIG audio DB value.
>
> Hi Caveh and Google,
>
> We have another CL for fw_config MAX98373_ALC5682I_I2S_UP4.
> https://review.coreboot.org/c/coreboot/+/44560
>
> Could you give us advise to merge the CL?
> Thank you.

Hi Google,

Is there suggestion for us to merge the CL?
Thank you.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> > > Patch Set 6:
> > >
> > > please have a look at https://review.coreboot.org/c/coreboot/+/44783.
> > >
> > > it proposes to pass the audio config hint up to the kernel based on
> > > the audio config specified in CBI.FW_CONFIG. this means we'll need
> > > to keep FW_CONFIG enabled. when FW_CONFIG is enabled, 
> > > volteer/fw_config.c:fw_config_handle() is going to run and as
> > > soon as someone adds a different audio config for UP4, we'll
> > > have to handle both cases. right now, fw_config_handle() does
> > > nothing for halvor, so the settings in gpio.c are sufficient,
> > > but i think this will break if we have another UP4 board that
> > > supports multiple audio configs.
> > >
> > > perhaps one way out is allow a compile-time (kconfig?) over-ride
> > > for the FW_CONFIG audio DB value.
> >
> > Hi Caveh and Google,
> >
> > We have another CL for fw_config MAX98373_ALC5682I_I2S_UP4.
> > https://review.coreboot.org/c/coreboot/+/44560
> >
> > Could you give us advise to merge the CL?
> > Thank you.
>
> Hi Google,
>
> Is there suggestion for us to merge the CL?
> Thank you.

If we add the fw_config CL, this one shouldn't be needed, correct?

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> > > Patch Set 6:
> > > > Patch Set 6:
> > > >
> > > > please have a look at https://review.coreboot.org/c/coreboot/+/44783.
> > > >
> > > > it proposes to pass the audio config hint up to the kernel based on
> > > > the audio config specified in CBI.FW_CONFIG. this means we'll need
> > > > to keep FW_CONFIG enabled. when FW_CONFIG is enabled, 
> > > > volteer/fw_config.c:fw_config_handle() is going to run and as
> > > > soon as someone adds a different audio config for UP4, we'll
> > > > have to handle both cases. right now, fw_config_handle() does
> > > > nothing for halvor, so the settings in gpio.c are sufficient,
> > > > but i think this will break if we have another UP4 board that
> > > > supports multiple audio configs.
> > > >
> > > > perhaps one way out is allow a compile-time (kconfig?) over-ride
> > > > for the FW_CONFIG audio DB value.
> > >
> > > Hi Caveh and Google,
> > >
> > > We have another CL for fw_config MAX98373_ALC5682I_I2S_UP4.
> > > https://review.coreboot.org/c/coreboot/+/44560
> > >
> > > Could you give us advise to merge the CL?
> > > Thank you.
> >
> > Hi Google,
> >
> > Is there suggestion for us to merge the CL?
> > Thank you.
>
> If we add the fw_config CL, this one shouldn't be needed, correct?

Thanks for the comment.
I will drop the CL later.

[Frank Wu (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, 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/+/44409

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
---
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
1 file changed, 7 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/44409/7

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7:

Update the CL based on https://partnerissuetracker.corp.google.com/issues/163382106#comment31 .
Would you help review it?
Thank you very much.

[Angel Pons (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7: Code-Review+1

[Sathyanarayana Nujella (Code Review)]

Sathyanarayana Nujella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44409 )

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7: Code-Review+1

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44409/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44409/7//COMMIT_MSG@13 
PS7, Line 13: 
Did you test if audio works now with this and the kernel patch?

[Nick Vaccaro (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7: Code-Review+2

[Frank Wu (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44409/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44409/7//COMMIT_MSG@13 
PS7, Line 13:
> Did you test if audio works now with this and the kernel patch?

I do not have a halvor board now.
I will test that once I get one.
Thank you.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer/halvor: Update settings for audio function
......................................................................

mb/google/volteer/halvor: Update settings for audio function

Configure overridetree settings for audio function.

BUG=b:153680359, b:163382106
TEST=FW_NAME=halvor emerge-volteer coreboot chromeos-bootimage

Signed-off-by: Frank Wu 
Change-Id: I107f6fc21b99d80d69931139dc50e7d5873a8e52
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44409
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Sathyanarayana Nujella 
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Nick Vaccaro 
---
M src/mainboard/google/volteer/variants/halvor/overridetree.cb
1 file changed, 7 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Sathyanarayana Nujella: Looks good to me, but someone else must approve
  Nick Vaccaro: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/mainboard/google/volteer/variants/halvor/overridetree.cb b/src/mainboard/google/volteer/variants/halvor/overridetree.cb
index e337a02..73767c6 100644
--- a/src/mainboard/google/volteer/variants/halvor/overridetree.cb
+++ b/src/mainboard/google/volteer/variants/halvor/overridetree.cb
@@ -29,7 +29,9 @@
    			register "uid" = "0"
    			register "desc" = ""Right Speaker Amp""
    			register "name" = ""MAXR""
-				device i2c 31 on end
+				device i2c 31 on
+					probe AUDIO MAX98373_ALC5682I_I2S_UP4
+				end
    		end
    		chip drivers/i2c/max98373
    			register "vmon_slot_no" = "2"
@@ -37,7 +39,9 @@
    			register "uid" = "1"
    			register "desc" = ""Left Speaker Amp""
    			register "name" = ""MAXL""
-				device i2c 32 on end
+				device i2c 32 on
+					probe AUDIO MAX98373_ALC5682I_I2S_UP4
+				end
    		end
    	end # I2C #0			0xA0E8
    	device pci 15.1 on
@@ -113,13 +117,6 @@
    			device i2c 15 on end
    		end
    	end # I2C5				0xA0C6
-		device pci 1f.3 on
-			chip drivers/generic/max98357a
-				register "hid" = ""MX98357A""
-				register "sdmode_gpio" =  "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_F18)"
-				register "sdmode_delay" = "5"
-				device generic 0 on end
-			end
-		end # Intel HD audio		0xA0C8-A0CF
+		device pci 1f.3 on end # Intel HD audio		0xA0C8-A0CF
    end
 end