Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45398 )
Change subject: soc/mediatek/mt8192: add pmif driver ......................................................................
Patch Set 14:
(16 comments)
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/Ma... File src/soc/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/Ma... PS14, Line 12: bootblock Do we really need to enable PMIF in bootblock? Can we do that in romstage?
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/pmif_sw.h:
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/in... PS14, Line 13: (0x00) : #define SWINF_FSM_REQ (0x02) : #define SWINF_FSM_WFDLE (0x04) : #define SWINF_FSM_WFVLDCLR (0x06) : #define SWINF_INIT_DONE (0x01) no need to add () if this is a single number
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/in... PS14, Line 25: x always quote x as (x) if you're going to apply operations on it.
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.
What if we package swinf structure in reg as something like
struct pmif_swinf { type sta; type something; .. }
... struct mtk_pmif { struct pmif_swinf[0x10]; }
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
static unsigned int pmif_check_swinf(struct pmif *arb, uint32 expected, ... ) { ... while (GET(...) != expected); }
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.
if (read32() & 0x1)
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);
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);
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif_clk.c:
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 91: 50 Please explain why 50 is needed, or add a reference to which chapter this defined in which datasheet.
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 146: PROJECT_CODE, 0xb16); align with the ( above.
e.g. SET32_BITFIELDS(... PROJECT_CODE, ...);
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 163: PMIC_CG_MD align with the ( above
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 166: 0 align with the ( above
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 174: PMIC_WRAP_SWRST align with the ( above
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 176: PMIC_WRAP_SWRST align with the ( above
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 180: PMIC_CG_MD align with the ( above
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif_spi.c:
https://review.coreboot.org/c/coreboot/+/45398/14/src/soc/mediatek/mt8192/pm... PS14, Line 321: 100 add a reference for where we can find this 100 being defined.