Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32734 )
Change subject: mb/supermicro/x11ssh: Add Supermicro X11SSH-TF ......................................................................
Patch Set 74:
(21 comments)
https://review.coreboot.org/c/coreboot/+/32734/74//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32734/74//COMMIT_MSG@12 PS74, Line 12: SeaBios SeaBIOS, just like in https://www.seabios.org/SeaBIOS
https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/su... File Documentation/mainboard/supermicro/x11ssh-tf.md:
https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/su... PS74, Line 30: S5 resume S5 or S3 resume?
https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/su... PS74, Line 37: 10GB Nit: That's probably 10 Gigabit (not GigaByte), so 10Gb
https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/su... File Documentation/mainboard/supermicro/x11ssh_flash.jpg:
PS74: Maybe highlight the SOIC-16 flash chip on the top edge of the mainboard as well, with some text saying which is which, or using a different color.
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/Kconfig:
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 67: : config SUBSYSTEM_VENDOR_ID : hex : default 0x8086 Is this used anywhere?
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 1: BOARD_SUPERMICRO_X11SSH_PLUS_TF : bool "X11SSH+-TF" I don't see any 'X11SSH+-TF', so I'd remove the plus sign and "PLUS" from the Kconfig symbol name
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/bootblock.c:
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 37: garbeled garbled
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/mainboard.c:
PS74: I think you can remove this file if it's empty
https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/x... PS68, Line 210: device pci 01.1 on
Sorry, I meant "link training", not bus. […]
According to MNL-1783, the PEG x16 port is split in two, one half is routed to SLOT6 (PCIe x8 slot), and the other half is routed to the LSI SAS3008 (only on the X11SSH-CTF, unpopulated on the X11SSH-TF).
Looks like function 0 is for the SAS controller, whereas function 1 corresponds to the SLOT6 PCIe port.
https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/x... PS68, Line 211: 4X
X8 slot with X4 data width
According to MNL-1783, SLOT6 is x8 electrical.
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 30: "Device4Enable" = "1" This is not accounted for in the devicetree
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 114: X550T Maybe mention it's the 10GbE device
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 203: device domain 0 on Maybe add a 'subsystemid 0x1234 0x5678 inherit' line right after this line ('device domain 0 on')
(of course, not with 0x1234 0x5678, but rather with values from the vendor firmware)
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 205: unused Looks like it's routed to the LSI SAS3008 (only on the X11SSH-CTF, unpopulated on the X11SSH-TF).
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 208: JPCIE1 SLOT6
Also, while you're at it, please add an empty newline to separate CPU devices from PCH devices, for clarity.
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 216: JPCIE1 SLOT4
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 219: device pci 00.0 on end # Aspeed 2400 VGA Are the two nested instances of "device pci 00.0" intentional?
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 223: 10GBE Nit: 10GbE (since it's Gbit, not GByte)
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 239: device pnp 2e.0 off end This device doesn't seem to exist
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 263: device pnp 2e.b on # SUART3 Is this UART connected anywhere?
https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x... PS74, Line 267: device pnp 2e.c on # SUART4 Is this UART connected anywhere?