Attention is currently required from: Alicja Michalska, Felix Singer, Maxim, Michał Kopeć, Michał Żygowski, Nicholas Chin, Paul Menzel.
60 comments:
Commit Message:
Patch Set #8, Line 23: broken
"Broken" as in not enabled in vendor firmware?
Patch Set #8, Line 23: PCI-E passtrough
I thought passthrough requires VT-d? Just mention both:
VT-x/VT-d (PCI-E passtrough, broken on stock)
Patch Set #8, Line 27: also broken on stock
Oh, that stinks. You should be able to disable `HAVE_ACPI_RESUME` as a workaround.
Patch Set #8, 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?
Patch Set #8, Line 31: - Automatic fan control (IT8613E can't read CPU_TIN at the moment)
That likely requires enabling PECI on the Super I/O.
Patch Set #8, 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.
Patch Set #8, Line 37: consumtion
typo: consum**p**tion
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?
Does vendor firmware enable AER on the root ports? A sufficiently detailed `lspci` log (run as root) would probably tell.
Patch Set #8, 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.
File src/mainboard/erying/tgl/Kconfig:
Patch Set #8, Line 24: default "erying-tgl"
What does SMBIOS on stock firmware say?
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. Did disabling ASPM in coreboot help at all? If not, please remove this - it's only a distraction.
Patch Set #8, Line 39: default y
Why?
File src/mainboard/erying/tgl/Kconfig.name:
Patch Set #8, Line 4: bool "erying-tgl"
This is a very generic name, given the specific model listed in the commit message (`Polestar G613 Pro (TGL-H)`). I would use the same model here, as this is what people see in menuconfig. I would also rename the board's Kconfig symbol from `BOARD_ERYING_TGL` to `BOARD_ERYING_POLESTAR_G613_PRO`.
I'm OK if you want to keep the mainboard folder named `tgl`, as long as there's other boards that could be added as variants in the future.
File src/mainboard/erying/tgl/bootblock.c:
Patch Set #8, 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). IT8625E datasheet agrees (register 0x2a bit 7 is 3VSBSW# enable).
File src/mainboard/erying/tgl/devicetree.cb:
Patch Set #7, Line 125: device ref pch_espi on
The ITE it8613e chip is also used in `AOOSTAR R1` motherboard - port CB:82010 . […]
I would just use one `genX_dec` register to forward 0xa00 .. 0xa3f to LPC, even if not all of the space is decoded in the Super I/O.
There's only 4 LPC generic I/O decode registers, it makes zero sense to use all of them for tiny ranges. The code doing that (IIRC it's in `soc/intel/common` code) is rather stupid in this regard. It's trying its best, but it completely falls apart when you have a bunch of tiny, almost contiguous ranges.
File src/mainboard/erying/tgl/devicetree.cb:
Patch Set #8, Line 19: register "HybridStorageMode" = "1"
Are you sure this is needed?
Patch Set #8, Line 25: register "PcieClkSrcClkReq[0]" = "PCIE_CLK_NOTUSED"
I'm not sure if ASPM works properly when CLKREQ# isn't set up. It likely does, but without the extra power savings from clock gating
`OC_SKIP` as it seems you don't use OC mapping.
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:
```
register "SataPortsEnable" = "{
[0] = 1,
[1] = 1,
[2] = 1,
[3] = 1,
}"
register "SataPortsDevSlp" = "{
[0] = 1,
[1] = 1,
[2] = 1,
[3] = 1,
}"
register "SataSalpSupport" = "1"
```
Patch Set #8, Line 90: register "PcieRpAdvancedErrorReporting[4]" = "1"
You can disable AER here.
Patch Set #8, Line 118: register "SerialIoUartMode[PchSerialIoIndexUART0]" = "PchSerialIoPci"
Is this actually used? Kconfig selects `DRIVERS_UART_8250IO` which is primarily used for Super I/O UARTs.
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?
irq 0x29 = 0xc0
irq 0x2c = 0x41
irq 0x2d = 0x02
You're setting some of these in C code already
File src/mainboard/erying/tgl/dsdt.asl:
Scope (\_SB.PCI0.LPCB)
{
}
Dead code?
File src/mainboard/erying/tgl/gpio.h:
Patch Set #8, Line 10: _PAD_CFG_STRUCT(GPP_A0, PAD_FUNC(NF1) | PAD_RESET(DEEP) | PAD_BUF(TX_RX_DISABLE), PAD_PULL(NATIVE)), /* ESPI_IO0 */
`line length of 126 exceeds 122 columns`
I don't know why intelp2m failed to generate nice macros for these.
Patch Set #8, Line 168: PAD_CFG_NF(GPD9, NONE, PWROK, NF1), /* SLP_WLAN# */
`please, no space before tabs`
Please fix.
File src/mainboard/erying/tgl/ramstage.c:
Patch Set #8, Line 21: params->CpuPcieRpAdvancedErrorReporting[0] = 1;
You can disable AER here.
Patch Set #8, Line 23: params->CpuPcieRpPeerToPeerMode[0] = 1;
Why?
Patch Set #8, Line 24: params->CpuPcieRpPtmEnabled[0] = 1;
Why?
Patch Set #8, Line 27: params->CpuPcieRpTransmitterHalfSwing[0] = 0;
Why?
Patch Set #8, Line 40: params->PchLegacyIoLowLatency = 1;
Why?
Patch Set #8, Line 41: params->PchDmiAspmCtrl = 0;
Why? That's DMI ASPM, it doesn't affect PCIe ASPM.
Patch Set #8, Line 44: params->PcieRpMaxPayload[4] = 2; // M.2 Gen3
Why set this at all?
Plus, I'm pretty sure this value is out of range: https://github.com/tianocore/edk2-platforms/blob/73cfdc4afff3e641be217b31b985761ef8338412/Silicon/Intel/TigerlakeSiliconPkg/Include/ConfigBlock/PcieRp/PchPcieRp/PchPcieRpConfig.h#L163
When the last value in an enum contains `Max` in its name, it's typically used as the size for arrays.
```
enum PCH_PCIE_MAX_PAYLOAD {
PchPcieMaxPayload128 = 0,
PchPcieMaxPayload256,
PchPcieMaxPayloadMax
};
```
Patch Set #8, Line 45: params->PcieRpPmSci[4] = 1;
Why?
params->CpuPcieRpFunctionSwap = 1;
params->PcieRpFunctionSwap = 0;
Why?
Patch Set #8, Line 71: params->ITbtPcieTunnelingForUsb4 = 0;
Does this do anything?
Patch Set #8, Line 72: params->PchUsbOverCurrentEnable = 0;
Why? Just don't assign overcurrent pins to USB ports in the devicetree.
Patch Set #8, Line 74: params->PavpEnable = 1;
Should be set via a Kconfig already.
// 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 devicetree.
File src/mainboard/erying/tgl/romstage_fsp_params.c:
Patch Set #8, Line 24: mupd->FspmConfig.EnableAbove4GBMmio = 1;
Shouldn't this be configurable?
mupd->FspmConfig.OcSupport = 1;
mupd->FspmConfig.OcLock = 0;
Should be turned into a config option at SoC level.
// iGPU
mupd->FspmConfig.GttSize = 3; // 8MB
mupd->FspmConfig.ApertureSize = 3; // 512MB
mupd->FspmConfig.GtPsmiSupport = 0;
mupd->FspmConfig.IgdDvmt50PreAlloc = 2; // 64MB
Should be configurable, if set at all. Note that the iGPU's stolen memory (what is exclusively reserved for the iGPU) only needs to fit the framebuffer contents, as the iGPU can get more memory for other stuff through OS drivers.
mupd->FspmConfig.CpuPcieRpPcieSpeed[0] = 0; // Auto
mupd->FspmConfig.CpuPcieRpPcieSpeed[1] = 0; // Auto
There should be no need to set these.
mupd->FspmConfig.CpuPcieRpCdrRelock[0] = 1;
mupd->FspmConfig.CpuPcieRpCdrRelock[1] = 1;
Why?
mupd->FspmConfig.CpuPcieNewFom[0] = 1;
mupd->FspmConfig.CpuPcieNewFom[1] = 1;
Why? I don't know what exactly this does, but the default is 0. This can make the PCIe root ports not work well.
Patch Set #8, Line 43: mupd->FspmConfig.DmiMaxLinkSpeed = 3;
No need
mupd->FspmConfig.DmiAspm = 0;
mupd->FspmConfig.DmiAspmCtrl = 0;
DMI ASPM is independent from PCIe ASPM. Please remove.
Patch Set #8, Line 48: mupd->FspmConfig.SpdProfileSelected = 0; // Default profile
Any reason to set this?
Patch Set #8, Line 49: mupd->FspmConfig.RefClk = 0; // 133MHz
Any reason to set this?
Patch Set #8, Line 50: mupd->FspmConfig.VddVoltage = 1350; // 1.35V (Applicable only with XMP Profile 1)
Does your board have the means to change the DIMM voltage? If not, this is just lying to MRC.
mupd->FspmConfig.Ratio = 0; // Auto
mupd->FspmConfig.RingDownBin = 0;
mupd->FspmConfig.GearRatio = 0; // Auto
Aren't these the defaults?
Patch Set #8, Line 54: mupd->FspmConfig.ECT = 1;
Early Command Training is usually disabled for DDR4, any reason to enable it here?
mupd->FspmConfig.WRVC1D = 1;
mupd->FspmConfig.WRVC1D = 1;
This is set twice, shouldn't be set at all anyway.
mupd->FspmConfig.ECT = 1;
mupd->FspmConfig.LCT = 1;
mupd->FspmConfig.SOT = 1;
mupd->FspmConfig.ERDMPRTC2D = 0;
mupd->FspmConfig.RDMPRT = 1;
mupd->FspmConfig.RCVET = 1;
mupd->FspmConfig.JWRL = 1;
mupd->FspmConfig.EWRTC2D = 1;
mupd->FspmConfig.ERDTC2D = 1;
mupd->FspmConfig.WRTC2D = 1;
mupd->FspmConfig.WRVC1D = 1;
mupd->FspmConfig.WRVC1D = 1;
mupd->FspmConfig.DIMMODTT = 1;
mupd->FspmConfig.DIMMRONT = 1;
mupd->FspmConfig.WRDSEQT = 1;
mupd->FspmConfig.WRSRT = 0;
mupd->FspmConfig.RDODTT = 1;
mupd->FspmConfig.RDEQT = 1;
mupd->FspmConfig.RDAPT = 1;
mupd->FspmConfig.RDTC2D = 1;
mupd->FspmConfig.WRVC2D = 1;
mupd->FspmConfig.RDVC2D = 1;
mupd->FspmConfig.CMDVC = 1;
mupd->FspmConfig.MrcSafeConfig = 0;
mupd->FspmConfig.LpDdrDqDqsReTraining = 1;
mupd->FspmConfig.SafeMode = 0;
mupd->FspmConfig.OverrideDowngradeForMixedMemory = 0;
mupd->FspmConfig.MemTestOnWarmBoot = 1;
Why change this? Changing which MRC training steps get executed is very risky as it can increase the likelihood of memory training failures, which is really bad. Failing to boot is the **best**-case scenario, unstability and potential data corruption is the real threat.
Patch Set #8, Line 82: mupd->FspmConfig.DdrFreqLimit = 2933; // Setting to auto for now.
That doesn't look like "auto"
Patch Set #8, Line 83: mupd->FspmConfig.SaGv = 0;
Does it matter for TGL-H?
Patch Set #8, Line 85: mupd->FspmConfig.NModeSupport = 1; // Board type is 1N (1DPC).
N-Mode (command rate) is not bound to DPC (the number of DIMMs per channel). You can run at 2N on 1DPC, and you can try to run at 1N on 2DPC (but CA margins will suck, so expect serious unstability).
Depending on the DIMMs you use and their XMP, you may need to run them at 2N even on 1DPC. So I wouldn't force 1N command rate.
Patch Set #8, Line 86: mupd->FspmConfig.RhPrevention = 0;
Please don't force disable Row Hammer prevention upon others. If you really want to disable Row Hammer prevention anyway, turn it into a config option.
mupd->FspmConfig.RefreshWm = 1;
mupd->FspmConfig.ExitOnFailure = 1;
Why set these?
Patch Set #8, Line 90: memcfg_init(mupd, &mem_config, &spd_info, half_populated);
nit: Put `const bool half_populated = false;` immediately above this line.
To view, visit change 80853. To unsubscribe, or for help writing mail filters, visit settings.