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

[Jitao Shi (Code Review)]

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

Jitao Shi 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 6:

(42 comments)

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

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

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/a6033db7_0b07e1c4?usp... :
PS2, Line 162: 0x8000
> not done

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/17c926ac_6a10e898?usp... :
PS2, Line 198: mtk_dvo_power_on
> Then can we rename this to `mtk_dpintf_power_on` for consistency?

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/516d0683_23cb8f1b?usp... :
PS4, Line 102: mtk_dvo_gs[MTK_DVO_8K_30FPS]
> Why is `struct mtk_dvo_gs_info *gs_info` removed? If the index is fixed, let's add the local variabl […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/b182c460_cd4eef17?usp... :
PS4, Line 106: 0xFFFFFFF
> 0xFFFFFFFF

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/740c5522_cc27c6fb?usp... :
PS4, Line 108: 0xFFFFFFF
> 0xFFFFFFFF

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/4150c64c_7fa60eae?usp... :
PS4, Line 133: [dvo]
> How about `[eDPTX] dvo`?

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/2a99f151_0a3ad32e?usp... :
PS6, Line 32: const struct mtk_dvo_sync_param *sync)
> exceed 96 characters

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/10f80c46_1018c7a7?usp... :
PS2, Line 372: write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR0, 0xFFFFFFFF);
             : 	write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR1, 0x00040000);
> So, could you answer my question? Do we need to set the same register twice?

this clock is already configured in mainboard.c,we will remove in dptx.c and mtk_dpintf.c
mainboard_init()->mtk_display_init()->mtk_ddp_init()

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

https://review.coreboot.org/c/coreboot/+/85949/comment/3adc373c_6f8ba858?usp... :
PS4, Line 41: case DPTX_PATTERN_UNKNOWN:
            : 		printk(BIOS_ERR, "Pattern unknown\n");
            : 		mtk_dp_mask(mtk_dp, REG_3400_DP_TRANS_P0, 0x0, PATTERN_EN_DP_TRANS_4P_MASK);
            : 		return;
            : 	default:
            : 		mtk_dp_mask(mtk_dp, REG_3400_DP_TRANS_P0, 0x0, PATTERN_EN_DP_TRANS_4P_MASK);
            : 		return;
> Merge these 2 cases: […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/df538bcd_b60e8af3?usp... :
PS4, Line 54: 					   u8 dpcd_adjust_req[DP_LANSE_ADJUST_SIZE])
> Align. […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/067d80af_a0ccce17?usp... :
PS4, Line 92: lanerate
> linkrate

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/3bd4ef2e_8200a532?usp... :
PS4, Line 94: lane_count
> Rename to `lanecount_enhanced_frame`.

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/c4a0fff6_3b32e655?usp... :
PS4, Line 96: u8 target_link_rate = lanerate;
            : 	u8 target_lane_count = lanecount;
> No need for these `target_` variables. Directly use `linkrate` and `lanecount`.

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/2547a7ee_6941c260?usp... :
PS4, Line 140: DPTX_PLUG_OUT
> You're changing the behavior in PS4. […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/428ed1aa_d9ec36fa?usp... :
PS4, Line 406: "
> Add `%s: ` for `__func__`

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/027f3c9d_bdcd4c2a?usp... :
PS6, Line 384: mdelay
> are you sure this delay it correct ?

udelay(50) is ok.
will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/f98f8c5d_077a12ab?usp... :
PS2, Line 52: 	switch (out_format) {
> Not done. Please define a local variable `u32 val`.

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/50203c06_a1a10870?usp... :
PS2, Line 146: 	switch (value) {
> Not done. Please define a local variable `u32 val`.

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/53609ed4_cef796e4?usp... :
PS2, Line 307: mtk_edp_hal_reset_swing_pre_emphasis
> Yes, I know these 2 functions are different. I'm asking if we can name the functions better. […]

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/e62b134c_547ad0a3?usp... :
PS4, Line 53: 	switch (out_format) {
> > `switch and case should be at the same indent` […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/f2aab74a_05497a83?usp... :
PS4, Line 86: 					u8 *swing_val, u8 *pre_emphasis)
> align […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/e4aca7c6_4bfee723?usp... :
PS4, Line 135: 			GENMASK(2, 0));
> move to previous line

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/5e36429d_a0daa501?usp... :
PS4, Line 142: 	switch (value) {
> > `switch and case should be at the same indent` […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/690ebf7e_1efedf72?usp... :
PS4, Line 337: (mtk_dp_phy_read
> Align with `WAIT_AUX_READY_RETRY_TIMES`

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/3e266d9a_c42a5a12?usp... :
PS4, Line 337: 0x146c
> upper case

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/44db0c4c_6a907dd5?usp... :
PS4, Line 362: pre_emphasis
> preemphasis

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/69d9864b_58b733d0?usp... :
PS4, Line 362: mtk_dp_set_swing_pre_emphasis
> Now that this function contains only a single line, we can write the content of `mtk_edp_phy_set_swi […]

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/b66e8ed5_ab5a0242?usp... :
PS4, Line 363: pre_emphasis
> preemphasis

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/ff1393e5_abcec3b1?usp... :
PS4, Line 388: ,
> remove `,`

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/1b8491bf_1aa660f6?usp... :
PS4, Line 389: break
> return

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/f95aa360_5892fc72?usp... :
PS6, Line 316: mdelay
> why does it take a long time ?

will fix next Patchset,
No need this delay，udelay(10) is ok.

https://review.coreboot.org/c/coreboot/+/85949/comment/8d182643_d7024ddd?usp... :
PS6, Line 338: mdelay
> why does it take a long time ?

Here is polling PHY ready, 
Retry 5 times with a delay of 2ms each time。
we will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/2be2ce4c_0f358c1a?usp... :
PS6, Line 137: 0x6c
> 0x6C

will fix next patchset

https://review.coreboot.org/c/coreboot/+/85949/comment/13abe559_3ee7a584?usp... :
PS6, Line 141: c
> C

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/ae4608d9_516ce462?usp... :
PS4, Line 124: #define VACT_SHIFT		16
> align

will fix next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/24fe96fc_fa01fc3e?usp... :
PS2, Line 199: gs_level
> not done

will be used in mt8196/dp_intf.c next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/947b1459_be0d5bba?usp... :
PS4, Line 26: pre_emphasis
> preemphasis

will be fix next patch

https://review.coreboot.org/c/coreboot/+/85949/comment/9c207345_9af51a85?usp... :
PS4, Line 31: pre_emphasis
> preemphasis

will be fix next patch.

https://review.coreboot.org/c/coreboot/+/85949/comment/fa5e1616_9cb8463e?usp... :
PS4, Line 32: pre_emphasis
> preemphasis

will be fix next patch

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

https://review.coreboot.org/c/coreboot/+/85949/comment/956405e8_5c1f3a20?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
> So, you think `mtk_dp_` makes better sense than `dptx_hal_`? Why?

will be change to dptx_hal_ next patchset

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

https://review.coreboot.org/c/coreboot/+/85949/comment/b27ebdfb_5f70bc97?usp... :
PS4, Line 204: #define DP_TRAIN_VOLTAGE_PREEMPHASIS_LEVEL_3		0x3
> Duplicate of `DPTX_PREEMPHASIS3` defined in src/soc/mediatek/common/dp/include/soc/dptx_common.h.

will fix next patchset