David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel Xeon Scalable Processor support ......................................................................
Patch Set 50: Code-Review-1
(13 comments)
Almost there!
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi/pci_irq.asl:
PS50: please indent with tabs
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi/uncore.asl:
PS50: please indent uniformly with tabs
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... File src/soc/intel/xeon_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 165: npr == 0 npr == NULL
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 166: assign_resource_to_stack add_res_to_stack()
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 197: bool first = 1 int first? Or if you prefer to keep it as a bool, use true/false instead of 1/0.
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 284: continue; Join lines 281-283 so that the parens terminate after the condition. Right now it looks like `continue` is left out of the if-statement (I had to open up my editor to be sure there wasn't a bug...).
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 317: 0 NULL
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 512: 0 NULL
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 516: 0 NULL
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 518: struct bus nit: use the object's size in case the type ever changes, i.e. sizeof(*iiostack_bus)
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 519: struct bus ditto
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... File src/soc/intel/xeon_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... PS50, Line 92: * nit: indentation
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... PS50, Line 188: nit: extra line