Attention is currently required from: Hung-Te Lin, Rex-BC Chen. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52668 )
Change subject: soc/mediatek/mt8195: add pmif/spmi/pmic driver ......................................................................
Patch Set 3:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52668/comment/45eb7bc1_f718824f PS3, Line 2: henryc.chen If you used *Henry Chen*, that’d be great:
$ git config --global user.name "Henry Chen" $ git commit --amend --author="Henry Chen henryc.chen@mediatek.com"
https://review.coreboot.org/c/coreboot/+/52668/comment/77cbefb2_f0fa8240 PS3, Line 9: MT8195 What is the difference to `mediatek/mt8192`? The code looks similar.
https://review.coreboot.org/c/coreboot/+/52668/comment/3d1441de_984242d7 PS3, Line 10: and spi, so we add pmif driver to control pmics. This change seems to include firmware (encoded in header files). That should be mentioned here, and where it’s from.
https://review.coreboot.org/c/coreboot/+/52668/comment/b9891ec4_1e445c01 PS3, Line 11: 1. Please document the datasheet name and revision. 2. Is the code copied from somewhere or written from scratch?
https://review.coreboot.org/c/coreboot/+/52668/comment/118d7836_e3c8e393 PS3, Line 12: henryc.chen Dito.
File src/soc/mediatek/mt8195/mt6315.c:
https://review.coreboot.org/c/coreboot/+/52668/comment/ac739684_98b75eb4 PS3, Line 6: /* disable magic key protection */ What does that mean? Please reference the datasheet name and section.
File src/soc/mediatek/mt8195/pmif_clk.c:
PS3: This looks very similar to `src/soc/mediatek/mt8192/pmif_clk.c`. Please do not duplicate code.
https://review.coreboot.org/c/coreboot/+/52668/comment/faef42c2_a41b3f17 PS3, Line 108: if (diff_by_min < diff_by_max) { : cal_result = min; : current_val = pmif_get_ulposc_freq_mhz(min); : } else { : cal_result = max; : current_val = pmif_get_ulposc_freq_mhz(max); : } Maybe:
cal_result = (diff_by_min < diff_by_max) ? min : max; current_val = pmif_get_ulposc_freq_mhz(cal_result);
https://review.coreboot.org/c/coreboot/+/52668/comment/e4b21816_dad0e7a2 PS3, Line 119: printk(BIOS_ERR, "[%s] calibration fail: %dM\n", __func__, current_val); Please also print out `CAL_TOL_RATE` and `target_val`.