Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29957 )
Change subject: libpayload: Add UART for qcs405 ......................................................................
Patch Set 20:
(8 comments)
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... File payloads/libpayload/drivers/serial/qcs405.c:
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 286: .uart_dm_base = UART2_DM_BASE, This should be taken from lib_sysinfo.serial->baseaddr rather than hardcoded. (You'll need to implement uart_fill_lb() in coreboot to set that up.)
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 287: .blsp_uart = BLSP1_UART2, This seems unused?
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 291: #define read32(addr) readl(addr) These are already defined in <io.h>, don't define them again.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 329: * @wait: indicates blocking call or not blocking call This parameter is never set by this driver so you should get rid of it (and associated code).
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 347: readl readl() is deprecated, use read32() for new code.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 373: if (*data == 0) '\0' may be a legal input byte.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 544: /* For simplicity sake let's rely on coreboot initalizing the UART. */ You say it, but why aren't you actually doing it? Why do you need that big msm_boot_uart_dm_init() function if coreboot already did that?
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 556: consout.putchar = serial_putchar; These can be assigned in the struct initializer for consin/consout, they don't need to be here.