Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45751 )
Change subject: src/{include/acpi,mainboard}: Introduce CURRENT_DSDT_REV macro ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/45751/3/src/include/acpi/acpi.h File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/45751/3/src/include/acpi/acpi.h@44 PS3, Line 44: #define CURRENT_DSDT_REV 0x02 /* DSDT revision: ACPI v2.0 and greater */ I have two questions: 1. In many of the dsdt.asl files there is one additional statement in the comment: "...needs to be 2 for 64bit". With this change this comment is gone now. Is this something you are aware of? Shall we keep this information or is it that old now so that everybody is aware of that? 2. If you look a bit down this file you can see how the FADT table versions are handled:
/* FADT TABLE Revision values */ #define ACPI_FADT_REV_ACPI_1_0 1 #define ACPI_FADT_REV_ACPI_2_0 3 #define ACPI_FADT_REV_ACPI_3_0 4 #define ACPI_FADT_REV_ACPI_4_0 4 #define ACPI_FADT_REV_ACPI_5_0 5 #define ACPI_FADT_REV_ACPI_6_0 6
Would it make more sense to adapt this scheme so that in the future (if needed) a mainboard can select a different version? Or are you sure that the DSDT will have the same version among the complete tree all the time in future?
For instance we already have src/mainboard/razer/blade_stealth_kbl/dsdt.asl and /src/mainboard/51nb/x210/dsdt.asl with a DSDT version of 0x5.
At least I would like to see the typical "ACPI_" prefix in this define as usually done in this file.