Jonathan Zhang 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:
(29 comments)
Thanks!
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
Done
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. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 21: ramstage-y += boardid.c
same as previous comment
Done
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.
Done
PS38:
Please indent this file uniformly using tabs.
Done
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
Done
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.
Done
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.
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 20: #include <fsp/soc_binding.h>
unnecessary #include
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 21: #include <FspmUpd.h>
#include <soc/romstage.h> […]
Done
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... […]
Done
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. […]
Done
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()
Done
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)
Done
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);
Done
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);
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 26: *
nit: extra asterix
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 1020:
Needs an #endif
Done
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)
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 30: **/
Nit: Use */ (without the extra asterix)
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 44: /**
Nit: extra asterix
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 46: **/
Nit: extra asterix
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 48:
indent
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 91: /**
Nit: extra asterix
Done
https://review.coreboot.org/c/coreboot/+/38549/38/src/mainboard/ocp/tiogapas... PS38, Line 93: **/
Nit: extra asterix
Done
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. […]
Done