Akash Asthana 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)
Thanks for reviewing.
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. […]
These are clock control registers(not QUP registers), we are writing them to enable DFS(dynamic frequency selection) mode for QUPs which is PORed with SPI & I2C use case(in kernel/HLOS). We are discussing this change internally, for the time being, we are moving it out into separate HACK patch to unblock this Gerrit.
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] =
Whoops, sorry for missing this. […]
This will violate HPG sequence requirement provided to us by the HW team which says that we have to do those common QUPV3 wrapper initialization every time before load firmware.
Suppose we moved common initialization to bootblock.c and loaded FW on demand for SPI use case and it started SPI transactions, now if we want to load FW for I2C usecase on same QUPV3 wrapper's QUP instance, we still need to do common QUPV3 wrapper initialization before loading FW to QUP. This can trouble running SPI.
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).
Done
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 […]
Done
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 d […]
Done
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 QUPV […]
Done
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.
Done
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 […]
Done