Attention is currently required from: Hung-Te Lin, Xi Chen, Paul Menzel, Yu-Ping Wu. Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61334 )
Change subject: soc/mediatek: Pass dram info to cbmem ......................................................................
Patch Set 20:
(8 comments)
File src/soc/mediatek/common/memory.c:
https://review.coreboot.org/c/coreboot/+/61334/comment/27767be5_c19e16cc PS18, Line 107:
only one space
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/0f32c484_45b95104 PS18, Line 106: struct mem_chip *p = &curr_dram_info; : const struct ddr_base_info *blob = curr_ddr_info;
move these to the parameters and pass them from the caller? […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/11144760_2b7fda6a PS18, Line 107: blob
probably not 'blob'. […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/a32eef8d_978d5a19 PS18, Line 110: MEM_CHIP_LPDDR4
MEM_CHIP_LPDDR4X (and remove comment) […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/77f98cc4_9e260e51 PS18, Line 112: unsigned int i = 0
move to beginning of the function and declare only one time
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/4f5a32d6_b45ac8d0 PS18, Line 128: /* Add cbmem */
the cbmem_add is clear enough, no need to have this comment
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/8575e02d_e71b21d0 PS18, Line 130: + sizeof(curr_dram_info.channel) * CHANNEL_MAX)
so we allocated more but won't copy the rest of them? (the memcpy only copies sizeof(curr_dram_info)
Yes, right, missing the channel info.
File src/soc/mediatek/mt8186/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/61334/comment/a2fe2b54_5dc32b16 PS18, Line 74: CPPFLAGS_common += -Isrc/commonlib/bsd/include/commonlib/bsd
Why do we need this? Can't we just include <commonlib/bsd/mem_chip_info.h> in memory. […]
Ack
-
Xixi Chen (Code Review)