Attention is currently required from: Sean Rhodes, Andy Pont. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58428 )
Change subject: mainboard/starlabs: Add LabTop Mk IV ......................................................................
Patch Set 66: Code-Review+1
(15 comments)
File src/mainboard/starlabs/labtop/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/58428/comment/45f64760_68bfe2d0 PS66, Line 4: i3-10110u and i7-10710u nit: `u` in the model names should be uppercase
File src/mainboard/starlabs/labtop/include/memory.h:
https://review.coreboot.org/c/coreboot/+/58428/comment/0e1acdc2_eb99c56a PS66, Line 6: u8 get_memory_config_straps(void); : const struct cnl_mb_cfg *get_memory_cfg(struct cnl_mb_cfg *mem_cfg); You don't need this file at all if you make these functions static (in romstage.c).
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/58428/comment/8f6ca2ce_76398478 PS66, Line 125: device pci 19.0 off end # I2C4 Even if it's unused, you should enable this PCI device so that UART #2 can be enumerated properly. I recall that the PCI spec requires that multifunction devices implement function 0.
You should also add `[PchSerialIoIndexI2C4] = PchSerialIoSkipInit,` to `SerialIoDevMode` so that FSP leaves the device enabled but skips configuring it.
https://review.coreboot.org/c/coreboot/+/58428/comment/4569f439_25fda37f PS66, Line 182: off Not sure if this should be set to `hidden`
File src/mainboard/starlabs/labtop/variants/cml/gpio.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/3c79cf18_99f50989 PS66, Line 10: romstage bootblock
https://review.coreboot.org/c/coreboot/+/58428/comment/bdd63d42_d7bf1633 PS66, Line 12: PAD_CFG_GPO(GPP_E22, 1, PLTRST), : PAD_CFG_GPO(GPP_E23, 1, PLTRST), The native functions for these GPIOs are DPPD_CTRLCLK and DPPD_CTRLDATA, respectively. Do you know why they're used as GPOs?
https://review.coreboot.org/c/coreboot/+/58428/comment/3e136dce_c3810d57 PS66, Line 14: PAD_CFG_GPI(GPP_H6, NONE, PLTRST), : PAD_CFG_GPI(GPP_H7, NONE, PLTRST), Am curious about these as well.
File src/mainboard/starlabs/labtop/variants/cml/romstage.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/86463f2a_b544053a PS66, Line 12: u8 get_memory_config_straps(void) This function should be declared as static.
https://review.coreboot.org/c/coreboot/+/58428/comment/6e272a00_cbefd682 PS66, Line 19: Missing opening parenthesis `(` here
https://review.coreboot.org/c/coreboot/+/58428/comment/ec26876f_bcf341e6 PS66, Line 38: gpio_t nit: `const gpio_t`
https://review.coreboot.org/c/coreboot/+/58428/comment/ae08805b_86783fa9 PS66, Line 39: (u8) Is it necessary to use `u8`? I'd simply use `u32` or `unsigned int` to avoid casts.
https://review.coreboot.org/c/coreboot/+/58428/comment/65d91544_c4ec6913 PS66, Line 42: const struct cnl_mb_cfg *get_memory_cfg(struct cnl_mb_cfg *mem_cfg) This function should be declared as static.
https://review.coreboot.org/c/coreboot/+/58428/comment/a85d6f07_7c510337 PS66, Line 46: struct cnl_mb_cfg std_memcfg = { : .rcomp_resistor = {121, 81, 100}, : .rcomp_targets = {100, 40, 20, 20, 26}, : .dq_pins_interleaved = 0, : .vref_ca_config = 2, : .ect = 0, : }; I'd put this initialisation in `mainboard_memory_init_params()` so that this function only fills in the SPD data.
static bool is_dual_channel(const unsigned int memid) { return memid != 3 && memid != 5 && memid != 7; }
static void fill_spd_data(struct cnl_mb_cfg *mem_cfg) { const unsigned int memid = get_memory_config_straps(); printk(BIOS_DEBUG, "Memory config straps: 0x%.2x\n", memid); /* * If we are using single channel ID = 3, 5 or 7 then we only * populate .spd[0].If we are dual channel then we also populate * .spd[2] as well. */ mem_cfg->spd[0].read_type = READ_SPD_CBFS; mem_cfg->spd[0].spd_spec.spd_index = memid; if (is_dual_channel(memid)) { mem_cfg->spd[2].read_type = READ_SPD_CBFS; mem_cfg->spd[2].spd_spec.spd_index = memid; } }
void mainboard_memory_init_params(FSPM_UPD *memupd) { struct cnl_mb_cfg memcfg = { .rcomp_resistor = {121, 81, 100}, .rcomp_targets = {100, 40, 20, 20, 26}, .dq_pins_interleaved = 0, .vref_ca_config = 2, .ect = 0, };
const uint8_t vtd = get_uint_option("vtd", 1); memupd->FspmTestConfig.VtdDisable = !vtd;
const uint8_t ht = get_uint_option("hyper_threading", memupd->FspmConfig.HyperThreading); memupd->FspmConfig.HyperThreading = ht;
fill_spd_data(&memcfg); cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); }
https://review.coreboot.org/c/coreboot/+/58428/comment/748fd5fc_d4bcda54 PS66, Line 66: memid != 3 && memid != 5 && memid != 7 Idea: put this into a helper function:
static bool is_dual_channel(const unsigned int memid) { return memid != 3 && memid != 5 && memid != 7; }
https://review.coreboot.org/c/coreboot/+/58428/comment/f7e62c80_47f7a6b7 PS66, Line 72: ; Semicolon not needed
-
Angel Pons (Code Review)