Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29970 )
Change subject: Mistral: QCS405: Added RPM support ......................................................................
Patch Set 14: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/29970/14/src/soc/qualcomm/qcs405/Makefile.in... File src/soc/qualcomm/qcs405/Makefile.inc:
https://review.coreboot.org/#/c/29970/14/src/soc/qualcomm/qcs405/Makefile.in... PS14, Line 61: ifneq (,$(findstring $(RPM_FILE),$(rpm_file))) This is a hack, I don't not think we should submit any code like this. These blobs should be submitted to the 3rdparty/blobs repository, and once they're there you don't need this code that checks for existence and builds a non-working image without them only to trick Jenkins.
There are still many license issues to be solved for these projects, and they'll not just go away if we submit code that looks like it builds but doesn't actually work without them. I think you should keep these patches floating until they're actually resolved. If you want to speed this up, you can ask Mike Stefanick to start responding to my mails again (I haven't heard anything from Qualcomm for a month about this and that's not helpful if we want to make progress).
https://review.coreboot.org/#/c/29970/14/src/soc/qualcomm/qcs405/include/soc... File src/soc/qualcomm/qcs405/include/soc/memlayout.ld:
https://review.coreboot.org/#/c/29970/14/src/soc/qualcomm/qcs405/include/soc... PS14, Line 34: REGION(rpm, 0x00200000, 0xA4000, 0x0) nit: Do you really need two regions here? They're both covering the same space. If you just want one region, get rid of the RPMSRAM_START/_END. Those are only needed if you want to nest multiple subregions into an SRAM region.
-
Julius Werner (Code Review)