Attention is currently required from: Cliff Huang, Furquan Shaikh, Angel Pons, Michael Niewöhner, Patrick Rudolph, Karthik Ramasubramanian. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56004 )
Change subject: soc/intel/common/block/acpi: Move pep.asl to acpigen ......................................................................
Patch Set 2:
(6 comments)
File src/soc/intel/common/block/acpi/Kconfig:
https://review.coreboot.org/c/coreboot/+/56004/comment/937d6428_19cf0510 PS1, Line 18: config SOC_INTEL_COMMON_BLOCK_ACPI_PEP
I'm not sure if I follow. […]
The files are related, this patch train replaces pep.asl with pep.c (converting all of the ASL to runtime generated AML in the SSDT).
pep.asl is currently used by: ADL, CNL, EKL, JSL, SKL and TGL. This patch train intends to make all of the required changes to make pep.asl obsolete. Please see the follower patches to note that each SoC adds the Kconfig and then called generate_acpi_power_engine in their PMC ACPI generation code.
File src/soc/intel/common/block/acpi/pep.c:
https://review.coreboot.org/c/coreboot/+/56004/comment/10104db1_a2af773c PS1, Line 25: 0x63
How about: […]
Done
https://review.coreboot.org/c/coreboot/+/56004/comment/387abdea_e6580c06 PS1, Line 33: static void lpi_get_constraints(void *unused)
please, no spaces at the start of a line […]
Done
https://review.coreboot.org/c/coreboot/+/56004/comment/d4bcc91d_b2075cce PS1, Line 40: "\NULL"
I vaguely recall seeing some ASL that used the host bridge device here. […]
Right now, this is (aside from renaming \DUMY to \NULL), a copying of ASL to AML generation. The ASL files uses a non-existent device called \DUMY right now (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...). If we want to update this to do something different, how about a different patch?
However, do you mean referencing _SB.PCI0.MCHC instead of a non-existent device?
https://review.coreboot.org/c/coreboot/+/56004/comment/50a468da_7092844d PS1, Line 37: acpigen_emit_byte(RETURN_OP); : acpigen_write_package(1); : acpigen_write_package(3); : acpigen_emit_namestring("\NULL"); /* non-existent */ : acpigen_write_integer(0); /* disabled, no constraints */ : acpigen_write_package(2); : acpigen_write_integer(0); /* revision */ : acpigen_write_package(2); : acpigen_write_integer(0xff); /* apply to all LPIT states */ : acpigen_write_integer(0); /* minimum D-state */ : acpigen_write_package_end(); /* inner 3 */ : acpigen_write_package_end(); /* inner 2 */ : acpigen_write_package_end(); /* inner 1 */ : acpigen_write_package_end(); /* outer */
How about using scopes to show what closes what? […]
I like it, done!
File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/56004/comment/be18158b_a992f0e6 PS1, Line 94: void generate_acpi_power_engine(const struct device *dev);
No. Please never guard function declarations with preprocessor. […]
Cliff, this is how we typically handle things in coreboot, trying to actively minimize `#if` and friends. The compiler and linker are very good at removing dead code, which we can do with a regular `if (CONFIG(...))` statement.