Attention is currently required from: Star Labs. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55128 )
Change subject: src/mainboard: Add Star Labs labtop series ......................................................................
Patch Set 15:
(33 comments)
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/55128/comment/704e7102_7f91ac3f PS15, Line 26: # select HAVE_IFD_BIN : # select HAVE_ME_BIN Remove?
https://review.coreboot.org/c/coreboot/+/55128/comment/04b5cbce_04e223c6 PS15, Line 61: config MAX_CPUS : int : default 8 if BOARD_STARLABS_LABTOP_KBL : default 12 Same values are already specified in SoC Kconfig
https://review.coreboot.org/c/coreboot/+/55128/comment/63bf6de2_f6289a78 PS15, Line 66: #config DRIVER_TPM_SPI_CHIP : # int : # default 2 Commented-out, remove?
https://review.coreboot.org/c/coreboot/+/55128/comment/0f13da90_48ba54f9 PS15, Line 90: config IFD_BIN_PATH : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/$(CONFIG_VARIANT_DIR)/flashregion_0_flashdescriptor.bin" : : config ME_BIN_PATH : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/$(CONFIG_VARIANT_DIR)/flashregion_2_intel_me.bin" Do these files exist in the blobs repo?
https://review.coreboot.org/c/coreboot/+/55128/comment/fae8d25d_23838581 PS15, Line 107: config EC_STARLABS_IT_BIN_PATH : string : depends on EC_STARLABS_IT_BIN : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/$(CONFIG_VARIANT_DIR)/flashregion_8_ec.bin" Same here
https://review.coreboot.org/c/coreboot/+/55128/comment/c34e4887_44b3ec96 PS15, Line 120: default "3rdparty/blobs/mainboard/starlabs/Logo.bmp" Same here
File src/mainboard/starlabs/labtop/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/55128/comment/5eb80fe7_1ad20cfe PS15, Line 6: /* Power button device. */ : Device (PWRB) : { : Name (_HID, EisaId ("PNP0C0C")) : Name (PBST, One) : : Method (_STA, 0, NotSerialized) : { : Return (0x0F) : } : } This should be removed.
File src/mainboard/starlabs/labtop/mainboard.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/efb4922c_25648899 PS15, Line 33: #if CONFIG(BOARD_STARLABS_LABTOP_CML) Preprocessor not needed.
https://review.coreboot.org/c/coreboot/+/55128/comment/a375f537_fae3be68 PS15, Line 52: /* Override smbios_mainboard_board_type */ : smbios_board_type smbios_mainboard_board_type(void) : { : return SMBIOS_BOARD_TYPE_MOTHERBOARD; : } Remove.
File src/mainboard/starlabs/labtop/ramstage.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/ddfc8fc1_91fbbeb6 PS15, Line 1: Spurious blank line?
https://review.coreboot.org/c/coreboot/+/55128/comment/9e2cb0f3_f04d268b PS15, Line 10: #if CONFIG(BOARD_STARLABS_LABTOP_CML) Doing something like CB:48143 avoids the need for preprocessor.
File src/mainboard/starlabs/labtop/spd/spd_util.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/ba45d584_7ce129cf PS15, Line 8: void mainboard_fill_dq_map_data(void *dq_map_ptr) : { : /* DQ byte map */ : const u8 dq_map[2][12] = { : {0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0, 0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00}, : {0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC, 0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00} }; : memcpy(dq_map_ptr, dq_map, sizeof(dq_map)); : } : : void mainboard_fill_dqs_map_data(void *dqs_map_ptr) : { : /* DQS CPU<>DRAM map */ : const u8 dqs_map[2][8] = {{0, 6, 3, 1, 5, 2, 7, 4}, {7, 5, 3, 6, 2, 4, 0, 1} }; : memcpy(dqs_map_ptr, dqs_map, sizeof(dqs_map)); : } Meaningless with DDR4
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/c789a88d_229b0ec1 PS15, Line 101: StarPoint This doesn't look like a valid HID
https://review.coreboot.org/c/coreboot/+/55128/comment/d7309dc1_cca14fcb PS15, Line 140: device pci 00.0 on end This root port is wired to a slot, so this device may not always be present. It should not be in the devicetree.
File src/mainboard/starlabs/labtop/variants/cml/include/variant/gpio.h:
PS15: This file shouldn't be a header, but a compilation unit (.c file) of its own.
File src/mainboard/starlabs/labtop/variants/cml/include/variant/hda_verb.h:
PS15: Same here, this should be a compilation unit (.c file).
https://review.coreboot.org/c/coreboot/+/55128/comment/3c3ad758_3d1b18ed PS15, Line 13: 0x0000002b This makes more sense if written in decimal. And shouldn't it be 33 instead?
https://review.coreboot.org/c/coreboot/+/55128/comment/aab5965d_aa18f3e2 PS15, Line 20: 0x0017209E, : 0x00172111, : 0x001722EC, : 0x00172310, There's an AZALIA_SUBVENDOR() macro
https://review.coreboot.org/c/coreboot/+/55128/comment/d04bce83_d32bef5b PS15, Line 36: /* ONE DOES NOT SIMPLY : MAKE IT WORK WITH WINDOWS */ I cna understand the frustration, but is this comment necessary?
File src/mainboard/starlabs/labtop/variants/cml/romstage.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/48c59b57_885be030 PS15, Line 52: /* : * The dqs_map arrays map the DDR4 pins to the SoC pins : * for both channels. : * : * the index = pin number on DDR4 part : * the value = pin number on SoC : */ : .dqs_map[DDR_CH0] = {0, 6, 1, 3, 5, 2, 7, 4}, : .dqs_map[DDR_CH1] = {7, 5, 3, 6, 2, 4, 0, 1}, This doesn't matter for DDR4.
File src/mainboard/starlabs/labtop/variants/kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/0b1ba636_d5f47e7a PS15, Line 19: # Send an extra VR mailbox command for the PS4 exit issue : # register "SendVrMbxCmd" = "2" Commented-out, remove?
https://review.coreboot.org/c/coreboot/+/55128/comment/30eef020_9dcdcc4e PS15, Line 24: .up_delay_ms= 200,// T3 : .down_delay_ms= 0,// T10 : .cycle_delay_ms = 500,// T12 : .backlight_on_delay_ms=50,// T7 : .backlight_off_delay_ms = 0,// T9 : .backlight_pwm_hz = 200, nit: alignment...
https://review.coreboot.org/c/coreboot/+/55128/comment/e9562999_34baab02 PS15, Line 48: : [PchSerialIoIndexI2C2]= PchSerialIoPci, : [PchSerialIoIndexI2C3]= PchSerialIoPci, Corresponding PCI devices are off.
https://review.coreboot.org/c/coreboot/+/55128/comment/9c993c55_ba4d6c79 PS15, Line 52: [PchSerialIoIndexI2C5]= PchSerialIoPci, : [PchSerialIoIndexSpi0]= PchSerialIoPci, : [PchSerialIoIndexSpi1]= PchSerialIoPci, Corresponding PCI devices are off.
https://review.coreboot.org/c/coreboot/+/55128/comment/23b80b6f_1cc45be6 PS15, Line 55: [PchSerialIoIndexUart0] = PchSerialIoSkipInit, If unused, can be disabled. No other function of PCI device 30 is enabled.
https://review.coreboot.org/c/coreboot/+/55128/comment/8934f13d_e4dcb4b5 PS15, Line 110: StarPoint Doesn't look like a valid HID
https://review.coreboot.org/c/coreboot/+/55128/comment/772bbcec_a5de3eb6 PS15, Line 159: device pci 00.0 on end Same as CML: this PCIe port is wired to a slot, and this device may not always be present.
https://review.coreboot.org/c/coreboot/+/55128/comment/33b9e4f1_e7b95725 PS15, Line 198: subsystemid 0x10ec 0x111e Suspicious indentation
https://review.coreboot.org/c/coreboot/+/55128/comment/8873a471_60710e6d PS15, Line 200: off Should be on
File src/mainboard/starlabs/labtop/variants/kbl/include/variant/gpio.h:
PS15: Same here, this should be a compilation unit (.c file).
File src/mainboard/starlabs/labtop/variants/kbl/include/variant/hda_verb.h:
PS15: Same here, this should be a compilation unit (.c file).
https://review.coreboot.org/c/coreboot/+/55128/comment/d471ab48_4e22489f PS15, Line 13: 0x0000002b Same as CML, this makes more sense if written in decimal. And should be 18, otherwise you have a lovely buffer overflow.
https://review.coreboot.org/c/coreboot/+/55128/comment/236513cb_51d533b4 PS15, Line 18: 0x0017201E, : 0x00172111, : 0x001722EC, : 0x00172310, Use the AZALIA_SUBVENDOR macro?
-
Angel Pons (Code Review)