Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44699 )
Change subject: soc/mediatek/mt8192: Define DRAM registers and APIs ......................................................................
Patch Set 45: Code-Review+2
(7 comments)
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG@8 PS6, Line 8:
mt8192 is a new design, is not copied from mt8183 or mt8173.
Ack
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG@8 PS6, Line 8:
On architecure, mt8192 is different from mt8183 or mt8173, so no common code can use now.
Ack
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 157: u8 chn;
The size of structure member should be 1 or 2 or 4 bytes. […]
Ack
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 158: u8 rank;
As above.
Ack
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 160: u8 density; : u8 *pll_mode; : u32 frequency; : u32 vcore_voltage;
Use native types?
Ack
https://review.coreboot.org/c/coreboot/+/44699/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/44699/1/src/soc/mediatek/mt8192/inc... PS1, Line 17: #define dramc_err(_x_...) printk(BIOS_ERR, _x_) : #define dramc_info(_x_...) printk(BIOS_INFO, _x_) : #if CONFIG(DEBUG_DRAM) : #define dramc_dbg(_x_...) printk(BIOS_INFO, _x_) : #else : #define dramc_dbg(_x_...) : #endif
I am sorry, HAOUAS, "should we use this for coreboot tree " means?
I think it's fine to keep it here.
https://review.coreboot.org/c/coreboot/+/44699/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/dramc_register.h:
https://review.coreboot.org/c/coreboot/+/44699/1/src/soc/mediatek/mt8192/inc... PS1, Line 6: types.h
Yes, just use stdint.h, use types.h is also ok.
Done