T Michael Turney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27360 )
Change subject: sdm845: Add interface between CB & QCLib ......................................................................
Patch Set 35:
(38 comments)
I believe all comments from this patch (patch no longer in train) are addressed by current patches, when next patch-train is pushed, should be 19.April.2019.
https://review.coreboot.org/#/c/27360/26/src/mainboard/google/cheza/chromeos... File src/mainboard/google/cheza/chromeos.fmd:
https://review.coreboot.org/#/c/27360/26/src/mainboard/google/cheza/chromeos... PS26, Line 36: RW_LIMITS_CFG 4K
There aren't any security concerns. […]
Done
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qclib.h:
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/include/soc... PS30, Line 74: qclib_cb_if_table_header
This is the whole table, not just the header, so just call it qclib_cb_if_table.
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qclib.h:
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 32: #if IS_ENABLED(CONFIG_QC_SDI_ENABLE)
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 41: te_index_ddr_information
ACK, removing on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 70: #define MAX_NUMBER_OF_ENTRIES 16
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 72: #define QCLIB_MAGIC_NUMBER 0x51434C49425F4342 /* QCLIB_CB */
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 92: fmap_region
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/include/soc... PS26, Line 97: fmap_name
Actually, this is the FMAP region name, will create new LENGTH #define rather than overload TE_NAME_ […]
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 46: .te = {
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 69: struct fmap_region fmap_region_table[fmap_region_max] = {
Will add the two fmap_ APIs here, on next submission as trial, when they look good, they can be move […]
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 94: "RW_DDR_TRAINING");
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 139: if (fr->size != sram_size) {
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 199: RamStage
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 200: ddr_mmu = (struct ddr_mmu_information *)_ddr_information;
Removed mmu from names. […]
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 203: ddr_mmu->size = ddr_size * MiB;
This comes out of QCLib interface, either it gets changed here or in QCLib. […]
QCLib is still passing back block size and this multiply is still occurring, please confirm you are OK with this.
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 215: u8
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 266: #if IS_ENABLED(CONFIG_QC_SDI_ENABLE)
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 293: GA_BMASK_ENABLE_UART_LOGGING;
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 303: size
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 307: 1
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 318: PBL_DATA_PTR
Currently working on this, legacy QCLib code needs to be reworked to remove this dependency.
Done
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 351:
ACK on next submission
Done
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 200: static void write_qclib_serial_log(int te_index)
ACK
Done Protection is via table entry only being added with if (CONFIG_CONSOLE_CBMEM).
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 212: int te_index
Ack
Done
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 295: e.g. console log copy
Ack
Done If enabled, the console buffer is first table entry added so should be first checked on return and copied to output.
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 319: init_and_read_fmap_region(QCLIB_FR_DDR_TRAINING);
Ack
Done
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 376: printk(BIOS_DEBUG, "QCLib completed\n\n\n");
Ack
Done
https://review.coreboot.org/#/c/27360/30/src/soc/qualcomm/sdm845/qclib_execu... PS30, Line 386: dump_te_table();
Ack
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 61: /* after validation, move to src/lib/fmap.c */
Yes), please move these to src/lib/fmap. […]
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 68: return rdev_readat(&rdev, buffer, 0, MIN(size, region_device_sz(&rdev)));
line over 80 characters
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 78: rdev_eraseat(&rdev, 0, region_device_sz(&rdev));
This can also return an error value that needs to be checked.
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 79: size
This should imply a (size <= region_device_sz(&rdev)) check, but I think it would still be better to […]
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 102: cbmemc_tx_byte
I think this may still not compile if CBMEM_CONSOLE is not enabled, because even though we know it c […]
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 107: printk(BIOS_INFO, "[%s]:[%s]\n", __func__, te->name);
I don't think we need this anymore? (I'm trying to get rid of all prints that will be out-of-order i […]
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 124: te->size
Just to clarify, QcLib will change this size value to contain the actual size of the data structure […]
Done If QCLib sets "dirty" bit the size should reflect correct amount of data to write-back. If it doesn't this is bug in QCLib.
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 133: printk(BIOS_INFO, " not implemented\n");
...and when we take the string above out, this should be changed to print the names. […]
Done
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 223: dump_te_table();
Do we still need this?
Need it, probably not, I find it good diagnostic data to have in the log output. Please confirm you want it removed.
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 262: printk(BIOS_DEBUG, "QCLib completed\n\n\n");
Okay, if we can't put this in write_qclib_log_to_cbmemc() (and I agree that would be bad for the cas […]
Done
-
T Michael Turney (Code Review)