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 5:
(18 comments)
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. […]
Its same Angel. its 32MB in size
#define PCH_PRESERVED_BASE_ADDRESS 0xFC800000 ///< Pch preserved MMIO base address #define PCH_PRESERVED_MMIO_SIZE 0x02000000 ///< 32MB
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. […]
Thank u, very valid point
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
Good candidate for common code migration
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.
i could always see INTB is IRQ10 and rest of IRQ11
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.
yes good candidate for common code migration
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?
9 refer as interrupt number and 1 refer as level trigger for 8259
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
Ack
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`)
Done
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. […]
this is to disqualify XTAL power while measuring low power KPI as per PnP team requirement.
Good candidate for common code.
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
Ack
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 160: 0xff
OC_SKIP
Ack
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 219: else
else... […]
Didn't get this Angel
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?
Ack
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 ?
Ack
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?
Answer should be yes as this is getting called from ec_lpc.c as part of acpi_fill_ssdt.
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... […]
my bad, TGL and ADL should use the same PMC MUX programming for Type-C
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
Ack
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?
Honestly this file a good candidate for common code, many function can be moved into common, i believe i had a CL in past from common code (2017/18) time didn't merge for some reason, i can again revive the same post ADL upstream. Thoughts ?
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. […]
Yes.
SMBASE is between 0:31 and IEDBASE is 32:64