Julius Werner 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:
(8 comments)
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
RO_VPD(PRESERVE) 16K RO_DDR_TRAINING(PRESERVE) 8K RO_LIMITS_CFG(PRESERVE) 4K RO_FSG(PRESERVE)
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).
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 (still :( ) waiting on legal for the blobs?
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 be reserved?
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 it currently has, so most of these can probably be much lower (e.g. this one probably 8).
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 as well... for those things that have a specific length for a reason, I think decimals are usually easier to read)
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 starting to need larger heap allocations, so it's nice to just allow 2MB or so for the ramstage if we have the space (cf. https://review.coreboot.org/c/coreboot/+/32373).
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);
line over 80 characters
Please fix.
-
Julius Werner (Code Review)