Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Yu-Ping Wu.
ot_song fan 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 3:
(36 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/85841/comment/a1281a8a_bb8a15d4?usp... : PS1, Line 9: Mt8196 use mt6685 clk_buf, and will use new RC mode with srclken_rc.
Please add more details.
Done
File src/soc/mediatek/mt8196/clkbuf_ctl.c:
https://review.coreboot.org/c/coreboot/+/85841/comment/db7e6724_7770c9ed?usp... : PS1, Line 9:
Remove 1 tab.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/6940bbd6_b1c0f4a5?usp... : PS1, Line 15: *
space
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/4ddcb85b_278ebb85?usp... : PS1, Line 17: #define XO_IMPEDANCE_SHIFT 3
Add a blank line below.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/fc2d117e_9facd531?usp... : PS1, Line 20: XO_MODE_SHIFT
Remove and use `XO_EN_MODE_SHIFT` defined above. I assume they are the same thing.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/d08c099d_84904790?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.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/0682eb17_c3af0fc3?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.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/241af7c3_c5783d81?usp... : PS1, Line 74: struct ox_cfg ox[] = {
static const
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/1fe32b17_fb4d9c07?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. […]
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/4c2c870d_dc73cc58?usp... : PS1, Line 207: unsigned int
Should this be u16?
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/f7a1526b_68a800af?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.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/e1ac267f_87ed6d10?usp... : PS1, Line 222: u8
`int`. Please fix all `u8 i` in this file.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/c91c011e_fdab8ba0?usp... : PS1, Line 228: \n
Print the newline at the end of the string (NOT before). […]
This newline is for last loop of #226.
https://review.coreboot.org/c/coreboot/+/85841/comment/d2252b30_218f82ee?usp... : PS1, Line 238:
remove one blank line
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/ace2d030_bca9eee7?usp... : PS1, Line 260: clk_buf_dump_clkbuf_log
dump_clkbuf_log
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/ad294047_1bfbd095?usp... : PS1, Line 269:
Add […]
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/d7dc9a2f_46e93a33?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. […]
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/f3f7bebe_7aa954fc?usp... : PS1, Line 283: RG_XO_DIG26M_DIV2_MASK,
align
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/8bc48d34_584e313a?usp... : PS1, Line 286: __func__
We don't need to print the function name so many times. […]
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/517c48bf_58dccd56?usp... : PS1, Line 286: 0x79A
Print `%#X` for `MT6685_DCXO_EXTBUF1_CW0`.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/5f23acd0_8244e192?usp... : PS1, Line 291: 0x54C
`MT6685_XO_BUF_CTL0_L`
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/0c174da1_a91442b0?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`.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/ce89e5ea_6f377276?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: […]
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/3f78227e_b12ed956?usp... : PS1, Line 339: t
T
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/a60ee25e_04cf330c?usp... : PS1, Line 341: 0x1
align with first param
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/5dcd5e28_c82419ef?usp... : PS1, Line 355: t
T
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/cc2bb543_77792f18?usp... : PS1, Line 380: /* 1.8 RC mode setting
Wrong comment format.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/85a8f0be_b4be3de6?usp... : PS1, Line 385: (
remove
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/56b3ff18_65b2b281?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`?
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/b9b7bd5a_c1589fe1?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`.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/3febb9c3_6856a0db?usp... : PS1, Line 418: d
D
Done
File src/soc/mediatek/mt8196/include/soc/clkbuf_ctl.h:
https://review.coreboot.org/c/coreboot/+/85841/comment/54e95fca_5913f240?usp... : PS1, Line 78: *
Space. Same for all similar cases below.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/e758e4e0_921680bf?usp... : PS1, Line 453: CLOCK_BUFFER_HW_CONTROL
Add `,`
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/dc218bf5_540dda16?usp... : PS1, Line 464: CLK_BUF_OUTPUT_IMPEDANCE_7
Add `,`
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/ed971e88_434c8198?usp... : PS1, Line 536: clk_buf_set_default
Remove if unused.
Done
https://review.coreboot.org/c/coreboot/+/85841/comment/f5e29d75_3c3f138a?usp... : PS1, Line 538: pwrap_interface_init
Remove if unused.
Done