Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/#/c/32057/7/src/mainboard/google/kukui/romstage.... File src/mainboard/google/kukui/romstage.c:
https://review.coreboot.org/#/c/32057/7/src/mainboard/google/kukui/romstage.... PS7, Line 32: pmic_set_vsim2_cali(); There should be a short code comment here why this is necessary, e.g.
/* Adjust VSIM2 down to 2.7V because it is shared with IT6505. */
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... PS7, Line 42: void pmic_set_vsim2_cali(void);
Hi Julius, […]
I guess the hardcoding of 0.06 still feels a bit weird? Maybe this should be pmic_adjust_vsim2_cali(int millivolts), and then you can call it with -60 for Kukui. Or maybe pmic_set_vsim2(2700).
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c@74... PS10, Line 745: vsim2_cali_0mv = (vsim2_cali_0mv > 6) ? (vsim2_cali_0mv - 6) : 0; nit: I think writing this as
vsim2_cali_0mv = MIN(vsim2_cali_0mv - 6, 0);
would be more readable.