Attention is currently required from: Krishna Prabhakaran, Selma Bensaid, Meera Ravindranath, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52758 )
Change subject: drivers/intel/gma: Support IGD Opregion 2.1 ......................................................................
Patch Set 10:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52758/comment/21cd0172_78a93893 PS10, Line 12: incase in case
File src/drivers/intel/gma/Kconfig:
https://review.coreboot.org/c/coreboot/+/52758/comment/45c1167e_db1730ba PS10, Line 57: register (0xe0) rather than the SWSCI register (0xe8). Place new Kconfig options here.
https://review.coreboot.org/c/coreboot/+/52758/comment/7a189c9a_2c82f426 PS10, Line 149: config INTEL_IGD_OPREGION_VERSION This shouldn't be guarded by `if GFX_GMA`. I'd add the Kconfig options on line 57 (see comment there)
https://review.coreboot.org/c/coreboot/+/52758/comment/bd09c08d_c09e941a PS10, Line 151: default 0x200 Does the OpRegion version need to be specified in hex? I feel having specific Kconfig options for the versions would be cleaner:
config INTEL_GMA_OPREGION_2_0 bool default n if INTEL_GMA_OPREGION_2_1 default y
config INTEL_GMA_OPREGION_2_1 bool default n
Then, make a helper function to return the version number:
static u32 opregion_version(void) { if (CONFIG(INTEL_GMA_OPREGION_2_0)) return 0x200;
if (CONFIG(INTEL_GMA_OPREGION_2_1)) return 0x201;
return dead_code_t(u32); }
By writing the function this way, GCC can automatically inline it, as it either returns an integer constant or `dead_code_t` forces a build failure. #include <assert.h> to use the `dead_code_t` macro.
File src/drivers/intel/gma/opregion.c:
https://review.coreboot.org/c/coreboot/+/52758/comment/982207dc_f904afa4 PS10, Line 362: CONFIG_INTEL_IGD_OPREGION_VERSION This now has a different value. Looks like it's not intentional?
Before: 2 << 24 == 0x02000000 After: 0x200