Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25208 )
Change subject: sdm845: Add QCLib to RomStage to perform IP init ......................................................................
Patch Set 72:
(7 comments)
Thanks for the update! The move to common looks good, but please note that there are still many older comments still applying to this version that need to be addressed.
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/include/soc... File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/include/soc... PS72, Line 79: void soc_blob_load(void); Please prefix all external functions with qclib_ or something like that.
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c@135 PS72, Line 135: printk(BIOS_INFO, "Board Rev(0x%x)\n", board_id()); This is already printed in ramstage once, do we need it here?
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c@193 PS72, Line 193: Private IP That sounds a bit weird. Isn't DDR always going to be part of this (at least with the current code)? Let's just say "initialize DDR" because that's the most important part here.
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/common/qclib.c@209 PS72, Line 209: ret_code = doit(&qclib_cb_if_table, NULL); Now that we only have one argument, can we switch to using prog_run() for this?
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/sdm845/soc_blob_lo... File src/soc/qualcomm/sdm845/soc_blob_load.c:
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/sdm845/soc_blob_lo... PS72, Line 1: /* This should probably also be called qclib_execute.c to make clear that it belongs to the common file of the same name.
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/sdm845/soc_blob_lo... PS72, Line 29: printk(BIOS_INFO, "soc_blob_load(sdm845)\n");
Prefer using '"%s... […]
This print is probably unneeded.
https://review.coreboot.org/#/c/25208/72/src/soc/qualcomm/sdm845/soc_blob_lo... PS72, Line 57: die("Couldn't run soc_blob_load.\n");
Prefer using '"%s... […]
Probably cleaner to return -1 from here and have all the die()s in the caller.
-
Julius Werner (Code Review)