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/+/85841?usp=email )
Change subject: soc/mediatek/mt8196: Add clk_buf drivers ......................................................................
Patch Set 1:
(33 comments)
File src/soc/mediatek/mt8196/clkbuf_ctl.c:
https://review.coreboot.org/c/coreboot/+/85841/comment/faa5d937_81451b44?usp... : PS1, Line 9: Remove 1 tab.
https://review.coreboot.org/c/coreboot/+/85841/comment/babf3769_3f5b5932?usp... : PS1, Line 15: * space
https://review.coreboot.org/c/coreboot/+/85841/comment/909c5380_f53be9fd?usp... : PS1, Line 17: #define XO_IMPEDANCE_SHIFT 3 Add a blank line below.
https://review.coreboot.org/c/coreboot/+/85841/comment/3bfe5c77_93c73f70?usp... : PS1, Line 20: XO_MODE_SHIFT Remove and use `XO_EN_MODE_SHIFT` defined above. I assume they are the same thing.
https://review.coreboot.org/c/coreboot/+/85841/comment/7ae7ef64_cfbb7eb5?usp... : PS1, Line 22: #define clk_buf_enable_xo(xo_id) \ : mt6685_write_field(ox[(xo_id)].reg, \ : XO_RC_MODE, \ : XO_EN_MODE_MASK, \ : XO_MODE_SHIFT) : : #define clk_buf_disable_xo(xo_id) \ : mt6685_write_field(ox[(xo_id)].reg, \ : XO_DISABLE_MODE, \ : XO_EN_MODE_MASK, \ : XO_MODE_SHIFT) : : #define clk_buf_impedance_config(xo_id) \ : mt6685_write_field(ox[(xo_id)].reg, \ : ox[(xo_id)].impedance, \ : XO_IMPEDANCE_MASK, \ : XO_IMPEDANCE_SHIFT) : : #define clk_buf_driving_config(xo_id) \ : mt6685_write_field(ox[(xo_id)].reg, \ : ox[(xo_id)].driving, \ : XO_DRIVING_MASK, \ : XO_DRIVING_SHIFT) Change to static inline functions.
https://review.coreboot.org/c/coreboot/+/85841/comment/ef303452_f5dd5900?usp... : PS1, Line 48: /* SPMI master/slave, might change in the future */ : #define CLKBUF_SPMI_MASTER SPMI_MASTER_1 : #define CLKBUF_SPMI_SLAVE SPMI_SLAVE_9 Remove if unused.
https://review.coreboot.org/c/coreboot/+/85841/comment/8fa75995_1c8bd167?usp... : PS1, Line 74: struct ox_cfg ox[] = { static const
https://review.coreboot.org/c/coreboot/+/85841/comment/fe0c57b1_0c60d12e?usp... : PS1, Line 148: impedance = 0, : .driving = 0, : .voter_mask = { : .low_byte = 0x0, : .high_byte = 0x0, : }, : For `CLOCK_BUFFER_DISABLE`, fields `impedance`, `driving` and `voter_mask` are NOT used. Let's skip the initialization for them.
https://review.coreboot.org/c/coreboot/+/85841/comment/17787338_b3b88ae9?usp... : PS1, Line 207: unsigned int Should this be u16?
https://review.coreboot.org/c/coreboot/+/85841/comment/9a6b1e1e_ebf0256c?usp... : PS1, Line 209: unsigned int out = 0; : unsigned char tmp = 0; : : tmp = mt6685_read8(MT6685_PMRC_CON0); : out = tmp; : tmp = mt6685_read8(MT6685_PMRC_CON1); : out |= tmp << 8; Use `mt6685_read_low_high`, which I suggested in another comment.
https://review.coreboot.org/c/coreboot/+/85841/comment/f9786ea6_31c3d1d1?usp... : PS1, Line 222: u8 `int`. Please fix all `u8 i` in this file.
https://review.coreboot.org/c/coreboot/+/85841/comment/380ddc02_dae31932?usp... : PS1, Line 228: \n Print the newline at the end of the string (NOT before). You'll need to add a newline for the string in line #224.
https://review.coreboot.org/c/coreboot/+/85841/comment/27ccb7fa_0550a087?usp... : PS1, Line 260: clk_buf_dump_clkbuf_log dump_clkbuf_log
https://review.coreboot.org/c/coreboot/+/85841/comment/c74c9641_4a016e97?usp... : PS1, Line 269: Add
``` _Static_assert(MT6685_XO_BUF_CTL0_L + 2 * (XO_NUMBER - 1) == MT6685_XO_BUF_CTL12_L, "Wrong reg for MT6685_XO_BUF_CTL12_L"); _Static_assert(MT6685_XO_BUF_CTL0_H + 2 * (XO_NUMBER - 1) == MT6685_XO_BUF_CTL12_H, "Wrong reg for MT6685_XO_BUF_CTL12_H"); ```
https://review.coreboot.org/c/coreboot/+/85841/comment/bcfc35de_0034a599?usp... : PS1, Line 271: xo_buf_cw[i] = mt6685_read8(MT6685_DCXO_EXTBUF1_CW0 + i); : val = mt6685_read8(MT6685_XO_BUF_CTL0_L + (2 * i)); : xo_buf_vote[i] = val; : : val = mt6685_read8(MT6685_XO_BUF_CTL0_H + (2 * i)); : xo_buf_vote[i] |= (val << 8); Add a helper function for reading L and H regs.
``` static u16 mt6685_read_low_high(u32 low_reg, u32 high_reg) { u16 data = mt6685_read8(low_reg); data |= mt6685_read8(high_reg) << 8; return data; } ```
https://review.coreboot.org/c/coreboot/+/85841/comment/220c4646_b68b9848?usp... : PS1, Line 283: RG_XO_DIG26M_DIV2_MASK, align
https://review.coreboot.org/c/coreboot/+/85841/comment/743b0a51_120ceb19?usp... : PS1, Line 286: 0x79A Print `%#X` for `MT6685_DCXO_EXTBUF1_CW0`.
https://review.coreboot.org/c/coreboot/+/85841/comment/71ddb3d0_e77f2acf?usp... : PS1, Line 286: __func__ We don't need to print the function name so many times. How about adding `[clkbuf]` prefix for these logs?
https://review.coreboot.org/c/coreboot/+/85841/comment/298c2cd6_2162bbe6?usp... : PS1, Line 291: 0x54C `MT6685_XO_BUF_CTL0_L`
https://review.coreboot.org/c/coreboot/+/85841/comment/c0572e55_fb3fb3fb?usp... : PS1, Line 333: XO_BBCK1_VOTE_L_ADDR Write `MT6685_XO_BUF_CTL0_L` here to be consistent with `clk_buf_dump_clkbuf_log`.
https://review.coreboot.org/c/coreboot/+/85841/comment/795b99d6_577ec7dc?usp... : PS1, Line 324: for (i = 0; i < XO_NUMBER; ++i) { : if (ox[i].mode == CLOCK_BUFFER_DISABLE) { : ox[i].voter_mask.low_byte = 0; : ox[i].voter_mask.high_byte = 0; : } : } : : /* 1.4 Write RC voting table */ : for (i = 0; i < XO_NUMBER; i++) { : mt6685_write8(XO_BBCK1_VOTE_L_ADDR + (2 * i), : ox[i].voter_mask.low_byte); : mt6685_write8(XO_BBCK1_VOTE_H_ADDR + (2 * i), : ox[i].voter_mask.high_byte); : } Combine them:
``` for (i ...) { u8 low, high; if (ox[i].mode == CLOCK_BUFFER_DISABLE) { low = 0; high = 0; } else { low = ox[i].voter_mask.low_byte; high = ox[i].voter_mask.high_byte; } mt6685_write8(..., low); mt6685_write8(..., high); } ```
https://review.coreboot.org/c/coreboot/+/85841/comment/5e6716b8_4560067f?usp... : PS1, Line 339: t T
https://review.coreboot.org/c/coreboot/+/85841/comment/bce1cc5c_ee5c4e72?usp... : PS1, Line 355: t T
https://review.coreboot.org/c/coreboot/+/85841/comment/347104da_69c19cc1?usp... : PS1, Line 380: /* 1.8 RC mode setting Wrong comment format.
https://review.coreboot.org/c/coreboot/+/85841/comment/8b6b5442_ed773adb?usp... : PS1, Line 385: ( remove
https://review.coreboot.org/c/coreboot/+/85841/comment/8dd6a3eb_fb05cde5?usp... : PS1, Line 397: mt6685_write8(XO_STATIC_AUXOUT_SEL_ADDR, sel); Should we use `mt6685_write_field` and specify `XO_STATIC_AUXOUT_SEL_MASK` and `_SHIFT`?
https://review.coreboot.org/c/coreboot/+/85841/comment/3d733d86_cdd70fd3?usp... : PS1, Line 399: tmp = mt6685_read8(XO_STATIC_AUXOUT_L_ADDR); : val = tmp; : : tmp = mt6685_read8(XO_STATIC_AUXOUT_H_ADDR); : val |= tmp << 8; Use `mt6685_read_low_high`.
https://review.coreboot.org/c/coreboot/+/85841/comment/286cc195_5ac5e984?usp... : PS1, Line 418: d D
File src/soc/mediatek/mt8196/include/soc/clkbuf_ctl.h:
https://review.coreboot.org/c/coreboot/+/85841/comment/70f717b6_a5be9570?usp... : PS1, Line 78: * Space. Same for all similar cases below.
https://review.coreboot.org/c/coreboot/+/85841/comment/703bad3f_74ea4c2e?usp... : PS1, Line 453: CLOCK_BUFFER_HW_CONTROL Add `,`
https://review.coreboot.org/c/coreboot/+/85841/comment/1588e3d2_3f7af4c7?usp... : PS1, Line 464: CLK_BUF_OUTPUT_IMPEDANCE_7 Add `,`
https://review.coreboot.org/c/coreboot/+/85841/comment/84220ba8_090fdfeb?usp... : PS1, Line 536: clk_buf_set_default Remove if unused.
https://review.coreboot.org/c/coreboot/+/85841/comment/941d3bc5_ac17e1bb?usp... : PS1, Line 538: pwrap_interface_init Remove if unused.
-
Yu-Ping Wu (Code Review)