Lean Sheng Tan 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:
(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.
Sure.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 85: Io
"IO space" or "I/O space"
Ack
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.
Ack
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 introdu […]
ok
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?
Sure.
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?
let me change this.
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. […]
Yeah you are right, it was not meant to be compile-able. The next patch will add the tables in :)
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 71: mch_table
Same here.
Same as above.
https://review.coreboot.org/c/coreboot/+/44799/3/src/soc/intel/elkhartlake/b... PS3, Line 89: pch_table
And here.
Same as above.
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.
Sure I will add into to do.
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?
I am not sure why JSL is giving the same value for both, but the right value will be updated in EHL in future patch.