Attention is currently required from: Ravi kumar, Shelley Chen, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59195 )
Change subject: WIP soc: Added dram information to cbmem WIP ......................................................................
Patch Set 3:
(5 comments)
File src/soc/qualcomm/common/include/soc/mmu_common.h:
https://review.coreboot.org/c/coreboot/+/59195/comment/145a6170_c6c21fff PS3, Line 13: static struct region * const ddr_region = (struct region *)_ddr_information; We don't need ddr_region anymore because mem_chip_info contains the same information (and more). Let's get rid of the ddr_region code and replace it with this while we are adding this. (Please also give us an SC7180 QcLib that supports this then, so we don't break that.)
https://review.coreboot.org/c/coreboot/+/59195/comment/be3514ea_af240d9d PS3, Line 14: struct region This isn't a struct region, this is the mem_chip_info struct (which you should define somewhere in some generic header, e.g. <mem_chip_info.h>).
File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/c/coreboot/+/59195/comment/cb274e8d_ebc11d9b PS3, Line 25: /* memchip info */ This comment adds absolutely nothing useful to this code.
File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/c/coreboot/+/59195/comment/d0e53a4c_435dafa5 PS3, Line 27: #if TEST_CODE /* Added this structure for testing purpose, will be remove once changes are reviewd */ Please clean this stuff up so we can properly read the file.
File src/soc/qualcomm/sc7280/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/59195/comment/8bb1b4b7_490f4e7e PS3, Line 51: REGION(mem_chip_info, 0x1494E000, 1K, 1K) /* memchip info */ I don't really think we need to put this in a separate SRAM region anymore. You can just have the structure allocated as a global variable to pass to QcLib (it's not that big), and then copy it straight into CBMEM afterwards (you can use the platform_romstage_postram() hook for that).