mturney mturney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35494 )
Change subject: sc7180: initial SoC support ......................................................................
Patch Set 6:
(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 topl […]
Done
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 […]
Done
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?
Ack
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 alre […]
We are removing for now and if need to reserve it, will indicate for what reason.
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
Ack
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
Ack
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?
Done
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?
Done
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 […]
Done
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?
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35494/5/src/soc/qualcomm/sc7180/uar... PS5, Line 25: gpio_t uart_gpio = { 0 };
... […]
Done