Sean Rhodes 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:
(34 comments)
Patchset:
PS15: Does this mean the patch has to submitted again?
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/55128/comment/521d3a57_74265cfc PS15, Line 26: # select HAVE_IFD_BIN : # select HAVE_ME_BIN
Remove?
Nope. See previous comment.
https://review.coreboot.org/c/coreboot/+/55128/comment/0da78656_5ff93500 PS15, Line 61: config MAX_CPUS : int : default 8 if BOARD_STARLABS_LABTOP_KBL : default 12
Same values are already specified in SoC Kconfig
Wasn't when we wrote it, will change.
https://review.coreboot.org/c/coreboot/+/55128/comment/1d09876d_221aae18 PS15, Line 66: #config DRIVER_TPM_SPI_CHIP : # int : # default 2
Commented-out, remove?
Nope
https://review.coreboot.org/c/coreboot/+/55128/comment/a356ecec_1f1a729d 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?
Nope. See previous comment.
https://review.coreboot.org/c/coreboot/+/55128/comment/3e80dd11_adf7aa27 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
Same here
https://review.coreboot.org/c/coreboot/+/55128/comment/f26694dc_e57708f0 PS15, Line 120: default "3rdparty/blobs/mainboard/starlabs/Logo.bmp"
Same here
Same here
File src/mainboard/starlabs/labtop/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/55128/comment/0116447f_1bff4b2c 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.
For what reason?
File src/mainboard/starlabs/labtop/mainboard.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/f65fe767_fe59ba14 PS15, Line 33: #if CONFIG(BOARD_STARLABS_LABTOP_CML)
Preprocessor not needed.
How?
https://review.coreboot.org/c/coreboot/+/55128/comment/ef8c8967_d877e625 PS15, Line 52: /* Override smbios_mainboard_board_type */ : smbios_board_type smbios_mainboard_board_type(void) : { : return SMBIOS_BOARD_TYPE_MOTHERBOARD; : }
Remove.
Why?
File src/mainboard/starlabs/labtop/ramstage.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/255f3f65_9910e935 PS15, Line 1:
Spurious blank line?
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/c211b425_77c58056 PS15, Line 10: #if CONFIG(BOARD_STARLABS_LABTOP_CML)
Doing something like CB:48143 avoids the need for preprocessor.
Is there any advantage? Not being difficult, simply interested.
File src/mainboard/starlabs/labtop/spd/spd_util.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/1b87730e_b4839123 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
Ack
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/04193031_2f2c3b58 PS15, Line 101: StarPoint
This doesn't look like a valid HID
It isn't.
https://review.coreboot.org/c/coreboot/+/55128/comment/77cf0156_ed2defa3 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 will always be present on our hardware, can remove though
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.
Ack
File src/mainboard/starlabs/labtop/variants/cml/include/variant/hda_verb.h:
PS15:
Same here, this should be a compilation unit (.c file).
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/5416d634_2820eb40 PS15, Line 13: 0x0000002b
This makes more sense if written in decimal. […]
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/8c592616_f7f12c9b PS15, Line 20: 0x0017209E, : 0x00172111, : 0x001722EC, : 0x00172310,
There's an AZALIA_SUBVENDOR() macro
Cool.
https://review.coreboot.org/c/coreboot/+/55128/comment/137e61b8_58e2a82f PS15, Line 36: /* ONE DOES NOT SIMPLY : MAKE IT WORK WITH WINDOWS */
I cna understand the frustration, but is this comment necessary?
A comment is necassary. I'll give you that "Drivers in WU illogically only reference Front or Rear ports so must be set incorrectly" is probably more helpful, but who doesn't love a Sean Bean meme?
File src/mainboard/starlabs/labtop/variants/cml/romstage.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/2d56f8b5_13f5d8c6 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.
Done
File src/mainboard/starlabs/labtop/variants/kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/246a1c2e_fc355517 PS15, Line 19: # Send an extra VR mailbox command for the PS4 exit issue : # register "SendVrMbxCmd" = "2"
Commented-out, remove?
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/b636f7ab_a53e1f95 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...
As far as I know, nit is an achaic term for brightness of a display. What do you mean? I'm not up to date with the lingo that the cool kids use...
https://review.coreboot.org/c/coreboot/+/55128/comment/b6f83aa2_1efaab97 PS15, Line 48: : [PchSerialIoIndexI2C2]= PchSerialIoPci, : [PchSerialIoIndexI2C3]= PchSerialIoPci,
Corresponding PCI devices are off.
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/ee4ff975_8e064a6e PS15, Line 52: [PchSerialIoIndexI2C5]= PchSerialIoPci, : [PchSerialIoIndexSpi0]= PchSerialIoPci, : [PchSerialIoIndexSpi1]= PchSerialIoPci,
Corresponding PCI devices are off.
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/6da260e3_3ef999a7 PS15, Line 55: [PchSerialIoIndexUart0] = PchSerialIoSkipInit,
If unused, can be disabled. No other function of PCI device 30 is enabled.
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/78a93f74_2717e688 PS15, Line 110: StarPoint
Doesn't look like a valid HID
It's not.
https://review.coreboot.org/c/coreboot/+/55128/comment/04caa196_058bf58b 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.
It will on our hardware
https://review.coreboot.org/c/coreboot/+/55128/comment/b9107369_5fd0ddd9 PS15, Line 198: subsystemid 0x10ec 0x111e
Suspicious indentation
What's the relavance of it being suspicious? It works.
https://review.coreboot.org/c/coreboot/+/55128/comment/0c145388_61ad9544 PS15, Line 200: off
Should be on
Should be off
File src/mainboard/starlabs/labtop/variants/kbl/include/variant/gpio.h:
PS15:
Same here, this should be a compilation unit (.c file).
Ack
File src/mainboard/starlabs/labtop/variants/kbl/include/variant/hda_verb.h:
PS15:
Same here, this should be a compilation unit (.c file).
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/6d93b6e7_159ded9d PS15, Line 13: 0x0000002b
Same as CML, this makes more sense if written in decimal. […]
Ack
https://review.coreboot.org/c/coreboot/+/55128/comment/63af8e91_1664e004 PS15, Line 18: 0x0017201E, : 0x00172111, : 0x001722EC, : 0x00172310,
Use the AZALIA_SUBVENDOR macro?
Ack