Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42095 )
Change subject: sc7180: Remove QcLib specific changes from CB UART ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42095/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42095/1//COMMIT_MSG@20 PS1, Line 20: Sorry, I'm still a bit confused about what this patch does and why. So previously coreboot was configuring the QUP source clock to 7.3728 MHz, but now you want it at 19.2 MHz instead? I can believe that either value would work, I just don't understand why it changes now and what this had to do with QcLib?
https://review.coreboot.org/c/coreboot/+/42095/1/src/soc/qualcomm/sc7180/qup... File src/soc/qualcomm/sc7180/qupv3_config.c:
https://review.coreboot.org/c/coreboot/+/42095/1/src/soc/qualcomm/sc7180/qup... PS1, Line 57: QUPV3_UART_SRC_HZ If we no longer need this constant, please also remove it from clock.h. Also remove the corresponding entry from the qup_cfg array in clock.c if we don't need to support this frequency anymore.
https://review.coreboot.org/c/coreboot/+/42095/1/src/soc/qualcomm/sc7180/qup... PS1, Line 55: if (protocol != SE_PROTOCOL_UART) { How is the QUP clock configured now if you just take this out? Shouldn't we still call
clock_configure_qup(bus, SRC_XO_HZ)
somewhere? Are you just hoping that this was already set up by PBL or something? I would prefer if coreboot does its own clock setup, even if it just writes the same values that PBL would've put in there already.