Attention is currently required from: Roger Lu, Yuchen Huang. Yu-Ping Wu 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 60:
(33 comments)
File src/soc/mediatek/mt8192/clkbuf.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/069b1447_0e32a819 PS60, Line 16: _BF_MASK I wonder if we should publicize _BF_MASK and _BF_VALUE (by removing the leading underscore). @hungte, what do you think?
https://review.coreboot.org/c/coreboot/+/46878/comment/00dd6a12_a4d3d221 PS60, Line 18: ( No need for parentheses. Same for BUF_WRITE_FIELD.
https://review.coreboot.org/c/coreboot/+/46878/comment/e7d8ff79_818c2ca5 PS60, Line 19: FIELDS Just "FIELD" since there's only one field.
https://review.coreboot.org/c/coreboot/+/46878/comment/715b7165_3bdba029 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.
https://review.coreboot.org/c/coreboot/+/46878/comment/42aa99cc_b7017dd8 PS60, Line 139: 1.update "1. Update". Same for 2.
File src/soc/mediatek/mt8192/include/soc/clkbuf.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/bf1768ae_10f203d9 PS60, Line 32: 0x1D1C Let's align the values using tabs.
File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/52e1b106_5991f642 PS60, Line 174: PMIF_MAX Not used.
File src/soc/mediatek/mt8192/include/soc/srclken_rc.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/cda8df52_977df0ca PS60, Line 123: SRCLKEN_DBG Please move to srclken_rc.c and change to macro.
File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/0a9086c4_31d6bc6e PS60, Line 182: int unsigned int
https://review.coreboot.org/c/coreboot/+/46878/comment/b306e70f_efa7daad PS60, Line 196: /* pmic vld/rdy control spi mode enable : * srclken control spi mode disable : * vreq control spi mode disable : */ format:
/* * pmic vld/rdy control spi mode enable * srclken control spi mode disable * vreq control spi mode disable */
Same below.
https://review.coreboot.org/c/coreboot/+/46878/comment/777dff7f_a126ff1d PS60, Line 251: default This indicates an error. Can we call die()?
https://review.coreboot.org/c/coreboot/+/46878/comment/1593e74b_b65b37fd PS60, Line 283: [%s] PMIF_VLD_RDY How about
%s: Select PMIF_VLD_RDY
https://review.coreboot.org/c/coreboot/+/46878/comment/7c4b98be_afddc1da PS60, Line 283: ERR INFO or DEBUG?
File src/soc/mediatek/mt8192/srclken_rc.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/702d51ad_0b1383c5 PS60, Line 17: ? What does that mean?
https://review.coreboot.org/c/coreboot/+/46878/comment/c733e228_123cef93 PS60, Line 19: Extra space
https://review.coreboot.org/c/coreboot/+/46878/comment/d91d8253_796a8cbc PS60, Line 77: id This field is not used.
https://review.coreboot.org/c/coreboot/+/46878/comment/adeeef1b_315711a1 PS60, Line 96: SUB_CTRL_CON(CHN_SUSPEND, DCXO_STABLE_TIME, XO_STABLE_TIME, SW_BBLPM_LOW, Write
[CHN_SUSPEND] = SUB_CTRL_CON(...).
https://review.coreboot.org/c/coreboot/+/46878/comment/429d444f_298fd6ea PS60, Line 98: SW_BBLPM_LOW, : SW_FPM_HIGH, SW_MODE These are the same for all elements in this array. Could we fix the values in the macro definition of SUB_CTRL_CON?
https://review.coreboot.org/c/coreboot/+/46878/comment/ca274836_5610b83f PS60, Line 124: static struct pmif *pmif_arb; Move inside pmic_read().
https://review.coreboot.org/c/coreboot/+/46878/comment/90aed587_24425912 PS60, Line 149: 0%d Do you mean "%02d"?
https://review.coreboot.org/c/coreboot/+/46878/comment/cb589c98_9b6262f8 PS60, Line 158: if Use switch, and die() for other invalid mode.
https://review.coreboot.org/c/coreboot/+/46878/comment/9a1eb0f1_82c746b2 PS60, Line 180: 0%d Same
https://review.coreboot.org/c/coreboot/+/46878/comment/4192e92d_5f4d6574 PS60, Line 189: else if (mode == SW_FPM_LOW) How about just "else"? Or even simply write
SET32_BITFIELDS(..., !!mode);
https://review.coreboot.org/c/coreboot/+/46878/comment/14967e18_3e5bbe10 PS60, Line 203: else if (mode == SW_BBLPM_LOW) Same.
https://review.coreboot.org/c/coreboot/+/46878/comment/206db3bf_7597d1b7 PS60, Line 242: int Give the enum SRCLKEN_RC_{ENABLE,DISABLE} a name and use it. It's fine to move the definition here.
https://review.coreboot.org/c/coreboot/+/46878/comment/72a675bb_df85a12a PS60, Line 250: /*enable debug trace*/ /* enable debug trace */
https://review.coreboot.org/c/coreboot/+/46878/comment/5a6d3ee2_7388a960 PS60, Line 259: n N
Same for all comments below.
https://review.coreboot.org/c/coreboot/+/46878/comment/897bff72_65594dab PS60, Line 295: Remove extra tab
https://review.coreboot.org/c/coreboot/+/46878/comment/5d32831f_a977479f PS60, Line 384: if Move the if clause out of the while loop.
int retry = 200; while ((read32(...) & 0xa) != chk_sta && retry-- > 0) udelay(10); if (retry == 0) { ... }
https://review.coreboot.org/c/coreboot/+/46878/comment/102de2aa_487c2679 PS60, Line 386: %d fail %02d failed
https://review.coreboot.org/c/coreboot/+/46878/comment/31345493_9c00d482 PS60, Line 387: __func__ __func__ is already contained in rc_info.
https://review.coreboot.org/c/coreboot/+/46878/comment/9bd45853_e79a98bb PS60, Line 400: r R
https://review.coreboot.org/c/coreboot/+/46878/comment/3f814289_1a63c21f PS60, Line 400: * Missing space