Change in coreboot[master]: soc/amd/picasso: Add UPDs for support edp phy tuning adjust

[chris wang (Code Review)]

Hello Chris Wang,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
......................................................................

soc/amd/picasso: Add UPDs for support edp phy tuning adjust

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/48733/1

diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index c0a6457..2e6f8d4 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -57,6 +57,18 @@
    SD_EMMC_DRIVE_STRENGTH_D,
 };

+/// dpphy_override
+enum sysinfo_dpphy_override {
+	ENABLE_DVI_TUNINGSET   = 0x01,
+	ENABLE_HDMI_TUNINGSET  = 0x02,
+	ENABLE_HDMI6G_TUNINGSET  = 0x04,
+	ENABLE_DP_TUNINGSET  = 0x08,
+	ENABLE_DP_HBR3_TUNINGSET  = 0x10,
+	ENABLE_DP_HBR_TUNINGSET  = 0x20,
+	ENABLE_DP_HBR2_TUNINGSET  = 0x40,
+	ENABLE_EDP_TUNINGSET  = 0x80,
+};
+
 struct soc_amd_picasso_config {
    struct soc_amd_common_config common_config;
    /*
@@ -219,6 +231,19 @@

/* The array index is the general purpose PCIe clock output number. */
    enum gpp_clk_req_setting gpp_clk_config[GPP_CLK_OUTPUT_COUNT];
+
+	/* eDP phy tuning settings */
+	uint8_t   dp_phy_override;
+	uint16_t  edp_phy_sel;
+	uint8_t   edp_version;
+	uint16_t  edp_table_size;
+
+	struct {
+		uint8_t   dp_vs_pemph_level;
+		uint8_t   deemph_6db4;
+		uint8_t   boostadj;
+		uint16_t  margin_deemph;
+	} edp_tuningset;
 };

#endif /* __PICASSO_CHIP_H__ */
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c
index c7befe4..86b3eba 100644
--- a/src/soc/amd/picasso/fsp_params.c
+++ b/src/soc/amd/picasso/fsp_params.c
@@ -142,6 +142,21 @@
    scfg->fch_ioapic_id = CONFIG_PICASSO_FCH_IOAPIC_ID;
 }

+static void fsp_edp_tuning_upds(FSP_S_CONFIG *scfg,
+			const struct soc_amd_picasso_config *cfg)
+{
+	if (cfg->dp_phy_override) {
+		scfg->DpPhyOverride = cfg->dp_phy_override;
+		scfg->EDpPhySel = cfg->edp_phy_sel;
+		scfg->EDpVersion = cfg->edp_version;
+		scfg->EDpTableSize = cfg->edp_table_size;
+		scfg->DpVsPemphLevel = cfg->edp_tuningset.dp_vs_pemph_level;
+		scfg->MarginDeemPh = cfg->edp_tuningset.margin_deemph;
+		scfg->Deemph6db4 = cfg->edp_tuningset.deemph_6db4;
+		scfg->BoostAdj = cfg->edp_tuningset.boostadj;
+	}
+}
+
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
    const struct soc_amd_picasso_config *cfg;
@@ -152,4 +167,5 @@
    fsp_fill_pcie_ddi_descriptors(scfg);
    fsp_assign_ioapic_upds(scfg);
    fsp_usb_oem_customization(scfg, cfg);
+	fsp_edp_tuning_upds(scfg, cfg);
 }

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
......................................................................

Patch Set 1:

will those options only be needed for edp or also for other display outputs?

[chris wang (Code Review)]

chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
......................................................................

Patch Set 1:
> Patch Set 1:
>
> will those options only be needed for edp or also for other display outputs?

no, it will only apply for eDP.

[chris wang (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Chris Wang, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
......................................................................

soc/amd/picasso: Add UPDs for support edp phy tuning adjust

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/48733/2

[Paul Menzel (Code Review)]

Attention is currently required from: chris wang.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
......................................................................

Patch Set 2:

(4 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48733/comment/81157040_80fcac5e 
PS2, Line 7: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
Maybe:
> Set UPDs for tuning eDP phy

File src/soc/amd/picasso/chip.h:

https://review.coreboot.org/c/coreboot/+/48733/comment/666607ff_b3af68e7 
PS2, Line 60: /// dpphy_override
Please use consistent commenting style in this file, which seems C89.

https://review.coreboot.org/c/coreboot/+/48733/comment/1deb85a3_adc9b3c9 
PS2, Line 62:    
Plesae use one space, or be consistent with the rest (though I do not see the benefit of using two spaces.)

https://review.coreboot.org/c/coreboot/+/48733/comment/9df4ef83_0802ffca 
PS2, Line 238: uint8_t   dp_phy_override;
             : 	uint16_t  edp_phy_sel;
             : 	uint8_t   edp_version;
             : 	uint16_t  edp_table_size;
             : 
             : 	struct {
             : 		uint8_t   dp_vs_pemph_level;
             : 		uint8_t   deemph_6db4;
             : 		uint8_t   boostadj;
             : 		uint16_t  margin_deemph;
             : 	} edp_tuningset;
Please use one space as in the rest of the file.

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[chris wang (Code Review)]

Attention is currently required from: chris wang.
Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Chris Wang, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

soc/amd/picasso: Set UPDs for tuning eDP phy

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/48733/3

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[chris wang (Code Review)]

Attention is currently required from: Paul Menzel.
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 3:

(4 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48733/comment/3dbbf409_5cf3dff4 
PS2, Line 7: soc/amd/picasso: Add UPDs for support edp phy tuning adjust
> Maybe: […]

Done

File src/soc/amd/picasso/chip.h:

https://review.coreboot.org/c/coreboot/+/48733/comment/1190ca28_28fcd8dd 
PS2, Line 60: /// dpphy_override
> Please use consistent commenting style in this file, which seems C89.

Done

https://review.coreboot.org/c/coreboot/+/48733/comment/f78ed3d5_4ae174a5 
PS2, Line 62:
> Plesae use one space, or be consistent with the rest (though I do not see the benefit of using two s […]

Done

https://review.coreboot.org/c/coreboot/+/48733/comment/56380732_a892252d 
PS2, Line 238: uint8_t   dp_phy_override;
             : 	uint16_t  edp_phy_sel;
             : 	uint8_t   edp_version;
             : 	uint16_t  edp_table_size;
             : 
             : 	struct {
             : 		uint8_t   dp_vs_pemph_level;
             : 		uint8_t   deemph_6db4;
             : 		uint8_t   boostadj;
             : 		uint16_t  margin_deemph;
             : 	} edp_tuningset;
> Please use one space as in the rest of the file.

Done

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[Nikolai Vyssotski (Code Review)]

Attention is currently required from: Paul Menzel, chris wang.
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 3:

(1 comment)

File src/soc/amd/picasso/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/48733/comment/eacea328_b0a27074 
PS3, Line 148: 	if (cfg->dp_phy_override) {
It seems like we should be checking for (cfg->dp_phy_override & ENABLE_EDP_TUNINGSET) here as we are only updating eDP tuning settings.

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[chris wang (Code Review)]

Attention is currently required from: Paul Menzel, chris wang.
Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Chris Wang, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

soc/amd/picasso: Set UPDs for tuning eDP phy

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/48733/4

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[chris wang (Code Review)]

Attention is currently required from: Paul Menzel, Nikolai Vyssotski.
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 4:

(1 comment)

File src/soc/amd/picasso/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/48733/comment/73f86970_5c1fa61e 
PS3, Line 148: 	if (cfg->dp_phy_override) {
> It seems like we should be checking for (cfg->dp_phy_override & ENABLE_EDP_TUNINGSET) here as we are […]

Done

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[Nikolai Vyssotski (Code Review)]

Attention is currently required from: Paul Menzel, chris wang.
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[chris wang (Code Review)]

Attention is currently required from: Paul Menzel, chris wang.
Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Nikolai Vyssotski, Chris Wang, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

soc/amd/picasso: Set UPDs for tuning eDP phy

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 35 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/48733/5

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[Nikolai Vyssotski (Code Review)]

Attention is currently required from: Furquan Shaikh, Kangheui Won, Paul Menzel, chris wang.
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 5: Code-Review+1

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[EricR Lai (Code Review)]

Attention is currently required from: Furquan Shaikh, Kangheui Won, Paul Menzel, chris wang.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48733 )

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/amd/picasso: Set UPDs for tuning eDP phy

[Patrick Georgi (Code Review)]

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

Change subject: soc/amd/picasso: Set UPDs for tuning eDP phy
......................................................................

soc/amd/picasso: Set UPDs for tuning eDP phy

Add UPDs for edp phy tuning adjust.

BUG=b:171269338
BRANCH=zork
TEST=Build, verify the parameter pass to picasso-fsp

Signed-off-by: Chris Wang 
Change-Id: I389bc4b5726f70bb1edfd858dba1c575cf68050b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48733
Tested-by: build bot (Jenkins) 
Reviewed-by: Nikolai Vyssotski 
Reviewed-by: EricR Lai 
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
2 files changed, 35 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  EricR Lai: Looks good to me, approved
  Nikolai Vyssotski: Looks good to me, but someone else must approve

diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index 66ac98d..9498303 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -57,6 +57,18 @@
    SD_EMMC_DRIVE_STRENGTH_D,
 };

+/* dpphy_override */
+enum sysinfo_dpphy_override {
+	ENABLE_DVI_TUNINGSET = 0x01,
+	ENABLE_HDMI_TUNINGSET = 0x02,
+	ENABLE_HDMI6G_TUNINGSET = 0x04,
+	ENABLE_DP_TUNINGSET = 0x08,
+	ENABLE_DP_HBR3_TUNINGSET = 0x10,
+	ENABLE_DP_HBR_TUNINGSET = 0x20,
+	ENABLE_DP_HBR2_TUNINGSET = 0x40,
+	ENABLE_EDP_TUNINGSET = 0x80,
+};
+
 struct soc_amd_picasso_config {
    struct soc_amd_common_config common_config;
    /*
@@ -221,6 +233,16 @@
    enum gpp_clk_req_setting gpp_clk_config[GPP_CLK_OUTPUT_COUNT];
    /* If using an external 48MHz OSC for codec, will disable internal X48M_OSC */
    bool acp_i2s_use_external_48mhz_osc;
+
+	/* eDP phy tuning settings */
+	uint8_t dp_phy_override;
+
+	struct {
+		uint8_t dp_vs_pemph_level;
+		uint8_t deemph_6db4;
+		uint8_t boostadj;
+		uint16_t margin_deemph;
+	} edp_tuningset;
 };

#endif /* __PICASSO_CHIP_H__ */
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c
index c7befe4..3a27a16 100644
--- a/src/soc/amd/picasso/fsp_params.c
+++ b/src/soc/amd/picasso/fsp_params.c
@@ -142,6 +142,18 @@
    scfg->fch_ioapic_id = CONFIG_PICASSO_FCH_IOAPIC_ID;
 }

+static void fsp_edp_tuning_upds(FSP_S_CONFIG *scfg,
+			const struct soc_amd_picasso_config *cfg)
+{
+	if (cfg->dp_phy_override & ENABLE_EDP_TUNINGSET) {
+		scfg->DpPhyOverride = cfg->dp_phy_override;
+		scfg->DpVsPemphLevel = cfg->edp_tuningset.dp_vs_pemph_level;
+		scfg->MarginDeemPh = cfg->edp_tuningset.margin_deemph;
+		scfg->Deemph6db4 = cfg->edp_tuningset.deemph_6db4;
+		scfg->BoostAdj = cfg->edp_tuningset.boostadj;
+	}
+}
+
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
    const struct soc_amd_picasso_config *cfg;
@@ -152,4 +164,5 @@
    fsp_fill_pcie_ddi_descriptors(scfg);
    fsp_assign_ioapic_upds(scfg);
    fsp_usb_oem_customization(scfg, cfg);
+	fsp_edp_tuning_upds(scfg, cfg);
 }