Attention is currently required from: Ravi kumar. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49768 )
Change subject: sc7280: Add CPUCP firmware support ......................................................................
Patch Set 82:
(4 comments)
File src/soc/qualcomm/sc7280/cpucp_load_reset.c:
https://review.coreboot.org/c/coreboot/+/49768/comment/ebfabaee_dda7787e PS80, Line 11: #define EPSSFAST_EPSS_MUC_CLK_CTRL ((void *)0x18580000)
Please define a struct overlay for this register block like in the other drivers.
Done
https://review.coreboot.org/c/coreboot/+/49768/comment/bc5a3bfc_078970e3 PS80, Line 27: while ((read32(EPSSFAST_EPSS_MUC_CLK_CTRL) & 0x1) != 0x1)
Please don't create endless loops without timeout (use wait_ms()).
Done
File src/soc/qualcomm/sc7280/cpucp_load_reset.c:
https://review.coreboot.org/c/coreboot/+/49768/comment/94a26662_61a11cd1 PS82, Line 15: uint32_t val = read32(&epsstop_ptr->access_override); You're obviously supposed to remove this now. The equivalent to your previous code is just
setbits32(&epsstop_ptr->access_override, 0x1);
File src/soc/qualcomm/sc7280/include/soc/cpucp.h:
https://review.coreboot.org/c/coreboot/+/49768/comment/c599bab6_b767b018 PS82, Line 40: epsstop_ptr nit: We generally don't call these _ptr (e.g. the equivalent for the GCC register block is just called `gcc`, not `gcc_ptr`). So I think you should just call these `epss_top` and `epss_fast`.