Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44802 )
Change subject: soc/intel/elkhartlake: Do initial SoC commit till ramstage ......................................................................
Patch Set 5: Code-Review+1
(9 comments)
There are a few files where the includes are not sorted. We had patches in the past just to sort them alphabetically. Could you please have a look.
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/K... PS5, Line 184: config CBFS_SIZE : hex : default 0x200000 Is this something we can decide on SoC-level? There are good reasons to increase this size from mainboard point of view.
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/K... PS5, Line 189: default "src/vendorcode/intel/fsp/fsp2_0/elkhartlake/" I guess in the mid term FSP for EHL will be part of the public FSP repo, right? Any idea when that will be available? Or is it already and you can switch to 3rdparty/fsp/ElkhartLakeFspBinPkg here?
https://review.coreboot.org/c/coreboot/+/44802/3/src/soc/intel/elkhartlake/c... File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/44802/3/src/soc/intel/elkhartlake/c... PS3, Line 63: uint32_t tcc_offset; Is this entry for the TCC data set itself?
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... File src/soc/intel/elkhartlake/espi.c:
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 70: u32 Use uint32_t here to be consistent?
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 129: u8 uint8_t?
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 163: (inb(0x61)) & 0xf0 You could skip the outer paranthesis: reg8 = inb(0x61) & 0xf0;
Or move the closing one to the end: reg8= (inb(0x61) & 0xf0);
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 164: 0x61 nit: One day we should provide a #define for this register (NMI_STS_CNT) and the next one (0x70: NMI_EN)
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/p... File src/soc/intel/elkhartlake/p2sb.c:
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/p... PS5, Line 16: Sideband sideband
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/s... File src/soc/intel/elkhartlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/s... PS5, Line 172: char Could that be void? You never access this pointer as char but always as void.