Attention is currently required from: Angel Pons. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58428 )
Change subject: mainboard/starlabs: Add LabTop Mk IV ......................................................................
Patch Set 69:
(21 comments)
File Documentation/mainboard/starlabs/labtop_cml.md:
https://review.coreboot.org/c/coreboot/+/58428/comment/45500f76_410c38d3 PS66, Line 40: [^1]
What does this mean?
Added the relating `*1`
File src/mainboard/starlabs/labtop/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/58428/comment/227bba8d_3c3f82f1 PS66, Line 4: i3-10110u and i7-10710u
nit: `u` in the model names should be uppercase
Done
File src/mainboard/starlabs/labtop/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/58428/comment/606279e5_f89a2f3b PS66, Line 3: #if CONFIG(BOARD_STARLABS_STARBOOK_TGL) : #define EC_GPE_SCI 0x6e : #else : #define EC_GPE_SCI 0x50 : #endif
Idea for another patch: Turn this into a Kconfig option
Definitely - added!
File src/mainboard/starlabs/labtop/include/memory.h:
https://review.coreboot.org/c/coreboot/+/58428/comment/b1f51d6f_61e390fb 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).
Done
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/58428/comment/af283991_bac16fdf 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. […]
Would the same apply to TGL?
File src/mainboard/starlabs/labtop/variants/cml/romstage.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/91812c2e_603b6b18 PS66, Line 12: u8 get_memory_config_straps(void)
This function should be declared as static.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/79eef0c1_db662be0 PS66, Line 12: u8 get_memory_config_straps(void)
This function should be declared as static.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/407ecab3_8e698b18 PS66, Line 19:
Missing opening parenthesis `(` here
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/6c58d032_9427e4a5 PS66, Line 19:
Missing opening parenthesis `(` here
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/5a6e83d5_ffe10193 PS66, Line 38: gpio_t
nit: `const gpio_t`
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/29505032_0e3010e9 PS66, Line 38: gpio_t
nit: `const gpio_t`
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/9e497fa8_f6597e25 PS66, Line 39: (u8)
Is it necessary to use `u8`? I'd simply use `u32` or `unsigned int` to avoid casts.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/1077e248_4dc96668 PS66, Line 39: (u8)
Is it necessary to use `u8`? I'd simply use `u32` or `unsigned int` to avoid casts.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/f663f46c_a4e6009f PS66, Line 42: const struct cnl_mb_cfg *get_memory_cfg(struct cnl_mb_cfg *mem_cfg)
This function should be declared as static.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/a5712739_c4c5442f PS66, Line 42: const struct cnl_mb_cfg *get_memory_cfg(struct cnl_mb_cfg *mem_cfg)
This function should be declared as static.
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/b9262d38_0465038a 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 […]
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/a984e575_61dd7221 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 […]
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/dbc59c78_47d8d90a PS66, Line 66: memid != 3 && memid != 5 && memid != 7
Idea: put this into a helper function: […]
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/ee0a89b2_6f31dea0 PS66, Line 66: memid != 3 && memid != 5 && memid != 7
Idea: put this into a helper function: […]
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/243230df_38c05d6d PS66, Line 72: ;
Semicolon not needed
Done
https://review.coreboot.org/c/coreboot/+/58428/comment/4d18adff_bfcb6d37 PS66, Line 72: ;
Semicolon not needed
Done
-
Sean Rhodes (Code Review)