Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Ravindra, Sridhar Siricilla, Subrata Banik, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59359 )
Change subject: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core ......................................................................
Patch Set 3: Code-Review-1
(4 comments)
Patchset:
PS3: Having read this code I'm very much unconvinced of the code to count big/small core into a bitmask and using it in SSDT generated code.
I propose to add the cpu information in some way in struct device and generate simple code in dsdt/ssdt based on that. That is better than encoding it into a bitmask, passing it to the ssdt generator and then consuming it as a magic number inside generated ssdt code.
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59359/comment/05e7f1df_e42b5393 PS3, Line 102: acpi_get_cpu_hybrid_info(&cpu_hybrid_info); : acpigen_write_method(XPPC_PACKAGE_NAME, 1); : : if (cpu_is_nominal_freq_supported()) { : : /* : * If Nominal Frequency is supported, update Nominal Frequency and set core's : * Nominal Performance to a scaling factor which depends on core type(big or : * small). : */ : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 22, : cpu_hybrid_info.nominal_freq); : : /* LOCAL2_OP = (CORE >> core_id) & 1 */ : acpigen_emit_byte(SHIFT_RIGHT_OP); : acpigen_write_integer(cpu_hybrid_info.type_mask); : acpigen_emit_byte(ARG0_OP); : acpigen_emit_byte(LOCAL1_OP); : acpigen_write_and(LOCAL1_OP, ONE_OP, LOCAL2_OP); : : /* : * If core id is big Core, then set big core's scaling factor : * to core's nominal frequency otherwise set small core's scaling factor. : */ : acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.big_core_nom_perf); : acpigen_write_else(); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.small_core_nom_perf); : acpigen_pop_len(); : } : : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(CPPC_PACKAGE_NAME); : acpigen_pop_len(); Is it worth it to implement it fully in SSDT? It looks like you could implement it in DSDT and just fill in some namespace int variables in SSDT.
https://review.coreboot.org/c/coreboot/+/59359/comment/317853d3_92efaa37 PS3, Line 152: acpigen_write_dword(core_id); why not call XPPC using the actual information you want: big or small core instead of passing the core ID and then decoding that information. That bitmask thing will be very hard to read when decompiling the bytecode.
https://review.coreboot.org/c/coreboot/+/59359/comment/3d60b14d_0441d864 PS3, Line 98: void acpi_write_xppc_method(int core_id) : { : struct cpu_hybrid cpu_hybrid_info; : : acpi_get_cpu_hybrid_info(&cpu_hybrid_info); : acpigen_write_method(XPPC_PACKAGE_NAME, 1); : : if (cpu_is_nominal_freq_supported()) { : : /* : * If Nominal Frequency is supported, update Nominal Frequency and set core's : * Nominal Performance to a scaling factor which depends on core type(big or : * small). : */ : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 22, : cpu_hybrid_info.nominal_freq); : : /* LOCAL2_OP = (CORE >> core_id) & 1 */ : acpigen_emit_byte(SHIFT_RIGHT_OP); : acpigen_write_integer(cpu_hybrid_info.type_mask); : acpigen_emit_byte(ARG0_OP); : acpigen_emit_byte(LOCAL1_OP); : acpigen_write_and(LOCAL1_OP, ONE_OP, LOCAL2_OP); : : /* : * If core id is big Core, then set big core's scaling factor : * to core's nominal frequency otherwise set small core's scaling factor. : */ : acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.big_core_nom_perf); : acpigen_write_else(); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.small_core_nom_perf); : acpigen_pop_len(); : } : : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(CPPC_PACKAGE_NAME); : acpigen_pop_len(); : } : : void acpigen_write_CPPC_hybrid_method(int core_id) : { : char scope[16]; : : if (core_id == 0) : snprintf(scope, 12, XPPC_PACKAGE_NAME, 0); : else : snprintf(scope, 16, CONFIG_ACPI_CPU_STRING "." XPPC_PACKAGE_NAME, 0); : : acpigen_write_method("_CPC", 0); : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(scope); : acpigen_write_dword(core_id); : acpigen_pop_len(); I'm a bit confused. Each CPU has _CPC method and they all use the implementation called XPPC_PACKAGE_NAME of CPU0? Might just as well generate it for each CPU and drop that weird bitmask code altogether.