Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25373 )
Change subject: sdm845: Add UART support ......................................................................
Patch Set 64:
(8 comments)
https://review.coreboot.org/#/c/25373/64/src/mainboard/google/cheza/Kconfig File src/mainboard/google/cheza/Kconfig:
https://review.coreboot.org/#/c/25373/64/src/mainboard/google/cheza/Kconfig@... PS64, Line 24: default 0x00A84000 Since this is used by the SoC code it should also be defined in sdm845/Kconfig, so that this definition here is just overriding the default for that definition there. You should also add a short help text there to explain what it is.
I think it may be easier to just use an int Kconfig for the QUP number (1 through 15) rather than the base address, since you'll also need to call clock_enable_qup() in your init function (we want to get rid of CB:29489 at some point), and the base address doesn't really work from there. You can use the 'qup' structure defined in sdm845/qcom_qup_se.c (already used by the SPI and I2C stuff) to get the base address from the QUP number.
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c File src/soc/qualcomm/sdm845/uart.c:
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@120 PS64, Line 120: GENI_FW_REVISION_RO_REG This is still not using a struct overlay. Please use the QUP register struct overlay that was now added by the SPI/I2C code with the 'qup' structure in sdm845/qcom_qup_se.c and sdm845/include/soc/qcom_qup_se.h.
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@128 PS64, Line 128: * we are discussing with clock team */ How is this coming along? Ideally you should be using get_uart_baudrate() and approximate the clock rate there as best as you can with the divisors you have.
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@134 PS64, Line 134: write32((void *)0x00118148, 0x1); /* Update the clock */ These are all GCC registers, right? This driver shouldn't update them itself. It should make a call to some clock.c function that does that (probably clock_configure_qup()?).
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@142 PS64, Line 142: gpio_configure(GPIO(5), 1, GPIO_PULL_UP, GPIO_2MA, GPIO_INPUT); This needs to be dependent on the QUP you're actually using... the global 'qup' struct has the info for this, too (compare similar code for SPI and I2C drivers).
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@147 PS64, Line 147: * configuration Can we put this check at the top of this function? Presumably, if the previous stage has set up this it has probably also run all the other setup before this point.
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@213 PS64, Line 213: if (uart_initialized == false) { This should be unnecessary. You can rely on the framework calling uart_init() before the first call to uart_tx_byte().
https://review.coreboot.org/#/c/25373/64/src/soc/qualcomm/sdm845/uart.c@245 PS64, Line 245: } There's no need for all these wrapper functions. Just put the code from uart_qupv3_init() directly into the body of uart_init(), etc.