Lean Sheng Tan 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:
(11 comments)
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 main […]
Good idea. would you want to submit a patch to move this config to mainboard for all project? :)
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 w […]
Usually FSP upd will be published post PV. We will keep it here first and update with new patch later on this.
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?
this is the UPD entry for FSP S TCC (Thermal Control Circuit) Activation Offset
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 21: Chapter
nit: please cros check Chapter number/Doc details
Done
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 70: u32
Use uint32_t here to be consistent?
Done
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/e... PS5, Line 129: u8
uint8_t?
Done
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: […]
Done
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 […]
Great idea! Done.
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/f... File src/soc/intel/elkhartlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/44802/5/src/soc/intel/elkhartlake/f... PS5, Line 23: #define CAMERA1_CLK 0x8000 /* Camera 1 Clock */
Remove Camera clocks if not supported by EHL.
Done
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
Done
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.
Will revisit this later.