18 comments:
File src/soc/intel/alderlake/espi.c:
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
Patch Set #5, 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
Patch Set #5, 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
Patch Set #5, 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
Patch Set #5, 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
File src/soc/intel/alderlake/finalize.c:
uint32_t reg32;
uint8_t *pmcbase;
config_t *config;
uint8_t reg8;
Scope of `reg32` and `reg8` can be reduced
Ack
Patch Set #5, Line 65: SA_DEV_ROOT
I can't see it anywhere (got replaced by `config_of_soc`)
Done
/* 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.
File src/soc/intel/alderlake/fsp_params.c:
OC_SKIP
Ack
OC_SKIP
Ack
else... […]
Didn't get this Angel
Patch Set #5, Line 230: params->Device4Enable = config->Device4Enable;
Use `is_dev_enabled` with `SA_DEVFN_DPTF` instead?
Ack
File src/soc/intel/alderlake/include/soc/meminit.h:
Patch Set #5, Line 83: uint8_t
bool ?
Ack
File src/soc/intel/alderlake/pmc.c:
Patch Set #5, 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.
Patch Set #5, 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
File src/soc/intel/alderlake/pmutil.c:
* S5 because the PCH does not set the WAK_STS bit when waking
* from a true G3 state.
*/
nit: comment is misaligned
Ack
File src/soc/intel/alderlake/smmrelocate.c:
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 ?
Patch Set #5, 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
To view, visit change 45759. To unsubscribe, or for help writing mail filters, visit settings.