Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46402 )
Change subject: soc/mediatek/mt8192: devapc: add basic devapc drivers ......................................................................
Patch Set 46:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46402/46/src/soc/mediatek/mt8192/de... File src/soc/mediatek/mt8192/devapc.c:
https://review.coreboot.org/c/coreboot/+/46402/46/src/soc/mediatek/mt8192/de... PS46, Line 6: static unsigned long devapc_ao_base; I think relying on this static variable is not a good idea. Let's change this to:
static uint32_t *getreg(uintptr_t base, unsigned offset) { return (uint32_t *)(base + offset); }
Then you can do the init as:
static void infra_master_init(uintptr_t base) { SET32_BITFLIELDS(getreg(base, MAS_SEC), ...); ... }
That's more clear to read and understand.
https://review.coreboot.org/c/coreboot/+/46402/46/src/soc/mediatek/mt8192/de... PS46, Line 43: devapc_ao_base this can be a local variable.
https://review.coreboot.org/c/coreboot/+/46402/46/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/devapc.h:
https://review.coreboot.org/c/coreboot/+/46402/46/src/soc/mediatek/mt8192/in... PS46, Line 20: #define AO_APC_CON (devapc_ao_base + (unsigned long)OFF_AO_APC_CON) : #define MAS_SEC (devapc_ao_base + (unsigned long)OFF_MAS_SEC_0) : #define MAS_DOM_0 (devapc_ao_base + (unsigned long)OFF_MAS_DOM_0) : #define MAS_DOM_1 (devapc_ao_base + (unsigned long)OFF_MAS_DOM_1) I think this coding style (have define in header that uses some static variable inside c, and didn't use the variable explicitly in c) is confusing. Let's move them to C file.