Attention is currently required from: Angel Pons, Felix Singer, Maxim, Michał Kopeć, Michał Żygowski, Nicholas Chin, Paul Menzel.
Alicja Michalska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80853?usp=email )
Change subject: mb/erying: Add Erying Polestar G613 Pro (TGL-H) ......................................................................
Patch Set 8:
(32 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80853/comment/ed2187e7_d7b23892 : PS8, Line 23: PCI-E passtrough
I thought passthrough requires VT-d? Just mention both: […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/6d94200d_26b1a7a7 : PS8, Line 23: broken
"Broken" as in not enabled in vendor firmware?
Broken, as it either causes VM to hang, or the entire host 😊
https://review.coreboot.org/c/coreboot/+/80853/comment/d584e28c_1af202e9 : PS8, Line 30: - One of USB2 FP connectors, as well as NGFF USB isn't mapped (yet)
If you enable all USB ports, does it work?
Haven't tried that, but that will be trivial to fix. I simply didn't have 2.0 header next to COM port populated while mapping them, so I just need to boot stock firmware, attach a USB device and map it in devicetree.
https://review.coreboot.org/c/coreboot/+/80853/comment/22e2c461_7b94b80d : PS8, Line 31: - Automatic fan control (IT8613E can't read CPU_TIN at the moment)
That likely requires enabling PECI on the Super I/O.
That was my suspicion as well, but it doesn't seem to be the case.
https://review.coreboot.org/c/coreboot/+/80853/comment/e4cfd725_678f2e65 : PS8, Line 33: Can be flashed using `flashrom -p internal -w build/coreboot.rom`, as
I'd suggest adding `--ifd -i bios` to avoid interfering with the ME.
Agreed, good idea 😊
https://review.coreboot.org/c/coreboot/+/80853/comment/cb2feedc_e62b3045 : PS8, Line 37: consumtion
typo: consum**p**tion
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/ca508e97_9af8ac22 : PS8, Line 39: Likewise, I can't wrap my head around PCI-E AERs I'm getting if I boot : the machine without `pcie_aspm=off` parameter: : - BadTLP : - BadDLLP : - Timeout : - Rollover
On which PCIe root ports? […]
RP5, PEG0, PEG1. Yes, it's enabled in stock firmware: https://github.com/ellyq/erying-logs/blob/main/tgl/stock-uefi-settings/pcie-...
https://review.coreboot.org/c/coreboot/+/80853/comment/c95fce47_7039dd8d : PS8, Line 49: Starting to suspect Intel's FSP might be buggy, as I haven't had those
I know for a fact that FSP has bugs, but I don't think this is what's causing these issues.
Acknowledged
File src/mainboard/erying/tgl/Kconfig:
https://review.coreboot.org/c/coreboot/+/80853/comment/ececc8c9_3043e0b1 : PS8, Line 24: default "erying-tgl"
What does SMBIOS on stock firmware say?
"INTEL HM570". I wanted to avoid using that SMBIOS name, as it's obviously not manufactured by intel. Vendor-provided firmware is very janky.
https://review.coreboot.org/c/coreboot/+/80853/comment/b1ded9ad_3d378eb1 : PS8, Line 29: config PCIEXP_ASPM : default n : : config PCIEXP_CLK_PM : default n : : config PCIEXP_L1_SUB_STATE : default n
I'm not sure if FSP honours these. […]
It does enable/disable ASPM based on Kconfig setting. I left it there explicitly until I get ASPM working to make sure it won't be selected by default.
File src/mainboard/erying/tgl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/80853/comment/415b546b_ee9eb759 : PS8, Line 4: bool "erying-tgl"
This is a very generic name, given the specific model listed in the commit message (`Polestar G613 P […]
There are no mITX versions of TGL platform, so it would be safe to leave it as-it.
Starting with ADL they started releasing mATX and mITX boards (mATX with IT8613E SIO and mITX with Nuvoton), so those will need to be split into variants.
I know for a fact that MrChromebox had bought a MTL mITX board and is working on coreboot port 😜
TGL was self-explanatory, although I guess full vendor name looks a bit nicer.
File src/mainboard/erying/tgl/bootblock.c:
https://review.coreboot.org/c/coreboot/+/80853/comment/8bb3d312_80af3aba : PS8, Line 13: ite_reg_write(GPIO_DEV, 0x29, 0xc1); /* 3VSB - RAM loses power in S3 anyway */
There's an `ite_set_3vsbsw()` function that sets a different register (0x2a). […]
Just noticed this myself yesterday, will definitely give it a try.
File src/mainboard/erying/tgl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80853/comment/84f67744_10be377f : PS7, Line 125: device ref pch_espi on
I would just use one `genX_dec` register to forward 0xa00 .. […]
Agreed. Will need to test this, haven't yet had the time to do so.
File src/mainboard/erying/tgl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80853/comment/5cfcddd8_099965e1 : PS8, Line 19: register "HybridStorageMode" = "1"
Are you sure this is needed?
It shouldn't matter with "H" SKU. I've tested Optane 800P and 900P (U.2) with it being on and off - doesn't seem to make any difference.
https://review.coreboot.org/c/coreboot/+/80853/comment/7e373f08_a11d6be9 : PS8, Line 51: OC0
`OC_SKIP` as it seems you don't use OC mapping.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/f5ceb129_a378c5ad : PS8, Line 75: : register "SataPortsEnable[0]" = "1" : register "SataPortsEnable[1]" = "1" : register "SataPortsEnable[2]" = "1" : register "SataPortsEnable[3]" = "1" : register "SataSalpSupport" = "1" : register "SataPortsDevSlp[0]" = "1" : register "SataPortsDevSlp[1]" = "1" : register "SataPortsDevSlp[2]" = "1" : register "SataPortsDevSlp[3]" = "1"
nit: You can make this a bit less verbose: […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/6d707982_436b7e57 : PS8, Line 90: register "PcieRpAdvancedErrorReporting[4]" = "1"
You can disable AER here.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/a603817d_7c83a6c5 : PS8, Line 118: register "SerialIoUartMode[PchSerialIoIndexUART0]" = "PchSerialIoPci"
Is this actually used? Kconfig selects `DRIVERS_UART_8250IO` which is primarily used for Super I/O U […]
Probably not :P
https://review.coreboot.org/c/coreboot/+/80853/comment/0260700b_056d6e23 : PS8, Line 144: register "TMPIN1.mode" = "THERMAL_DIODE" : register "TMPIN2.mode" = "THERMAL_DIODE" : : register "FAN2.mode" = "FAN_SMART_AUTOMATIC" # CPU_FAN : register "FAN3.mode" = "FAN_SMART_AUTOMATIC" # SYS_FAN : : register "FAN2.smart.tmpin" = "1" : register "FAN2.smart.tmp_off" = "35" : register "FAN2.smart.tmp_start" = "42" : register "FAN2.smart.tmp_full" = "72" : register "FAN2.smart.tmp_delta" = "2" : register "FAN2.smart.pwm_start" = "26" : register "FAN2.smart.slope" = "24" : : register "FAN3.smart.tmpin" = "1" : register "FAN3.smart.tmp_off" = "35" : register "FAN3.smart.tmp_start" = "42" : register "FAN3.smart.tmp_full" = "72" : register "FAN3.smart.tmp_delta" = "2" : register "FAN3.smart.pwm_start" = "26" : register "FAN3.smart.slope" = "24"
Indent with one extra tab?
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/0058a4ae_4ebe67f9 : PS8, Line 171: irq 0x29 = 0xc0 : irq 0x2c = 0x41 : irq 0x2d = 0x02
You're setting some of these in C code already
Acknowledged
File src/mainboard/erying/tgl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/80853/comment/f1a809d9_b79d1563 : PS8, Line 27: Scope (_SB.PCI0.LPCB) : { : }
Dead code?
I left it there in case I needed to work on ACPI for EC, seems like I forgot to remove it. Thanks!
File src/mainboard/erying/tgl/ramstage.c:
https://review.coreboot.org/c/coreboot/+/80853/comment/c9e96dbe_7fdd7ff3 : PS8, Line 21: params->CpuPcieRpAdvancedErrorReporting[0] = 1;
You can disable AER here.
Sure, but then AERs will still happen - we simply won't know about it.
https://review.coreboot.org/c/coreboot/+/80853/comment/ab536501_d6c87d30 : PS8, Line 23: params->CpuPcieRpPeerToPeerMode[0] = 1;
Why?
AFAIK, that's needed for PCI-E Resizable BAR (ReBAR), which is pretty much mandatory for modern GPUs (and yes, I know, it's not necessary to set it on M.2 slot).
https://review.coreboot.org/c/coreboot/+/80853/comment/3d07717d_726d176a : PS8, Line 27: params->CpuPcieRpTransmitterHalfSwing[0] = 0;
Why?
It was configured the same way in vendor's firmware. I'm trying to leave as much settings on-par with stock as possible for now.
https://review.coreboot.org/c/coreboot/+/80853/comment/976a67ac_2ae6144e : PS8, Line 40: params->PchLegacyIoLowLatency = 1;
Why?
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/528cb060_6f4d6618 : PS8, Line 41: params->PchDmiAspmCtrl = 0;
Why? That's DMI ASPM, it doesn't affect PCIe ASPM.
Yes. If I enable DMI ASPM, M.2 slot connected through the chipset goes haywire. Lots of errors, crashes while trying to boot into Linux, and speed drops to 200MB/s.
Disabling DMI ASPM was necessary to make the platform stable (same with PchLegacyIoLowLatency).
https://review.coreboot.org/c/coreboot/+/80853/comment/e36b1f7f_d2b92a16 : PS8, Line 44: params->PcieRpMaxPayload[4] = 2; // M.2 Gen3
Why set this at all? […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/3db89783_346ba1f8 : PS8, Line 68: : params->CpuPcieRpFunctionSwap = 1; : params->PcieRpFunctionSwap = 0;
Why?
It was configured the same way in vendor's firmware.
https://review.coreboot.org/c/coreboot/+/80853/comment/958b0e43_3a436775 : PS8, Line 71: params->ITbtPcieTunnelingForUsb4 = 0;
Does this do anything?
Now that I think about it, probably no. This board has USB4 controller enabled and I had a lot of issues with USB3 completely freezing the platform so I've been trying various settings.
https://review.coreboot.org/c/coreboot/+/80853/comment/9cae9b3c_3ff8f34e : PS8, Line 72: params->PchUsbOverCurrentEnable = 0;
Why? Just don't assign overcurrent pins to USB ports in the devicetree.
It was left there for debugging and I forgot about it. I had platform freezes related to USB3 controller and couldn't figure out why for a while.
https://review.coreboot.org/c/coreboot/+/80853/comment/71fce86c_68cb0738 : PS8, Line 74: params->PavpEnable = 1;
Should be set via a Kconfig already.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/27120067_5f25bb81 : PS8, Line 18: : // PEG0 - Gen4 NVME : params->CpuPcieRpSlotImplemented[0] = 1; : params->CpuPcieRpAdvancedErrorReporting[0] = 1; : params->CpuPcieRpMaxPayload[0] = 2; : params->CpuPcieRpPeerToPeerMode[0] = 1; : params->CpuPcieRpPtmEnabled[0] = 1; : params->CpuPcieRpLtrEnable[0] = 1; : params->CpuPcieRpPmSci[0] = 1; : params->CpuPcieRpTransmitterHalfSwing[0] = 0; : : // PEG1 - PCI-E x16 : params->CpuPcieRpSlotImplemented[1] = 1; : params->CpuPcieRpAdvancedErrorReporting[1] = 1; : params->CpuPcieRpMaxPayload[1] = 2; : params->CpuPcieRpPeerToPeerMode[1] = 1; : params->CpuPcieRpPtmEnabled[1] = 1; : params->CpuPcieRpLtrEnable[1] = 1; : params->CpuPcieRpPmSci[1] = 1; : params->CpuPcieRpTransmitterHalfSwing[1] = 0; : : // Power management - ASPM is broken even on vendor FW : params->PchLegacyIoLowLatency = 1; : params->PchDmiAspmCtrl = 0; : : // PCH RootPorts : params->PcieRpMaxPayload[4] = 2; // M.2 Gen3 : params->PcieRpPmSci[4] = 1; : // params->PcieRpEnableCpm[4] = 0; : // params->PcieRpL1Substates[4] = 0; : // params->PcieRpAspm[4] = 0; : : params->PcieRpMaxPayload[8] = 1; : params->PcieRpPmSci[8] = 1;// M.2 NGFF : // params->PcieRpEnableCpm[8] = 0; : // params->PcieRpL1Substates[8] = 0; : // params->PcieRpAspm[8] = 0; : : params->PcieRpMaxPayload[10] = 1; // RTL8111 NIC : params->PcieRpPmSci[10] = 1; : // params->PcieRpEnableCpm[10] = 0; : // params->PcieRpL1Substates[10] = 0; : // params->PcieRpAspm[10] = 0; : : params->PcieRpMaxPayload[11] = 1; // PCI-E x1 Gen3 : params->PcieRpPmSci[11] = 1; : // params->PcieRpEnableCpm[11] = 0; : // params->PcieRpL1Substates[11] = 0; : // params->PcieRpAspm[11] = 0; : : // FSP settings : params->CpuPcieRpFunctionSwap = 1; : params->PcieRpFunctionSwap = 0; : params->ITbtPcieTunnelingForUsb4 = 0; : params->PchUsbOverCurrentEnable = 0; : params->RC1pFreqEnable = 1; : params->PavpEnable = 1; : params->CdynmaxClampEnable = 0; : params->PchEspiHostC10ReportEnable = 1;
A bunch of these should be set by default in FSP already, and others can be configured in the device […]
I know, I wanted to keep devicetree as "clean" as possible, so people who aren't very familiar with the project can open it and modify settings (such as Power Limits) without feeling intimidated.
-
Alicja Michalska (Code Review)