Attention is currently required from: Hung-Te Lin, Nico Huber, Martin Roth, Paul Menzel, Julius Werner, Angel Pons, Yu-Ping Wu. Xi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50294 )
Change subject: vendor: mediatek: Add mediatek mt8192 dram initialization code ......................................................................
Patch Set 18:
(13 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50294/comment/78400e5b_4be651fc PS16, Line 7: codes
code
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/1e33d53a_6ac8c0a0 PS16, Line 12: an
a
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/17ce104f_deee0fb8 PS16, Line 12: took
"taken" or "considered"
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/f1d8fb21_9134963b PS16, Line 14: (
One space before "("
Done
File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/50294/comment/ca4b84e7_5277d3e1 PS15, Line 617: MT8192
Will you work on DRAM for more platforms in future? […]
Currently, only mt8192.
https://review.coreboot.org/c/coreboot/+/50294/comment/9f951cc4_540d1a44 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"
Currently, mainly on DRAM, in MT8192 section now.
https://review.coreboot.org/c/coreboot/+/50294/comment/a0fc3b44_d91b4a48 PS15, Line 620: /
if this is for 8192 then the path should be mediatek/mt8192/
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/b3ae454b_9eed87e2 PS15, Line 621: 2
mt8192/
Done
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/659d7440_254aaadb PS16, Line 1: SOC_MEDIATEK_MT8192
can we move this to mt8192/Kconfig?
move the file to soc/mediatek/common.
https://review.coreboot.org/c/coreboot/+/50294/comment/6b23dbe9_ae6237ed PS16, Line 3: config DEBUG_DRAM : bool "Output verbose DRAM related debug messages" : default y : help : This option enables additional DRAM related debug messages.
What Angel suggested sounds good. […]
Thanks for all your advice. Reuse DEBUG_RAM_SETUP is very good. Add dramc_dbg in mediatek common file: soc/mediatek/common/.../dramc_common.h CB:51125
#define dramc_dbg(_x_...) do { \ if (CONFIG(DEBUG_DRAM)) \ printk(BIOS_INFO, _x_); \ } while (0)
Another reason we use dramc_xxx macro is that we want a simple log level macro: eg: #define err(_x_...) printk(BIOS_ERR, _x_) #define info(_x_...) printk(BIOS_INFO, _x_)
Do these macros exist in common log header file?
https://review.coreboot.org/c/coreboot/+/50294/comment/d59c5780_0c93fca8 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. […]
Move them to soc/mediatek/common.
File src/vendorcode/mediatek/mt8192/dramc/ANA_init_config.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/024ea23a_6336d079 PS16, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
I also prefer BSD3. @xixi please check with your internal teams and move to BSD3 if possible.
use GPL-2.0.
File src/vendorcode/mediatek/mt8192/include/print.h:
https://review.coreboot.org/c/coreboot/+/50294/comment/64ed3d47_43934ba0 PS16, Line 7: #define printf print
What about just add […]
Done