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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/Ma... File src/soc/qualcomm/sc7180/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/Ma... PS39, Line 14: bootblock-y += qcom_qup_se.c Should add these to verstage and romstage as well, even if current platforms don't need it, it's nice to have the general driver toolkit available in all stages.
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
This is the first patch in the train, so if Jenkins gives you a -1 on it that means there's definitely still something wrong with it.
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/35499/39/src/soc/qualcomm/sc7180/in... PS39, Line 32: 0x8C0000 Please write all 8 digits (e.g. 0x008C0000) for readability (and ideally we should sort everything in here by address, too).
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 326: union proto_rx_trans_len rx_trans_len; nit: I think these would look better if you wrote them with an inline unnamed union, actually... e.g. just put
union { u32 uart_tx_trans_len; u32 spi_rx_trans_len; u32 i2c_rx_trans_len; };
right in here (and same for the other two). That way you don't have to scroll up when reading this to know which fields are really merged into this slot, and you don't have to write a lengthy regs->tx_trans_len.spi_tx_trans_len everywhere (it just becomes regs->spi_tx_trans_len).
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) { Where is this set? I just see you set 'mode = MIXED' at the top and then never change it again. How can we end up in any of these cases?
-
Julius Werner (Code Review)