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 6:
(15 comments)
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... PS3, Line 242: /* CNVi */ : uint8_t CnviMode; : uint8_t CnviBtCore;
Why did these get removed?
using is_dev_enable directly
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);
Thank you! In the meantime, please mention that a bug has been filed (one `l`), and that this W/A sh […]
Ack
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/cpu... File src/soc/intel/alderlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/cpu... PS3, Line 150: /* TODO(adurbin): This should only be done on a cold boot. Also, some : * of these banks are core vs package scope. For now every CPU clears : * every bank. */
fun fact, AFAICT this TODO has been in the codebase for 7 years
yes indeed. 😊
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/esp... File src/soc/intel/alderlake/espi.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/esp... PS3, Line 132: u8
stdint. […]
please check patchset 6
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fin... File src/soc/intel/alderlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fin... PS3, Line 57: /* TODO: Add Thermal Configuration */
Is there still a reason that `pch_thermal_configuration()` isn't called here? (should we be calling […]
Eventually we will add those thermal configuration code, but right now we don't have any FF device hence add thermal configuration has put under TODO. hope that is okay.
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... PS3, Line 152: 0xff
OC_SKIP
Please check latets patch set. its done already 😊
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... PS3, Line 160: 0xff
OC_SKIP
Please check latets patch set. its done already 😊
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 219: else
If devicetree has enabled DITO config for this SATA port (config->SataPortsEnableDitoConfig[i]), it […]
Thanks Angel
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 27: 16
16 DIMM slots?
2 memory controller, each has 8 channel
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 38:
nit:tab
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 128: : /* FIXME: Rewrite loop below without this. */ : extern struct chip_operations drivers_intel_pmc_mux_ops; : : /* By default, TGL uses the PMC MUX for all ports, so port_number is unused */ : const struct device *soc_get_pmc_mux_device(int port_number) : { : const struct device *pmc; : struct device *child; : : child = NULL; : pmc = pcidev_path_on_root(PCH_DEVFN_PMC); : if (!pmc || !pmc->link_list) : return NULL; : : while ((child = dev_bus_each_child(pmc->link_list, child)) != NULL) : if (child->chip_ops == &drivers_intel_pmc_mux_ops) : break; : : /* child will either be the correct device or NULL if not found */ : return child; : }
Good news, this is being removed! CB:45747, feel free to rebase there or not and I'll clean up my me […]
delete it
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. */
You can try adding printk's when this code runs to see if they are initialized, then check those fie […]
Removed now as part of CB:45747 and will add into devicetree.cb
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 217: /* : * Check for any power failure to determine if this a wake from : * S5 because the PCH does not set the WAK_STS bit when waking : * from a true G3 state. : */
alignment of `*`
done as part of latest patchset
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 253: ps->tco1_sts, ps->tco2_sts);
can fit on previous line
Ack
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/smm... File src/soc/intel/alderlake/smmrelocate.c:
PS5:
Also, Arthur Heymans has done some cleanup already: https://review.coreboot. […]
yes, saw that, after ADL also merge, Arthur could fine tune this files as part of CB:45477