Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40927 )
Change subject: soc/intel/xeon_sp/cpx: update ACPI xSDT ......................................................................
Patch Set 22:
(6 comments)
Thanks!
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 27: static void acpi_init_gnvs(global_nvs_t *gnvs)
I would move this code inside `acpi_create_gnvs`
Done
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... File src/soc/intel/xeon_sp/cpx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 36: PM1I, 64, // 0x15 - PM1 wake status bit : GPEI, 64, // 0x1D - GPE wake status bit : U2WE, 16, // 0x25 - USB2 Wake Enable Bitmap : U3WE, 8, // 0x27 - USB3 Wake Enable Bitmap
These four comments aren't indented with a tab
Done
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... File src/soc/intel/xeon_sp/cpx/acpi/iiostack.asl:
PS22:
Maybe you could do like the W83977F Super I/O, which relies on external #define values when includin […]
Good point. We are planning for another round of code base restructuring to solidify the xeon-sp code base. At that time, we will definitely take this into consideration: a. We merged in POC code for SKX-SP. b. We are merging in code for CPX-SP, with many improvements. c. With above, we will have two sets of xeon_sp support code. Our plan is to continue developing new features on top of CPX-SP code, while in parallel, unify and improve SKX-SP code and CPX-SP code as necessary. Community feedback like this is more than welcome.
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 59: /* indicate unrecognized UUID */ \
one tab too much?
Done
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... File src/soc/intel/xeon_sp/cpx/acpi/pci_irq.asl:
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 12: Add
Use ASL 2.0 syntax? […]
Done
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... File src/soc/intel/xeon_sp/cpx/acpi/uncore.asl:
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 27: Scope (_SB)
Shouldn't this be _SB instead?
Done