[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.

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:

(106 comments)

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

https://review.coreboot.org/c/coreboot/+/85949/comment/6a57763f_ef75c985?usp... :
PS2, Line 12: static struct mtk_dvo_gs_info mtk_dvo_gs[MTK_DVO_GSL_MAX] = {
> const

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/4ba1c177_1ab59731?usp... :
PS2, Line 32: struct
> const

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/0d44b788_3f8b0fa5?usp... :
PS2, Line 34: HFP_MASK
> This is wrong. Should be `HSYNC_MASK` here.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/a745119f_bdccf223?usp... :
PS2, Line 35: HFP_MASK << HFP
> This is wrong. `HFP_MASK = (0xFFFF << 0)` already contains the shift. […]

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/688ed4c6_80142a97?usp... :
PS2, Line 40: struct
> const

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/f374e500_ec025e7e?usp... :
PS2, Line 62: PIC_HSIZE_MASK << SRC_HSIZE
> These are all wrong.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/3a6ccd95_5787b3ac?usp... :
PS2, Line 107: 0xffffffff
> Use upper case for consistency.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/d1d6e336_fabb5ec6?usp... :
PS2, Line 133: eDPTX reg dvo
> Can we consistently use the same prefix `[eDPTX]` in this file?

Acknowledged

https://review.coreboot.org/c/coreboot/+/85949/comment/49be081c_c105c31c?usp... :
PS2, Line 133:
> one space

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/0f19bb37_923de8b1?usp... :
PS2, Line 141: ;
> ` = {0}`

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/09bd259e_3db6030f?usp... :
PS2, Line 147: 	vsync_lodd.back_porch =
> Order: sync_width -> front_porch -> back_porch -> shift_half_line.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/36d563e4_0aab9cd8?usp... :
PS2, Line 153: hsync.sync_width = edid->mode.hspw / 4;
             : 	hsync.back_porch =
             : 		(edid->mode.hbl - edid->mode.hso - edid->mode.hspw - edid->mode.hborder) / 4;
             : 	hsync.front_porch = (edid->mode.hso - edid->mode.hborder) / 4;
             : 	hsync.shift_half_line = false;
> Move before `vsync_lodd`.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/7a57aa5f_51966d1f?usp... :
PS2, Line 160:
> remove the blank line

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/d0c1868d_1632afd1?usp... :
PS2, Line 187: const struct mtk_dvo dvo_data = {
> static

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/408f9e2f_f41d42dd?usp... :
PS2, Line 188: (
> remove

Done

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

https://review.coreboot.org/c/coreboot/+/85949/comment/a66d76e0_9ba3cce5?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 variable back.

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/3455fe2d_ceb8f354?usp... :
PS2, Line 16:  = 0x0
> remove

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/9a022d26_c37142c9?usp... :
PS2, Line 39: 		break;
> Also add `DPTX_PATTERN_UNKNOWN`

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/e553f56a_a4d779c7?usp... :
PS2, Line 49: 2
> DP_LANSE_ADJUST_SIZE

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/c3646093_ac6e1f30?usp... :
PS2, Line 51: swing
> Prefer `preemphasis` over `pre_emphasis`, as we already have the former name in src/soc/mediatek/com […]

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/92e1c5c4_f9da055f?usp... :
PS2, Line 57: 		int shift = lane % 2 ? DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT : 0;
> Add `assert(index < DP_LANSE_ADJUST_SIZE)`

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/2bdb904a_acb654ef?usp... :
PS2, Line 68: 3
> Define a macro

Acknowledged

https://review.coreboot.org/c/coreboot/+/85949/comment/08b982a6_55502e34?usp... :
PS2, Line 87: = 0x0
> remove

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/9805e8fa_b2f536c6?usp... :
PS2, Line 108: #%x
> `%#x` (Please fix all similar cases in this file)

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/6e32547b_9f0f1596?usp... :
PS2, Line 167: BIOS_INFO
> Is this an error?

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/8303ed5a_0094c6b6?usp... :
PS2, Line 194: = 0x0
> not needed

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/82dbc56d_e44129cc?usp... :
PS2, Line 194: = 0x0
> remove

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/2bbe7e4a_093785f1?usp... :
PS2, Line 257: (linkrate >= mtk_dp->train_info.sys_max_linkrate) ?
             : 				       mtk_dp->train_info.sys_max_linkrate : linkrate;
> Use `MIN(... […]

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/a9db5276_1a154736?usp... :
PS2, Line 276: 	case DP_LINKRATE_HBR2:
> Does this not support DP_LINKRATE_HBR25?

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/574c7ddf_746daa08?usp... :
PS2, Line 300: -EIO
> Let's use `DPTX_TRANING_FAIL` for consistency.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/e2dea9ce_41709785?usp... :
PS2, Line 312: -EINVAL
> DPTX_TRANING_FAIL

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/883f1bc6_85e51dee?usp... :
PS2, Line 315: 		}
> one blank line below

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/0d74278d_a1dce10a?usp... :
PS2, Line 318: ret
> DPTX_TRANING_FAIL

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/6d141d73_646801c0?usp... :
PS2, Line 320: /* Reduce lane count */
             : 			if (lanecount == 0)
             : 				return -EIO;
             : 			lanecount /= 2;
> Write `if (lanecount == DP_LANECOUNT_4) ...`.  Please refer to src/soc/mediatek/common/dp/dptx.c.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/5fa24b2a_63d5578f?usp... :
PS2, Line 327: can run to this
> reach here

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/adcec262_2c0d4241?usp... :
PS2, Line 332: -ETIMEDOUT
> DPTX_TRANING_FAIL

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/6e472f68_20cd4fd6?usp... :
PS2, Line 345: 0
> DPTX_PASS

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/b40025a0_649eba91?usp... :
PS2, Line 364: static struct mtk_dp *edp;
> If `mtk_edp_enable` can be removed, then this can also be removed.

Acknowledged

https://review.coreboot.org/c/coreboot/+/85949/comment/560812a7_6004456f?usp... :
PS2, Line 369: 	printk(BIOS_INFO, "[eDPTX] begin\n");
> Can we remove the log?

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/3d45edce_957460f1?usp... :
PS2, Line 374: m
> Does this mean `MHz`?

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/197c1897_fceb6d3e?usp... :
PS2, Line 375: ff
> upper case

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/5e28d88a_c26f0a93?usp... :
PS2, Line 376: 0x8000
> 0x00008000

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/3fbc1e2d_d8dc5781?usp... :
PS2, Line 394: 	dptx_set_trainingstart(&mtk_edp);
> check return value

Done

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

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

```
case XXX:
case YYY:
    printk;
    mtk_dp_mask;
    return;
```

https://review.coreboot.org/c/coreboot/+/85949/comment/b2d1cac7_bd42ee0c?usp... :
PS4, Line 54: 					   u8 dpcd_adjust_req[DP_LANSE_ADJUST_SIZE])
Align. If there's no way to do it without exceeding the line length limit, we can rename the static function to a shorter name such as `update_swing_pre_emphasis`.

https://review.coreboot.org/c/coreboot/+/85949/comment/3a9125a4_b60e9a2b?usp... :
PS4, Line 92: lanerate
linkrate

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

https://review.coreboot.org/c/coreboot/+/85949/comment/6b648482_84f98773?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`.

https://review.coreboot.org/c/coreboot/+/85949/comment/110e76eb_9e76b154?usp... :
PS4, Line 140: DPTX_PLUG_OUT
You're changing the behavior in PS4. In `dptx_set_trainingstart()` below, we check the return value `DPTX_TRANING_FAIL` instead of `DPTX_PLUG_OUT`.

https://review.coreboot.org/c/coreboot/+/85949/comment/a7681545_194b422f?usp... :
PS4, Line 385: add 26Mhz
`Add 26MHz`

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/b77a225b_3d41a932?usp... :
PS2, Line 24: 0xff
> Upper case for all values

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/70f582b0_ea91405a?usp... :
PS2, Line 37: 0x60
> `BIT(6) | BIT(5)`

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/22b7d613_e047b2f9?usp... :
PS2, Line 38: 10 << 2
> Use hex

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/2a1ceed9_bb82209f?usp... :
PS2, Line 63: 	default:
> If this is an error, print an error message and return.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/dcad7125_668732e4?usp... :
PS2, Line 73: BIT(9) | BIT(8)
> 0x3 << 8

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/4bcee111_ea24316e?usp... :
PS2, Line 82: void mtk_edp_phy_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, u8 lane_count,
> Can be static.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/b3fd58fb_a426d7ec?usp... :
PS2, Line 85: 	if (lane_count >= DPTX_LANE_MAX) {
> Change to assert.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/7cdbf1aa_9601b908?usp... :
PS2, Line 89: && i < DPTX_LANE_MAX
> Remove.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/6b54dd9d_fa75322f?usp... :
PS2, Line 94: lane_count
> i

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/ae53e3c0_596e33e3?usp... :
PS2, Line 98:
> extra blank line

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/23265908_d2a56756?usp... :
PS2, Line 99: int
> Wrong type. Please upload a separate patch to fix this and preemphasis.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/8ac8c171_daaedffb?usp... :
PS2, Line 102: if (lane_num >= DPTX_LANE_MAX) {
             : 		printk(BIOS_ERR, "invalid lane number: %d\n", lane_num);
             : 		return false;
             : 	}
> assert

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/638b4623_49d5dbd3?usp... :
PS2, Line 107: i < DPTX_LANE_MAX
> remove

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/c4f12a1b_7976348f?usp... :
PS2, Line 112: lane_num
> i

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/86057521_39902697?usp... :
PS2, Line 134: 	/* edp phy init*/
> remove

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/189263e3_ecfe1e8d?usp... :
PS2, Line 138: BIT(0) | BIT(1) | BIT(2),
             : 			BIT(0) | BIT(1) | BIT(2));
> Use GENMASK

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/643fe726_f3e9678c?usp... :
PS2, Line 161: break
> return

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/51a3df4f_a285a65b?usp... :
PS2, Line 205: 0x1fff
> upper case

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/003c8cce_0d2f97ae?usp... :
PS2, Line 302: 0x0000000f, 0x0000000f
> 0xF, 0xF

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/95a6e92d_70081345?usp... :
PS2, Line 327: 	printk(BIOS_DEBUG, "[eDPTX] DP_PHY_DIG_TX_CTL_0:%#x\n", val);
> remove this or the log in line #324

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/f21449c0_e6e191ee?usp... :
PS2, Line 346: do {
             : 		val = mtk_dp_phy_read(mtk_dp, 0x146c);
             : 		if ((val & mask) == mask)
             : 			break;
             : 		mdelay(100);
             : 	} while (retry--);
>

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/f46fe800_f5cec680?usp... :
PS2, Line 369: 	if ((value << 2) <= UINT8_MAX) {
> use assert

assert not added yet

https://review.coreboot.org/c/coreboot/+/85949/comment/349b5452_30312210?usp... :
PS2, Line 380: for (int lane = 0; lane < lane_count; lane++)
             : 		printk(BIOS_INFO,
             : 		       "Link training lane%d: swing_val = 0x%x, pre-emphasis = 0x%x\n", lane,
             : 		       swing_val[lane], pre_emphasis[lane]);
> Remove. These values are already printed in `mtk_edp_phy_set_swing_pre_emphasis`.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/06ae3c88_fe6064b9?usp... :
PS2, Line 408: Set idle pattern failed,
> %s: Unexpected

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/32f53ea4_3a679100?usp... :
PS2, Line 408:
> `__func__`

Done

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

https://review.coreboot.org/c/coreboot/+/85949/comment/88744085_852aa373?usp... :
PS4, Line 39: 0xA << 2
0x28

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

Please fix.

https://review.coreboot.org/c/coreboot/+/85949/comment/ca419085_9838d333?usp... :
PS4, Line 86: 					u8 *swing_val, u8 *pre_emphasis)
align

Feel free to rename the function to a shorter name.

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

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

Please fix.

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/4f2feb50_c1d96da3?usp... :
PS4, Line 362: pre_emphasis
preemphasis

https://review.coreboot.org/c/coreboot/+/85949/comment/c1886608_6c727c5b?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_swing_pre_emphasis` here, and remove `mtk_edp_phy_set_swing_pre_emphasis`.

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/2e5bc6dc_d52b0751?usp... :
PS2, Line 75: a
> A

Done

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/ed3302a3_dd10f4ad?usp... :
PS2, Line 206: shift_half_line
> Remove if unused

Done

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

https://review.coreboot.org/c/coreboot/+/85949/comment/9cca1368_54ede587?usp... :
PS2, Line 6: #include <soc/dptx_common.h>
> one blank line below

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/61729c99_6c2218e1?usp... :
PS2, Line 7: #define DP_LANSE_ADJUST_SIZE		2
> Move to src/soc/mediatek/common/dp/include/soc/dptx_common. […]

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/2c6a69a2_f8e0e015?usp... :
PS2, Line 8: #endif /* __SOC_MEDIATEK_MT8196_INCLUDE_SOC_DPTX_H__ */
> one blank line before `#endif`

Done

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

https://review.coreboot.org/c/coreboot/+/85949/comment/2d921d22_4b7ceeab?usp... :
PS4, Line 26: pre_emphasis
preemphasis

https://review.coreboot.org/c/coreboot/+/85949/comment/0d8b2436_0b3f0c5f?usp... :
PS4, Line 31: pre_emphasis
preemphasis

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

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

https://review.coreboot.org/c/coreboot/+/85949/comment/721bdb3c_1ae2b0f1?usp... :
PS2, Line 26: enum {
            : 	ENODEV = 1,
            : 	ETIMEDOUT,
            : 	EIO,
            : 	EINVAL,
            : };
> Remove these, and use `DPTX_*` values defined in src/soc/mediatek/common/dp/include/soc/dptx_common. […]

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/e4a0cd48_c4e03d0c?usp... :
PS2, Line 36: void mtk_edp_phy_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, u8 lane_count,
> Remove here and change to static.

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/a5c121ef_13927d4b?usp... :
PS2, Line 37: u8
> align

Done

https://review.coreboot.org/c/coreboot/+/85949/comment/f46d333f_20fc2d1c?usp... :
PS2, Line 42: #endif /* __SOC_MEDIATEK_MT8196_DP_DPTX_HAL_H__ */
> Add blank line above

Done

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

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