Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32288 )
Change subject: qualcomm: Add QCLib interface support to common/ ......................................................................
Patch Set 2:
(8 comments)
Thanks, I think this is pretty much done... but please clean up all the Jenkins errors as well.
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/include/soc/... File src/soc/qualcomm/common/include/soc/mmu_common.h:
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/include/soc/... PS2, Line 31: // _SOC_QUALCOMM_MMU_COMMON_H_ Please always use /* C89 comments */ in coreboot.
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/include/soc/... File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/include/soc/... PS2, Line 19: #include <fmap.h> nit: I think this is only needed in the .c file?
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@140 PS2, Line 140: 0 nit: technically, this one should be NULL, not 0
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@151 PS2, Line 151: WARNING nit: anything that makes DDR init fail should probably at least be ERROR
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@157 PS2, Line 157: qclib_cb_if_table.global_attributes = nit: let's write this as |= just in case we add new code for other attributes above later
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@162 PS2, Line 162: nit: it's odd to have a tab here, is that intentional?
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@171 PS2, Line 171: qcsdi.entry nit: use prog_entry(&qcsdi) (below as well, and for &qclib)
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@215 PS2, Line 215: QCLIB_BA_SAVE_TO_STORAGE) nit: indent this continuation line one more tab so it's clearer to distinguish from the if-body below