Attention is currently required from: Tarun Tuli, Subrata Banik, Paul Menzel. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63969 )
Change subject: soc/intel/alderlake: provide a list of D-states to enter LPM ......................................................................
Patch Set 13:
(9 comments)
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/ac6a5041_fe0b707a PS12, Line 179: min_pci_d_states
Yes we could instead add statements for each device to populate at run time if we feel that improv […]
To me, I think it makes more sense to store a map of PCI device/path to its required D-state, e.g.
``` const static struct { unsigned int pci_dev; unsigned int d_state; } min_pci_d_states[] = { { SA_DEVFN_ROOT, ACPI_DEVICE_SLEEP_D3 }, { SA_DEVFN_CPU_PCIE1_0, ACPI_DEVICE_SLEEP_D3 }, { SA_DEVFN_IGD, ACPI_DEVICE_SLEEP_D3 }, }; ``` etc.
https://review.coreboot.org/c/coreboot/+/63969/comment/588664be_8350c115 PS12, Line 470:
For clarification, we basically won't emit anything if the device is not enabled. […]
soc_lpi_include_device() already includes a check for that.
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/05cbaca3_56e9e5df PS13, Line 23: #define CPU_PATH_PREFIX "\_SB.CP" See `CONFIG_ACPI_CPU_STRING`
https://review.coreboot.org/c/coreboot/+/63969/comment/9ef4b2d3_3e3b43e1 PS13, Line 116: typedef enum { : D0, /* 0 */ : D1, /* 1 */ : D2, /* 2 */ : D3, /* 3 */ : UNDEF : } D_STATES; In coreboot, we don't usually use a lot of typedefs, and not all capitalized names, either so
``` enum d_states { D0, /* 0 */ D1, /* 1 */ D2, /* 2 */ D3, /* 3 */ UNDEF }; ```
But also see the existing ACPI_DEVICE_SLEEP_D0, etc., would they work here?
https://review.coreboot.org/c/coreboot/+/63969/comment/b38a22c4_73829610 PS13, Line 440: if (dev && dev->enabled) { : switch (dev->path.type) { : case DEVICE_PATH_PCI: : return (min_pci_d_states[dev->path.pci.devfn] != UNDEF); : : case DEVICE_PATH_APIC: : return true; : : default: : return false; : } : } : return false; if you take my suggestion from above, this simplifies to: ``` if (!dev || !dev->enabled) return false;
return dev->path.type == DEVICE_PATH_PCI || dev->path.type == DEVICE_PATH_APIC); ```
https://review.coreboot.org/c/coreboot/+/63969/comment/8c511dec_479cf89e PS13, Line 455: extern int cpu_get_apic_id(int logical_cpu); `#include <cpu/cpu.h>`
https://review.coreboot.org/c/coreboot/+/63969/comment/abc813c2_2363195e PS13, Line 499: default: : /* Unhandled */ : acpigen_emit_namestring(NULL); : break; : } Probably print an error instead? soc_lpi_include_device() should take care of this case already.
https://review.coreboot.org/c/coreboot/+/63969/comment/f37e3634_2df4c714 PS13, Line 505: 1 symbolic constants (e.g. `#define BLAH 1`) would be helpful for readability, and reduce the need for comments
https://review.coreboot.org/c/coreboot/+/63969/comment/3576fd6b_8f7ecd18 PS13, Line 521: D0; nit: add `#define DEFAULT_CPU_D_STATE D0`