Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45205 )
Change subject: sc7280: Provide initial SoC support ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45205/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45205/2//COMMIT_MSG@7 PS2, Line 7: sc7280
Please start to put the vendor in the prefix too. […]
Is there a rule about that? Considering that patch subject space is often too short to really convey a good summary of a patch anyway, I don't think we should force people to waste too much of it on prefix. SoC names don't really clash between vendors.
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/inc... File src/soc/qualcomm/sc7280/include/soc/symbols.h:
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/inc... PS6, Line 15: DECLARE_REGION(aop) Should probably just dump these into <soc/symbols_common.h> rather than duplicate between chips. Extra symbols don't hurt even if a chip isn't using them.
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/qcl... File src/soc/qualcomm/sc7280/qclib.c:
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/qcl... PS6, Line 9: int qclib_soc_blob_load(void) Please factor this out in qualcomm/common if it's needed by more than one SoC. Since all the SoC's we're using for now need it anyway, I'd just throw it into qclib_load_and_run(). If you really want to keep it separate you could guard it with a Kconfig, or make a separate qclib_add_pmic_stuff() and call that from here or something.
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/tim... File src/soc/qualcomm/sc7280/timer.c:
https://review.coreboot.org/c/coreboot/+/45205/6/src/soc/qualcomm/sc7280/tim... PS6, Line 7: void init_timer(void) Should consider putting this in qualcomm/common and just using SRC_XO_HZ from <soc/clock.h> for the frequency (or define a separate ARCH_TIMER_HZ in each <soc/clock.h> that can then be defined to SRC_XO_HZ or whatever else it might be for each individual SoC).