Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27360 )
Change subject: sdm845: Add interface between CB & QCLib ......................................................................
Patch Set 35:
(2 comments)
One comment from PS31 on 9.Nov.2018 is not addressed yet, regarding CONFIG_CBFS_PREFIX being part of #define for name, e.g. "/pmic". Please confirm, after reviewing latest patch that this is still desired change.
Yes, I still think that would be a bit cleaner.
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/27360/26/src/soc/qualcomm/sdm845/qclib_execu... PS26, Line 203: ddr_mmu->size = ddr_size * MiB;
QCLib is still passing back block size and this multiply is still occurring, please confirm you are […]
I think we should change it for clarity, for all the other TE blobs the 'size' value is in bytes. It's going to be a bit nasty because it needs to change on both ends at once, but that will be easier now than later when we have another couple of SoCs using this interface.
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... File src/soc/qualcomm/sdm845/qclib_execute.c:
https://review.coreboot.org/#/c/27360/32/src/soc/qualcomm/sdm845/qclib_execu... PS32, Line 223: dump_te_table();
Need it, probably not, I find it good diagnostic data to have in the log output. […]
I'm okay with leaving it in.
-
Julius Werner (Code Review)