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 5: Code-Review+1
(20 comments)
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 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);
so far yes, we have bug filled and mostly its early FSP hence scope for improvement is here
Thank you! In the meantime, please mention that a bug has been filed (one `l`), and that this W/A should be removed once fixed in FSP. For example:
TODO: a bug has been filed, remove this workaround once FSP is updated.
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... File src/soc/intel/alderlake/espi.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 27: */ This comment has been here since Cannonlake. Does it need an update?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 68: soc_setup_dmi_pcr_io_dec(&io_dec_arr[0]); I see this function being called from lpc_lib.c already. Do we need to write these twice?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 71: static void pch_enable_ioapic(const struct device *dev) This function is the same for all Intel SoCs except Broadwell
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 121: PCH_IRQ10 Is this one using IRQ10 for any particular reason? I see many other platforms do this too, but I don't know why.
Plus, this looks pretty much the same for other SoCs. If PIRQ routing is still necessary for some reason (which I would like to know about), this should be moved to common code. If not required, we might as well remove it as PIRQ shouldn't be necessary on modern systems.
If in doubt, we can move this to common code now, and maybe remove it later.
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 192: i8259_configure_irq_trigger(9, 1); Why these two numbers?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fin... File src/soc/intel/alderlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fin... PS5, Line 49: uint32_t reg32; : uint8_t *pmcbase; : config_t *config; : uint8_t reg8; Scope of `reg32` and `reg8` can be reduced
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fin... PS5, Line 65: SA_DEV_ROOT I can't see it anywhere (got replaced by `config_of_soc`)
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fin... PS5, Line 78: /* Disable XTAL shutdown qualification for low power idle. */ : if (config->s0ix_enable) { : reg32 = read32(pmcbase + CPPMVRIC); : reg32 |= XTALSDQDIS; : write32(pmcbase + CPPMVRIC, reg32); : } This seems to come from other SoCs, and its purpose is unclear. What does this do?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 152: 0xff OC_SKIP
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 160: 0xff OC_SKIP
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 219: else else... bug?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 230: params->Device4Enable = config->Device4Enable; Use `is_dev_enabled` with `SA_DEVFN_DPTF` instead?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/inc... PS5, Line 83: uint8_t bool ?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... File src/soc/intel/alderlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... PS5, Line 130: /* FIXME: Rewrite loop below without this. */ Are `dev->vendor` and `dev->device` initialized when `soc_get_pmc_mux_device` gets called?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... PS5, Line 133: /* By default, TGL uses the PMC MUX for all ports, so port_number is unused */ Hmmmmm... But this is not TGL
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmu... File src/soc/intel/alderlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmu... PS5, Line 220: * S5 because the PCH does not set the WAK_STS bit when waking : * from a true G3 state. : */ nit: comment is misaligned
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>
Done. […]
Thank you 😊
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/smm... File src/soc/intel/alderlake/smmrelocate.c:
PS5: How much of this is ADL-specific?
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/smm... PS5, Line 58: * According the BWG the IEDBASE MSR is in bits 63:32. It's This comment has been there since Haswell. Is it accurate?