Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45229 )
Change subject: mb/supermicro/x11-lga1151-series: add x11ssh-f board as a variant ......................................................................
Patch Set 5:
(24 comments)
https://review.coreboot.org/c/coreboot/+/45229/3/Documentation/mainboard/sup... File Documentation/mainboard/supermicro/x11-lga1151-series/x11ssh-f/x11ssh-f.md:
https://review.coreboot.org/c/coreboot/+/45229/3/Documentation/mainboard/sup... PS3, Line 37: - S3 resume not working
S3 doesn't work with vendor, which is mentioned in the document.
Sorry, I may be just blind... I don't see any comment on vendor fw, just "S3 resume not working" (which would mean "not working in coreboot").
What about this? "S3 not working (vendor and coreboto) due to lacking S3 support in Intel SPS"
https://review.coreboot.org/c/coreboot/+/45229/4/Documentation/mainboard/sup... File Documentation/mainboard/supermicro/x11-lga1151-series/x11ssh-f/x11ssh-f.md:
https://review.coreboot.org/c/coreboot/+/45229/4/Documentation/mainboard/sup... PS4, Line 39: failed to be initialize
This seems to be because of a problematic kernel command line parameter. […]
Add some notes about using IGD for computaion, please, if you're able to resolve the issue
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-f/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 23: _PAD_CFG_STRUCT(GPP_A12, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE), 0 this does not match the prior version (GPO) but now means PAD_NC. Can you provide the raw values from inteltool, please?
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 30: PAD_CFG_GPI_TRIG_OWN(GPP_A18, NONE, PLTRST, OFF, ACPI), changed from DRIVER to ACPI; please provide raw inteltool output
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 40: _PAD_CFG_STRUCT(GPP_B4, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B5, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B6, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B7, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B8, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B9, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0), : _PAD_CFG_STRUCT(GPP_B10, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | 1, 0 TX_RX_DISABLE + no NFx -> PAD_NC was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 54: /* GPIO */ drop comment
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 77: _PAD_CFG_STRUCT(GPP_C8, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), : PAD_CFG_GPI_TRIG_OWN(GPP_C9, NONE, PLTRST, OFF, ACPI), : _PAD_CFG_STRUCT(GPP_C10, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), not sure about these, yet; provide intel raw output, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 85: AD_CFG_STRUCT(GPP_C14, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE), TX_RX_DISABLE + no NFx -> PAD_NC was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 94: G_STRUCT(GPP_C22, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(EDGE_SINGLE) | PAD_IRQ_ROUTE(SMI) : | PAD_BUF(TX_DISABLE) | (1 << 1), PAD_PULL( the prior version is correct
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 97: PAD_CFG_STRUCT(GPP_C23, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE), TX_RX_DISABLE + no NFx -> PAD_NC was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 101: _PAD_CFG_STRUCT(GPP_D0, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE), 0), : TX_RX_DISABLE + no NFx -> PAD_NC was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 104: _PAD_CFG_STRUCT(GPP_D2, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(EDGE_SINGLE) | PAD_IRQ_ROUTE(NMI) : | PAD_BUF(TX_DISABLE) | (1 << 1), PAD_PULL(20K_PU the prior version was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 126: TRUCT(GPP_D22, PAD_FUNC(GPIO) | PAD_RESET(RSMRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 < probably PAD_CFG_GPI or even NC; provide raw value, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 131: PAD_NC(GPP_E0, NONE), why did this change from NF1 to NC?
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 137: RUCT(GPP_E6, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(EDGE_SINGLE) | PAD_IRQ_ROUTE(NMI) : | PAD_BUF(TX_DISABLE) | (1 << 1), PAD_PULL(20K the prior version was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 157: G_STRUCT(GPP_F9, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), probably PAD_CFG_GPI or even NC; provide raw value, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 173: FG_STRUCT(GPP_G0, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), : _PAD_CFG_STRUCT(GPP_G1, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), : _PAD_CFG_STRUCT(GPP_G2, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), : _PAD_CFG_STRUCT(GPP_G3, PAD_FUNC(GPIO) | PAD_RESET(DEEP) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 probably PAD_CFG_GPI or even NC; provide raw value, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 189: PAD_CFG_GPI_TRIG_OWN(GPP_G12, NONE, PLTRST, OFF, ACPI), : PAD_CFG_GPI_TRIG_OWN(GPP_G13, NONE, PLTRST, OFF, ACPI), : PAD_CFG_GPI_TRIG_OWN(GPP_G14, NONE, PLTRST, OFF, ACPI), : PAD_CFG_GPI_TRIG_OWN(GPP_G15, NONE, PLTRST, OFF, ACPI), : PAD_CFG_GPI_TRIG_OWN(GPP_G16, NONE, PLTRST, OFF, ACPI changed from DRIVER to ACPI; reason?
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 202: _PAD_CFG_STRUCT(GPP_H1, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | ( probably PAD_CFG_GPI or even NC; provide raw value, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 206: _PAD_CFG_STRUCT(GPP_H4, PAD_FUNC(GPIO) | PAD_RESET(PLTRST) : | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1) probably PAD_CFG_GPI or even NC; provide raw value, please
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 236: _PAD_CFG_STRUCT(GPD7, PAD_FUNC(GPIO) | PAD_TRIG(OFF) : | PAD_BUF(TX_RX_DISABLE) | 1, 0), : TX_RX_DISABLE + no NFx -> PAD_NC was right
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-f/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 14: # Additional FSP Configuration : # This board has an IGD with no output. : register "PrimaryDisplay" = "Display_Auto" :
I will change it into "Display_PCH_PCIe", but would it interfere if a graphic card is installed on t […]
Sorry for the confusion; I haven't thought about that case. Let's keep Display_Auto, so both PEG and PCH PCIe work. :-)
https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1... PS4, Line 18: # PCIe configuration : # Enable i210-AT (GbE) : register "PcieRpEnable[0]" = "1" : register "PcieRpEnable[1]" = "1" : : # Enable M.2 : register "PcieRpEnable[4]" = "1" : : # Enable ASpeed PCI bridge : register "PcieRpEnable[6]" = "1" : : # Enable JPCIE1 : register "PcieRpEnable[8]" = "1" :
What do you mean? Tree-style configs already lies below, or can these above and the tree-style confi […]
The PcieRpEnable fsp settings are going to be set via the on/off setting in the devicetree in the near future. This is already done e.g. in cannonlake. To make review easier later, we started moving these settings into the devicetree:
device pci 1c.4 on # PCI Express Port 5 register "PcieRpEnable[4]" = "1" smbios_slot_desc ... end
https://review.coreboot.org/c/coreboot/+/45229/5/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-f/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45229/5/src/mainboard/supermicro/x1... PS5, Line 15: # This board has an IGD with no output. : register "PrimaryDisplay" = "Display_PCH_PCIe" That was my fault, sorry. Let's keep auto see https://review.coreboot.org/c/coreboot/+/45229/4/src/mainboard/supermicro/x1...