Attention is currently required from: Angel Pons, Arthur Heymans, Cliff Huang, Lance Zhao, Martin L Roth, Maximilian Brune, Nico Huber, Patrick Rudolph, Tim Wawrzynczak.
David Milosevic has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )
Change subject: acpi: Add PPTT support ......................................................................
Patch Set 22:
(4 comments)
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/6b8bc89c_3e51356e : PS17, Line 12: typedef
Plus, if a function's signature is too long, you can spread the parameter list of a function over several lines. If you have too many parameters, consider passing a pointer to a struct instead.
That's something I usually try to avoid. I have removed the typedefs now.
https://review.coreboot.org/c/coreboot/+/78071/comment/789dc5a5_29b922c9 : PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
Looks like https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_M... says that the "processor hierarchy node structure (Type 0) [...] can be used to describe a single processor or a group". Looks like the code doesn't make use of this yet?
If they mean clusters, current code allows you to create clusters. See https://review.coreboot.org/c/coreboot/+/79108/3 Otherwise, not sure what they mean by group.
There's a few things to keep in mind:
- The processor info needs to match that of MADT, not sure if MADT accounts for processor groups. - It is assumed that whoever defines acpi_get_pptt_topology() can figure out how to group the processors better than common code (e.g. soc/rockchip/rk3399 knows the chip has 2x Cortex-A72 and 4x Cortex-A53, common code doesn't).
Agreed.
If one processor has L1->L2->0 and another processor only has L2->0 (where 0 means NULL and L2 is the exact same data), one could try to reuse parts of the first processor's L1->L2-0 data for the second processor. This can also be done by comparing pointers and will work provided that acpi_get_pptt_topology() also reuses said pointers.
Assuming both of these processors are on the same topology level, then yes, one can reuse the already generated `L2->0` chain. That is also what the current code is doing. It saves previously generated cache entries (per topology level) in order to reuse them later for another processor, if needed.
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/78071/comment/c222d388_e3060fbf : PS21, Line 1405: PPTT_PROCESSOR_FLAG_*
Macros do not exist
Done
https://review.coreboot.org/c/coreboot/+/78071/comment/f5370641_4ed9d9bc : PS21, Line 1416: PPTT_CACHE_FLAG_*
Macros do not exist
Done
-
David Milosevic (Code Review)