Attention is currently required from: Shelley Chen, Ravi kumar, mturney mturney, Julius Werner. Ravi Kumar Bokka has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55960 )
Change subject: sc7280: Refactor QUP driver ......................................................................
Patch Set 17:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/55960/comment/1b53853f_43b9b457 PS15, Line 9: Refactor QUP driver by separating : common and SoC specific driver code :
This should probably say something like "Enable common qup driver in sc7280" instead.
Ack
File src/soc/qualcomm/sc7280/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55960/comment/d75fd996_d09d0062 PS8, Line 94: GSI_FW_FILE := $(SC7280_BLOB)/qup_fw/gsi_fw.bin : GSI_FW_CBFS := $(CONFIG_CBFS_PREFIX)/gsi_fw : $(GSI_FW_CBFS)-file := $(GSI_FW_FILE) : $(GSI_FW_CBFS)-type := raw : $(GSI_FW_CBFS)-compression := none : cbfs-files-y += $(GSI_FW_CBFS)
I don't see this in 3rdparty/qc_blobs. […]
Ack
File src/soc/qualcomm/sc7280/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55960/comment/5ac6480e_8d7acfe8 PS15, Line 93: ################################################################################
Actually, just found this CL (for AOP): https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/55960/comment/eac2120e_944eb626 PS15, Line 102: CPUCP_FILE := $(SC7280_BLOB)/cpucp/cpucp.elf
And this CL: https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/55960/comment/2074967f_c1f9ed7f PS15, Line 110: SHRM_FILE := $(SC7280_BLOB)/shrm/shrm.elf
And the appropriate place for loading of shrm.elf seems to be in this CL: https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/55960/comment/f03f1ce7_5325c260 PS15, Line 93: ################################################################################ : AOP_FILE := $(SC7280_BLOB)/aop/aop.mbn : AOP_CBFS := $(CONFIG_CBFS_PREFIX)/aop : $(AOP_CBFS)-file := $(AOP_FILE) : $(AOP_CBFS)-type := payload : $(AOP_CBFS)-compression := $(CBFS_COMPRESS_FLAG) : cbfs-files-y += $(AOP_CBFS) : : ################################################################################ : CPUCP_FILE := $(SC7280_BLOB)/cpucp/cpucp.elf : CPUCP_CBFS := $(CONFIG_CBFS_PREFIX)/cpucp : $(CPUCP_CBFS)-file := $(CPUCP_FILE) : $(CPUCP_CBFS)-type := payload : $(CPUCP_CBFS)-compression := $(CBFS_COMPRESS_FLAG) : cbfs-files-y += $(CPUCP_CBFS) : : ################################################################################ : SHRM_FILE := $(SC7280_BLOB)/shrm/shrm.elf : SHRM_CBFS := $(CONFIG_CBFS_PREFIX)/shrm : $(SHRM_CBFS)-file := $(SHRM_FILE) : $(SHRM_CBFS)-type := payload : $(SHRM_CBFS)-compression := none : cbfs-files-y += $(SHRM_CBFS)
Please ignore this comment and move the loading of the different blobs to the CLs referenced.
Ack
File src/soc/qualcomm/sc7280/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/55960/comment/aca54394_2aad98e3 PS15, Line 31: define QUP_SERIAL0_BASE 0x00980000 : #define QUP_SERIAL1_BASE 0x00984000 : #define QUP_SERIAL2_BASE 0x00988000 : #define QUP_SERIAL3_BASE 0x0098C000 : #define QUP_SERIAL4_BASE 0x00990000 : #define QUP_SERIAL5_BASE 0x00994000 : #define QUP_SERIAL6_BASE 0x00998000 : #define QUP_SERIAL7_BASE 0x0099C000 : #define QUP_WRAP0_BASE 0x009C0000 : #define QUP_0_GSI_BASE 0x00904000 : : /* QUPV3_1 */ : #define QUP_SERIAL8_BASE 0x00A80000 : #define QUP_SERIAL9_BASE 0x00A84000 : #define QUP_SERIAL10_BASE 0x00A88000 : #define QUP_SERIAL11_BASE 0x00A8C000 : #define QUP_SERIAL12_BASE 0x00A90000 : #define QUP_SERIAL13_BASE 0x00A94000 : #define QUP_SERIAL14_BASE 0x00A98000 : #define QUP_SERIAL15_BASE 0x00A9C000 : #define QUP_WRAP1_BASE 0x00AC0000 : #define QUP_1_GSI_BASE 0x00A04000 :
Please change these to tabs so we're consistent with the rest of the #defines.
Ack
https://review.coreboot.org/c/coreboot/+/55960/comment/76b44e0f_ff9ceae7 PS15, Line 31: define QUP_SERIAL0_BASE 0x00980000 : #define QUP_SERIAL1_BASE 0x00984000 : #define QUP_SERIAL2_BASE 0x00988000 : #define QUP_SERIAL3_BASE 0x0098C000 : #define QUP_SERIAL4_BASE 0x00990000 : #define QUP_SERIAL5_BASE 0x00994000 : #define QUP_SERIAL6_BASE 0x00998000 : #define QUP_SERIAL7_BASE 0x0099C000 : #define QUP_WRAP0_BASE 0x009C0000 : #define QUP_0_GSI_BASE 0x00904000 : : /* QUPV3_1 */ : #define QUP_SERIAL8_BASE 0x00A80000 : #define QUP_SERIAL9_BASE 0x00A84000 : #define QUP_SERIAL10_BASE 0x00A88000 : #define QUP_SERIAL11_BASE 0x00A8C000 : #define QUP_SERIAL12_BASE 0x00A90000 : #define QUP_SERIAL13_BASE 0x00A94000 : #define QUP_SERIAL14_BASE 0x00A98000 : #define QUP_SERIAL15_BASE 0x00A9C000 : #define QUP_WRAP1_BASE 0x00AC0000 : #define QUP_1_GSI_BASE 0x00A04000 :
Please change these to tabs so we're consistent with the rest of the #defines.
Ack
File src/soc/qualcomm/sc7280/qcom_qup_se.c:
https://review.coreboot.org/c/coreboot/+/55960/comment/32f5de19_f2ade482 PS15, Line 6: 0
Can we use the enums that you defined (ie: QUPV3_0_SE0, QUPV3_0_SE1, etc. […]
Ack