Attention is currently required from: Xi Chen, Nico Huber, Martin Roth, Paul Menzel, Julius Werner, Yu-Ping Wu. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50294 )
Change subject: vendor: mediatek: Add mediatek mt8192 dram initialization codes ......................................................................
Patch Set 16:
(9 comments)
File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/50294/comment/5a9056c4_5da72340 PS15, Line 617: MT8192 Will you work on DRAM for more platforms in future? If yes, I wonder if you'd like to add a section to the "Platforms" above, say
MEDKATEK DRAM CALIBRATION M:.. S:... F: src/vendor/mediatek
https://review.coreboot.org/c/coreboot/+/50294/comment/a159eeb8_19bf77d3 PS15, Line 618: Xi Chen xixi.chen@mediatek.com If you'll work on non-dram topics as well, feel free to move your self to "MEDIATEK SOCS"
https://review.coreboot.org/c/coreboot/+/50294/comment/4dcafa51_4c92076a PS15, Line 620: / if this is for 8192 then the path should be mediatek/mt8192/
https://review.coreboot.org/c/coreboot/+/50294/comment/1f48d569_64e81555 PS15, Line 621: 2 mt8192/
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/862de68f_d6e027a8 PS16, Line 1: SOC_MEDIATEK_MT8192 can we move this to mt8192/Kconfig?
https://review.coreboot.org/c/coreboot/+/50294/comment/7ca7e6c8_5bf6c2b0 PS16, Line 3: config DEBUG_DRAM : bool "Output verbose DRAM related debug messages" : default y : help : This option enables additional DRAM related debug messages. this is the only config in dramc.
I wonder if we can revise it to something like
extern int mtk_dramc_debug; #define dramc_dbg(_x_...) { if (mtk_dramc_debug) printk(BIOS_INFO, _x_); }
And in soc memory.c add
int mtk_drac_debug = CONFIG(DEBUG_DRAM);
Then we don't need KCONFIG in vendor code.
https://review.coreboot.org/c/coreboot/+/50294/comment/733df007_e22101ba PS16, Line 9: config MEDIATEK_DRAM_DVFS : bool : default n : help : This option enables DRAM calibration with multiple frequencies (low, : medium and high frequency groups, with total 7 frequencies) for DVFS : feature. All supported data rates are: 800, 1200, 1600, 1866, 2400, : 3200, 4266. : : config MEDIATEK_DRAM_DVFS_LIMIT_FREQ_CNT : bool : default y : select MEDIATEK_DRAM_DVFS : help : This options limit DRAM frequency calibration count from total 7 to 3, : other frequency will directly use the low frequency shu result. : : config MEMORY_TEST : bool : default y : help : This option enables memory basic compare test to verify the DRAM read : or write is as expected. these options are not really used in the dramc implementation here. I think you can remove them? especially MEMORY_TEST is in src/soc/mediatek/mt8192/memory.c. You can consider moving that to src/soc/mediakte/common or mt8192.
File src/vendorcode/mediatek/mt8192/dramc/ANA_init_config.c:
PS13:
Even vendorcode files always need to have an SPDX header
Hi Julius, we found that current submit hooks didn't check SPDX for files in vendor folder, was that on purpose?
File src/vendorcode/mediatek/mt8192/include/print.h:
https://review.coreboot.org/c/coreboot/+/50294/comment/ce59c436_c3afe4ab PS16, Line 7: #define printf print What about just add
#define print(fmt...) printk(BIOS_INFO, fmt)
Then we don't need print.c ?