Attention is currently required from: Alicja Michalska, Felix Singer, Maxim, Michał Kopeć, Michał Żygowski, Nicholas Chin, Paul Menzel.
Angel Pons 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:
(60 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80853/comment/358823be_58a28592 : PS8, Line 23: broken "Broken" as in not enabled in vendor firmware?
https://review.coreboot.org/c/coreboot/+/80853/comment/9542d55c_c4ba429b : PS8, Line 23: PCI-E passtrough I thought passthrough requires VT-d? Just mention both:
VT-x/VT-d (PCI-E passtrough, broken on stock)
https://review.coreboot.org/c/coreboot/+/80853/comment/e7b631b2_61f6d330 : PS8, Line 27: also broken on stock Oh, that stinks. You should be able to disable `HAVE_ACPI_RESUME` as a workaround.
https://review.coreboot.org/c/coreboot/+/80853/comment/23b74060_9ed6d760 : 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?
https://review.coreboot.org/c/coreboot/+/80853/comment/cd35cd38_0c26bc61 : 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/d57c054e_c408f430 : 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/a9c7a99f_9e8560da : PS8, Line 37: consumtion typo: consum**p**tion
https://review.coreboot.org/c/coreboot/+/80853/comment/337a9e41_40bee4cf : 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?
Does vendor firmware enable AER on the root ports? A sufficiently detailed `lspci` log (run as root) would probably tell.
https://review.coreboot.org/c/coreboot/+/80853/comment/c06e0499_6c87d873 : 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.
File src/mainboard/erying/tgl/Kconfig:
https://review.coreboot.org/c/coreboot/+/80853/comment/382a6aaa_2a65aa7d : PS8, Line 24: default "erying-tgl" What does SMBIOS on stock firmware say?
https://review.coreboot.org/c/coreboot/+/80853/comment/969a4438_8b0701c7 : 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. Did disabling ASPM in coreboot help at all? If not, please remove this - it's only a distraction.
https://review.coreboot.org/c/coreboot/+/80853/comment/2b7df21f_bcd6f628 : PS8, Line 39: default y Why?
File src/mainboard/erying/tgl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/80853/comment/c7e37903_84b5394e : PS8, 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:
https://review.coreboot.org/c/coreboot/+/80853/comment/4865731e_007dc9cf : 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). IT8625E datasheet agrees (register 0x2a bit 7 is 3VSBSW# enable).
File src/mainboard/erying/tgl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80853/comment/074fb85d_1868ed7a : PS7, 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:
https://review.coreboot.org/c/coreboot/+/80853/comment/f51ed994_fafd5905 : PS8, Line 19: register "HybridStorageMode" = "1" Are you sure this is needed?
https://review.coreboot.org/c/coreboot/+/80853/comment/7ed2b095_ef07f3c0 : PS8, 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
https://review.coreboot.org/c/coreboot/+/80853/comment/d1214083_227bbe0f : PS8, Line 51: OC0 `OC_SKIP` as it seems you don't use OC mapping.
https://review.coreboot.org/c/coreboot/+/80853/comment/39d1b1b4_1e8cb356 : 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:
``` register "SataPortsEnable" = "{ [0] = 1, [1] = 1, [2] = 1, [3] = 1, }" register "SataPortsDevSlp" = "{ [0] = 1, [1] = 1, [2] = 1, [3] = 1, }" register "SataSalpSupport" = "1" ```
https://review.coreboot.org/c/coreboot/+/80853/comment/2ed65824_1028b12c : PS8, Line 90: register "PcieRpAdvancedErrorReporting[4]" = "1" You can disable AER here.
https://review.coreboot.org/c/coreboot/+/80853/comment/4a0bf328_6c403582 : PS8, Line 118: register "SerialIoUartMode[PchSerialIoIndexUART0]" = "PchSerialIoPci" Is this actually used? Kconfig selects `DRIVERS_UART_8250IO` which is primarily used for Super I/O UARTs.
https://review.coreboot.org/c/coreboot/+/80853/comment/feb0adfa_9626f1b7 : 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?
https://review.coreboot.org/c/coreboot/+/80853/comment/90e2d15e_b37a8c2a : PS8, Line 171: 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:
https://review.coreboot.org/c/coreboot/+/80853/comment/420bdf40_bee4f2f7 : PS8, Line 27: Scope (_SB.PCI0.LPCB) : { : } Dead code?
File src/mainboard/erying/tgl/gpio.h:
https://review.coreboot.org/c/coreboot/+/80853/comment/e54d8906_245034de : PS8, 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/fc79496d_7ed90bfe : PS8, 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:
https://review.coreboot.org/c/coreboot/+/80853/comment/462345b4_f7c21c51 : PS8, Line 21: params->CpuPcieRpAdvancedErrorReporting[0] = 1; You can disable AER here.
https://review.coreboot.org/c/coreboot/+/80853/comment/0395a1f4_1929b1f3 : PS8, Line 23: params->CpuPcieRpPeerToPeerMode[0] = 1; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/c4b3d8a7_b2ee8a69 : PS8, Line 24: params->CpuPcieRpPtmEnabled[0] = 1; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/6d764e44_e7715d2c : PS8, Line 27: params->CpuPcieRpTransmitterHalfSwing[0] = 0; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/8e1c0e8e_4a763524 : PS8, Line 40: params->PchLegacyIoLowLatency = 1; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/cab0a023_0e1ef9ba : PS8, Line 41: params->PchDmiAspmCtrl = 0; Why? That's DMI ASPM, it doesn't affect PCIe ASPM.
https://review.coreboot.org/c/coreboot/+/80853/comment/c1aba91b_ce889a9e : PS8, 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/73cfdc4afff3e641be217b31b98...
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 }; ```
https://review.coreboot.org/c/coreboot/+/80853/comment/c2b4ffb8_2c83cd5a : PS8, Line 45: params->PcieRpPmSci[4] = 1; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/8d6515bf_9ee1a238 : PS8, Line 68: : params->CpuPcieRpFunctionSwap = 1; : params->PcieRpFunctionSwap = 0; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/084503f0_cc862ed3 : PS8, Line 71: params->ITbtPcieTunnelingForUsb4 = 0; Does this do anything?
https://review.coreboot.org/c/coreboot/+/80853/comment/d9774c06_d9db93f4 : PS8, Line 72: params->PchUsbOverCurrentEnable = 0; Why? Just don't assign overcurrent pins to USB ports in the devicetree.
https://review.coreboot.org/c/coreboot/+/80853/comment/3d35f48f_99dcf1cb : PS8, Line 74: params->PavpEnable = 1; Should be set via a Kconfig already.
https://review.coreboot.org/c/coreboot/+/80853/comment/aa8703dd_ccc0eba1 : 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 devicetree.
File src/mainboard/erying/tgl/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/80853/comment/3a403e25_957e2b42 : PS8, Line 24: mupd->FspmConfig.EnableAbove4GBMmio = 1; Shouldn't this be configurable?
https://review.coreboot.org/c/coreboot/+/80853/comment/40fcc9eb_76c38d27 : PS8, Line 25: mupd->FspmConfig.OcSupport = 1; : mupd->FspmConfig.OcLock = 0; Should be turned into a config option at SoC level.
https://review.coreboot.org/c/coreboot/+/80853/comment/4ea89152_a98fdcda : PS8, Line 28: // 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/4a6b8cc2_cd4db35e : PS8, Line 35: mupd->FspmConfig.CpuPcieRpPcieSpeed[0] = 0; // Auto : mupd->FspmConfig.CpuPcieRpPcieSpeed[1] = 0; // Auto There should be no need to set these.
https://review.coreboot.org/c/coreboot/+/80853/comment/94836710_f1bcb69c : PS8, Line 37: mupd->FspmConfig.CpuPcieRpCdrRelock[0] = 1; : mupd->FspmConfig.CpuPcieRpCdrRelock[1] = 1; Why?
https://review.coreboot.org/c/coreboot/+/80853/comment/c5b03466_fbfa5a0e : PS8, Line 39: 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/d03cbe33_b5aba2f0 : PS8, Line 43: mupd->FspmConfig.DmiMaxLinkSpeed = 3; No need
https://review.coreboot.org/c/coreboot/+/80853/comment/4b6d7c86_624d2df2 : PS8, Line 44: mupd->FspmConfig.DmiAspm = 0; : mupd->FspmConfig.DmiAspmCtrl = 0; DMI ASPM is independent from PCIe ASPM. Please remove.
https://review.coreboot.org/c/coreboot/+/80853/comment/68a1c25f_36173c15 : PS8, Line 48: mupd->FspmConfig.SpdProfileSelected = 0; // Default profile Any reason to set this?
https://review.coreboot.org/c/coreboot/+/80853/comment/5baf81f0_a926b308 : PS8, Line 49: mupd->FspmConfig.RefClk = 0; // 133MHz Any reason to set this?
https://review.coreboot.org/c/coreboot/+/80853/comment/d8e07bb8_54aaf415 : PS8, 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/2ab384b5_fc5dfc30 : PS8, Line 51: mupd->FspmConfig.Ratio = 0; // Auto : mupd->FspmConfig.RingDownBin = 0; : mupd->FspmConfig.GearRatio = 0; // Auto Aren't these the defaults?
https://review.coreboot.org/c/coreboot/+/80853/comment/9bd843ca_babbc016 : PS8, Line 54: mupd->FspmConfig.ECT = 1; Early Command Training is usually disabled for DDR4, any reason to enable it here?
https://review.coreboot.org/c/coreboot/+/80853/comment/a59255b7_8670fb9e : PS8, Line 64: mupd->FspmConfig.WRVC1D = 1; : mupd->FspmConfig.WRVC1D = 1; This is set twice, shouldn't be set at all anyway.
https://review.coreboot.org/c/coreboot/+/80853/comment/41a39670_a3c11878 : PS8, Line 54: 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/3747ef12_e02029c0 : PS8, Line 82: mupd->FspmConfig.DdrFreqLimit = 2933; // Setting to auto for now. That doesn't look like "auto"
https://review.coreboot.org/c/coreboot/+/80853/comment/f8252e70_1f557622 : PS8, Line 83: mupd->FspmConfig.SaGv = 0; Does it matter for TGL-H?
https://review.coreboot.org/c/coreboot/+/80853/comment/ce4d4fdd_2327c192 : PS8, 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/24e0195c_ed49fb6b : PS8, 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/e89ac1ae_4a50add0 : PS8, Line 87: mupd->FspmConfig.RefreshWm = 1; : mupd->FspmConfig.ExitOnFailure = 1; Why set these?
https://review.coreboot.org/c/coreboot/+/80853/comment/3d91d603_ede1523b : PS8, Line 90: memcfg_init(mupd, &mem_config, &spd_info, half_populated); nit: Put `const bool half_populated = false;` immediately above this line.