Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35499 )
Change subject: sc7180: Add QUPv3 FW load & config ......................................................................
Patch Set 41:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/qupv3_config.h:
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/in... PS40, Line 54: #define QUPV3_UART_SRC_HZ 7372800
Hi Julius, […]
I'm not sure I understand what you mean. Generally, this is the UART *source* clock, I would not expect it to be updated often (or ever, really, unless we discover a problem with the old one). The actual output baud rate for the UART wires is determined by Kconfig (CONFIG_TTYS0_BAUD, reported through get_baud_rate()) and qupv3_uart.c:uart_init() determines the right divisors to get from the source clock defined here to that rate. We should be able to change the baud rate without changing this constant.
If this constant does need to change, then the clock code has to change anyway. The number chosen here must be a number that is available in the qup_cfg[] table in clock.c (and 7372800 is currently the only number there). That's why I'm saying there should be a single global constant that both qupv3_uart.c and clock.c share -- because there is already an implicit requirement that those two need to stay in sync, and using the same constant in the code just makes that requirement more obvious to the reader.
Since the clock rate is produced by the clock controller and consumed by the UART, I think <soc/clock.h> is probably the right place to put that shared constant.
-
Julius Werner (Code Review)