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

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, 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 4:

(11 comments)

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

https://review.coreboot.org/c/coreboot/+/85949/comment/a29f5ec6_3fcdb5b3?usp... :
PS2, Line 91: 0x20
> Done

not done

https://review.coreboot.org/c/coreboot/+/85949/comment/4a3b716d_e8a8fdfc?usp... :
PS2, Line 99: gs_level
> Done

not done

https://review.coreboot.org/c/coreboot/+/85949/comment/558a564c_70b28a91?usp... :
PS2, Line 162: 0x8000
> Done

not done

https://review.coreboot.org/c/coreboot/+/85949/comment/a725cef6_fa644d8f?usp... :
PS2, Line 198: mtk_dvo_power_on
> Done

Then can we rename this to `mtk_dpintf_power_on` for consistency?

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

https://review.coreboot.org/c/coreboot/+/85949/comment/0a0bc373_7d554dce?usp... :
PS2, Line 257: 	mtk_dp->train_info.linkrate = (linkrate >= mtk_dp->train_info.sys_max_linkrate) ?
> Done

Not done.

In PS4 you're still setting `mtk_dp->train_info.linkrate` here.

https://review.coreboot.org/c/coreboot/+/85949/comment/c88cbd5d_e2b9e8ed?usp... :
PS2, Line 372: write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR0, 0xFFFFFFFF);
             : 	write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR1, 0x00040000);
> Done

So, could you answer my question? Do we need to set the same register twice?

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

https://review.coreboot.org/c/coreboot/+/85949/comment/a2726af1_9002114a?usp... :
PS2, Line 52: 	switch (out_format) {
> Done

Not done. Please define a local variable `u32 val`.

https://review.coreboot.org/c/coreboot/+/85949/comment/2db36043_30024981?usp... :
PS2, Line 146: 	switch (value) {
> Done

Not done. Please define a local variable `u32 val`.

https://review.coreboot.org/c/coreboot/+/85949/comment/5b89ab17_feefaf04?usp... :
PS2, Line 307: mtk_edp_hal_reset_swing_pre_emphasis
> Done

Yes, I know these 2 functions are different. I'm asking if we can name the functions better. Does it make sense to rename this function to `dptx_hal_reset_swing_preemphasis`?

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

https://review.coreboot.org/c/coreboot/+/85949/comment/659e697c_eb415d39?usp... :
PS2, Line 199: gs_level
> Done

not done

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

https://review.coreboot.org/c/coreboot/+/85949/comment/54123b85_9b3b445e?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
> Done

So, you think `mtk_dp_` makes better sense than `dptx_hal_`? Why?