Attention is currently required from: Hung-Te Lin, Joel Kitching, Roger Lu, Yu-Ping Wu, Yidi Lin, Yuchen Huang. Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46878 )
Change subject: soc/mediatek/mt8192: add clkbuf and srclken_rc MT6359P driver ......................................................................
Patch Set 63:
(35 comments)
File src/soc/mediatek/mt8192/Kconfig:
https://review.coreboot.org/c/coreboot/+/46878/comment/370eda4e_0dd60128 PS60, Line 89: srclken_rc
can you also add what does that stand for? for example […]
Done
File src/soc/mediatek/mt8192/clkbuf.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/bff45baa_bf890758 PS60, Line 16: _BF_MASK
Sounds good to me.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/2feccab1_b3afb983 PS60, Line 18: (
No need for parentheses. Same for BUF_WRITE_FIELD.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/2ddfa810_cae69c56 PS60, Line 19: FIELDS
Just "FIELD" since there's only one field.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/67a1fff4_0fc3cc44 PS60, Line 62: clk_buf_dump_clkbuf_log
Just "dump_clkbuf_log"? No need to have "clk_buf" prefix since it's a static function.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/55aaaeb0_93ce1394 PS60, Line 98: /* 1.0 XO_WCN/XO_RF switch from VS1 to LDO VRFCK_1 */ : /* unlock pmic key */
do we still have some 1.0 switching from vs1 to ldo vrfck_1, or the new 1. […]
"1.0 XO_WCN/XO_RF switch from VS1 to LDO VRFCK_1" this is for XO_WCN/XO_RF setting, which we don't have. Just remove it.
https://review.coreboot.org/c/coreboot/+/46878/comment/9e851f14_56efdbf8 PS60, Line 105: S
be consistent on upper case or lower case - we can do either […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/90cca20d_057c22d7 PS60, Line 139: 1.update
"1. Update". Same for 2.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/39a10797_b45105c0 PS60, Line 140: /* : * XO_SOC_VOTE=11'h005 : */
remove this since the code is now very clear.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/3bfe6aec_6be592bc PS60, Line 152: /* : * XO_PMIC_TOP_DIG_SW=0 : */
remove this since the code is now very clear.
Done
File src/soc/mediatek/mt8192/include/soc/clkbuf.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/7d1ab472_f6713c58 PS60, Line 32: 0x1D1C
Let's align the values using tabs.
Done
File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/def3880d_e1da4130 PS60, Line 174: PMIF_MAX
Not used.
Done
File src/soc/mediatek/mt8192/include/soc/srclken_rc.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/45c5b7fc_4d333148 PS60, Line 123: SRCLKEN_DBG
Please move to srclken_rc.c and change to macro.
Done
File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/924cb3fe_28b13df6 PS60, Line 182: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/d29367fa_2a2709ef PS60, Line 196: /* pmic vld/rdy control spi mode enable : * srclken control spi mode disable : * vreq control spi mode disable : */
format: […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/a432936f_2241a0a7 PS60, Line 251: default
This indicates an error. […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/9f567488_db0cb3b0 PS60, Line 283: [%s] PMIF_VLD_RDY
How about […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/d9e5de2b_5a7b077e PS60, Line 283: ERR
INFO or DEBUG?
INFO
File src/soc/mediatek/mt8192/srclken_rc.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/33a52ac8_b6b38a8d PS60, Line 17: ?
What does that mean?
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/fb023adc_96b0fd74 PS60, Line 19:
Extra space
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/7caf25e1_600fda1a PS60, Line 77: id
This field is not used.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/be56c6a8_186f3400 PS60, Line 96: SUB_CTRL_CON(CHN_SUSPEND, DCXO_STABLE_TIME, XO_STABLE_TIME, SW_BBLPM_LOW,
Write […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/b9af8e87_95834f36 PS60, Line 98: SW_BBLPM_LOW, : SW_FPM_HIGH, SW_MODE
These are the same for all elements in this array. […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/715f1a63_152cf12a PS60, Line 124: static struct pmif *pmif_arb;
Move inside pmic_read().
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/4b242426_79814224 PS60, Line 149: 0%d
Do you mean "%02d"?
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/f81e693c_7c9e7a57 PS60, Line 158: if
Use switch, and die() for other invalid mode.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/97b0aa03_eb2131cf PS60, Line 180: 0%d
Same
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/ee7dd25c_b8efe994 PS60, Line 242: int
Give the enum SRCLKEN_RC_{ENABLE,DISABLE} a name and use it. It's fine to move the definition here.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/ed0b5399_94ed93f3 PS60, Line 250: /*enable debug trace*/
/* enable debug trace */
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/ec865df8_ce025556 PS60, Line 259: n
N […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/26c32a79_973f15c9 PS60, Line 295:
Remove extra tab
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/c8a9d93a_27233c48 PS60, Line 384: if
Move the if clause out of the while loop. […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/843b10bb_3ae3385b PS60, Line 386: %d fail
%02d failed
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/19b61719_41236366 PS60, Line 387: __func__
__func__ is already contained in rc_info.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/2593542a_c3534e27 PS60, Line 400: r
R
Done