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.
28 comments:
Commit Message:
Patch Set #53, Line 16: Emmisburg
Emmi*t*sburg
Please list the datasheet(s) with name and revision.
Patch Set #53, 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:
Patch Set #53, 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:
Patch Set #53, Line 28: uint8_t
unsigned int
https://notabs.org/coding/smallIntsBigPenalty.htm
unsigned int?
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.
Patch Set #53, Line 93: uint8_t
Ditto.
File src/soc/intel/xeon_sp/gnr/Kconfig:
Patch Set #53, Line 20: GraniteRapids
Granite Rapids
File src/soc/intel/xeon_sp/gnr/chip.c:
Patch Set #53, Line 56: fsp_silicon_init();
Should be close to the printk() statement referring to this?
File src/soc/intel/xeon_sp/gnr/cpu.c:
Patch Set #53, Line 33: //FIXME:
Please add a space after //.
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.
{X86_VENDOR_INTEL, CPUID_GRANITERAPIDS, CPUID_ALL_STEPPINGS_MASK},
{X86_VENDOR_INTEL, CPUID_SIERRAFOREST, CPUID_ALL_STEPPINGS_MASK},
Add space after { and before }?
Patch Set #53, Line 87: return num_virts * soc_get_num_cpus();
So `num_phys` isn’t actually needed?
CPU
CPU
Patch Set #53, Line 114: printk(BIOS_ERR, "microcode not found in CBFS!\n");
What is the consequence of this error?
Patch Set #53, Line 116: intel_microcode_load_unlocked(microcode_patch);
Can this be called with NULL?
Patch Set #53, 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:
Patch Set #53, Line 6: #define CPUID_GRANITERAPIDS 0xA06D0
Use tabs for alignment?
File src/soc/intel/xeon_sp/gnr/include/soc/vpd.h:
Patch Set #53, 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:
Patch Set #53, Line 43: case 0x4000000: // 64M
Use the macros instead of comments?
File src/soc/intel/xeon_sp/gnr/soc_acpi.c:
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:
Patch Set #53, Line 131: uint8_t
unsigned int
Patch Set #53, 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:
Patch Set #53, 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:
Patch Set #53, 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:
Patch Set #53, Line 25: return;
Do this in a separate patch?
To view, visit change 81316. To unsubscribe, or for help writing mail filters, visit settings.