Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45759 )
Change subject: soc/intel/alderlake/ramstage: Do initial SoC commit till ramstage ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/Mak... File src/soc/intel/alderlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/Mak... PS3, Line 45: r Please keep entries sorted in alphabetical order
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 23: #include <string.h>
<string.h> not used. […]
Untrue. `memcpy` appears in this file.
Also, `bool` is used, so just include <types.h>
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 181: uintptr_t pmc_bar = soc_read_pmc_base(); no need for a temporary variable
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 188: uint64_t gfxvtbar = MCHBAR64(GFXVTBAR) & VTBAR_MASK; Many things here can be const
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 233: TBT0BAR You could redefine these registers as follows:
#define TBTxBAR(x) (0x7888 + (x) * 8)
This way, the macros can be easily used in loops 😊
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 245: /* Add RMRR entry */ : const unsigned long tmp = current; : current += acpi_create_dmar_rmrr(current, 0, : sa_get_gsm_base(), sa_get_tolud_base() - 1); : current += acpi_create_dmar_ds_pci(current, 0, 2, 0); : acpi_dmar_rmrr_fixup(tmp, current); Shouldn't this be skipped if the IGD is disabled?
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... File src/soc/intel/alderlake/chip.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... PS3, Line 118: rotine ro*u*tine
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... PS3, Line 136: /* Snapshot the current GPIO IRQ polarities. FSP is setting a : * default policy that doesn't honor boards' requirements. */ : itss_snapshot_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END); Does this still apply to ADL?
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... File src/soc/intel/alderlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 281: bool <types.h>
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/smm... File src/soc/intel/alderlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/smm... PS3, Line 3: #include <types.h> I prefer to keep #includes sorted alphabetically.