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 18:
(3 comments)
Note that this code (and the previous patch about GPIOs) still needs to be deduplicated with the respective SDM845 code. I'd recommend waiting with merging these until that has happened.
https://review.coreboot.org/#/c/29962/18/src/soc/qualcomm/qcs405/clock.c File src/soc/qualcomm/qcs405/clock.c:
https://review.coreboot.org/#/c/29962/18/src/soc/qualcomm/qcs405/clock.c@251 PS18, Line 251: clock_configure(&gcc->blsp1_uart2_apps_clk, uart_cfg, 1843200, This should probably go into uart_init(), not here.
https://review.coreboot.org/#/c/29962/18/src/soc/qualcomm/qcs405/clock.c@259 PS18, Line 259: clock_configure(&gcc->blsp1_qup4_spi_clk, spi_cfg, 1000000, Clock configuration for individual peripheral drivers should be done by the peripheral driver (with frequency values passed in from mainboard code), not in the global clock init. So you probably want a function like clock_configure_spi(bus, freq) in here that is called by the spi_init(freq) of your SPI driver, and the freq value should be passed in from the mainboard/.../bootblock.c code that calls that.
Also, please always use the KHz and MHz macros to make frequency values more readable (e.g. 1*MHz instead of 1000000).
https://review.coreboot.org/#/c/29962/18/src/soc/qualcomm/qcs405/include/soc... File src/soc/qualcomm/qcs405/include/soc/clock.h:
https://review.coreboot.org/#/c/29962/18/src/soc/qualcomm/qcs405/include/soc... PS18, Line 24: #define REG(addr) ((void *)addr) Unused, please take it out (we usually don't like hiding casts in macros).