Prudhvi Yarlagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29957 )
Change subject: libpayload: Add UART for qcs405 ......................................................................
Patch Set 21:
(9 comments)
we have reviewed all comments and will be addressed soon in the next patch which is under progress.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/configs/config.... File payloads/libpayload/configs/config.mistral:
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/configs/config.... PS20, Line 6: CONFIG_LP_USB=y : CONFIG_LP_USB_EHCI=y : CONFIG_LP_USB_XHCI=y
strictly speaking this isn't covered by the commit message
We will remove this in our next patch. Sricharan and team to add this in a separate gerrit.
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. […]
We will add this uart_fill_lb() functionality in our next patch.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 287: .blsp_uart = BLSP1_UART2,
This seems unused?
We will remove this in our next patch.
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.
We will remove this in our next patch.
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).
We will remove this part of the code in our next patch.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 347: readl
readl() is deprecated, use read32() for new code.
We will change this in our next patch.
https://review.coreboot.org/#/c/29957/20/payloads/libpayload/drivers/serial/... PS20, Line 373: if (*data == 0)
'\0' may be a legal input byte.
Agree, We will remove this in our next patch.
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() f […]
We will remove this initialization part in our next patch.
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.
We will do this assignment in the struct initializer for consin/consout in our next patch.
-
Prudhvi Yarlagadda (Code Review)