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 63:
(8 comments)
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] =
On other boards we usually have mainboard code call a function to initialize a controller at the fir […]
So the expectation here is that i2c_init/spi_init.. function will load appropriate firmware to respective QUP.
======================================================= Further, you added comment in PS#25:
Actually, thinking about this some more I'm not sure if we really need to do that on-demand approach I described earlier. Answering my own question from patch set 6, we have now determined empirically that it seems to be possible to update QUP firmware again after it had already been loaded (except for UARTs, but I assume that's just an issue with the code that could be fixed?), so there's not such big reason to delay the other QUPs until RW firmware anymore.
However, even if we keep this approach there need to be some changes: the QSPI needs to be initialized in mainboard code (because the bus frequency is mainboard-dependent), so the QUP initialization needs to be called from there as well so it can run after QSPI (and thus access CBFS). If we have that we can get rid of this roundabout callback scheme and just pass the QUP mapping directly into the function. I think the structure can be simpler too, since QUPs are numbered 0 through 15 and there only needs to be one value if the other two are dependent on that... so I'd just make it a 16-element array of protocols (with 'none' as another valid protocol choice). See also the other comments from patch set 6 ===========================================================
Few queries to get clear understanding, though we haven't thought of this wrt time/scoping.
Is the loading of FW from respective protocol init function is needed for OTA upgrade OR something else in terms design ?
Could you please provide some refrence drivers which are doing the same.
If not then i shall make remaining changes you suggested for : Moving QSPI and QUP init to the mainboard and passing a 16-element array to qupv3 init function with none as an extra protocol.
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/include/soc/... File src/soc/qualcomm/sdm845/include/soc/qupv3_fw_config.h:
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/include/soc/... PS6, Line 65: bool dfs_mode;
Aren't mode and dfs_mode directly dependent on protocol (i.e. […]
dfs_mode is directly dependent on protocol(true for all except UART) so, I have removed it from se_cfg structure. But modes aren't directly depends on the protocol, for example, SPI can be operated in FIFO as well as DMA mode so, I am keeping that in latest patch.
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... File src/soc/qualcomm/sdm845/qupv3_fw_config.c:
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 142: #define SE_IRQ_EN_RMSK 0x0000000f
See general comments about how to do register accesses in other CLs (use struct overlays and the def […]
We are using struct overlay for register access.
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 145: __attribute__((packed))
Avoid __attribute__((packed)) unless any elements are really misaligned. […]
Agree, removed __attribute__((packed)) from latest patchset.
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 190: static void fw_init_qup_common(void);
You generally shouldn't need to forward declare functions unless they recursively call into each oth […]
Done
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 227: /* HPG section 3.1.7.1 */
"HPG" sounds like some sort of programming guide. […]
We cannot directly release it to you, our tech release team will take care of that. If you still need it, I can initiate a request for that.
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 353: fw_size_in_items
nit: could use an assert() to make sure it doesn't overrun the available register space (same goes f […]
Done
https://review.coreboot.org/#/c/25372/6/src/soc/qualcomm/sdm845/qupv3_fw_con... PS6, Line 422: REG_OUT(0x0015200C, 0, 0x3FFFFFC0);
Should probably be a separate function in clock.c. […]
We are using clock_enable_qup API to turn on all the clocks.
-
Akash Asthana (Code Review)