hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45398 )
Change subject: soc/mediatek/mt8192: add pmif driver ......................................................................
Patch Set 25:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45398/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45398/7//COMMIT_MSG@9 PS7, Line 9: power management interface(PMIF)
Please add a space before the (.
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/spmi.h:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/inc... PS7, Line 46: int slvid;
Can an ID be signed?
Done
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif_clk.c:
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/pmi... PS1, Line 13: #define FREQ_260MHZ 260 : /* calibation miss rate, unit: 0.1% */ : #define CAL_MIS_RATE 40 : /* : * FREQ METER ID : * Ask clkmgr owner to find this information : * at clock table[fmeter]. : */ : #define FREQ_METER_ABIST_AD_OSC_CK 37 : #define CAL_MAX_VAL 0x7f
use enum
move to header file and use enum.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif_spi.c:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 217: __func__, rdata, DEFAULT_VALUE_READ_TEST);
Please use more elaborate error messages.
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 287: if (ret != 0) {
Use CB_SUCCESS and friends.
Would you please give me a hint about CB_SUCCESS and friends? There seems to be no proper definition.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif_spmi.c:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 200: write32(&arb->mtk_pmif->inf_cmd_per_1, cmd_per);
Why does a different value have to be programmed here?
inf_cmd_per_1 is for the permission of interface 8~15 and inf_cmd_per_0 is for the permission of interface 0~7.