Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45192 )
Change subject: soc/intel/alderlake/romstage: Do initial SoC commit till romstage ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/esp... File src/soc/intel/alderlake/espi.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/esp... PS7, Line 13: <arch/io.h>
not used ?
Ack
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/esp... PS7, Line 15: <arch/ioapic.h>
not used? […]
Ack
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/inc... PS7, Line 9: size_t
<stddef. […]
Ack
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... PS7, Line 109: uint8_t dimm_cfg = DISABLE_DIMM1; /* Use only DIMM0 */
Doesn't this essentially force 1DPC only? I'm not sure how newer FSPs want the DRAM settings to be p […]
If you see this two line, it will be much clear
https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/jasperl... https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/jasperl...
so based on board design with DIMM0 and DIMM0, DIMM1 those inputs will refer as dimm_cfg. now you assume the call of memcfg_init is having dual DIMM hence half_populated = false hence it would fill SPD for all 8 channels. in some cost reduction design where we have only 1 DPC then we will assume the DIMM will be attached with channel 0 hence disabling channel 1 code line between 139-157 below. is that clear now ?
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... PS7, Line 183: mem_cfg->ECT = board_cfg->ect;
Random thought: It would be nice to know when ECT should be enabled. […]
https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/jasperl... https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/jasperl...
its mostly mainboard specific and as i could see ECT is generally enable for all memory technology unless some specific issue on that board to meet memory guideline.
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/res... File src/soc/intel/alderlake/reset.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/res... PS7, Line 31: die("unknown reset type");
I think other SoCs might be doing this, but I don't think it's a good idea. […]
good suggestion Angel. can i do that as incremental after this CL. so i can move this reset into soc/intel/common/block for now and refer as FSP based platform refer from there, its actually a common file.
Is that reasonable approach ?
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... PS7, Line 91: 1
We should provide a Kconfig option for this somewhere.
good idea, i will do that after this CL?
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... PS7, Line 98: dev ? dev->enabled : 0
is_dev_enabled(dev)
Ack
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... PS7, Line 8: SMBUS_BASE_ADDRESS
SMBus base address
Ack