T Michael Turney 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:
(9 comments)
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 . […]
Ack
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@136 PS2, Line 136: qclib_add_if_table_entry(QCLIB_TE_QCLIB_LOG_BUFFER, _qclib_serial_log,
line over 80 characters
Ack
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
Ack
https://review.coreboot.org/#/c/32288/2/src/soc/qualcomm/common/qclib.c@147 PS2, Line 147: qclib_add_if_table_entry(QCLIB_TE_DDR_TRAINING_DATA, _ddr_training, size, 0);
line over 80 characters
Ack
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
Ack
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
Ack
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?
Ack
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)
Ack
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 belo […]
Ack