Attention is currently required from: Hung-Te Lin, Jarried 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 2:
(50 comments)
File src/soc/mediatek/mt8196/dptx.c:
https://review.coreboot.org/c/coreboot/+/85949/comment/341b788c_506b9e09?usp... : PS2, Line 51: swing
Please be consistent with the `mtk_edp_set_swing_pre_emphasis` argument names. […]
Prefer `preemphasis` over `pre_emphasis`, as we already have the former name in src/soc/mediatek/common/dp/include/soc/dptx_hal_common.h.
https://review.coreboot.org/c/coreboot/+/85949/comment/d16a7de8_759aa6ad?usp... : PS2, Line 89: u8 target_link_rate = mtk_dp->train_info.linkrate; : u8 target_lane_count = mtk_dp->train_info.linklane_count; Should we pass the `linkrate` and `lanecount` from the for loop in `dptx_set_trainingstart()`, just like `dptx_trainingflow` in src/soc/mediatek/common/dp/dptx.c? Note that in the for loop, we reduce the local variable `linkrate` instead of `mtk_dp->train_info.linkrate`. That looks like a bug to me.
https://review.coreboot.org/c/coreboot/+/85949/comment/f25a0c3a_3f39dde4?usp... : PS2, Line 257: mtk_dp->train_info.linkrate = (linkrate >= mtk_dp->train_info.sys_max_linkrate) ? To simplify the data flow, can we set `mtk_dp->train_info` fields only at the end of this function (lines #334-335 below)? Before that, we can always use the local variables.
https://review.coreboot.org/c/coreboot/+/85949/comment/51abc8bf_c9ef5893?usp... : PS2, Line 257: (linkrate >= mtk_dp->train_info.sys_max_linkrate) ? : mtk_dp->train_info.sys_max_linkrate : linkrate; Use `MIN(...)`
https://review.coreboot.org/c/coreboot/+/85949/comment/107199e8_213ba5ca?usp... : PS2, Line 276: case DP_LINKRATE_HBR2: Does this not support DP_LINKRATE_HBR25?
https://review.coreboot.org/c/coreboot/+/85949/comment/b5651dd2_90c4a805?usp... : PS2, Line 300: -EIO Let's use `DPTX_TRANING_FAIL` for consistency.
https://review.coreboot.org/c/coreboot/+/85949/comment/a9f048ab_8a30f3de?usp... : PS2, Line 312: -EINVAL DPTX_TRANING_FAIL
https://review.coreboot.org/c/coreboot/+/85949/comment/629f6323_b38d7604?usp... : PS2, Line 315: } one blank line below
https://review.coreboot.org/c/coreboot/+/85949/comment/5366cb50_b20ebfa7?usp... : PS2, Line 318: ret DPTX_TRANING_FAIL
https://review.coreboot.org/c/coreboot/+/85949/comment/6866897c_7e98256d?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.
https://review.coreboot.org/c/coreboot/+/85949/comment/f283dcba_d9b3bc66?usp... : PS2, Line 327: can run to this reach here
https://review.coreboot.org/c/coreboot/+/85949/comment/5940fa47_9473a1f4?usp... : PS2, Line 332: -ETIMEDOUT DPTX_TRANING_FAIL
https://review.coreboot.org/c/coreboot/+/85949/comment/5463f012_51cd4804?usp... : PS2, Line 345: 0 DPTX_PASS
https://review.coreboot.org/c/coreboot/+/85949/comment/d41fd74d_159066a9?usp... : PS2, Line 394: dptx_set_trainingstart(&mtk_edp); check return value
File src/soc/mediatek/mt8196/dptx_hal.c:
https://review.coreboot.org/c/coreboot/+/85949/comment/dd06ce90_9e9bb5a4?usp... : PS2, Line 24: 0xff Upper case for all values
https://review.coreboot.org/c/coreboot/+/85949/comment/bcc8f6d0_ddfece72?usp... : PS2, Line 37: 0x60 `BIT(6) | BIT(5)`
https://review.coreboot.org/c/coreboot/+/85949/comment/69609fa0_da371d7d?usp... : PS2, Line 38: 10 << 2 Use hex
https://review.coreboot.org/c/coreboot/+/85949/comment/8454dca4_2373fc75?usp... : PS2, Line 52: switch (out_format) { ``` u32 val;
switch (out_format) { case DP_COLOR_FORMAT_RGB_444: case DP_COLOR_FORMAT_YUV_444: val = 0; ... }
mtk_dp_mask(mtk_dp, REG_303C_DP_ENCODER0_P0, val << 12, 0x7 << 12); ```
https://review.coreboot.org/c/coreboot/+/85949/comment/aec1e38a_b4f42005?usp... : PS2, Line 63: default: If this is an error, print an error message and return.
https://review.coreboot.org/c/coreboot/+/85949/comment/e3a7ac04_fa0a0fef?usp... : PS2, Line 73: BIT(9) | BIT(8) 0x3 << 8
https://review.coreboot.org/c/coreboot/+/85949/comment/32704c05_2cd5c341?usp... : PS2, Line 82: void mtk_edp_phy_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, u8 lane_count, Can be static.
https://review.coreboot.org/c/coreboot/+/85949/comment/ad0982ef_d740d75c?usp... : PS2, Line 85: if (lane_count >= DPTX_LANE_MAX) { Change to assert.
https://review.coreboot.org/c/coreboot/+/85949/comment/01483db9_dad5a502?usp... : PS2, Line 89: && i < DPTX_LANE_MAX Remove.
https://review.coreboot.org/c/coreboot/+/85949/comment/101cd09b_41357ed4?usp... : PS2, Line 94: lane_count i
https://review.coreboot.org/c/coreboot/+/85949/comment/3835bd11_8a8a2e7d?usp... : PS2, Line 98: extra blank line
https://review.coreboot.org/c/coreboot/+/85949/comment/15f356a6_747f8ca2?usp... : PS2, Line 99: int Wrong type. Please upload a separate patch to fix this and preemphasis.
https://review.coreboot.org/c/coreboot/+/85949/comment/4cb534c3_25c3a633?usp... : PS2, Line 102: if (lane_num >= DPTX_LANE_MAX) { : printk(BIOS_ERR, "invalid lane number: %d\n", lane_num); : return false; : } assert
https://review.coreboot.org/c/coreboot/+/85949/comment/6c60b8d3_bc7d2259?usp... : PS2, Line 107: i < DPTX_LANE_MAX remove
https://review.coreboot.org/c/coreboot/+/85949/comment/4c4a92d5_54766a40?usp... : PS2, Line 112: lane_num i
https://review.coreboot.org/c/coreboot/+/85949/comment/d77c0c92_b25bf998?usp... : PS2, Line 134: /* edp phy init*/ remove
https://review.coreboot.org/c/coreboot/+/85949/comment/9394f031_6161a919?usp... : PS2, Line 138: BIT(0) | BIT(1) | BIT(2), : BIT(0) | BIT(1) | BIT(2)); Use GENMASK
https://review.coreboot.org/c/coreboot/+/85949/comment/7001422b_f3d073c9?usp... : PS2, Line 146: switch (value) { ``` u32 val; switch (value) { case ...: val = ...; ... }
mtk_dp_phy_write(mtk_dp, 0x143C, val); ```
https://review.coreboot.org/c/coreboot/+/85949/comment/a936e838_71b19b94?usp... : PS2, Line 161: break return
https://review.coreboot.org/c/coreboot/+/85949/comment/62886a3b_7cd3674a?usp... : PS2, Line 205: 0x1fff upper case
https://review.coreboot.org/c/coreboot/+/85949/comment/43a9105f_0a966e05?usp... : PS2, Line 302: 0x0000000f, 0x0000000f 0xF, 0xF
https://review.coreboot.org/c/coreboot/+/85949/comment/525e226c_5626f237?usp... : PS2, Line 307: mtk_edp_hal_reset_swing_pre_emphasis Should we rename this to `dptx_hal_reset_swing_preemphasis`? This looks similar to `dptx_hal_reset_swing_preemphasis` in src/soc/mediatek/common/dp/dptx_hal.c.
And can we rename the current `dptx_hal_reset_swing_preemphasis` in this file to `dptx_hal_phy_reset_swing_preemphasis` (does this name make sense)?
https://review.coreboot.org/c/coreboot/+/85949/comment/433d750e_669675fc?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
https://review.coreboot.org/c/coreboot/+/85949/comment/9f2aa064_35b2e189?usp... : PS2, Line 346: do { : val = mtk_dp_phy_read(mtk_dp, 0x146c); : if ((val & mask) == mask) : break; : mdelay(100); : } while (retry--); ``` if (!retry(WAIT_AUX_READY_RETRY_TIMES, (mtk_dp_phy_read(...) & mask) == mask, mdelay(100))) { printk(BIOS_ERR, ...); } ```
https://review.coreboot.org/c/coreboot/+/85949/comment/37632be9_050802d9?usp... : PS2, Line 369: if ((value << 2) <= UINT8_MAX) { use assert
https://review.coreboot.org/c/coreboot/+/85949/comment/3030c7d6_62d430c0?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`.
https://review.coreboot.org/c/coreboot/+/85949/comment/42030be5_b90ead38?usp... : PS2, Line 408: Set idle pattern failed, %s: Unexpected
https://review.coreboot.org/c/coreboot/+/85949/comment/e1c8efcd_cee4b1a2?usp... : PS2, Line 408: `__func__`
File src/soc/mediatek/mt8196/include/soc/dp_intf.h:
https://review.coreboot.org/c/coreboot/+/85949/comment/5b770f7a_4aafb715?usp... : PS2, Line 199: gs_level Remove if unused
https://review.coreboot.org/c/coreboot/+/85949/comment/5756d87c_7c2ca6cc?usp... : PS2, Line 206: shift_half_line Remove if unused
File src/soc/mediatek/mt8196/include/soc/dptx.h:
https://review.coreboot.org/c/coreboot/+/85949/comment/96c85869_b901729b?usp... : PS2, Line 6: #include <soc/dptx_common.h> one blank line below
https://review.coreboot.org/c/coreboot/+/85949/comment/9bb6de46_86d173be?usp... : PS2, Line 7: #define DP_LANSE_ADJUST_SIZE 2 Move to src/soc/mediatek/common/dp/include/soc/dptx_common.h, as the `dptx_train_tps1` function in src/soc/mediatek/common/dp/dptx.c can also use this value in a future follow-up patch.
File src/soc/mediatek/mt8196/include/soc/dptx_hal.h:
https://review.coreboot.org/c/coreboot/+/85949/comment/4c73fcfd_64acb490?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.h
https://review.coreboot.org/c/coreboot/+/85949/comment/a576109b_6be83c9a?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.
https://review.coreboot.org/c/coreboot/+/85949/comment/4137ae6c_84039990?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 Ideally APIs in this file should start with `dptx_hal_`. Can we rename these?
https://review.coreboot.org/c/coreboot/+/85949/comment/78053605_83860752?usp... : PS2, Line 42: #endif /* __SOC_MEDIATEK_MT8196_DP_DPTX_HAL_H__ */ Add blank line above
-
Yu-Ping Wu (Code Review)