coreboot-gerrit January 2025

coreboot-gerrit@coreboot.org

1 participants
3269 discussions

[S] Change in coreboot[main]: soc/mediatek/mt8196: Add mtk-fsp loader in ramstage
by Yu-Ping Wu (Code Review) 17 Jan '25

17 Jan '25

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add thermal driver
by Zhaoqing Jiu (Code Review) 17 Jan '25

17 Jan '25

1 0

[M] Change in coreboot[main]: mb/google/rauru: Add edp driver in mainboard
by Bincai Liu (Code Review) 17 Jan '25

17 Jan '25

1 0

[XS] Change in coreboot[main]: soc/mediatek/mt8196: Add GCE ddren sel control to mminfra
by Yu-Ping Wu (Code Review) 17 Jan '25

17 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, Jason-jh Lin, Yidi Lin. Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86027?usp=email ) Change subject: soc/mediatek/mt8196: Add GCE ddren sel control to mminfra ...................................................................... Patch Set 3: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/86027/comment/d1891ee6_4d3501f2?us… : PS3, Line 12: request requests https://review.coreboot.org/c/coreboot/+/86027/comment/8721964a_10587f46?us… : PS3, Line 14: Otherwise, GCE will hang when accessing DRAM. Move the first few words to the previous line File src/soc/mediatek/mt8196/mminfra.c: https://review.coreboot.org/c/coreboot/+/86027/comment/557889e4_bfc8ac10?us… : PS3, Line 35: 0x1 Maybe just `1` for consistency? -- To view, visit https://review.coreboot.org/c/coreboot/+/86027?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I30309b0426f803e28858eb15652a649927f94c7e Gerrit-Change-Number: 86027 Gerrit-PatchSet: 3 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jason-jh Lin <jason-jh.lin(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Jason-jh Lin <jason-jh.lin(a)mediatek.corp-partner.google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 11:05:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[M] Change in coreboot[main]: mb/google/rauru: Add edp driver in mainboard
by Yu-Ping Wu (Code Review) 17 Jan '25

17 Jan '25

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/+/85951?usp=email ) Change subject: mb/google/rauru: Add edp driver in mainboard ...................................................................... Patch Set 4: (6 comments) File src/mainboard/google/rauru/chromeos.c: https://review.coreboot.org/c/coreboot/+/85951/comment/54e27e7f_55c68d8d?us… : PS2, Line 60: struct lb_gpio edp_pwm_gpios[] = { : {GPIO_BL_PWM_1V8.id, ACTIVE_HIGH, -1, "PWM control"}, : }; : lb_add_gpios(gpios, edp_pwm_gpios, ARRAY_SIZE(edp_pwm_gpios)); : struct lb_gpio backlight_gpios[] = { : {GPIO_AP_EDP_BKLTEN.id, ACTIVE_HIGH, -1, "backlight enable"}, : }; > Use a single array. Done File src/mainboard/google/rauru/devicetree.cb: https://review.coreboot.org/c/coreboot/+/85951/comment/6f7b266e_80d151cd?us… : PS2, Line 12: Panel > PANEL Done File src/mainboard/google/rauru/gpio.h: https://review.coreboot.org/c/coreboot/+/85951/comment/bd325e96_2d80a157?us… : PS2, Line 18: #define GPIO_BL_PWM_1V8 GPIO(DISP_PWM) > leave one blank line on above Done File src/mainboard/google/rauru/panel.c: https://review.coreboot.org/c/coreboot/+/85951/comment/b057a967_b684a628?us… : PS2, Line 3: #include <boardid.h> : #include <cbfs.h> : #include <delay.h> : #include <fw_config.h> : #include <gpio.h> : #include <soc/ddp.h> : #include <soc/dsi.h> : #include <soc/gpio_common.h> : #include <soc/mtcmos.h> > remove unused headers Done https://review.coreboot.org/c/coreboot/+/85951/comment/526daad6_d29f8e9c?us… : PS2, Line 39: Panel > PANEL Done File src/mainboard/google/rauru/panel_navi.c: https://review.coreboot.org/c/coreboot/+/85951/comment/8e155e1d_de98d9d6?us… : PS2, Line 3: #include <boardid.h> : #include <cbfs.h> : #include <delay.h> : #include <fw_config.h> : #include <gpio.h> : #include <soc/ddp.h> : #include <soc/dsi.h> : #include <soc/gpio_common.h> : #include <soc/mtcmos.h> > please remove unused headers. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/85951?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iea610c97351beb94a49cc1044701a523b7c85a6e Gerrit-Change-Number: 85951 Gerrit-PatchSet: 4 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 11:02:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

[M] Change in coreboot[main]: soc/mediatek/mt8196: Initialize MCUPM
by Paul Menzel (Code Review) 17 Jan '25

17 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, agogo. Paul Menzel has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85888?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: soc/mediatek/mt8196: Initialize MCUPM ...................................................................... Patch Set 9: (1 comment) File src/soc/mediatek/mt8196/mcupm.c: https://review.coreboot.org/c/coreboot/+/85888/comment/7e2aea9a_a7c515ec?us… : PS6, Line 21: "[EB_SPMC] Polling MCU_PORT_SET_R0_0 timeout, %#x\n", > I think the log is fine as long as MTK thinks the information is enough for them to debug. […] Sorry to re-open again. No, it’s an error message and not a debug message. In either case, I think the misunderstanding is, that you think these log messages are only for MediaTek developers. My view is, that later on also normal users are going to install coreboot on these devices and play with it, and the log messages – especially error message – should be as useful to them as possible. [EB_SPMC] CPUEB_PROTECT_RDY not set after %d us, value %#x\n would be much more useful in my opinion. -- To view, visit https://review.coreboot.org/c/coreboot/+/85888?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I223f245d384f32d54f6170a28b29573638f77296 Gerrit-Change-Number: 85888 Gerrit-PatchSet: 9 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: agogo <agogo.huang(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: agogo <agogo.huang(a)mediatek.corp-partner.google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 11:01:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: agogo <agogo.huang(a)mediatek.corp-partner.google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add eDP driver
by Yu-Ping Wu (Code Review) 17 Jan '25

17 Jan '25

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?us… : 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?us… : PS2, Line 32: struct > const Done https://review.coreboot.org/c/coreboot/+/85949/comment/0d44b788_3f8b0fa5?us… : PS2, Line 34: HFP_MASK > This is wrong. Should be `HSYNC_MASK` here. Done https://review.coreboot.org/c/coreboot/+/85949/comment/a745119f_bdccf223?us… : 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?us… : PS2, Line 40: struct > const Done https://review.coreboot.org/c/coreboot/+/85949/comment/f374e500_ec025e7e?us… : PS2, Line 62: PIC_HSIZE_MASK << SRC_HSIZE > These are all wrong. Done https://review.coreboot.org/c/coreboot/+/85949/comment/3a6ccd95_5787b3ac?us… : PS2, Line 107: 0xffffffff > Use upper case for consistency. Done https://review.coreboot.org/c/coreboot/+/85949/comment/d1d6e336_fabb5ec6?us… : 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?us… : PS2, Line 133: > one space Done https://review.coreboot.org/c/coreboot/+/85949/comment/0f19bb37_923de8b1?us… : PS2, Line 141: ; > ` = {0}` Done https://review.coreboot.org/c/coreboot/+/85949/comment/09bd259e_3db6030f?us… : 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?us… : 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?us… : PS2, Line 160: > remove the blank line Done https://review.coreboot.org/c/coreboot/+/85949/comment/d0c1868d_1632afd1?us… : PS2, Line 187: const struct mtk_dvo dvo_data = { > static Done https://review.coreboot.org/c/coreboot/+/85949/comment/408f9e2f_f41d42dd?us… : PS2, Line 188: ( > remove Done File src/soc/mediatek/mt8196/dp_intf.c: https://review.coreboot.org/c/coreboot/+/85949/comment/a66d76e0_9ba3cce5?us… : 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?us… : PS4, Line 106: 0xFFFFFFF 0xFFFFFFFF https://review.coreboot.org/c/coreboot/+/85949/comment/30c841c0_2ff45a1f?us… : PS4, Line 108: 0xFFFFFFF 0xFFFFFFFF https://review.coreboot.org/c/coreboot/+/85949/comment/2f7aa134_c702671c?us… : 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?us… : PS2, Line 16: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/9a022d26_c37142c9?us… : PS2, Line 39: break; > Also add `DPTX_PATTERN_UNKNOWN` Done https://review.coreboot.org/c/coreboot/+/85949/comment/e553f56a_a4d779c7?us… : PS2, Line 49: 2 > DP_LANSE_ADJUST_SIZE Done https://review.coreboot.org/c/coreboot/+/85949/comment/c3646093_ac6e1f30?us… : 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?us… : 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?us… : PS2, Line 68: 3 > Define a macro Acknowledged https://review.coreboot.org/c/coreboot/+/85949/comment/08b982a6_55502e34?us… : PS2, Line 87: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/9805e8fa_b2f536c6?us… : PS2, Line 108: #%x > `%#x` (Please fix all similar cases in this file) Done https://review.coreboot.org/c/coreboot/+/85949/comment/6e32547b_9f0f1596?us… : PS2, Line 167: BIOS_INFO > Is this an error? Done https://review.coreboot.org/c/coreboot/+/85949/comment/8303ed5a_0094c6b6?us… : PS2, Line 194: = 0x0 > not needed Done https://review.coreboot.org/c/coreboot/+/85949/comment/82dbc56d_e44129cc?us… : PS2, Line 194: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/2bbe7e4a_093785f1?us… : 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?us… : 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?us… : PS2, Line 300: -EIO > Let's use `DPTX_TRANING_FAIL` for consistency. Done https://review.coreboot.org/c/coreboot/+/85949/comment/e2dea9ce_41709785?us… : PS2, Line 312: -EINVAL > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/883f1bc6_85e51dee?us… : PS2, Line 315: } > one blank line below Done https://review.coreboot.org/c/coreboot/+/85949/comment/0d74278d_a1dce10a?us… : PS2, Line 318: ret > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/6d141d73_646801c0?us… : 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?us… : PS2, Line 327: can run to this > reach here Done https://review.coreboot.org/c/coreboot/+/85949/comment/adcec262_2c0d4241?us… : PS2, Line 332: -ETIMEDOUT > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/6e472f68_20cd4fd6?us… : PS2, Line 345: 0 > DPTX_PASS Done https://review.coreboot.org/c/coreboot/+/85949/comment/b40025a0_649eba91?us… : 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?us… : 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?us… : PS2, Line 374: m > Does this mean `MHz`? Done https://review.coreboot.org/c/coreboot/+/85949/comment/197c1897_fceb6d3e?us… : PS2, Line 375: ff > upper case Done https://review.coreboot.org/c/coreboot/+/85949/comment/5e28d88a_c26f0a93?us… : PS2, Line 376: 0x8000 > 0x00008000 Done https://review.coreboot.org/c/coreboot/+/85949/comment/3fbc1e2d_d8dc5781?us… : 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?us… : 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?us… : 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?us… : PS4, Line 92: lanerate linkrate https://review.coreboot.org/c/coreboot/+/85949/comment/404f8ea0_9dcdefe5?us… : PS4, Line 94: lane_count Rename to `lanecount_enhanced_frame`. https://review.coreboot.org/c/coreboot/+/85949/comment/6b648482_84f98773?us… : 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?us… : 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?us… : PS4, Line 385: add 26Mhz `Add 26MHz` https://review.coreboot.org/c/coreboot/+/85949/comment/8809a14a_2f374ef9?us… : 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?us… : PS2, Line 24: 0xff > Upper case for all values Done https://review.coreboot.org/c/coreboot/+/85949/comment/70f582b0_ea91405a?us… : PS2, Line 37: 0x60 > `BIT(6) | BIT(5)` Done https://review.coreboot.org/c/coreboot/+/85949/comment/22b7d613_e047b2f9?us… : PS2, Line 38: 10 << 2 > Use hex Done https://review.coreboot.org/c/coreboot/+/85949/comment/2a1ceed9_bb82209f?us… : 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?us… : PS2, Line 73: BIT(9) | BIT(8) > 0x3 << 8 Done https://review.coreboot.org/c/coreboot/+/85949/comment/4bcee111_ea24316e?us… : 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?us… : PS2, Line 85: if (lane_count >= DPTX_LANE_MAX) { > Change to assert. Done https://review.coreboot.org/c/coreboot/+/85949/comment/7cdbf1aa_9601b908?us… : PS2, Line 89: && i < DPTX_LANE_MAX > Remove. Done https://review.coreboot.org/c/coreboot/+/85949/comment/6b54dd9d_fa75322f?us… : PS2, Line 94: lane_count > i Done https://review.coreboot.org/c/coreboot/+/85949/comment/ae53e3c0_596e33e3?us… : PS2, Line 98: > extra blank line Done https://review.coreboot.org/c/coreboot/+/85949/comment/23265908_d2a56756?us… : 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?us… : 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?us… : PS2, Line 107: i < DPTX_LANE_MAX > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/c4f12a1b_7976348f?us… : PS2, Line 112: lane_num > i Done https://review.coreboot.org/c/coreboot/+/85949/comment/86057521_39902697?us… : PS2, Line 134: /* edp phy init*/ > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/189263e3_ecfe1e8d?us… : 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?us… : PS2, Line 161: break > return Done https://review.coreboot.org/c/coreboot/+/85949/comment/51a3df4f_a285a65b?us… : PS2, Line 205: 0x1fff > upper case Done https://review.coreboot.org/c/coreboot/+/85949/comment/003c8cce_0d2f97ae?us… : PS2, Line 302: 0x0000000f, 0x0000000f > 0xF, 0xF Done https://review.coreboot.org/c/coreboot/+/85949/comment/95a6e92d_70081345?us… : 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?us… : 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?us… : PS2, Line 369: if ((value << 2) <= UINT8_MAX) { > use assert assert not added yet https://review.coreboot.org/c/coreboot/+/85949/comment/349b5452_30312210?us… : 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?us… : PS2, Line 408: Set idle pattern failed, > %s: Unexpected Done https://review.coreboot.org/c/coreboot/+/85949/comment/32f53ea4_3a679100?us… : PS2, Line 408: > `__func__` Done File src/soc/mediatek/mt8196/dptx_hal.c: https://review.coreboot.org/c/coreboot/+/85949/comment/88744085_852aa373?us… : PS4, Line 39: 0xA << 2 0x28 https://review.coreboot.org/c/coreboot/+/85949/comment/bbcd660d_3c2051da?us… : 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?us… : 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?us… : PS4, Line 135: GENMASK(2, 0)); move to previous line https://review.coreboot.org/c/coreboot/+/85949/comment/c4584b3f_bd19c779?us… : 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?us… : PS4, Line 337: (mtk_dp_phy_read Align with `WAIT_AUX_READY_RETRY_TIMES` https://review.coreboot.org/c/coreboot/+/85949/comment/2cf6a6c0_8098806b?us… : PS4, Line 337: 0x146c upper case https://review.coreboot.org/c/coreboot/+/85949/comment/4f2feb50_c1d96da3?us… : PS4, Line 362: pre_emphasis preemphasis https://review.coreboot.org/c/coreboot/+/85949/comment/c1886608_6c727c5b?us… : 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?us… : PS4, Line 363: pre_emphasis preemphasis https://review.coreboot.org/c/coreboot/+/85949/comment/bf84c4a0_3a7ed4f2?us… : PS4, Line 388: , remove `,` https://review.coreboot.org/c/coreboot/+/85949/comment/e37077f3_b6606b90?us… : PS4, Line 389: break return File src/soc/mediatek/mt8196/include/soc/addressmap.h: https://review.coreboot.org/c/coreboot/+/85949/comment/2e5bc6dc_d52b0751?us… : 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?us… : PS4, Line 112: #define HACT_SHIFT 16 align https://review.coreboot.org/c/coreboot/+/85949/comment/3e81f408_b676f811?us… : 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?us… : 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?us… : PS2, Line 6: #include <soc/dptx_common.h> > one blank line below Done https://review.coreboot.org/c/coreboot/+/85949/comment/61729c99_6c2218e1?us… : 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?us… : 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?us… : PS4, Line 26: pre_emphasis preemphasis https://review.coreboot.org/c/coreboot/+/85949/comment/0d8b2436_0b3f0c5f?us… : PS4, Line 31: pre_emphasis preemphasis https://review.coreboot.org/c/coreboot/+/85949/comment/836b1f71_7493e716?us… : 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?us… : 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?us… : 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?us… : PS2, Line 37: u8 > align Done https://review.coreboot.org/c/coreboot/+/85949/comment/f46d333f_20fc2d1c?us… : 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?us… : 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. -- To view, visit https://review.coreboot.org/c/coreboot/+/85949?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iccba53f6c6181ca84624c216f9641a2ae9041671 Gerrit-Change-Number: 85949 Gerrit-PatchSet: 4 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bincai Liu <bincai.liu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 11:00:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add thermal driver
by Zhaoqing Jiu (Code Review) 17 Jan '25

17 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Yu-Ping Wu. Zhaoqing Jiu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86017?usp=email ) Change subject: soc/mediatek/mt8196: Add thermal driver ...................................................................... Patch Set 3: (18 comments) File src/soc/mediatek/mt8196/mt_thermal.c: PS2: > Rename to thermal.c. Done https://review.coreboot.org/c/coreboot/+/86017/comment/90518e79_e1551146?us… : PS2, Line 13: TIMES > RETRY_CNT Done https://review.coreboot.org/c/coreboot/+/86017/comment/0d2504c4_a7a5222c?us… : PS2, Line 18: TIMES > RETRY_CNT […] Done https://review.coreboot.org/c/coreboot/+/86017/comment/d6c39c78_82078f7b?us… : PS2, Line 105: /*============================================================= : * Local variable definition : *============================================================= : */ > Remove. Done https://review.coreboot.org/c/coreboot/+/86017/comment/6a433f9f_a7671fbd?us… : PS2, Line 109: g_ > Drop `g_` for global variables. Done https://review.coreboot.org/c/coreboot/+/86017/comment/713ca1ad_ce10c645?us… : PS2, Line 110: g_count_r[L_TS_LVTS_NUM]; : static uint8_t g_op_cali[LVTS_CONTROLLER_NUM]; > For these 2 global variables `count_r` (what does "r" mean?) and `op_cali` (I guess this is acceptab […] Done https://review.coreboot.org/c/coreboot/+/86017/comment/efb7c163_4b35a227?us… : PS2, Line 132: read32(&tc->regs->lvts_config_0) & DEVICE_ACCESS_START_BIT > `!(read32(&tc->regs->lvts_config_0) & DEVICE_ACCESS_START_BIT)` […] Done https://review.coreboot.org/c/coreboot/+/86017/comment/6507be3e_01dac7ba?us… : PS2, Line 133: write err: > remove Done https://review.coreboot.org/c/coreboot/+/86017/comment/9256e68f_9ea946cc?us… : PS2, Line 133: INFO > ERR Done https://review.coreboot.org/c/coreboot/+/86017/comment/84106d6e_84dd98c9?us… : PS2, Line 167: >= > `>` Done File src/soc/mediatek/mt8196/mt_thermal_sram_init.c: PS2: > Rename to thermal_sram.c. Done https://review.coreboot.org/c/coreboot/+/86017/comment/1fe88cc5_6891afd6?us… : PS2, Line 8: _sram + 0x00014000 > Better to use `_mcufw_reserved + 0x1000`. […] Done https://review.coreboot.org/c/coreboot/+/86017/comment/fac3c865_51a08f63?us… : PS2, Line 16: > const Done https://review.coreboot.org/c/coreboot/+/86017/comment/3b9faa73_99a686cc?us… : PS2, Line 16: 0x27bc86aa > upper case Done https://review.coreboot.org/c/coreboot/+/86017/comment/22f444ba_6381911e?us… : PS2, Line 17: THERMAL_SRAM_BASE > Is this supposed to be `THERMAL_ATC_SRAM_BASE`? We're writing the pattern to the whole thermal SRAM, […] Done https://review.coreboot.org/c/coreboot/+/86017/comment/7f27ebee_061a57ff?us… : PS2, Line 18: 4 > `sizeof(*buff)` Done https://review.coreboot.org/c/coreboot/+/86017/comment/38abaa9e_f416b1f2?us… : PS2, Line 32: int i = 0; : uint32_t pattern = 0xffffffff; : uint32_t *buff = (uint32_t *)THERMAL_STAT_SRAM_BASE; : for (i = 0; i < THERMAL_STAT_SRAM_LEN / 4; i++) { : *buff = pattern; : buff++; : } > I'm fine either way. I was just providing an alternative to the current code. Done https://review.coreboot.org/c/coreboot/+/86017/comment/f9cea59f_1fa3b037?us… : PS2, Line 48: int i = 0; : uint32_t pattern = 0xffffffff; : uint32_t *buff = (uint32_t *)GPU_THERMAL_STAT_SRAM_BASE; : for (i = 0; i < GPU_THERMAL_STAT_SRAM_LEN / 4; i++) { : *buff = pattern; : buff++; : } > > Would it be better to align the same style with thermal_cls_sram() function? […] Just keep it same as thermal_stat_cls_sram() -- To view, visit https://review.coreboot.org/c/coreboot/+/86017?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ieef94a6909e4da82461351bcb9292e9d01db3362 Gerrit-Change-Number: 86017 Gerrit-PatchSet: 3 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: Zhaoqing Jiu <zhaoqing.jiu(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 10:56:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Zhaoqing Jiu <zhaoqing.jiu(a)mediatek.corp-partner.google.com> Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add eDP driver
by Bincai Liu (Code Review) 17 Jan '25

17 Jan '25

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add eDP driver
by Bincai Liu (Code Review) 17 Jan '25

17 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Yu-Ping Wu. Bincai Liu 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: (94 comments) File src/soc/mediatek/mt8196/dp_intf.c: https://review.coreboot.org/c/coreboot/+/85949/comment/933f48d2_84acf25e?us… : 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/832bdbea_643f7955?us… : PS2, Line 18: { > Add `assert((val & mask) == val)`. it seems that we do not need to check whether all bits of val are also set in mask. https://review.coreboot.org/c/coreboot/+/85949/comment/9d500f64_0a6759fb?us… : PS2, Line 32: struct > const done https://review.coreboot.org/c/coreboot/+/85949/comment/53849480_7d922052?us… : PS2, Line 34: HFP_MASK > This is wrong. Should be `HSYNC_MASK` here. done https://review.coreboot.org/c/coreboot/+/85949/comment/9f259d11_ae8c61a8?us… : 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/81e9dbd4_e388d833?us… : PS2, Line 40: struct > const done https://review.coreboot.org/c/coreboot/+/85949/comment/c886f114_c12d7f63?us… : PS2, Line 62: PIC_HSIZE_MASK << SRC_HSIZE > These are all wrong. i fixed it, please help to check it again, thanks a lot https://review.coreboot.org/c/coreboot/+/85949/comment/baa01694_f69f1043?us… : PS2, Line 91: 0x20 > `0x20 << V_LAST_TRAILING_BLANK` Done https://review.coreboot.org/c/coreboot/+/85949/comment/c94af062_2e2dccf1?us… : PS2, Line 99: gs_level > If `gs_level` is not used, please remove the field from the struct. Also see the comment below. Done https://review.coreboot.org/c/coreboot/+/85949/comment/c881381f_f0ac787f?us… : PS2, Line 102: MTK_DVO_8K_30FPS > Should this be `dvo->gs_level`? Why do we hardcode the index here? gs_level is for different kind of resolutions for register DVO_BUF_SODI_HIGHT and DVO_BUF_SODI_LOW https://review.coreboot.org/c/coreboot/+/85949/comment/3321f4f9_04e24b35?us… : PS2, Line 107: 0xffffffff > Use upper case for consistency. Done https://review.coreboot.org/c/coreboot/+/85949/comment/266e33a6_441832c9?us… : PS2, Line 133: > one space Done https://review.coreboot.org/c/coreboot/+/85949/comment/59bb7db0_f11efd0f?us… : PS2, Line 133: eDPTX reg dvo > Can we consistently use the same prefix `[eDPTX]` in this file? Done https://review.coreboot.org/c/coreboot/+/85949/comment/51f8481e_4b11c048?us… : PS2, Line 141: ; > ` = {0}` Done https://review.coreboot.org/c/coreboot/+/85949/comment/e6b5822a_63631c14?us… : 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/983c1cfd_9d988c8f?us… : 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/4d115092_d8bc24fe?us… : PS2, Line 160: > remove the blank line Done https://review.coreboot.org/c/coreboot/+/85949/comment/e31dec74_b1aa16fc?us… : PS2, Line 160: > Remove blank line. […] yes https://review.coreboot.org/c/coreboot/+/85949/comment/2160b253_5dfd7161?us… : PS2, Line 162: 0x8000 > 0x00008000 Done https://review.coreboot.org/c/coreboot/+/85949/comment/2a8bd583_be222ca7?us… : PS2, Line 187: const struct mtk_dvo dvo_data = { > static Done https://review.coreboot.org/c/coreboot/+/85949/comment/3bc2a122_8b44d9c0?us… : PS2, Line 188: ( > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/e991b768_7c0c930f?us… : PS2, Line 198: mtk_dvo_power_on > This is named `mtk_dpintf_power_on` in src/soc/mediatek/common/dp/dp_intf.c. […] the function of mtk_dpintf_power_on and mtk_dvo_power_on is same File src/soc/mediatek/mt8196/dptx.c: https://review.coreboot.org/c/coreboot/+/85949/comment/e0c95c1e_c908841e?us… : PS2, Line 16: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/826c97cd_e6d428c7?us… : PS2, Line 39: break; > Also add `DPTX_PATTERN_UNKNOWN` Done https://review.coreboot.org/c/coreboot/+/85949/comment/24895af3_6ea8fb37?us… : PS2, Line 49: 2 > DP_LANSE_ADJUST_SIZE Done https://review.coreboot.org/c/coreboot/+/85949/comment/f682bcf0_04bd4952?us… : 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/de970064_127e1714?us… : 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/69797971_5bff4c56?us… : PS2, Line 68: 3 > DPTX_PREEMPHASIS3 ? Done https://review.coreboot.org/c/coreboot/+/85949/comment/d29f4b1e_f7d9df64?us… : PS2, Line 68: 3 > Define a macro Done https://review.coreboot.org/c/coreboot/+/85949/comment/efa5bdc7_ec7890cc?us… : PS2, Line 87: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/0a35157a_c10b76dd?us… : 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 […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/24a371fd_f5e1b415?us… : PS2, Line 108: #%x > `%#x` (Please fix all similar cases in this file) Done https://review.coreboot.org/c/coreboot/+/85949/comment/4c057b33_90e48e25?us… : PS2, Line 167: BIOS_INFO > Is this an error? Done https://review.coreboot.org/c/coreboot/+/85949/comment/cccd3108_e8f0883a?us… : PS2, Line 194: = 0x0 > not needed Done https://review.coreboot.org/c/coreboot/+/85949/comment/fc48cbe4_99986e19?us… : PS2, Line 194: = 0x0 > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/c1570b2b_bb41fde3?us… : 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 ( […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/96842ef1_c6a1e0a1?us… : 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/a41439a5_d79b9bcd?us… : PS2, Line 276: case DP_LINKRATE_HBR2: > Does this not support DP_LINKRATE_HBR25? yes https://review.coreboot.org/c/coreboot/+/85949/comment/62ed2db9_65b98dc5?us… : PS2, Line 300: -EIO > Let's use `DPTX_TRANING_FAIL` for consistency. Done https://review.coreboot.org/c/coreboot/+/85949/comment/b0e70f6a_c44b6845?us… : PS2, Line 312: -EINVAL > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/e5cd27b2_04bf8759?us… : PS2, Line 315: } > one blank line below Done https://review.coreboot.org/c/coreboot/+/85949/comment/c25fdd89_44b0f0d5?us… : PS2, Line 318: ret > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/91020bc9_9c399ffc?us… : 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/2281f4a4_f6a1f0ad?us… : PS2, Line 327: can run to this > reach here Done https://review.coreboot.org/c/coreboot/+/85949/comment/f9417c4a_2348754e?us… : PS2, Line 332: -ETIMEDOUT > DPTX_TRANING_FAIL Done https://review.coreboot.org/c/coreboot/+/85949/comment/4a74aba2_209c54ac?us… : PS2, Line 345: 0 > DPTX_PASS Done https://review.coreboot.org/c/coreboot/+/85949/comment/5d23f774_4d788b9b?us… : PS2, Line 348: static void dptx_init_port(struct mtk_dp *mtk_dp) > Feel free to resolve this comment first. […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/32ce3574_bbdcc977?us… : PS2, Line 364: static struct mtk_dp *edp; > If `mtk_edp_enable` can be removed, then this can also be removed. Done https://review.coreboot.org/c/coreboot/+/85949/comment/65db45a2_c14eb3ec?us… : PS2, Line 369: printk(BIOS_INFO, "[eDPTX] begin\n"); > Can we remove the log? yes https://review.coreboot.org/c/coreboot/+/85949/comment/450f6734_baddb6e0?us… : 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. […] this register is to select clock source for edp mac and dvo https://review.coreboot.org/c/coreboot/+/85949/comment/dd102dd0_30294b9d?us… : PS2, Line 374: m > Does this mean `MHz`? yes https://review.coreboot.org/c/coreboot/+/85949/comment/543ff85c_5e9407d5?us… : PS2, Line 375: 0xff000000 > We also write to this reg in `mtk_dvo_set_display_mode`, with a different value `0x80000000`. […] this register is to enable select clock source for eDP https://review.coreboot.org/c/coreboot/+/85949/comment/870e5d9c_be18b54b?us… : PS2, Line 375: ff > upper case Done https://review.coreboot.org/c/coreboot/+/85949/comment/c12fbd46_f6db9d92?us… : PS2, Line 376: 0x8000 > 0x00008000 Done https://review.coreboot.org/c/coreboot/+/85949/comment/8f9372b6_b2557f14?us… : PS2, Line 394: dptx_set_trainingstart(&mtk_edp); > check return value Done File src/soc/mediatek/mt8196/dptx_hal.c: https://review.coreboot.org/c/coreboot/+/85949/comment/56e2572f_c56bdca2?us… : PS2, Line 24: 0xff > Upper case for all values Done https://review.coreboot.org/c/coreboot/+/85949/comment/8955f432_eb6b6bee?us… : PS2, Line 37: 0x60 > `BIT(6) | BIT(5)` Done https://review.coreboot.org/c/coreboot/+/85949/comment/27e41c58_710ffb72?us… : PS2, Line 38: 10 << 2 > Use hex Done https://review.coreboot.org/c/coreboot/+/85949/comment/0c514e7c_4e911186?us… : PS2, Line 52: switch (out_format) { > ``` […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/2ef51dc4_5a621aee?us… : PS2, Line 63: default: > If this is an error, print an error message and return. Done https://review.coreboot.org/c/coreboot/+/85949/comment/da9d98a3_2caccbe4?us… : PS2, Line 73: BIT(9) | BIT(8) > 0x3 << 8 Done https://review.coreboot.org/c/coreboot/+/85949/comment/b5c38d83_522799d4?us… : 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/541215f8_1cfc9dad?us… : PS2, Line 85: if (lane_count >= DPTX_LANE_MAX) { > Change to assert. Done https://review.coreboot.org/c/coreboot/+/85949/comment/00c6431a_fa0ca8a9?us… : PS2, Line 89: && i < DPTX_LANE_MAX > Remove. Done https://review.coreboot.org/c/coreboot/+/85949/comment/24e85981_8114f9c5?us… : PS2, Line 94: lane_count > i Done https://review.coreboot.org/c/coreboot/+/85949/comment/d21b886d_803cfbb7?us… : PS2, Line 98: > extra blank line Done https://review.coreboot.org/c/coreboot/+/85949/comment/071286bd_32de284d?us… : 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/f377c826_b0e9d9e1?us… : 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/bdda736b_2e2db21e?us… : PS2, Line 107: i < DPTX_LANE_MAX > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/35e1ef40_3542a344?us… : PS2, Line 112: lane_num > i Done https://review.coreboot.org/c/coreboot/+/85949/comment/1bbc43c2_06f158ab?us… : PS2, Line 134: /* edp phy init*/ > remove Done https://review.coreboot.org/c/coreboot/+/85949/comment/90f2821d_99eac6a8?us… : 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/e8af58f5_78fa27e6?us… : PS2, Line 146: switch (value) { > ``` […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/7eb831ea_abc7f1f6?us… : PS2, Line 161: break > return Done https://review.coreboot.org/c/coreboot/+/85949/comment/07b3ee0f_215ce07d?us… : PS2, Line 205: 0x1fff > upper case Done https://review.coreboot.org/c/coreboot/+/85949/comment/873077bd_32f9e7cc?us… : PS2, Line 302: 0x0000000f, 0x0000000f > 0xF, 0xF Done https://review.coreboot.org/c/coreboot/+/85949/comment/2f7a8992_eed577a6?us… : 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_s […] registers in mtk_edp_hal_reset_swing_pre_emphasis and dptx_hal_reset_swing_preemphasis are different, i rename dptx_hal_reset_swing_preemphasis as dptx_hal_phy_reset_swing_preemphasis https://review.coreboot.org/c/coreboot/+/85949/comment/e5216cec_7befe2ce?us… : 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/8c8ad768_6e7a62b3?us… : 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/cbf90bf3_ad2c680c?us… : PS2, Line 369: if ((value << 2) <= UINT8_MAX) { > use assert Done https://review.coreboot.org/c/coreboot/+/85949/comment/530f7b7e_f91445dc?us… : 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/669ed096_4d9df34c?us… : PS2, Line 408: Set idle pattern failed, > %s: Unexpected Done https://review.coreboot.org/c/coreboot/+/85949/comment/819c1920_68493bcc?us… : PS2, Line 408: > `__func__` Done File src/soc/mediatek/mt8196/include/soc/addressmap.h: https://review.coreboot.org/c/coreboot/+/85949/comment/2c11d7e6_5020092e?us… : PS2, Line 75: a > A Done File src/soc/mediatek/mt8196/include/soc/dp_intf.h: https://review.coreboot.org/c/coreboot/+/85949/comment/6bdf16c0_260df67c?us… : PS2, Line 199: gs_level > Remove if unused Done https://review.coreboot.org/c/coreboot/+/85949/comment/1f5d4697_63f79b0e?us… : 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/447926d6_fc7626ff?us… : PS2, Line 6: #include <soc/dptx_common.h> > one blank line below Done https://review.coreboot.org/c/coreboot/+/85949/comment/13db37d8_793e54ad?us… : 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/ce0cc5aa_2a1793b9?us… : 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/6dfb24f6_46f1264e?us… : 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/5ad98942_a5b371f8?us… : 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/8d1f0e1a_f1275e76?us… : PS2, Line 37: u8 > align Done https://review.coreboot.org/c/coreboot/+/85949/comment/b5ad12f5_02854ad6?us… : 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_`. […] Done https://review.coreboot.org/c/coreboot/+/85949/comment/4de054c3_1cc156b9?us… : PS2, Line 42: #endif /* __SOC_MEDIATEK_MT8196_DP_DPTX_HAL_H__ */ > Add blank line above Done -- To view, visit https://review.coreboot.org/c/coreboot/+/85949?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iccba53f6c6181ca84624c216f9641a2ae9041671 Gerrit-Change-Number: 85949 Gerrit-PatchSet: 4 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bincai Liu <bincai.liu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Fri, 17 Jan 2025 10:52:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2025