Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35494 )
Change subject: sc7180: initial SoC support ......................................................................
Patch Set 5:
(12 comments)
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/Mak... File src/soc/qualcomm/sc7180/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/Mak... PS5, Line 43: $(objcbfs)/bootblock.bin: $(objcbfs)/bootblock.raw.bin Don't need this as long as all we do is copying, there's already a generic rule for this in the toplevel Makefile.inc.
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/gpi... File src/soc/qualcomm/sc7180/gpio.c:
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/gpi... PS5, Line 1: /* Do we need to add this file now? Would prefer to leave it (and the bitbang UART depending on it) for a later patch where you actually add the implementation. (Just don't select GENERIC_GPIO_LIB for now.)
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 19: 0x14699000 number doesn't match the one below?
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 35: REGION(bsram_reserved1, 0x14800000, 84K, 4K) nit: Can we add a comment what it is reserved for? (I think I asked about this region on SDM845 already, don't remember if it was ever really answered...)
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 40: REGION(bsram_align1, 0x14838C00, 1K, 1K) nit: No need to define regions for empty space, you can just leave it empty
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 48: REGION(bsram_unused, 0x14853400, 0x1CC00, 1K) nit: same here
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 57: REGION(dram_reserved, 0x80900000, 0x200000, 0x1000) nit: can we find better names to differentiate these two?
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 58: /* Various hardware/software subsystems make use of this area */ This comment looks like it's supposed to be two lines earlier?
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 61: RAMSTAGE(0x9F860000, 2M) nit: with the recent addition of FIT booting code (not used by Chrome OS (yet), but by others) which needs enough heap and CBFS scratch space to manipulate kernel and initramfs images, it has become en vogue to make these last two a lot bigger (e.g. 16MB each). Might as well do that if we have the space.
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/symbols.h:
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/inc... PS5, Line 25: DECLARE_REGION(el3_stack_canary); Looks like this is a leftover that was already removed from SDM845 at some point?
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/uar... File src/soc/qualcomm/sc7180/uart_bitbang.c:
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/uar... PS5, Line 21: #if 0 Can't commit stuff like this. I'd just leave this file out until later when you can actually make it work. (Just don't select HAVE_UART_SPECIAL for now if it doesn't compile otherwise.)
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/uar... PS5, Line 25: gpio_t uart_gpio = { 0 }; ...not really sure what this is all about, anyway?