Attention is currently required from: Tim Wawrzynczak, Michael Niewöhner, Patrick Rudolph, Karthik Ramasubramanian. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56005 )
Change subject: soc/intel/common/block/acpi: Add LPM requirements support to PEPD _DSM ......................................................................
Patch Set 3:
(4 comments)
File src/soc/intel/common/block/acpi/pep.c:
https://review.coreboot.org/c/coreboot/+/56005/comment/ac4be9c9_f7b34c2e PS3, Line 37: LPM_STATES If I understand this correctly, the LPM_STATES here refer to the S0ix states (S0i2.0, S0i2.1...S0i3.4) as outlined in `LPM_EN` register in PCH EDS vol2.
A few questions: 1. On TGL, we had set FSP UPD to decide what S0ix sub-states should be enabled. For ADL, we are leaving that UPD at default. Don't we need to configure those for brya?
2. Depending upon what S0ix sub-states are actually enabled, can we skip the requirements for any sub-states that are disabled by default? We can probably check LPM_EN state to determine which states need to collect the requirements.
3. Looks like this requirement collection is dependent on S0ix sub-states supported and enabled by the SoC and platform. In that case, shouldn't there be appropriate callbacks to get this information from the SoC rather than assuming the LPM information?
https://review.coreboot.org/c/coreboot/+/56005/comment/b3de49b5_db33cde3 PS3, Line 39: PMC_LPM_REQ_BASE + i * LPM_REQ_REG_STRIDE + : j * sizeof(uint32_t) I wonder if we need something like a `struct pmc_lpm` which provides information about:
1. num_substates /* Num of S0ix sub-states */ 2. num_status_regs /* Num of LPM status registers for each sub-state */ 3. lpm_ipc_offset /* Offset of LPM registers for PMC IPC */ 4. status_reg_stride /* Register stride for each sub-state */
This will allow it to be defined differently for each SoC.
https://review.coreboot.org/c/coreboot/+/56005/comment/2e52e17f_a4e7184f PS3, Line 45: pmc_send_ipc_cmd Just curious: How much time does this function take since it is making multiple calls to the PMC?
Also, have we ensured that we skip this in FSP so that we don't have to bear the penalty of doing the operations twice?
https://review.coreboot.org/c/coreboot/+/56005/comment/cb3905e4_dec7217f PS3, Line 197: DSM_UUID(PEP_S0IX_UUID, pep_s0ix, ARRAY_SIZE(pep_s0ix), (void *)reg), nit: Is there value in exposing this _DSM with 0 supported functions if !SOC_INTEL_COMMON_BLOCK_ACPI_PEP_LPM_REQ?
What do you think about replacing `acpigen_write_dsm_uuid_arr` below with:
``` acpigen_write_dsm(LPI_S0_HELPER_UUID, lpi_s0_helpers, ARRAY_SIZE(lpi_s0_helpers), NULL); if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_PEP_LPM_REQ)) { void *reg = read_pmc_lpm_requirements(); acpigen_write_dsm(PEP_S0IX_UUID, pep_s0ix, ARRAY_SIZE(pep_s0ix), reg); } ```