Change in coreboot[master]: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

mb/google/helios: Set SPKR_PA_EN PIN high for boot beep

This patch makes SPKR_PA_EN PIN output and high for boot beep
to work in pre-os environment.

BUG=b:135104721
BRANCH=NONE
TEST=Boot Beep is working with required ALC1011 depthcharge code
changes.

Change-Id: I012462f93e9e2bcafe5f18ce7d04e3fcd1db9ffa
Signed-off-by: Subrata Banik 
---
M src/mainboard/google/hatch/variants/helios/gpio.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/34705/1

diff --git a/src/mainboard/google/hatch/variants/helios/gpio.c b/src/mainboard/google/hatch/variants/helios/gpio.c
index ecb13f3..257b020 100644
--- a/src/mainboard/google/hatch/variants/helios/gpio.c
+++ b/src/mainboard/google/hatch/variants/helios/gpio.c
@@ -85,6 +85,8 @@
    PAD_NC(GPP_G5, NONE),
    /* G6  : GPP_G6 ==> NC  */
    PAD_NC(GPP_G6, NONE),
+	/* H3  : SPKR_PA_EN */
+	PAD_CFG_GPO(GPP_H3, 1, DEEP),
    /* H4  : I2C2_SDA ==> NC */
    PAD_NC(GPP_H4, NONE),
    /* H5  : I2C2_SCL ==> NC */

[Usha P (Code Review)]

Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1: Code-Review+1

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1: Code-Review+2

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1:

Just out of curiosity, what is the timing requirement for this signal?  Does it need to be enabled in coreboot or can the bootloader apply it?

[Meera Ravindranath (Code Review)]

Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1: Code-Review+1

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Does it need to be enabled in coreboot or can the bootloader apply it?

I guess you mean to say "depthchage" when you are referring "can the bootloader apply it?"
yes, DC can also do it and i don't believe it has my given timing sequence that SPKR_EN has to be enable "n" ms before sending i2C data and clock. Although we haven't tried moving that code into payload, i'm not big fan of doing GPIO programming in payload :), but certainly can be verified before sending init sequence to codec.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1: Code-Review+2
> Patch Set 1:
> > Patch Set 1:
> >
> > Does it need to be enabled in coreboot or can the bootloader apply it?
>
> I guess you mean to say "depthchage" when you are referring "can the bootloader apply it?"
> yes, DC can also do it and i don't believe it has my given timing sequence that SPKR_EN has to be enable "n" ms before sending i2C data and clock. Although we haven't tried moving that code into payload, i'm not big fan of doing GPIO programming in payload :), but certainly can be verified before sending init sequence to codec.

[Sathya Prakash M R (Code Review)]

Sathya Prakash M R has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1: Code-Review+1

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > Does it need to be enabled in coreboot or can the bootloader apply it?
> >
> > I guess you mean to say "depthchage" when you are referring "can the bootloader apply it?"
> > yes, DC can also do it and i don't believe it has my given timing sequence that SPKR_EN has to be enable "n" ms before sending i2C data and clock. Although we haven't tried moving that code into payload, i'm not big fan of doing GPIO programming in payload :), but certainly can be verified before sending init sequence to codec.

Adding to the question asked by Tim: Is this pin set to high only for boot beep? Or is it required later on for OS operation as well? If this is just for boot beep, then it should be set to low here and set high in depthcharge.

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

mb/google/helios: Set SPKR_PA_EN PIN high for boot beep

This patch makes SPKR_PA_EN PIN output and high for boot beep
to work in pre-os environment.

BUG=b:135104721
BRANCH=NONE
TEST=Boot Beep is working with required ALC1011 depthcharge code
changes.

Change-Id: I012462f93e9e2bcafe5f18ce7d04e3fcd1db9ffa
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34705
Tested-by: build bot (Jenkins) 
Reviewed-by: Usha P 
Reviewed-by: V Sowmya 
Reviewed-by: Meera Ravindranath 
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Sathya Prakash M R 
---
M src/mainboard/google/hatch/variants/helios/gpio.c
1 file changed, 2 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  V Sowmya: Looks good to me, approved
  Sathya Prakash M R: Looks good to me, but someone else must approve
  Tim Wawrzynczak: Looks good to me, approved
  Usha P: Looks good to me, but someone else must approve
  Meera Ravindranath: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/hatch/variants/helios/gpio.c b/src/mainboard/google/hatch/variants/helios/gpio.c
index ecb13f3..257b020 100644
--- a/src/mainboard/google/hatch/variants/helios/gpio.c
+++ b/src/mainboard/google/hatch/variants/helios/gpio.c
@@ -85,6 +85,8 @@
    PAD_NC(GPP_G5, NONE),
    /* G6  : GPP_G6 ==> NC  */
    PAD_NC(GPP_G6, NONE),
+	/* H3  : SPKR_PA_EN */
+	PAD_CFG_GPO(GPP_H3, 1, DEEP),
    /* H4  : I2C2_SDA ==> NC */
    PAD_NC(GPP_H4, NONE),
    /* H5  : I2C2_SCL ==> NC */

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34705 )

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 2:
> Patch Set 1:
> > Patch Set 1: Code-Review+2
> > > Patch Set 1:
> > > > Patch Set 1:
> > > >
> > > > Does it need to be enabled in coreboot or can the bootloader apply it?
> > >
> > > I guess you mean to say "depthchage" when you are referring "can the bootloader apply it?"
> > > yes, DC can also do it and i don't believe it has my given timing sequence that SPKR_EN has to be enable "n" ms before sending i2C data and clock. Although we haven't tried moving that code into payload, i'm not big fan of doing GPIO programming in payload :), but certainly can be verified before sending init sequence to codec.
>
> Adding to the question asked by Tim: Is this pin set to high only for boot beep? Or is it required later on for OS operation as well? If this is just for boot beep, then it should be set to low here and set high in depthcharge.

Furquan, as per my understanding this PIN should set to high for SPKR to work but looks like by some mean Kernel might able to play with it, we can dump this GPIO in kernel space and see the status. But as Satya had already mentioned that setting this GPIO to High doesn't cause any side effect for OS audio to work. He is still investigating about SPKR_EN PIN behavior from kernel space.

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/helios: Set SPKR_PA_EN PIN high for boot beep
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 1:
> > > Patch Set 1: Code-Review+2
> > > > Patch Set 1:
> > > > > Patch Set 1:
> > > > >
> > > > > Does it need to be enabled in coreboot or can the bootloader apply it?
> > > >
> > > > I guess you mean to say "depthchage" when you are referring "can the bootloader apply it?"
> > > > yes, DC can also do it and i don't believe it has my given timing sequence that SPKR_EN has to be enable "n" ms before sending i2C data and clock. Although we haven't tried moving that code into payload, i'm not big fan of doing GPIO programming in payload :), but certainly can be verified before sending init sequence to codec.
> >
> > Adding to the question asked by Tim: Is this pin set to high only for boot beep? Or is it required later on for OS operation as well? If this is just for boot beep, then it should be set to low here and set high in depthcharge.
>
> Furquan, as per my understanding this PIN should set to high for SPKR to work but looks like by some mean Kernel might able to play with it, we can dump this GPIO in kernel space and see the status. But as Satya had already mentioned that setting this GPIO to High doesn't cause any side effect for OS audio to work. He is still investigating about SPKR_EN PIN behavior from kernel space.

Sounds good. Yes, I think it would be good to understand the actual behavior and how SPKR_EN pin works both in boot beep case and in OS.