Attention is currently required from: Lance Zhao, Marc Jones, Martin Roth, Marshall Dawson, Paul Menzel, Rocky Phagura, Subrata Banik, Angel Pons, Patrick Rudolph. Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49286 )
Change subject: src/acpi: Add APEI EINJ support ......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49286/comment/b43fcad2_ba4fd323 PS1, Line 11: acpi_create_einj with an address where action table resides. The action table It would be more clear to explain that soc code should prepare the action table, and call acpi_create_einj() with the address of the action table.
https://review.coreboot.org/c/coreboot/+/49286/comment/fbcd7e20_9eb7de5e PS1, Line 13: E820 table. The action table must be shared memory between FW and OS. It should be marked as reserved in the E820 table.
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/49286/comment/0246a133_f6745c53 PS1, Line 790: for (i = 0; i < TRIGGER_ACTION_TABLES; i++) { Not all socs would define all the entries in action table. Either the size of action table, or the number of entries should be passed to this function.
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/49286/comment/2ea78b99_717f4751 PS1, Line 76: VFCT, NHLT, SPMI, CRAT, EINJ EINJ is not a proprietary table, we should move it up and follow alphabetic order.
https://review.coreboot.org/c/coreboot/+/49286/comment/15122650_e12a44c8 PS1, Line 930: #define NO_OP 0x04 please align the values to make it easier to read.
https://review.coreboot.org/c/coreboot/+/49286/comment/a8752850_b35a47f3 PS1, Line 961: acpi_einj_action_table_t trigger_action[4]; Here 4 is hardcoded. What is that number? Should we define a macro?