Subrata Banik 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:
(20 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
Ack
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>
Untrue. `memcpy` appears in this file. […]
Ack
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
Ack
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
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 233: TBT0BAR
You could redefine these registers as follows: […]
Ack
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?
Ack
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 3: #include <console/console.h>
not used?
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... PS3, Line 118: rotine
ro*u*tine
Ack
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?
so far yes, we have bug filled and mostly its early FSP hence scope for improvement is here
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 6: #include <intelblocks/msr.h>
not used. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/me.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 10: u32
<stdint. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/ramstage.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 6: #include <device/device.h>
not used?
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/loc... File src/soc/intel/alderlake/lockdown.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/loc... PS3, Line 16: uint8_t
<stdint. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmc... File src/soc/intel/alderlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmc... PS3, Line 25: uint8_t
<stdint. […]
Ack
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 39: size_t
<stddef. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 123: uint8_t
<stdint. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 281: bool
<types. […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/smi... File src/soc/intel/alderlake/smihandler.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/smi... PS3, Line 3: console/console.h>
not used?
Ack
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.
Done. thanks
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/sou... File src/soc/intel/alderlake/soundwire.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/sou... PS3, Line 48: size_t
<stddef. […]
Ack