Attention is currently required from: Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Felix Singer, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Shuo Liu, TangYiwei, Tim Chu.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81316?usp=email )
Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes ......................................................................
Patch Set 53:
(28 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81316/comment/072f41a7_98aca666 : PS53, Line 16: Emmisburg Emmi*t*sburg
https://review.coreboot.org/c/coreboot/+/81316/comment/bee45b4e_98b4a6d9 : PS53, Line 20: Please list the datasheet(s) with name and revision.
https://review.coreboot.org/c/coreboot/+/81316/comment/c96da4fc_5c348547 : PS53, Line 21: TEST=Build and boot on intel/archercity CRB Does Archer City also support Granite Rapdis?
Please also paste the CBMEM timestamps. For example, microcode updates are not done in parallel.
File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/81316/comment/92c64a7a_4f6dc672 : PS53, Line 954: F: src/vendorcode/intel/fsp/fsp2_0/graniterapids/ Please mention this in the commit message, or even make it a separate commit.
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/af34716f_cbcbd160 : PS53, Line 28: uint8_t unsigned int
https://notabs.org/coding/smallIntsBigPenalty.htm
https://review.coreboot.org/c/coreboot/+/81316/comment/1d668ad9_2fab5851 : PS53, Line 38: int unsigned int?
https://review.coreboot.org/c/coreboot/+/81316/comment/cc9a2dc3_5224f16f : PS53, Line 52: HOB Could the HOB behavior change? If so, mention the version?
Instead of a comment, you could also make it a info or debug level log message.
https://review.coreboot.org/c/coreboot/+/81316/comment/af65df02_c57f65ef : PS53, Line 93: uint8_t Ditto.
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/bc7289bc_636cb35e : PS53, Line 20: GraniteRapids Granite Rapids
File src/soc/intel/xeon_sp/gnr/chip.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/ba879a88_ecd4e148 : PS53, Line 56: fsp_silicon_init(); Should be close to the printk() statement referring to this?
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/ee3cf4b5_f9e98a67 : PS53, Line 33: //FIXME: Please add a space after //.
https://review.coreboot.org/c/coreboot/+/81316/comment/aaad19d3_50e07d1d : PS53, Line 39: Loading MCU on AP in parallel seems to fail in 10% of the cases : * so do it serialized. Please add a reference to the corresponding bug report.
https://review.coreboot.org/c/coreboot/+/81316/comment/6a4004fc_a27d4bc4 : PS53, Line 62: {X86_VENDOR_INTEL, CPUID_GRANITERAPIDS, CPUID_ALL_STEPPINGS_MASK}, : {X86_VENDOR_INTEL, CPUID_SIERRAFOREST, CPUID_ALL_STEPPINGS_MASK}, Add space after { and before }?
https://review.coreboot.org/c/coreboot/+/81316/comment/5818b9b6_a2494361 : PS53, Line 87: return num_virts * soc_get_num_cpus(); So `num_phys` isn’t actually needed?
https://review.coreboot.org/c/coreboot/+/81316/comment/0e8f8cfe_b1df3cb7 : PS53, Line 106: cpu CPU
https://review.coreboot.org/c/coreboot/+/81316/comment/d79e3dbf_f2014b59 : PS53, Line 106: cpu CPU
https://review.coreboot.org/c/coreboot/+/81316/comment/c2cc2194_a60453a6 : PS53, Line 114: printk(BIOS_ERR, "microcode not found in CBFS!\n"); What is the consequence of this error?
https://review.coreboot.org/c/coreboot/+/81316/comment/e8461203_00c91900 : PS53, Line 116: intel_microcode_load_unlocked(microcode_patch); Can this be called with NULL?
https://review.coreboot.org/c/coreboot/+/81316/comment/40370ae1_0fc629fb : PS53, Line 119: printk(BIOS_ERR, "MP initialization failure.\n"); Print the return value? Can the system still function with this failing?
File src/soc/intel/xeon_sp/gnr/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/74d22a90_1d4dc6aa : PS53, Line 6: #define CPUID_GRANITERAPIDS 0xA06D0 Use tabs for alignment?
File src/soc/intel/xeon_sp/gnr/include/soc/vpd.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/bd3414af_d99d8342 : PS53, Line 6: #define FAST_BOOT_EN "fast_boot_en" /* 1 or 0: enable or disable fast boot for warm/cold reset */ What is fast boot exactly?
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/759e722c_c9e486fc : PS53, Line 43: case 0x4000000: // 64M Use the macros instead of comments?
File src/soc/intel/xeon_sp/gnr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/c94e3f0a_f66d802e : PS53, Line 19: if (val & (1 << offset)) { : return MP_IRQ_POLARITY_LOW; : } else { : return MP_IRQ_POLARITY_HIGH; : } I’d use the ternary operator.
File src/soc/intel/xeon_sp/gnr/soc_util.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/7f904a48_516de9d8 : PS53, Line 131: uint8_t unsigned int
https://review.coreboot.org/c/coreboot/+/81316/comment/3ddb0774_ddc12550 : PS53, Line 139: return 0xff; Why pass the arguments, if they are not needed? Why aren’t they unsigned? Add some kind of comment
File src/soc/intel/xeon_sp/include/soc/acpi.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/395cde56_8f82720c : PS53, Line 21: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current); Do this in a separate patch?
File src/soc/intel/xeon_sp/include/soc/fsp_upd.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/94b2d554_f4233a31 : PS53, Line 15: * GNR needs below macro to be in effect before any FSP header references Why? Please reference the source.
File src/soc/intel/xeon_sp/lockdown.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/71745bba_ad76e1e1 : PS53, Line 25: return; Do this in a separate patch?