Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35960 )
Change subject: trogdor: libpayload uart/serial driver support ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/Kconfig File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/Kconfig... PS5, Line 267: bool "SC7180 SOC compatible serial port driver" Since it seems that this driver works for multiple Qualcomm SoCs, should we maybe give it a more generic name (e.g. QUALCOMM_QUPV3_SERIAL_CONSOLE or something like that)? Applies to the filename as well.
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/Kconfig... PS5, Line 271: config CONSOLE_UART Not necessary since we have the base address in the coreboot table instead.
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/qcom_qup_se.c:
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/drivers... PS5, Line 33: struct qup_regs *qup[12] = { This shouldn't be necessary since we can get the QUP base address from the coreboot table.
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/sc7180.c:
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/drivers... PS5, Line 101: struct qup_regs *regs = qup[idx]; You should do
struct qup_regs *regs = (void *)(uintptr_t)lib_sysinfo.serial->baseaddr;
instead. (Might wanna put that in a helper function or macro since you'll need it a lot.)
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/drivers... PS5, Line 115: { You should check
if (!lib_sysinfo.serial) return;
before registering the drivers here.
https://review.coreboot.org/c/coreboot/+/35960/5/payloads/libpayload/include... File payloads/libpayload/include/qcom_qup_se.h:
PS5: Since we'll only have the UART driver and no other QUP-related stuff in libpayload, I think it would be better to just inline all of these definitions in serial/sc7180.c.