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 39:
(3 comments)
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 52: clock_configure_dfsr(bus); If the non-UART QUPs initialize their base clock here, I think it would make sense to do that for the UART as well. So you should do something like
if (protocol == SE_PROTOCOL_UART) { clock_configure_qup(bus, QUP_UART_SRC_HZ); } else { ...clock_configure_dfsr... }
here. (QUP_UART_SRC_HZ should be #defined to 7372800 and be used everywhere that appears in clock.c and qupv3_uart.c.)
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/qu... PS39, Line 89: if (mode == GSI) {
Where is this set? I just see you set 'mode = MIXED' at the top and then never change it again. […]
In earlier versions the UARTs used 'mode = FIFO', but with this patch they'll also be MIXED. Does that still work? I think it might be clearer if you just get rid of the 'mode' distinction and branch based on SE_PROTOCOL_UART where necessary.
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/qu... PS39, Line 174: qup_load_se_firmware This is doing more than loading firmware (e.g. it initializes clocks), so it should have a more generic name, maybe just qupv3_init() or something. (Same goes for the name init_and_load_se_firmware()... in fact, I'm not sure if that really needs to be a separate function.)
-
Julius Werner (Code Review)