David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38549 )
Change subject: mainboard/ocp: Add support for OCP platform TiogaPass ......................................................................
Patch Set 38: Code-Review-1
(28 comments)
Mostly small stuff. The main thing is to drop the dummy IFD and ME files by removing HAVE_IFD_BIN and HAVE_ME_BIN from src/mainboard/ocp/tiogapass/Kconfig.
https://review.coreboot.org/c/coreboot/+/38549/38/configs/config.ocp_tiogapa... File configs/config.ocp_tiogapass:
https://review.coreboot.org/c/coreboot/+/38549/38/configs/config.ocp_tiogapa... PS38, Line 3: CONFIG_CPU_MICROCODE_CBFS_LOC=0xfff0fdc0 : CONFIG_CPU_MICROCODE_CBFS_LEN=0x7C00 No need to specify these, they are the defaults as per src/soc/intel/xeon_sp/Kconfig
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/Kconfig:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 25: select HAVE_IFD_BIN : select HAVE_ME_BIN Remove these and coreboot will not try to add an IFD or ME image. Then we can also remove those fake binary files.
For real builds, the user can either enable these with a pre-made .config file or `make menuconfig and Chipset --> Add Intel descriptor.bin file (which will also allow an ME path to be specified)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 18: romstage-y += boardid.c remove (see comment in tiogapass_boardid.h)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 21: ramstage-y += boardid.c same as previous comment
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/acpi/platform.asl:
PS38: Please indent this file uniformly using tabs.
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 20: #include <soc/nvs.h> nit: this is not needed
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/boardid.c:
PS38: This is currently unused, let's get rid of it for now and reintroduce it later if needed.
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/devicetree.cb:
PS38: Please indent this file uniformly using tabs.
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/romstage.c:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 18: #include "tiogapass_boardid.h" remove (see comment in tiogapass_boardid.h)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 20: #include <fsp/soc_binding.h> unnecessary #include
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 21: #include <FspmUpd.h> #include <soc/romstage.h>
(see comment below about mainboard_* function prototypes)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 26: void mainboard_config_gpios(FSPM_UPD * mupd); : void mainboard_config_iio(FSPM_UPD *mupd); : void mainboard_memory_init_params(FSPM_UPD *mupd); I'm surprised that the compiler doesn't err out on these... mainboard_config_gpios() and mainboard_memory_init_params() are defined in <soc/romstage.h> (part of the previous patch)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 33: void mainboard_config_gpios(FSPM_UPD *mupd) I think this is only called from mainboard_memory_init_params() in this file, so it can be static.
Also, there is a function declaration in the SoC's romstage.h file (in the previous patch) that should be removed.
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 40: void mainboard_config_iio(FSPM_UPD *mupd) this can be static since it's only called from mainboard_memory_init_params()
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 45: sizeof(tp_iio_bifur_table)/sizeof(UPD_IIO_BIFURCATION_DATA_ENTRY); ARRAY_SIZE(tp_iio_bifur_table)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 50: sizeof(tp_iio_pci_port_skt0)/sizeof(UPD_PCI_PORT_CONFIG); ARRAY_SIZE(tp_iio_pci_port_skt0);
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 55: sizeof(tp_pch_pci_port_skt0)/sizeof(UPD_PCH_PCIE_PORT); ARRAY_SIZE(tp_pch_pci_port_skt0);
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/skxsp_tp_gpio.h:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 24: * nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 26: * nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 1020: Needs an #endif
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/skxsp_tp_iio.h:
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 24: /** nit: Use /* (without the extra asterix)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 30: **/ Nit: Use */ (without the extra asterix)
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 44: /** Nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 46: **/ Nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 48: indent
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 91: /** Nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 93: **/ Nit: extra asterix
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... File src/mainboard/ocp/tiogapass/tiogapass_boardid.h:
PS38: This seems copy+pasted from harcuvar. Let's get rid of it for now, though we may introduce it in a more proper form if we need logic to tell apart board revisions / vendors for whatever reason.