Attention is currently required from: Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Paul Menzel, TangYiwei, Tim Chu.
Shuo Liu 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/92610315_17311d3f : PS53, Line 16: Emmisburg
Emmi*t*sburg
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/57311617_21286dda : PS53, Line 20:
Please list the datasheet(s) with name and revision.
We have to add this after SoC launch since it is shift-left opensource at the moment. I updated the commit message to clarify this,
This patch initially sets the code set up as a build target with Granite Rapids N-1 FSP (src/vc/intel/fsp/fsp2_0/ graniterapids).
https://review.coreboot.org/c/coreboot/+/81316/comment/398612ec_d9e30c24 : PS53, Line 21: TEST=Build and boot on intel/archercity CRB
Does Archer City also support Granite Rapdis? […]
Sorry, here the test means this patch doesn't impact archercity CRB. P.S. archercity CRB doesn't support GNR. I removed the TEST= line.
For CBMEM timestamps, we have to add it after SoC launch (now it is shift-left opensource but only pass build with N-1 FSP).
File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/81316/comment/ace9c878_1d5a43f6 : 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.
Done
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/fae8b3ec_dd9e4275 : PS53, Line 28: uint8_t
unsigned int […]
Interesting. Thanks! Updated.
https://review.coreboot.org/c/coreboot/+/81316/comment/5e83390f_cf52a62c : PS53, Line 57: IORESOURCE_SUBTRACTIVE
Is this actually a subtractive resource? I think various platforms in coreboot get this wrong. […]
I agree with you. Updated. This cannot be counted as subtractive decoding. I checked the lpc driver which reports the same set of resources in src/soc/intel/common/block/lpc/lpc.c, the subtractive tag is not used either.
P.S. If the platform is with an lpc driver loaded, the reporting here maybe be abled to be removed. however, let me keep it now and handle this in a separate patch/discussion (e.g. do we have a strong needs for that?)
https://review.coreboot.org/c/coreboot/+/81316/comment/d8fe7e05_dfac49ea : PS53, Line 93: uint8_t
Ditto.
Done
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/5bebec3a_0a8352ab : PS53, Line 20: GraniteRapids
Granite Rapids
Done
File src/soc/intel/xeon_sp/gnr/chip.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/668a11f4_2b6808df : PS53, Line 56: fsp_silicon_init();
Should be close to the printk() statement referring to this?
Done
File src/soc/intel/xeon_sp/gnr/chipset.cb:
https://review.coreboot.org/c/coreboot/+/81316/comment/5bbc4ddd_28d44e83 : PS53, Line 19: HEPT
typo: HPET
Done
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/9272e4aa_65260750 : PS53, Line 33: //FIXME:
Please add a space after //.
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/46066c81_75a073a0 : 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.
Oh sorry, this is originally forked from SPR codes and GNR do not need these comments. Updated.
https://review.coreboot.org/c/coreboot/+/81316/comment/0f32db08_5a1c5ae1 : 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 }?
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/56f62cb2_4c3faaf4 : PS53, Line 87: return num_virts * soc_get_num_cpus();
So `num_phys` isn’t actually needed?
This API calculates the thread counts which correspond to num_virts (logical cores), while num_phys is corresponding to physical cores (in SMT system, one physical cores has multiple threads, a.k.a. logical cores).
https://review.coreboot.org/c/coreboot/+/81316/comment/5805560f_76f22545 : PS53, Line 106: cpu
CPU
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/499e35a1_510addaf : PS53, Line 106: cpu
CPU
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/9766c75c_6c4e915a : PS53, Line 114: printk(BIOS_ERR, "microcode not found in CBFS!\n");
What is the consequence of this error?
It will caused the subsequent call to skip the microcode loading. When BIOS doesn't have a microcode with it, CPU will boot with its default microcode.
void intel_microcode_load_unlocked(const void *microcode_patch) { u32 current_rev; const struct microcode *m = microcode_patch;
if (!m) { printk(BIOS_WARNING, "microcode: failed because no ucode was found\n"); return; } ... }
https://review.coreboot.org/c/coreboot/+/81316/comment/7dbd878c_cf5485e9 : PS53, Line 116: intel_microcode_load_unlocked(microcode_patch);
Can this be called with NULL?
Yes.
https://review.coreboot.org/c/coreboot/+/81316/comment/06ac5908_f0339891 : PS53, Line 119: printk(BIOS_ERR, "MP initialization failure.\n");
Print the return value? Can the system still function with this failing?
Done
File src/soc/intel/xeon_sp/gnr/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/b561778a_6730642d : PS53, Line 6: #define CPUID_GRANITERAPIDS 0xA06D0
Use tabs for alignment?
Done
File src/soc/intel/xeon_sp/gnr/include/soc/vpd.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/1efc34e2_c2105d0e : 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?
Fast boot will use saved silicon init data to skip calculating these settings (e.g. MRC is skipped) so that to boot fast. I will make this a separate patch to make this clear.
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/3c22fa31_d790d7f9 : PS53, Line 43: case 0x4000000: // 64M
Use the macros instead of comments?
Done
File src/soc/intel/xeon_sp/gnr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/4a009029_ae210682 : PS53, Line 19: if (val & (1 << offset)) { : return MP_IRQ_POLARITY_LOW; : } else { : return MP_IRQ_POLARITY_HIGH; : }
I’d use the ternary operator.
Done
File src/soc/intel/xeon_sp/gnr/soc_util.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/bbf26cdb_e02185d5 : PS53, Line 131: uint8_t
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/5e3c350a_7541f32f : PS53, Line 139: return 0xff;
Why pass the arguments, if they are not needed? Why aren’t they unsigned? Add some kind of comment
I will remove this API at all. Updated.
File src/soc/intel/xeon_sp/include/soc/acpi.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/424226ef_3b4a9926 : PS53, Line 21: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current);
Do this in a separate patch?
We have to keep the declaration to pass build, however, I remove the null implementation,
unsigned long acpi_fill_cedt(unsigned long current) { return current; }
File src/soc/intel/xeon_sp/include/soc/fsp_upd.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/2481b650_74e50df3 : PS53, Line 15: * GNR needs below macro to be in effect before any FSP header references
Why? Please reference the source.
N-1 FSP header do not need this, I removed it at the moment. Will update when the real FSP header is released.
File src/soc/intel/xeon_sp/lockdown.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/f8b58b77_a7e8613c : PS53, Line 25: return;
Do this in a separate patch?
Done