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: soc: Add dram information to cbmem ......................................................................
Patch Set 5:
(4 comments)
File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/c/coreboot/+/59195/comment/be73372c_732616b5 PS5, Line 26: static void write_mem_chip_information(struct qclib_cb_if_table_entry *te); static functions don't need a prototype.
https://review.coreboot.org/c/coreboot/+/59195/comment/d67246f4_7f512041 PS5, Line 37: There should be some kind of check here to make sure the data was actually filled out (e.g. mem_chip_size != 0 or something, if that works). It can be an assert() if we make sure that both sc7180 and sc7280 qclib will support this before this patch is merged.
https://review.coreboot.org/c/coreboot/+/59195/comment/98c39176_5e572701 PS5, Line 40: ASSERT(mem_region_base != NULL); nit: can just write
assert(cbmem_add(...) != NULL);
to be shorter.
https://review.coreboot.org/c/coreboot/+/59195/comment/f0632188_827de64c 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 actual backing buffer, not just a pointer.
-
Julius Werner (Code Review)