Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25373 )
Change subject: sdm845: Add UART support ......................................................................
Patch Set 74:
(13 comments)
Hi T.mike,
It would be great if you could update the issue with using CONFIG_UART_FOR_CONSOLE (instead of 0) in uart_fill_lb() quickly (without waiting to circle back to the driver owner), since it's a trivial fix and it currently breaks runtime firmware console output (making things very annoying for the kernel folks when they run into random qtiseclib/hardware panics).
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@13 PS74, Line 13: <<<<<<< HEAD Something went wrong here
https://review.coreboot.org/#/c/25373/74/src/soc/qualcomm/sdm845/Kconfig@17 PS74, Line 17: select BOOTBLOCK_CONSOLE This is default y, there should be no need to select it explicitly.
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... right? Or is 9 the standard QUP that Qualcomm recommends for all SDM845 boards? In that case this is fine, but then we should remove the override from the mainboard Kconfig because this default is already correct.
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. It is what menuconfig will display when the user tries to configure this manually. So something like "Select the QUP instance to be used for UART console output."
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. If you need to access the raw address, you can cast regs to uintptr_t.
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
Also, should this maybe use DIV_ROUND_CLOSEST() instead?
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?
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
return read32(...); } return 0;
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)
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?
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
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()?
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. Looks like nothing is really using this coreboot table entry. Probably better to just leave it out.