[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add eDP driver

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Bincai Liu, Hung-Te Lin, Jarried Lin, Jitao Shi, Yidi Lin.

Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85949?usp=email )

Change subject: soc/mediatek/mt8196: Add eDP driver
......................................................................

Patch Set 7:

(30 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85949/comment/9e4b1cce_1949b817?usp... :
PS2, Line 10: traning
> Do you mean `training`?

Acknowledged

https://review.coreboot.org/c/coreboot/+/85949/comment/6868c607_48279676?usp... :
PS2, Line 11:
> Please mention what DVO is in the commit message.

Done

File src/soc/mediatek/mt8196/dp_intf.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/0ad7c2b4_9952687e?usp... :
PS2, Line 99: gs_level
> not done

Done

File src/soc/mediatek/mt8196/dp_intf.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/7c15959a_d563190d?usp... :
PS6, Line 32: const struct mtk_dvo_sync_param *sync)
> will fix next patchset

Done

File src/soc/mediatek/mt8196/dp_intf.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/a924af6b_f23a4a80?usp... :
PS7, Line 95: 0x20
`0x20 << EXT_TG_DLY_LINE`

https://review.coreboot.org/c/coreboot/+/85949/comment/92c4e99a_a9978ff3?usp... :
PS7, Line 100: if (dvo->gs_level >= MTK_DVO_GSL_MAX)
             : 		return;
`assert(dvo->gs_level < MTK_DVO_GSL_MAX)`

https://review.coreboot.org/c/coreboot/+/85949/comment/2c415ab8_49a8df17?usp... :
PS7, Line 181: .regs_ck = (void *)CKSYS_GP2_BASE,
             : 	.regs_mm = (void *)MMSYS1_CONFIG_BASE,
Remove these two unused fields from here and the header.

https://review.coreboot.org/c/coreboot/+/85949/comment/b3622ba3_7d2c61cd?usp... :
PS7, Line 191: mtk_dpintf_power_on
Now this name becomes inconsistent with other function names in this file. From your commit message, "DISP_DVO is a highly advanced variant of DP_INTF". Therefore I think it's fine to use "dvo" in the function name. Could you please change it back?

File src/soc/mediatek/mt8196/dptx.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/cc2b637f_b6b73c80?usp... :
PS4, Line 54: 					   u8 dpcd_adjust_req[DP_LANSE_ADJUST_SIZE])
> will fix next patchset

Done

File src/soc/mediatek/mt8196/dptx.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/225c44c6_6295f5e1?usp... :
PS7, Line 52: 					   u8 dpcd_adjust_req[DP_LANSE_ADJUST_SIZE])
align

https://review.coreboot.org/c/coreboot/+/85949/comment/0c52aa20_5747806c?usp... :
PS7, Line 70: DP_TRAIN_VOLTAGE_SWING_LEVEL_3
DPTX_SWING3

https://review.coreboot.org/c/coreboot/+/85949/comment/9d997953_4662d1a6?usp... :
PS7, Line 269: linkrate = mtk_dp->train_info.linkrate;
             : 	lanecount = mtk_dp->train_info.linklane_count;
This is wrong. By the time we reach here, `mtk_dp->train_info.*` haven't been set yet. Please remove these.

https://review.coreboot.org/c/coreboot/+/85949/comment/a090a9ee_3819df70?usp... :
PS7, Line 370: int mtk_edp_init(struct mtk_dp *mtk_dp,struct edid *edid)
> `space required after that ',' (ctx:VxV)`

Please fix.

https://review.coreboot.org/c/coreboot/+/85949/comment/6a9df618_b2612fc8?usp... :
PS7, Line 374: 0xFF000000
Please confirm if this is supposed to be 0xFF000000 or 0x80000000.

https://review.coreboot.org/c/coreboot/+/85949/comment/776863ec_6cc79d4d?usp... :
PS7, Line 376: m
u

https://review.coreboot.org/c/coreboot/+/85949/comment/4a8dcee4_d0aeda59?usp... :
PS7, Line 393: %s
`%s: Failed to ...`

File src/soc/mediatek/mt8196/dptx_hal.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/a39a2e91_708ec074?usp... :
PS4, Line 142: 	switch (value) {
> will fix next patchset

Done

File src/soc/mediatek/mt8196/dptx_hal.c:

https://review.coreboot.org/c/coreboot/+/85949/comment/dae8d545_e0448838?usp... :
PS7, Line 106: mtk_dp_phy_mask(mtk_dp, driving_offset[i], 0, 0x18);
             : 		mtk_dp_phy_mask(mtk_dp, driving_offset[i], 0, 0x6);
Can we merge these into a single call?

```
mtk_dp_phy_mask(mtk_dp, driving_offset[i], 0,
                EDP_TX_LN_VOLT_SWING_VAL_MASK | EDP_TX_LN_PRE_EMPH_VAL_MASK);
```

Also remove `{}` for the "for loop".

https://review.coreboot.org/c/coreboot/+/85949/comment/529f59fb_86cb8ac9?usp... :
PS7, Line 290: dptx_hal_reset_swing_preemphasis_mt8196
How about `dptx_hal_swing_emp_reset` (to be consistent with `dptx_hal_phyd_reset` below)?

File src/soc/mediatek/mt8196/include/soc/dp_intf.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/7d847483_ab1ee72d?usp... :
PS6, Line 137: 0x6c
> will fix next patchset

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/52698015_a736c4a8?usp... :
PS6, Line 141: c
> will fix next patchset

Done

File src/soc/mediatek/mt8196/include/soc/dp_intf.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/6794f69d_dc683556?usp... :
PS4, Line 112: #define HACT_SHIFT		16
> align

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/a48bafb7_c36e03cb?usp... :
PS4, Line 124: #define VACT_SHIFT		16
> will fix next patchset

Done

File src/soc/mediatek/mt8196/include/soc/dp_intf.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/5309e1f6_0ec64496?usp... :
PS2, Line 199: gs_level
> will be used in mt8196/dp_intf. […]

Done

File src/soc/mediatek/mt8196/include/soc/dptx_hal.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/ae9a392d_25f8c168?usp... :
PS7, Line 32: 				   u8 *preemphasis);
align

File src/soc/mediatek/mt8196/include/soc/dptx_hal.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/7072ece3_34935ef9?usp... :
PS4, Line 26: pre_emphasis
> will be fix next patch

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/fc3b46ca_eafe7f1d?usp... :
PS4, Line 31: pre_emphasis
> will be fix next patch.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/42559bfa_3458e87d?usp... :
PS4, Line 32: pre_emphasis
> will be fix next patch

Done

File src/soc/mediatek/mt8196/include/soc/dptx_hal.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/5d58daf5_66e4f402?usp... :
PS2, Line 38: void mtk_edp_phy_wait_aux_ldo_ready(struct mtk_dp *mtk_dp);
            : void mtk_edp_hal_phy_set_idle_pattern(struct mtk_dp *mtk_dp, u8 lane_count, bool enable);
            : void mtk_edp_set_swing_pre_emphasis
> will be change to dptx_hal_ next patchset

Done

File src/soc/mediatek/mt8196/include/soc/dptx_reg.h:

https://review.coreboot.org/c/coreboot/+/85949/comment/4f55888e_672bda9f?usp... :
PS7, Line 198: DP_TRAIN_VOLTAGE_SWING_LEVEL_3
Also duplicate of `DPTX_SWING3`.