[L] Change in coreboot[master]: soc/intel/common, alderlake: provide a list of D-states to enter LPM
Attention is currently required from: Tarun Tuli, Subrata Banik, Kapil Porwal.
Eran Mitrani has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70166 )
Change subject: soc/intel/common, alderlake: provide a list of D-states to enter LPM ......................................................................
Patch Set 7:
(13 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/70166/comment/12dedd94_dc40a586 PS3, Line 7: soc/intel/common
and alderlake
Done
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/70166/comment/8a7eb829_0f52d391 PS3, Line 225: : min_sleep_state_t **get_min_sleep_state_array(int *size) : { : *size = ARRAY_SIZE(min_pci_sleep_states); : return &min_pci_sleep_states; : }
nit: you cab avoid having double point (IMO, we don't need) […]
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/b1f8d2f9_3b42d1b7 PS3, Line 233: void soc_lpi_get_constraints(void *unused) : { : acpi_generate_lpi_constraint_table(); : }
Can't we remove this method itself?
No.. It's implemented differently for previous generations in src/soc/intel/common/block/acpi/pep.c
File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/70166/comment/a6979589_4277f7c0 PS3, Line 29: #define LPI_REVISION 0 : #define LPI_ENABLED 1
may be enum?
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/494c22d7_18a17967 PS3, Line 440: **
__weak struct min_sleep_state *get_min_sleep_state_array(size_t *size) […]
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/3145e674_9d88a407 PS3, Line 443: return NULL;
set size to 0 as we are returning NULL.
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/caf62f12_b00f89f3 PS3, Line 448: min_sleep_state_t **min_pci_sleep_states;
size_t size; […]
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/9aabd4c4_485579d4 PS3, Line 450: min_pci_sleep_states
shouldn't we do a NULL check here ?
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/8d910adb_74c1dd50 PS3, Line 464: *min_pci_sleep_states)[i].pci_dev
state[i]. […]
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/ef0fca31_ecdd4d92 PS3, Line 465: *min_pci_sleep_states)[i]
state[i]. […]
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/89178ffb_6d025adc PS3, Line 486:
Need to handle a case when `num_entries` is 0 here.
Done
https://review.coreboot.org/c/coreboot/+/70166/comment/cf5a79cb_e5a220e4 PS3, Line 537: printk(BIOS_INFO, "Returning SoC specific constraint package for %d devices\n", num_entries);
u can drop this
Done
File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/70166/comment/c69b94a6_7c7a1eba PS3, Line 138: typedef
I would avoid doing typedef
Done
-
Eran Mitrani (Code Review)