Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25208 )
Change subject: sdm845: Add QCLib to RomStage to perform DDR init ......................................................................
Patch Set 69:
(6 comments)
https://review.coreboot.org/#/c/25208/69/src/mainboard/google/cheza/chromeos... File src/mainboard/google/cheza/chromeos.fmd:
https://review.coreboot.org/#/c/25208/69/src/mainboard/google/cheza/chromeos... PS69, Line 35: RW_NVRAM 16K Where did RW_LIMITS_CFG and RW_DDR_TRAINING go (the spaces we want to preserve just in case an update is needed later)?
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qclib.h:
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/include/soc... PS69, Line 52: #define QCLIB_GA_ENABLE_UART_LOGGING 0x00000001 This used to be 0x2, was it updated in QcLib when you decided to remove SOC_DEBUG_FLOW? (In the future, let's avoid changing these once they have a particular value so the interface is more stable between different versions of coreboot and QcLib. If we deprecate a flag, just keep that in this list as a comment to make clear that it has been used before and shouldn't be reused.)
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/qclib_execu... PS69, Line 1: /* Please note that there are still many comments in CB:27360 that still apply here.
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/qclib_execu... PS69, Line 39: #define PBL_DATA_PTR 0x14810188 This happened, right? So let's remove this.
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/qclib_execu... PS69, Line 164: printk(BIOS_INFO, "Board Rev(0x%d)\n", board_id());
Prefixing 0x with decimal output is defective
This is already printed at the end of ramstage, I don't think it needs to be printed here.
https://review.coreboot.org/#/c/25208/69/src/soc/qualcomm/sdm845/qclib_execu... PS69, Line 240: prog_set_entry(&qclib, qclib.entry, (void *)PBL_DATA_PTR); This is no longer used, right? Instead, please set &qclib_cb_if_table as argument here, and then use prog_run() to run it rather than doing that manually.
-
Julius Werner (Code Review)