Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40112 )
Change subject: soc/intel/xeon_sp/cpx: add NUMA ACPI tables ......................................................................
Patch Set 12:
(13 comments)
Thanks for the review.
https://review.coreboot.org/c/coreboot/+/40112/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40112/9//COMMIT_MSG@9 PS9, Line 9: APCI
ACPI
Done
https://review.coreboot.org/c/coreboot/+/40112/9//COMMIT_MSG@10 PS9, Line 10: Enable turbo
Please format the items as a list.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/a... PS9, Line 194: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/a... PS9, Line 296: int
unsigned?
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/a... PS9, Line 313: current += 8+nodes*nodes;
Please add spaces around the operators.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/cpu.c:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/c... PS9, Line 31: * and CPUID.(EAX=1):EDX[14]==1 MCA*/
No leading asterisk, and please add a space in the end. […]
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/c... PS9, Line 33: if ((cpuid_regs.edx & (1<<7 | 1<<14)) != (1<<7 | 1<<14))
Please add spaces around the operators.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/c... PS9, Line 45: every bank. */
Is that copied from somewhere?
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/c... PS9, Line 186: * chip_info updated. Global chip_config is used as workaround
Please re-flow, and add a period/dot at the end of sentences.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... File src/soc/intel/xeon_sp/cpx/include/soc/acpi.h:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... PS9, Line 14: * GNU General Public License for more details.
Please use SPDX header.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... File src/soc/intel/xeon_sp/cpx/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... PS9, Line 14: * GNU General Public License for more details.
Use SPDX.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... File src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/i... PS9, Line 52: #define CBDMA_DEV_NUM 0x04
Align with tabs as above.
Done
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/s... File src/soc/intel/xeon_sp/cpx/soc_util.c:
https://review.coreboot.org/c/coreboot/+/40112/9/src/soc/intel/xeon_sp/cpx/s... PS9, Line 112: */
Put on line above?
Done