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 26:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/in... PS26, Line 21: Please standardize on tabs, not spaces for header #defines (and keep it in the same column for all #defines in a file).
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/qcom_qup_se.h:
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/in... PS26, Line 233: This sort of alignment should also be done with tabs if possible.
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... File src/soc/qualcomm/sc7180/qupv3_config.c:
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... PS26, Line 37: struct se_cfg *scfg) This se_cfg needs to go as well then, obviously (see below). Instead you should just pass the raw protocol and QUP index. The mode it seems(?) can be tied to the protocol (e.g. UART is always FIFO, the others always MIXED).
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... PS26, Line 196: fw_list[SE_PROTOCOL_SPI] = You should check if fw_list[protocol] is NULL here, and avoid reloading the same blob again if it's not.
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... PS26, Line 211: printk(BIOS_INFO, "%s: *ERROR* *INVALID PROTOCOL****\n", You can use assert() or die() for this.
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... PS26, Line 220: printk(BIOS_INFO, "%s: *ERROR* *PROTOCOL MISMATCH****\n", I... am very confused what this is supposed to be. The whole point of doing this whole exercise is that we *don't* need an explicit table mapping QUPs to protocols anymore. But now you still have that table (and not even in board-specific code) and you only use it to compare the stuff that the other code should already be doing correctly? Please take this (and the table) out.
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... File src/soc/qualcomm/sc7180/qupv3_fw_config.c:
https://review.coreboot.org/c/coreboot/+/35499/26/src/soc/qualcomm/sc7180/qu... PS26, Line 1: /* I don't understand what makes this file separate from qupv3_config.c. I think you can merge the two.
-
Julius Werner (Code Review)