Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25373 )
Change subject: sdm845: Add UART support ......................................................................
Patch Set 75:
(11 comments)
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/Kconfig File src/soc/qualcomm/sdm845/Kconfig:
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/Kconfig@40 PS74, Line 40: default 9
Not sure if this should have a default here, it's really completely dependent on each board... […]
Selection of QUP instance depends on PORed defined in file@ https://review.coreboot.org/c/coreboot/+/25372/74/src/mainboard/google/cheza... Because we load FW to QUPs based on above PORed use cases. Yes, 9 is the fixed QUP instance for console UART on SDM845 products.
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/Kconfig@42 PS74, Line 42: QUP SE instance is 9.
This should explain what the config does, not why you're choosing this default. […]
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qcom_qup_se.h:
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/include/soc... PS74, Line 462: uintptr_t base_addr;
No, don't store the same thing twice. […]
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c File src/soc/qualcomm/sdm845/uart.c:
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@81 PS74, Line 81: div = (unsigned int)(19.2 * MHz) / uart_freq;
This looks like it should use QUP_WRAP_CORE_2X_19_2_MHZ […]
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@120 PS74, Line 120: 0x8000000
This should probably be a named constant?
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@126 PS74, Line 126: unsigned char data = 0x0;
nit: Don't really need this variable. You can just do […]
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@141 PS74, Line 141: while (read32(®s->geni_status) & GENI_STATUS_M_GENI_CMD_ACTIVE_MASK)
nit: Just call uart_tx_flush(idx)
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@146 PS74, Line 146: write32(®s->geni_m_cmd0, 0x08000000);
Same named constant?
In UART protocol, we write 0x8000000 to the main engine to start TX transfers and the same to secondary engine to start RX transfer. I think it will be better to define 2 different constant START_UART_RX, START_UART_TX for better readability.
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@161 PS74, Line 161: 0
This needs to be CONFIG_UART_FOR_CONSOLE
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@162 PS74, Line 162: 115200
Shouldn't this be get_uart_baudrate()?
Done
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/uart.c@166 PS74, Line 166: lb_add_console(LB_TAG_CONSOLE_SERIAL8250MEM, data);
Not sure if you should be setting this tbh. This is not an 8250 console. […]
Done, Removed.
-
Akash Asthana (Code Review)