Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46393 )
Change subject: soc/mediatek/mt8192: Add dpm loader ......................................................................
Patch Set 33:
(16 comments)
https://review.coreboot.org/c/coreboot/+/46393/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46393/5//COMMIT_MSG@8 PS5, Line 8:
DPM is used for power manager, which is mediatek proprietary, it's binary release. […]
Done
https://review.coreboot.org/c/coreboot/+/46393/5//COMMIT_MSG@10 PS5, Line 10: TEST=Boots correctly on Asurada
Dpm is tested pass with firmware 13457.0. […]
Done
https://review.coreboot.org/c/coreboot/+/46393/28/src/soc/mediatek/mt8192/dp... File src/soc/mediatek/mt8192/dpm.c:
https://review.coreboot.org/c/coreboot/+/46393/28/src/soc/mediatek/mt8192/dp... PS28, Line 25: : dm_file_bytes = cbfs_boot_load_file(dm_file_name, dpm_dm_bin, : sizeof(dpm_dm_bin), CBFS_TYPE_RAW); : if (dm_file_bytes == 0) { : printk(BIOS_ERR, "binary %s not found\n", dm_file_name); : return -1; : } : : pm_file_bytes = cbfs_boot_load_file(pm_file_name, dpm_pm_bin, : sizeof(dpm_pm_bin), CBFS_TYPE_RAW); : if (pm_file_bytes == 0) { : printk(BIOS_ERR, "binary %s not found\n", pm_file_name); : return -2; : }
I will squash CB:46932 into this patch in next patchset.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... File src/soc/mediatek/mt8192/dpm.c:
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 27:
Just one tab, or […]
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 29: binary %s not found
Failed to load %s
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 34: sizeof(dpm_pm_bin), CBFS_TYPE_RAW);
Same.
Please review new API proposed in CB:47895 CB:47896 CB:47897.
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 36: binary %s not found
Failed to load %s
ditto
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 39: %d
Use "%zu" (or "%#zx" if you prefer hex), and no need to cast it to "int".
ditto
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 40: (int)
Please align with "BIOS_INFO".
ditto
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 43: write32(&mtk_dpm->sw_rstn, read32(&mtk_dpm->sw_rstn) | 0x10000000);
Use clrsetbits32.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 43: 0x10000000
Please define a macro for this.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 54: write32(&mtk_dpm->sw_rstn, read32(&mtk_dpm->sw_rstn) | 0x1);
Use clrsetbits32.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/dp... PS32, Line 54: 0x1
Please define a macro for this.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/in... PS32, Line 30: AUXADC_BASE = IO_PHYS + 0x01001000, : DPM_PM_SRAM_BASE = IO_PHYS + 0x00900000, : DPM_DM_SRAM_BASE = IO_PHYS + 0x00920000, : DPM_CFG_BASE = IO_PHYS + 0x00940000,
Please sort these by values.
Done
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/dpm.h:
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/in... PS32, Line 10: 0x8000
Use KiB?
unused, removed.
https://review.coreboot.org/c/coreboot/+/46393/32/src/soc/mediatek/mt8192/in... PS32, Line 44: #define mtk_dpm ((struct dpm_regs *)DPM_CFG_BASE)
Use global variable to be consistent with others.
Done