Angel Pons 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: Code-Review+1
(5 comments)
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`
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
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 including the ACPI code for the UARTs. CB:41530 shows the relevant file. Not sure if it would look nicer than the current approach though.
https://review.coreboot.org/c/coreboot/+/40927/22/src/soc/intel/xeon_sp/cpx/... PS22, Line 59: /* indicate unrecognized UUID */ \ one tab too much?
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?
OperationRegion (ITSS, SystemMemory, PCR_ITSS_PIRQA_ROUT + CONFIG_PCR_BASE_ADDRESS + (PID_ITSS << PCR_PORTID_SHIFT), 8)