Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44799 )
Change subject: soc/intel/elkhartlake/bootblock: Do initial SoC commit until bootblock ......................................................................
Patch Set 3: Code-Review+1
(11 comments)
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... File src/soc/intel/elkhartlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 6: #include <device/device.h> Move one line before in favor of alphabetical order.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 85: Io "IO space" or "I/O space"
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 91: IO Space Keep it consistent to the one above.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 93: ~0 Do you want 0xffffffff here? Maybe write it more explicit here as otherwise the compiler can introduce a 64 bit value? You can deal with it in a follow up commit as well if you have to touch things here anyway.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... File src/soc/intel/elkhartlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 4: #include <device/pci_ops.h> Adjust according to alphanumerical order?
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 33: u32 uint32_t in order to stay consequent in this file?
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 43: cpu_table cpu_table[] is not defined in this file whereas it is present in jasperlake. I have seen the TODO above but this code is not compileable this way.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 71: mch_table Same here.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 89: pch_table And here.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 106: igd_table And here. In Addition, IGD IDs are not mentioned in the TODO above.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/i... File src/soc/intel/elkhartlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/i... PS3, Line 15: #define PCH_TRACE_HUB_BASE_ADDRESS 0xfc800000 Dou you mean to match the PCH_PRESERVED_BASE_ADDRESS define here?