Maulik V Vaghela has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC gpio to native function ......................................................................
mb/google/dedede: Change HDMI DDC gpio to native function
HDMI DDC gpios were configured as NC till now in wdoo. This may cause HDMI i2c transfer to break and edid read may fail if FSP doesn't configure these gpios properly.
Configuring these gpios as native in coreboot to fix the potential issue.
Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43294/1
diff --git a/src/mainboard/google/dedede/variants/baseboard/gpio.c b/src/mainboard/google/dedede/variants/baseboard/gpio.c index 71c0c14..d146002 100644 --- a/src/mainboard/google/dedede/variants/baseboard/gpio.c +++ b/src/mainboard/google/dedede/variants/baseboard/gpio.c @@ -215,18 +215,18 @@ PAD_NC(GPP_E11, NONE), /* E12 : GPP_E12/IMGCLKOUT_4 */ PAD_NC(GPP_E12, NONE), - /* E13 : GPP_E13/DDI0_DDC_SCL */ - PAD_NC(GPP_E13, NONE), - /* E14 : GPP_E14/DDI0_DDC_SDA */ - PAD_NC(GPP_E14, NONE), - /* E15 : GPP_E15/DDI1_DDC_SCL */ - PAD_NC(GPP_E15, NONE), - /* E16 : GPP_E16/DDI1_DDC_SDA */ - PAD_NC(GPP_E16, NONE), - /* E17 : HDMI_DDC_SCL */ - PAD_NC(GPP_E17, NONE), - /* E18 : HDMI_DDC_SDA */ - PAD_NC(GPP_E18, NONE), + /* E13: DDI0_DDC_SCL */ + PAD_CFG_NF(GPP_E13, NONE, DEEP, NF1), + /* E14: DDI0_DDC_SDA */ + PAD_CFG_NF(GPP_E14, NONE, DEEP, NF1), + /* E15: DDI1_DDC_SCL */ + PAD_CFG_NF(GPP_E15, NONE, DEEP, NF1), + /* E16: DDI1_DDC_SDA */ + PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1), + /* E17: DDI2_DDC_SCL */ + PAD_CFG_NF(GPP_E17, NONE, DEEP, NF1), + /* E18: DDI2_DDC_SDA */ + PAD_CFG_NF(GPP_E18, NONE, DEEP, NF1), /* E19 : GPP_E19/IMGCLKOUT_5/PCIE_LNK_DOWN */ PAD_NC(GPP_E19, NONE), /* E20 : CNV_BRI_DT_R */
Evan Green has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC gpio to native function ......................................................................
Patch Set 1: Code-Review+1
This gets HDMI working for me!
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC gpio to native function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/1/src/mainboard/google/dedede... PS1, Line 221: PAD_CFG_NF(GPP_E14, NONE, DEEP, NF1), For non-HDMI skus (without external PU on the DB), is there any impact here?
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC gpio to native function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/1/src/mainboard/google/dedede... PS1, Line 221: PAD_CFG_NF(GPP_E14, NONE, DEEP, NF1),
For non-HDMI skus (without external PU on the DB), is there any impact here?
Hi Chen, No, It won't have impact on non-HDMI SKUs as this is just putting gpio into native mode which will enable display IP to use it when HDMI is enabled. If HDMI is not connected, it won't use the I2C lines.
Hello build bot (Jenkins), Evan Green, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43294
to look at the new patch set (#2).
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
mb/google/dedede: Change HDMI DDC GPIOs to native function
HDMI DDC gpios were configured as NC till now in waddledoo. This may cause HDMI i2c transfer to break and EDID read may fail if FSP doesn't configure these GPIOs properly.
Configuring these GPIOs as native in coreboot to fix the issue.
BUG=b:160324327 Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43294/2
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... PS2, Line 225: PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1), As I know, we never use DDI0/1 as the native HDMI interface so will we need to configure them as the NF1?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG@9 PS2, Line 9: HDMI DDC gpios were configured as NC till now in waddledoo. : This may cause HDMI i2c transfer to break and EDID read may fail if : FSP doesn't configure these GPIOs properly. Please re-flow for 75 characters per line.
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG@11 PS2, Line 11: FSP doesn't configure these GPIOs properly Was that observed in practice? Is that an FSP bug?
Hello build bot (Jenkins), Evan Green, Subrata Banik, Meera Ravindranath, Aamir Bohra, Ronak Kanabar, Kirtika Ruchandani, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43294
to look at the new patch set (#3).
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
mb/google/dedede: Change HDMI DDC GPIOs to native function
HDMI DDC gpios were configured as NC till now in waddledoo. This may cause HDMI i2c transfer to break and EDID read may fail if FSP doesn't configure these GPIOs properly.
Configuring these GPIOs as NF in coreboot to fix the issue.
BUG=b:160324327 Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43294/3
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG@11 PS2, Line 11: FSP doesn't configure these GPIOs properly
Was that observed in practice? Is that an FSP bug?
No Paul, it was issue of coreboot. Ideally coreboot owns gpio programming for the board.
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG@9 PS2, Line 9: HDMI DDC gpios were configured as NC till now in waddledoo. : This may cause HDMI i2c transfer to break and EDID read may fail if : FSP doesn't configure these GPIOs properly.
Please re-flow for 75 characters per line.
Done
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43294/2//COMMIT_MSG@11 PS2, Line 11: FSP doesn't configure these GPIOs properly
No Paul, it was issue of coreboot. Ideally coreboot owns gpio programming for the board.
Ack
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... PS2, Line 225: PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1),
As I know, we never use DDI0/1 as the native HDMI interface so will we need to configure them as the […]
Since this is generic configuration of dedede baseboard, we need to set it if we're planning to use it on other variants/boards. Ideally SoC supports on all ports, so I was setting it...I can remove configuration on port 0/1 if we're not planning to use it for any dedede variant. @Karthik, Can you please confirm this?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... PS2, Line 225: PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1),
Since this is generic configuration of dedede baseboard, we need to set it if we're planning to use […]
I agree with Marco. We can configure the GPIOs corresponding to DDI2 alone.
As per the schematics both the reference designs don't use DDI0/1. Also the SDA pins in DDI0/1 are used for hardware strapping purposes. So I am not sure if we will use these pins for other purposes in other variants. If we happen to do so, we can always override it in the concerned variant boards.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... PS2, Line 225: PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1),
I agree with Marco. We can configure the GPIOs corresponding to DDI2 alone. […]
I also looked at the schematics for Drawcia, Magalor and Boten. All of them don't use DDI0/1. So it is better/safe to leave DDI0/1 as NC and just configure DDI2 as NF1.
Hello build bot (Jenkins), Evan Green, Subrata Banik, Meera Ravindranath, Aamir Bohra, Ronak Kanabar, Kirtika Ruchandani, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43294
to look at the new patch set (#4).
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
mb/google/dedede: Change HDMI DDC GPIOs to native function
HDMI DDC gpios were configured as NC till now in waddledoo. This may cause HDMI i2c transfer to break and EDID read will fail due to wrong configuration
Configuring these GPIOs as NF in coreboot to fix the issue.
BUG=b:160324327 Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43294/4
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/43294/2/src/mainboard/google/dedede... PS2, Line 225: PAD_CFG_NF(GPP_E16, NONE, DEEP, NF1),
I also looked at the schematics for Drawcia, Magalor and Boten. All of them don't use DDI0/1. […]
Ack. Done
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 4:
Just curious about do we still need different VBTs to configure pins for DP or native HDMI? Thank.
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 4:
Patch Set 4:
Just curious about do we still need different VBTs to configure pins for DP or native HDMI? Thank.
No Marco...Once VBT is configured for "NativeDispalyportwithHDMIDVI" config on this port, It should be taken care.
Hello build bot (Jenkins), Evan Green, Subrata Banik, Meera Ravindranath, Aamir Bohra, Ronak Kanabar, Kirtika Ruchandani, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43294
to look at the new patch set (#6).
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
mb/google/dedede: Change HDMI DDC GPIOs to native function
HDMI DDC GPIOs were configured as NC till now in waddledoo. This may cause HDMI i2c transfer to break and EDID read will fail due to wrong configuration
Configuring these GPIOs as NF in coreboot to fix the issue.
BUG=b:160324327 BRANCH=None TEST=HDMI works on DDI2 onn Type-C port
Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43294/6
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 6: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43294 )
Change subject: mb/google/dedede: Change HDMI DDC GPIOs to native function ......................................................................
mb/google/dedede: Change HDMI DDC GPIOs to native function
HDMI DDC GPIOs were configured as NC till now in waddledoo. This may cause HDMI i2c transfer to break and EDID read will fail due to wrong configuration
Configuring these GPIOs as NF in coreboot to fix the issue.
BUG=b:160324327 BRANCH=None TEST=HDMI works on DDI2 onn Type-C port
Change-Id: If02f062132d7c3b01b07ea9401e81f451df35c3c Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43294 Reviewed-by: Ronak Kanabar ronak.kanabar@intel.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/dedede/variants/baseboard/gpio.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Karthik Ramasubramanian: Looks good to me, approved Ronak Kanabar: Looks good to me, approved
diff --git a/src/mainboard/google/dedede/variants/baseboard/gpio.c b/src/mainboard/google/dedede/variants/baseboard/gpio.c index 1284bc5..9b60da1 100644 --- a/src/mainboard/google/dedede/variants/baseboard/gpio.c +++ b/src/mainboard/google/dedede/variants/baseboard/gpio.c @@ -224,9 +224,9 @@ /* E16 : GPP_E16/DDI1_DDC_SDA */ PAD_NC(GPP_E16, NONE), /* E17 : HDMI_DDC_SCL */ - PAD_NC(GPP_E17, NONE), + PAD_CFG_NF(GPP_E17, NONE, DEEP, NF1), /* E18 : HDMI_DDC_SDA */ - PAD_NC(GPP_E18, NONE), + PAD_CFG_NF(GPP_E18, NONE, DEEP, NF1), /* E19 : GPP_E19/IMGCLKOUT_5/PCIE_LNK_DOWN */ PAD_NC(GPP_E19, NONE), /* E20 : CNV_BRI_DT_R */
-
Evan Green (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Marco Chen (Code Review)
-
Maulik V Vaghela (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Ronak Kanabar (Code Review)