Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29962 )
Change subject: qcs405: clock: Adding the clock support for qcs405 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/29962/3/src/soc/qualcomm/qcs405/clock.c File src/soc/qualcomm/qcs405/clock.c:
https://review.coreboot.org/#/c/29962/3/src/soc/qualcomm/qcs405/clock.c@65 PS3, Line 65: spi_cfg
which SPI buses are initialized here? We'll need SPI bus 4 rather early, so it should be covered her […]
This is the parent clock for the QSPI controller that is connected to SPI flash (the more recent SDM845 version names this qspi_core_cfg which is a little clearer). The other ("QUP") SPI busses use a different parent clock (this is qup_cfg in the SDM845 version of this file, where we merged what's called i2c_cfg and uart_cfg here into one table because it's really the same peripheral that can be programmed to do different things).
https://review.coreboot.org/#/c/29962/3/src/soc/qualcomm/qcs405/clock.c@75 PS3, Line 75: SRC_XO_19_2MHZ I don't understand how this can work. What you're doing here is essentially saying that you can provide a 30MHz source clock by using the 19.2MHz oscillator with no divisor. That doesn't make sense, that would give you 19.2MHz, not 30. The point of this table is to define operating points of a frequency you can provide (.hz) and the settings that need to be programmed into the GCC registers to provide it (.hw_ctl, .src and .div).
Note how, like everything else, this file needs to be deduplicated with SDM845 code (which has also changed quite a bit since you copied this... the clock code has now landed there, so you can start writing a patch to move it into a common directory now). I expect almost all the code will be shared, and maybe this table will need to be separately implemented by each SoC.
-
Julius Werner (Code Review)