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 23:
(16 comments)
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/inc... PS7, Line 134: unsigned int
Why not `uintptr_t`?
the address and value of pmic mt6359p is 32 bit, so I think u32 should be enough.
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/pmif_spmi.h:
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/inc... PS1, Line 89: {
enum {
Done
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/pmi... PS1, Line 115: ((read32(&arb->mtk_pmif->init_done) & 0x1))
Please address this.
Done
https://review.coreboot.org/c/coreboot/+/45398/1/src/soc/mediatek/mt8192/pmi... PS1, Line 157: int ret = 1; : : ret = pmif_clk_init(); : if (ret) : goto FAIL; : : ret = pmif_spmi_init(&pmif_spmi_arb[SPMI_MASTER_0]); : if (ret) : goto FAIL; : : ret = pmif_spi_init(&pmif_spi_arb[0]); : if (ret) : goto FAIL; : : return 0; : : FAIL: : /* assert(0); */ : return ret;
Please address this.
Done
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 22: &arb->mtk_pmif->swinf_0_sta + 0x10 * arb->swinf_no
slightly concerned about this. […]
Done
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 34: unsigned int reg_rdata; : struct stopwatch sw; : : stopwatch_init_usecs_expire(&sw, timeout_us); : do { : reg_rdata = read32(&arb->mtk_pmif->swinf_0_sta + 0x10 * arb->swinf_no); : if (stopwatch_expired(&sw)) { : printk(BIOS_ERR, "[%s] timeout\n", __func__); : return E_TIMEOUT; : } : } while (GET_SWINF_0_FSM(reg_rdata) != SWINF_FSM_WFVLDCLR);
this looks pretty redundant with pmif_check_idle. Can we merge them as a single function […]
Done
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 119: ((read32(&arb->mtk_pmif->init_done) & 0x1))
remove one level of quotes. […]
Done
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 167: (struct pmif *)&pmif_spmi_arb[SPMI_MASTER_0]
get_pmif_controller(PMIF_SPMI, SPMI_MASTER_0);
Done
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 171: (struct pmif *)&pmif_spi_arb[0]
get_pmif_controller(PMIF_SPI);
Done
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 52: }, {
Should { be on the next line?
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 102: u32 i = 0;
The native type should be enough for a counting variable.
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 131: printk(BIOS_ERR, "%s: Null argument", __func__);
Error messages should be more elaborate and helpful.
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 162: u32 swinf_no)
Fits into one line.
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 210: */
Please use one of the recommended coding styles [1]. […]
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 224: 0x2F7
Add enums or macros?
Done
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 251: return -E_NODEV;
Add a debug message?
Done