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:
(42 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/85949/comment/f7ce01f1_d1a11e7a?usp... : PS2, Line 10: traning Do you mean `training`?
https://review.coreboot.org/c/coreboot/+/85949/comment/7aed14d0_e400de71?usp... : PS2, Line 11: Please mention what DVO is in the commit message.
File src/soc/mediatek/mt8196/dp_intf.c:
https://review.coreboot.org/c/coreboot/+/85949/comment/b62590da_febc2816?usp... : PS2, Line 12: static struct mtk_dvo_gs_info mtk_dvo_gs[MTK_DVO_GSL_MAX] = { const
https://review.coreboot.org/c/coreboot/+/85949/comment/7ef56e53_f804cb50?usp... : PS2, Line 18: { Add `assert((val & mask) == val)`.
https://review.coreboot.org/c/coreboot/+/85949/comment/af17f6f1_d64c06d3?usp... : PS2, Line 32: struct const
https://review.coreboot.org/c/coreboot/+/85949/comment/56bfab5b_2df5db43?usp... : PS2, Line 34: HFP_MASK This is wrong. Should be `HSYNC_MASK` here.
https://review.coreboot.org/c/coreboot/+/85949/comment/298d3a4b_393992d0?usp... : PS2, Line 35: HFP_MASK << HFP This is wrong. `HFP_MASK = (0xFFFF << 0)` already contains the shift. Can we rename `HSYNC` to `HSYNC_SHIFT` in dp_intf.h (and do the same for all other similar values), and change `HFP_MASK` to `0xFFFF` (without the shift)?
https://review.coreboot.org/c/coreboot/+/85949/comment/167b1503_cc599098?usp... : PS2, Line 40: struct const
https://review.coreboot.org/c/coreboot/+/85949/comment/3e8d728e_798d91ba?usp... : PS2, Line 62: PIC_HSIZE_MASK << SRC_HSIZE These are all wrong.
https://review.coreboot.org/c/coreboot/+/85949/comment/0ba31c50_ead475f3?usp... : PS2, Line 91: 0x20 `0x20 << V_LAST_TRAILING_BLANK`
https://review.coreboot.org/c/coreboot/+/85949/comment/fa6cc3fe_2ad7e03d?usp... : PS2, Line 99: gs_level If `gs_level` is not used, please remove the field from the struct. Also see the comment below.
https://review.coreboot.org/c/coreboot/+/85949/comment/e228f978_b6582136?usp... : PS2, Line 102: MTK_DVO_8K_30FPS Should this be `dvo->gs_level`? Why do we hardcode the index here?
https://review.coreboot.org/c/coreboot/+/85949/comment/76b94a4c_50abb0e7?usp... : PS2, Line 107: 0xffffffff Use upper case for consistency.
https://review.coreboot.org/c/coreboot/+/85949/comment/dd53a279_6f0e931c?usp... : PS2, Line 133: one space
https://review.coreboot.org/c/coreboot/+/85949/comment/88df8a60_a2d365b8?usp... : PS2, Line 133: eDPTX reg dvo Can we consistently use the same prefix `[eDPTX]` in this file?
https://review.coreboot.org/c/coreboot/+/85949/comment/3b92b7ac_6e979fbd?usp... : PS2, Line 141: ; ` = {0}`
https://review.coreboot.org/c/coreboot/+/85949/comment/1863ba70_e99fbddc?usp... : PS2, Line 147: vsync_lodd.back_porch = Order: sync_width -> front_porch -> back_porch -> shift_half_line.
https://review.coreboot.org/c/coreboot/+/85949/comment/2be49675_19cbd580?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`.
https://review.coreboot.org/c/coreboot/+/85949/comment/eb0a5022_1ebcad2d?usp... : PS2, Line 160: Remove blank line. Is the comment above for lines #161-162 below?
https://review.coreboot.org/c/coreboot/+/85949/comment/9bf2d787_6c8058a9?usp... : PS2, Line 162: 0x8000 0x00008000
https://review.coreboot.org/c/coreboot/+/85949/comment/ecd0ec53_0075ec6b?usp... : PS2, Line 187: const struct mtk_dvo dvo_data = { static
https://review.coreboot.org/c/coreboot/+/85949/comment/90d2c9c3_1ed3bda8?usp... : PS2, Line 188: ( remove
https://review.coreboot.org/c/coreboot/+/85949/comment/3b6b437b_5b301b27?usp... : PS2, Line 198: mtk_dvo_power_on This is named `mtk_dpintf_power_on` in src/soc/mediatek/common/dp/dp_intf.c. Can you explain the difference?
File src/soc/mediatek/mt8196/dptx.c:
https://review.coreboot.org/c/coreboot/+/85949/comment/a37081cf_8531347e?usp... : PS2, Line 16: = 0x0 remove
https://review.coreboot.org/c/coreboot/+/85949/comment/e1f98ccd_ef9aafbe?usp... : PS2, Line 39: break; Also add `DPTX_PATTERN_UNKNOWN`
https://review.coreboot.org/c/coreboot/+/85949/comment/2a4d7475_d2af9ff3?usp... : PS2, Line 49: 2 DP_LANSE_ADJUST_SIZE
https://review.coreboot.org/c/coreboot/+/85949/comment/208d9cf3_e07b3893?usp... : PS2, Line 51: swing Please be consistent with the `mtk_edp_set_swing_pre_emphasis` argument names. Either `swing` or `swing_val`. Either `preemphasis` or `pre_emphasis`.
https://review.coreboot.org/c/coreboot/+/85949/comment/f0b41527_738d8458?usp... : PS2, Line 57: int shift = lane % 2 ? DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT : 0; Add `assert(index < DP_LANSE_ADJUST_SIZE)`
https://review.coreboot.org/c/coreboot/+/85949/comment/0a0ace8e_5e3cb5e4?usp... : PS2, Line 68: 3 Define a macro
https://review.coreboot.org/c/coreboot/+/85949/comment/a7f30528_ffe8b225?usp... : PS2, Line 87: = 0x0 remove
https://review.coreboot.org/c/coreboot/+/85949/comment/d04d1400_a0a25a03?usp... : PS2, Line 108: #%x `%#x` (Please fix all similar cases in this file)
https://review.coreboot.org/c/coreboot/+/85949/comment/f1e94cbb_2d5b7360?usp... : PS2, Line 167: BIOS_INFO Is this an error?
https://review.coreboot.org/c/coreboot/+/85949/comment/2ca5354a_d63c9fc9?usp... : PS2, Line 194: = 0x0 remove
https://review.coreboot.org/c/coreboot/+/85949/comment/72c648ce_7afae419?usp... : PS2, Line 348: static void dptx_init_port(struct mtk_dp *mtk_dp) @yidilin@google.com
This can also be shared with existing SoCs, by adding a no-op `dptx_hal_phy_init` for other existing SoCs.
https://review.coreboot.org/c/coreboot/+/85949/comment/3a54c75d_9d7cd5d7?usp... : PS2, Line 364: static struct mtk_dp *edp; If `mtk_edp_enable` can be removed, then this can also be removed.
https://review.coreboot.org/c/coreboot/+/85949/comment/3c6ce6c5_85adec93?usp... : PS2, Line 369: printk(BIOS_INFO, "[eDPTX] begin\n"); Can we remove the log?
https://review.coreboot.org/c/coreboot/+/85949/comment/d92730c5_8baa46ff?usp... : PS2, Line 372: write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR0, 0xFFFFFFFF); : write32p(MMSYS1_CONFIG_BASE + MMSYS1_CG_CLR1, 0x00040000); We also write to these regs in `mtk_dvo_set_display_mode` with the same values. Do we need these writes here?
https://review.coreboot.org/c/coreboot/+/85949/comment/8c7f7a97_f29f53cb?usp... : PS2, Line 374: m Does this mean `MHz`?
https://review.coreboot.org/c/coreboot/+/85949/comment/039eaf1c_85cd8f71?usp... : PS2, Line 375: 0xff000000 We also write to this reg in `mtk_dvo_set_display_mode`, with a different value `0x80000000`. Could you explain it? Is this write necessary here?
https://review.coreboot.org/c/coreboot/+/85949/comment/b4d99d90_8b86305d?usp... : PS2, Line 375: ff upper case
https://review.coreboot.org/c/coreboot/+/85949/comment/195c34ef_fbb94f3a?usp... : PS2, Line 376: 0x8000 0x00008000
https://review.coreboot.org/c/coreboot/+/85949/comment/65d79f70_a820a51e?usp... : PS2, Line 397: Can we call `dptx_video_enable` here just like src/soc/mediatek/common/dp/dptx.c? Then `mtk_edp_enable` can be removed.
-
Yu-Ping Wu (Code Review)