Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel SkyLake Scalable Processor support ......................................................................
Patch Set 15:
(20 comments)
Thanks for the review. This is first batch of answers. A few of them to be answered.
https://review.coreboot.org/c/coreboot/+/38548/15/src/drivers/intel/fsp2_0/K... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/15/src/drivers/intel/fsp2_0/K... PS15, Line 59: SOC_INTEL_SKYLAKE_SP
Not yet
I do have a FSP 2.0 based SKYLAKE-SP FSP. It is an engineering build, Intel has no plan to make it public at the moment. This change is needed for me to build soc/intel/skylake_sp code and make TiogaPass image.
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/M... File src/soc/intel/skylake_sp/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/M... PS15, Line 23: subdirs-$(CONFIG_HAVE_SMI_HANDLER) += ../../../cpu/x86/smm
I'd move this line at the end of the block, as it's the only one that is conditional
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/iiostack.asl:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... PS15, Line 17: #define MAKE_IIO_DEV(id,rt) \
wait... […]
What's the question/ask?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/pci_irq.asl:
PS15:
Indenting could use some more tabs
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... PS15, Line 19:
Datasheet? EDS? BIOS spec? BWG?
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... PS15, Line 147: msr1
Is this intentional?
Not sure what the question is. please clarify.
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... PS15, Line 305: printk(BIOS_ERR, "MP initialization failure.\n");
Can we survive if MP init failed?
If you look at other soc code which call mp_init_with_smm(), they all have similar logic. If MP init failed, we should bail out. That may be left as a later exercise that updates all callers of mp_init_with_smm().
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 24: CPUID_SKYLAKESP_A0_A1
nit: add an underscore `_` to split SKYLAKESP: […]
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/gpio_fsp.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 86: (0x1 | (0x1 << 3)), ///< Set pad for both output and input
This looks a bit odd
Why is this odd? With GpioDirInOut, we set two bits, for both output and input.
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 24: H
Hm?
src/include/device/pci_ids.h defined PCI_DEVICE_ID_INTEL_SKL_H_AUDIO. SKL_H means Skylake CPU with PCH. Here using the same to keep consistency.
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/hob_iiouds.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 102: PciResourceIoLimit
poor things, they are flying away
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 149: CSI Link
CSI... […]
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/hob_memmap.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 249: SV_HOOKS
Is this used?
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 23: #define RDMSR(id) \
This could very well be a function returning the read msr
We need to print out the marco name. A function will not be able to accomplish that.
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 27: 08x
08?
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/ramstage.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 30: int dimm, int index);
should fit in 96 chars
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/soc_config.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 16:
double blank line
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 37:
double blank line
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/s... File src/soc/intel/skylake_sp/smihandler.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/s... PS15, Line 24: die("Not implemented");
Do we need to die here?
Done
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/u... File src/soc/intel/skylake_sp/upd_display.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/u... PS15, Line 24: old->field, new->field)
fits in 96 chars
Done