Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35499 )
Change subject: sc7180: Add QUPv3 FW load & config ......................................................................
Patch Set 40:
(9 comments)
Please make sure you always address all comments -- we cannot submit patches unless they pass CI and all comments are resolved in Gerrit. (You can click on "Comment Threads" to easily see all still unresolved comments across all patch sets.)
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/bo... File src/soc/qualcomm/sc7180/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/bo... PS39, Line 20: #include <soc/qi2s.h>
src/soc/qualcomm/sc7180/bootblock.c:20:10: fatal error: soc/qi2s.h: No such file or directory […]
*ping*
Still outstanding.
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/qcom_qup_se.h:
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/in... PS39, Line 471: int size);
Should fit on one 96 characters line.
I'd say before the rest of the codebase is transitioned to 96 characters we shouldn't force new submissions to use it.
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/qupv3_config.h:
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/in... PS40, Line 54: #define QUPV3_UART_SRC_HZ 7372800 Again, this should also be used for where 7372800 appears in clock.c. You probably want to define it in <soc/clock.h>.
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/qu... File src/soc/qualcomm/sc7180/qupv3_config.c:
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/qu... PS39, Line 89: if (mode == GSI) {
In earlier versions the UARTs used 'mode = FIFO', but with this patch they'll also be MIXED. […]
Looks like you did this? (Please make sure to always mark comments you addressed as "Done", Gerrit won't let us submit a patch until all comments are resolved.)
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/qu... PS39, Line 174: qup_load_se_firmware
Merged two functions.
Done
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/qu... File src/soc/qualcomm/sc7180/qupv3_config.c:
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/qu... PS40, Line 22: size_t fw_size; You don't need this, so you can just pass NULL to cbfs_boot_map_with_leak().
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/qu... PS40, Line 31: switch (protocol) { How about just
const char * filename[] = { [SE_PROTOCOL_SPI] = "fallback/spi_fw", [SE_PROTOCOL_UART] = "fallback/uart_fw", [SE_PROTOCOL_I2C] = "fallback/i2c_fw", }
if (fw_list[protocol] >= SE_PROTOCOL_MAX || !filename[protocol]) die(...);
if (!fw_list[protocol]) { fw_list[protocol] = cbfs_boot_map_with_leak(filename[protocol], CBFS_TYPE_RAW, NULL); if (!fw_list[protocol]) die(...); }
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/qu... PS40, Line 51: assert(protocol < SE_PROTOCOL_MAX); There are actually other protocols you defined that are not in this list (i.e. I3C). I think it would be best to just die() here.
https://review.coreboot.org/c/coreboot/+/35499/40/src/soc/qualcomm/sc7180/qu... PS40, Line 75: /* To maintain Div=4 for QcLib, configure clock to 7372800Hz for sc7180 */ nit: QcLib really shouldn't reconfigure any UART registers to begin with when being called from coreboot. It would be good if we could fix that from the QcLib side.
-
Julius Werner (Code Review)