T Michael Turney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25208 )
Change subject: sdm845: Add QCLib to RomStage to perform IP init ......................................................................
Patch Set 73:
(17 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 updat […]
Ack
https://review.coreboot.org/#/c/25208/73/src/mainboard/google/cheza/chromeos... File src/mainboard/google/cheza/chromeos.fmd:
https://review.coreboot.org/#/c/25208/73/src/mainboard/google/cheza/chromeos... PS73, Line 28: RO_PRESERVE(PRESERVE) {
BTW we can remove RO_PRESERVE now and just write this as […]
Ack
https://review.coreboot.org/#/c/25208/73/src/mainboard/google/cheza/chromeos... PS73, Line 38: RW_LIMITS_CFG 4K
I think these two (RW_DDR_TRAINING and RW_LIMITS_CFG) should also be marked (PRESERVE).
Ack
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/include/soc... File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/include/soc... PS72, Line 79: void soc_blob_load(void);
Please prefix all external functions with qclib_ or something like that.
Ack
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c@135 PS72, Line 135: printk(BIOS_INFO, "Board Rev(0x%x)\n", board_id());
This is already printed in ramstage once, do we need it here?
Done
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c@209 PS72, Line 209: ret_code = doit(&qclib_cb_if_table, NULL);
Now that we only have one argument, can we switch to using prog_run() for this?
Done
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/Makefile.in... File src/soc/qualcomm/sdm845/Makefile.inc:
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/Makefile.in... PS73, Line 51: ifeq ($(CONFIG_QC_SDI_ENABLE),y)
Can we factor all this stuff out into a separate commit so we can check the code in while we're (sti […]
If we pull this out, the build-bot will not be happy and give a -1, at least it has in the past.
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/memlayout.ld:
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/include/soc... PS73, Line 36: REGION(fw_reserved2, 0x14800000, 0x16000, 0x1000)
Wasn't this reserved for the PBL shared data? If we're no longer using that, does it still need to b […]
QCLib still uses the PBL shared data, CB is no longer responsible for passing it in.
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/include/soc... PS73, Line 45: 0x100
The align value just means the minimum alignment that this section needs to have, not what alignment […]
I will play with the alignments at a later date, trying to address a number of more important items now.
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/include/soc... PS73, Line 46: 0x1000
Maybe write this as 4K so it can be more easily compared with the corresponding FMAP entry? (others […]
Ack
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/include/soc... PS73, Line 58: 160K
nit: This doesn't really have to do with our project, but for some other use cases people are starti […]
Ack
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 futu […]
OK, we will keep deprecated in list moving forward. Probably figured it didn't matter since it isn't upstream yet.
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/qclib.c File src/soc/qualcomm/sdm845/qclib.c:
https://review.coreboot.org/#/c/25208/73/src/soc/qualcomm/sdm845/qclib.c@47 PS73, Line 47: qclib_add_if_table_entry(QCLIB_TE_LIMITS_CFG_DATA, _limits_cfg, size, 0);
Please fix.
Ack
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.
I think I addressed all of the earlier comments when I created the common/ patch. I will double-check.
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.
Ack
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());
This is already printed at the end of ramstage, I don't think it needs to be printed here.
Ack
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 […]
Ack