Attention is currently required from: Ravi kumar, Shelley Chen, mturney mturney, Julius Werner. mturney mturney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59195 )
Change subject: soc: Add dram information to cbmem ......................................................................
Patch Set 6:
(1 comment)
File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/c/coreboot/+/59195/comment/825f60ff_9bb107ac PS5, Line 174: mem_chip_info
This pointer is still NULL here, how is this supposed to work? The global variable needs to be an ac […]
Julius, This is correct as we have written it, though it may not be what you are expecting.
The call to qclib_add_if_table_entry() is required to add the MEM_CHIP_INFO table entry, which is important so QcLib can actually fill in the address/size and mark the entry as dirty so it gets written back after the QcLib call.
Note that what is important here is the table entry, not the data in the table entry. Each table entry contains an address and a size, as well as the flag field that indicates the buffer is dirty and needs to be saved. If CB was passing data to QcLib, you are correct that this would be a problem as written. The buffer used is allocated by QcLib in a region that is visible to CB (SMEM) and the DDR code performing the initialization. A buffer allocated by CB and passed to QcLib may not be accessible by the DDR code. In this case QcLib does not use the data passed in for this entry and uses the table entry to provide valid data back to CB.
If you look at the write-back function I think you will see it is using the address/size passed back from QcLib to allocate the CBMEM buffer and then copy actual data to the CBMEM buffer.
-
mturney mturney (Code Review)