Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25373 )
Change subject: sdm845: Add UART support ......................................................................
Patch Set 74:
(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 definit […]
Done
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. […]
Done
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 […]
Done
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. […]
Done, We removed this part of code instead we are using clock APIs to configure the clock.
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... […]
Done
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 […]
Done
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. […]
Done
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. […]
Done
-
Akash Asthana (Code Review)