Angel Pons 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:
(6 comments)
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 passed in, though.
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. Is it mainboard-specific, or is it only needed for certain memory types? (e.g. LPDDR)
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. If we're going to hang the platform, we might as well try to reset anyway.
BTW, if these FSP status codes are stable across FSP implementations, I'd say this function could be placed in drivers/fsp. This could be somewhat troublesome as there's no vendor-agnostic concept of `global` reset. So as a shorter-term goal, this could be factored out into soc/intel/common, if applicable.
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.
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... PS7, Line 98: dev ? dev->enabled : 0 is_dev_enabled(dev)
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