Change in coreboot[master]: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg

In order for the SOC to be able to control the Aux line orientation for
Type-C ports that do not have a retimer the IomTypeCPortPadCfg UPD needs
to be configurable through devicetree to correctly set the GPIO pins that
the SOC should use to flip orientation.

BUG=b:145220205
BRANCH=NONE
TEST=booted Volteer proto 2 and verified that the AUX channels flip
when the cable is flipped

Change-Id: I2e48adb624c7922170eafb8dfcaed680f008936e
Signed-off-by: Brandon Breitenstein 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/40244/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index e105061..647ba33 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -221,6 +221,19 @@
    uint8_t TcssXdciEn;

/*
+	 * IOM Port Config
+	 * If a port orientation needs to be controlled by the SOC this setting must be
+	 * updated to reflect the correct GPIOs being used for the SOC port flipping.
+	 * There are 4 ports each with a pair of GPIOs for Pull Up and Pull Down
+	 * 0,1 are pull up and pull down for port 0
+	 * 2,3 are pull up and pull down for port 1
+	 * 4,5 are pull up and pull down for port 2
+	 * 6,7 are pull up and pull down for port 3
+	 * values to be programmed correspond to the GPIO family and offsets
+	 */
+	uint32_t IomTypeCPortPadCfg[8];
+
+	/*
     * SOC Aux orientation override:
     * This is a bitfield that corresponds to up to 4 TCSS ports on TGL.
     * Even numbered bits (0, 2, 4, 6) control the retimer being handled by SOC.
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index ff6d3a9..d3b6f91 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -95,7 +95,7 @@

params->TcssAuxOri = config->TcssAuxOri;
    for (i = 0; i < 8; i++)
-		params->IomTypeCPortPadCfg[i] = 0x09000000;
+		params->IomTypeCPortPadCfg[i] = config->IomTypeCPortPadCfg[i];

/* USB */
    for (i = 0; i < ARRAY_SIZE(config->usb2_ports); i++) {

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Abandoned

Not actually needed setting the GPIO in mainboard gpio file is enough for this

[Furquan Shaikh (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 98: 0x09000000
I think should be set to all 0s. That ensures that FSP skips Aux orientation configuration completely.

memset(params->IomTypeCPortPadCfg, 0, sizeof(params->IomTypeCPortPadCfg));

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 98: 0x09000000
> I think should be set to all 0s. […]

good point...we can address this after the rest though correct? I think the 0x09000000 is default setting here and we haven't really done any testing setting this to 0. I can make this change as a separate patch that doesn't rely on the rest of the Aux orientation stuff

[Furquan Shaikh (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 98: 0x09000000
> good point... […]

SG

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
PS1, Line 234: 8
If you want to revive this I think it's easier split this out by port. I'm failing to understand the comment '0, 1 for port 0' and why this dimension is 8. And why this is a 32-bit wide variable. I think we should make these configs as straight forward as possible and manipulate the data to match the UPD expectations, even if the interface is confusing. I'd like to see that for all these options, tbh.

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
PS1, Line 234: 8
> If you want to revive this I think it's easier split this out by port. […]

this is 8 wide due to there being 2 fields for each type C port. The first is for Pull Up GPIO and the Second is for Pull Down GPIO. As discussed with Furquan turns out this setting is only if you are not configuring the GPIO from the mainboard so this is UPD does not need to be modified by coreboot.

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has restored this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Restored

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 2:

since IOM needs to know about these GPIOs this setting needs to be correctly assigned or there is some weirdness with the pulls being correct from the SOC side and the DP may or may not be detected on multiple plug in and out

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 2:
> Patch Set 2:
>
> since IOM needs to know about these GPIOs this setting needs to be correctly assigned or there is some weirdness with the pulls being correct from the SOC side and the DP may or may not be detected on multiple plug in and out

Can we get more details on weirdness? And what specific IOM needs as well as why?

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > since IOM needs to know about these GPIOs this setting needs to be correctly assigned or there is some weirdness with the pulls being correct from the SOC side and the DP may or may not be detected on multiple plug in and out
>
> Can we get more details on weirdness? And what specific IOM needs as well as why?

it is all documented here https://docs.google.com/document/d/1nhWN1rzMFXBOb6XVSvQeNwPanKTq_Iai_kLdKfFi...

The gpios are not always properly configured without the UPD setting, whereas with the UPD the gpios are always correctly configured

[Nick Vaccaro (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 2:

@Aaron, are you comfortable with this change at this point?

[Nick Vaccaro (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40244/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40244/2//COMMIT_MSG@10 
PS2, Line 10: retimer 
nit - Please add a comma after "retimer,"

[Brandon Breitenstein (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Patrick Rudolph,

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

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

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg

In order for the SOC to be able to control the Aux line orientation for
Type-C ports that do not have a retimer, the IomTypeCPortPadCfg UPD needs
to be configurable through devicetree to correctly set the GPIO pins that
the SOC should use to flip orientation.

BUG=b:145220205
BRANCH=NONE
TEST=booted Volteer proto 2 and verified that the AUX channels flip
when the cable is flipped

Change-Id: I2e48adb624c7922170eafb8dfcaed680f008936e
Signed-off-by: Brandon Breitenstein 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/40244/3

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3:
> Patch Set 2:
>
> @Aaron, are you comfortable with this change at this point?

I guess so. I don't agree with the policy, but we can move on.

[Caveh Jalali (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3: Code-Review+1

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3: Code-Review+1

[Brandon Breitenstein (Code Review)]

Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3:

(3 comments)

all comments were already addressed just didn't mark them resolved all are resolved

https://review.coreboot.org/c/coreboot/+/40244/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40244/2//COMMIT_MSG@10 
PS2, Line 10: retimer
> nit - Please add a comma after "retimer,"

Ack

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/chi... 
PS1, Line 234: 8
> this is 8 wide due to there being 2 fields for each type C port. […]

Ack

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40244/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 98: 0x09000000
> SG

Ack

[Nick Vaccaro (Code Review)]

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

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3: Code-Review+2

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg

In order for the SOC to be able to control the Aux line orientation for
Type-C ports that do not have a retimer, the IomTypeCPortPadCfg UPD needs
to be configurable through devicetree to correctly set the GPIO pins that
the SOC should use to flip orientation.

BUG=b:145220205
BRANCH=NONE
TEST=booted Volteer proto 2 and verified that the AUX channels flip
when the cable is flipped

Change-Id: I2e48adb624c7922170eafb8dfcaed680f008936e
Signed-off-by: Brandon Breitenstein 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40244
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Reviewed-by: Wonkyu Kim 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 14 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nick Vaccaro: Looks good to me, approved
  Caveh Jalali: Looks good to me, but someone else must approve
  Wonkyu Kim: Looks good to me, approved

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 3047037..a32cebe 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -222,6 +222,19 @@
    uint8_t TcssDma1En;

/*
+	 * IOM Port Config
+	 * If a port orientation needs to be controlled by the SOC this setting must be
+	 * updated to reflect the correct GPIOs being used for the SOC port flipping.
+	 * There are 4 ports each with a pair of GPIOs for Pull Up and Pull Down
+	 * 0,1 are pull up and pull down for port 0
+	 * 2,3 are pull up and pull down for port 1
+	 * 4,5 are pull up and pull down for port 2
+	 * 6,7 are pull up and pull down for port 3
+	 * values to be programmed correspond to the GPIO family and offsets
+	 */
+	uint32_t IomTypeCPortPadCfg[8];
+
+	/*
     * SOC Aux orientation override:
     * This is a bitfield that corresponds to up to 4 TCSS ports on TGL.
     * Even numbered bits (0, 2, 4, 6) control the retimer being handled by SOC.
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 73c41c8..cf106cb 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -103,7 +103,7 @@

params->TcssAuxOri = config->TcssAuxOri;
    for (i = 0; i < 8; i++)
-		params->IomTypeCPortPadCfg[i] = 0x09000000;
+		params->IomTypeCPortPadCfg[i] = config->IomTypeCPortPadCfg[i];

/* Chipset Lockdown */
    if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) {

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40244 )

Change subject: soc/tigerlake: Add devicetree configurability for IomTypeCPortPadCfg
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3654
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3653
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3652
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3651

Please note: This test is under development and might not be accurate at all!