Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25372 )
Change subject: sdm845: Add QUPv3 FW load & config ......................................................................
Patch Set 75:
(8 comments)
https://review.coreboot.org/#/c/25372/75/src/mainboard/google/cheza/mainboar... File src/mainboard/google/cheza/mainboard.c:
https://review.coreboot.org/#/c/25372/75/src/mainboard/google/cheza/mainboar... PS75, Line 52: write32((void *)0x11874c, 0x21); Note that this can't stay here like this. I forgot what it does by now, but if it actually needs to be done in firmware we need to find a way to do it cleanly. (You can mvoe it into a separate HACK patch for now if you don't want to block this one.)
https://review.coreboot.org/#/c/25372/6/src/mainboard/google/cheza/qupv3_con... File src/mainboard/google/cheza/qupv3_config.c:
https://review.coreboot.org/#/c/25372/6/src/mainboard/google/cheza/qupv3_con... PS6, Line 18: struct se_cfg se_mappings[QUPV3_SE_MAX] =
In SDM845, we have two QUPV3 wrappers HW, each wrapper QUPV3 wrapper HW has 8 Serial engines. […]
Whoops, sorry for missing this. So you're concerned about the sequence in fw_init_qup_common()? How about we just call that from sdm845/bootblock.c but have the rest of the QUP initialization triggered by the individual drivers that need it? As far as I can tell, init_and_load_se_firmware() only touches a single QUP's register space. That's the part we should run on-demand.
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qupv3_config.h:
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/include/soc... PS75, Line 23: #endif // _SDM845_QUPV3_CONFIG_H_ Please use /* C89 */ instead of // C99 comments (please fix globally).
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... File src/soc/qualcomm/sdm845/qupv3_fw_config.c:
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... PS75, Line 166: static uintptr_t qupv3_se_base_addr[QUPV3_SE_MAX] = { You should be using the global 'qup' array we introduced for the SPI and I2C drivers instead of this now.
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... PS75, Line 200: reg_value = read32(se_base + GENI_OUTPUT_CTRL_REG) & All these register accesses should be using the struct qup_regs that we introduced for the SPI/I2C drivers now.
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... PS75, Line 396: reg_value = read32((void *)qupv3_common_base_list[i] + For all these register accesses you should make a new struct overlay for the common part of the QUPV3_WRAP.
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... PS75, Line 425: write32((void *)0x0015200C + 0, 0x3FFFFFC0); All accesses to GCC registers should be made from clock.c.
https://review.coreboot.org/#/c/25372/75/src/soc/qualcomm/sdm845/qupv3_fw_co... PS75, Line 427: fw_init_qup_common(); Can we change this to something like
qup_common_init(qup_wrap0); qup_common_init(qup_wrap1);
to avoid having to loop over a 2-entry array?