Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29967 )
Change subject: qclib: Add qclib support with interface tables ......................................................................
Patch Set 25:
(8 comments)
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/common/qclib.c File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/common/qclib.c@146 PS25, Line 146: QCLIB_BA_SAVE_TO_STORAGE This flag is meant for communication from QcLib back out to coreboot. coreboot should not set it to start with.
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/include/soc... File src/soc/qualcomm/qcs405/include/soc/memlayout.ld:
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/include/soc... PS25, Line 43: 0x80000 nit: prefer giving sizes in decimal (e.g. 512K instead of 0x80000)
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/include/soc... PS25, Line 45: 0x1000 nit: prefer giving alignments in decimal, and only put the alignment restriction there that the region actually needs (e.g. for most of these probably not more than 8).
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/include/soc... File src/soc/qualcomm/qcs405/include/soc/qclib.h:
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/include/soc... PS25, Line 19: void qclib_load_and_run(void); Isn't this in some qualcomm/common header? This should be in some qualcomm/common header.
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/mmu.c File src/soc/qualcomm/qcs405/mmu.c:
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/mmu.c@31 PS25, Line 31: mmu_config_range((void *)0x80000000, 0x40000000, MA_MEM | MA_S | MA_RW); You can't do this here. This runs in the bootblock before DRAM is initialized. The DRAM mapping is already added by the common code (qc_mmu_dram_config_post_dram_init()), you don't need to worry about it anymore.
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/mmu.c@39 PS25, Line 39: MA_MEM | MA_NS | MA_RW); This seems to be the same line as above? Again, you don't need it, common code already does that.
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/soc_blob_lo... File src/soc/qualcomm/qcs405/soc_blob_load.c:
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/soc_blob_lo... PS25, Line 22: #define DCB_NAME "/dcb" Strings should be inline for consistency with SDM845 code
https://review.coreboot.org/#/c/29967/25/src/soc/qualcomm/qcs405/soc_blob_lo... PS25, Line 29: printk(BIOS_INFO, "%s:qcs405\n", __func__); Unnecessary
-
Julius Werner (Code Review)