coreboot-gerrit April 2024

coreboot-gerrit@coreboot.org

1 participants
3293 discussions

[L] Change in coreboot[main]: drivers/intel/touch: Add driver for Intel Touch Controller and Devices
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Cliff Huang, Eran Mitrani, Eric Lai, Jingyuan Liang, Kyoung Il Kim, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81331?usp=email ) Change subject: drivers/intel/touch: Add driver for Intel Touch Controller and Devices ...................................................................... Patch Set 5: (2 comments) File src/drivers/intel/touch/touch.c: https://review.coreboot.org/c/coreboot/+/81331/comment/eb8dd085_c8cae8f9 : PS5, Line 347: config = ((const struct device *)dev)->chip_info; Redundant? https://review.coreboot.org/c/coreboot/+/81331/comment/eb918e45_5d8ffd4d : PS5, Line 358: config = ((const struct device *)dev)->chip_info; Redundant? -- To view, visit https://review.coreboot.org/c/coreboot/+/81331?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I52dfcd187c92141cb8fc47f4143b90d243439d4e Gerrit-Change-Number: 81331 Gerrit-PatchSet: 5 Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Jingyuan Liang <jingyliang(a)google.com> Gerrit-Reviewer: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Henry Barnor <hbarnor(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Jingyuan Liang <jingyliang(a)google.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Sun, 28 Apr 2024 11:44:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/google/brya/var/bujia: Add VBT data file
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Paul Menzel, Shon Wang. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81553?usp=email ) Change subject: mb/google/brya/var/bujia: Add VBT data file ...................................................................... Patch Set 8: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/81553?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id56461708250eaedd288ddbf788d686153df0b96 Gerrit-Change-Number: 81553 Gerrit-PatchSet: 8 Gerrit-Owner: Shon Wang <shon.wang(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Shon Wang <shon.wang(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 11:22:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/aoostar: Add AOOSTAR R1 (WTR_R1)
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Federico Amedeo Izzo. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82010?usp=email ) Change subject: mb/aoostar: Add AOOSTAR R1 (WTR_R1) ...................................................................... Patch Set 8: (6 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/82010/comment/266bbfca_2e6e1621 : PS8, Line 47: VDT VBT? https://review.coreboot.org/c/coreboot/+/82010/comment/8cf30f73_43b348fe : PS8, Line 49: vgabios blob (ID 8086,0406) Shouldn't be needed if you use FSP GOP for video init. Also, 8086:0406 is "Haswell Integrated Graphics Controller", which is blatantly wrong. File src/mainboard/aoostar/wtr_r1/Kconfig: https://review.coreboot.org/c/coreboot/+/82010/comment/da424775_4283f1e2 : PS8, Line 28: config MAINBOARD_VENDOR : string : default "AOOSTAR" Already set in vendor Kconfig, please drop File src/mainboard/aoostar/wtr_r1/devicetree.cb: https://review.coreboot.org/c/coreboot/+/82010/comment/9d2e54f5_a7a6deff : PS8, Line 194: : [PchSerialIoIndexUART1] = PchSerialIoDisabled, : [PchSerialIoIndexUART2] = PchSerialIoDisabled, Disabled equals zero. Devicetree "register"s are default-initialised to zero, so this can be dropped. https://review.coreboot.org/c/coreboot/+/82010/comment/8a90558f_06e67568 : PS8, Line 201: : [PchSerialIoIndexGSPI1] = PchSerialIoDisabled, : [PchSerialIoIndexGSPI2] = PchSerialIoDisabled, : [PchSerialIoIndexGSPI3] = PchSerialIoDisabled, Disabled equals zero. Devicetree "register"s are default-initialised to zero, so this can be dropped. https://review.coreboot.org/c/coreboot/+/82010/comment/05b4af84_80c771d7 : PS8, Line 205: : register "serial_io_gspi_cs_mode" = "{ : [PchSerialIoIndexGSPI0] = 0, : [PchSerialIoIndexGSPI1] = 0, : [PchSerialIoIndexGSPI2] = 0, : [PchSerialIoIndexGSPI3] = 0, : }" : register "serial_io_gspi_cs_state" = "{ : [PchSerialIoIndexGSPI0] = 0, : [PchSerialIoIndexGSPI1] = 0, : [PchSerialIoIndexGSPI2] = 0, : [PchSerialIoIndexGSPI3] = 0, : }" Devicetree "register"s are default-initialised to zero, so this can be dropped. -- To view, visit https://review.coreboot.org/c/coreboot/+/82010?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I9414eb742b6b90459e010b038c1994537e9801a5 Gerrit-Change-Number: 82010 Gerrit-PatchSet: 8 Gerrit-Owner: Federico Amedeo Izzo <federico(a)izzo.pro> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Federico Amedeo Izzo <federico(a)izzo.pro> Gerrit-Comment-Date: Sun, 28 Apr 2024 11:21:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[NoOp] Change in blobs[main]: mb/erying/tgl: Add blobs necessary for platform bring-up
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/blobs/+/80852?usp=email ) Change subject: mb/erying/tgl: Add blobs necessary for platform bring-up ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: @Felix Can these blobs be redistributed? -- To view, visit https://review.coreboot.org/c/blobs/+/80852?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: blobs Gerrit-Branch: main Gerrit-Change-Id: I9ee9e031969b477d2d9f63f7e49a113bd4380f91 Gerrit-Change-Number: 80852 Gerrit-PatchSet: 2 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 11:15:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/erying: Add Erying Polestar G613 Pro (TGL-H)
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

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/73cfdc4afff3e641be217b31b9… 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. -- To view, visit https://review.coreboot.org/c/coreboot/+/80853?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e Gerrit-Change-Number: 80853 Gerrit-PatchSet: 8 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Maxim <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Maxim <max.senia.poliak(a)gmail.com> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 11:13:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alicja Michalska <ahplka19(a)gmail.com> Comment-In-Reply-To: Maxim <max.senia.poliak(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: drivers/intel/fsp2_0: Remove x64-specific assertion from fsp_header
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Andrey Petrov, Appukuttan V K, Ronak Kanabar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81662?usp=email ) Change subject: drivers/intel/fsp2_0: Remove x64-specific assertion from fsp_header ...................................................................... Patch Set 8: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/81662?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6013af342670e6377a3fe7641d7d9b52c9b6f57c Gerrit-Change-Number: 81662 Gerrit-PatchSet: 8 Gerrit-Owner: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Usha P <usha.p(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 09:57:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/intel/mtl: use different names per mtlrvp variants
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Kapil Porwal, Paul Menzel, Subrata Banik, YH Lin. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82084?usp=email ) Change subject: soc/intel/mtl: use different names per mtlrvp variants ...................................................................... Patch Set 4: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/82084/comment/8a079b3c_2f272547 : PS4, Line 7: soc/intel/mtl: ``mb/intel/mtlrvp`` https://review.coreboot.org/c/coreboot/+/82084/comment/7d387cec_aa0ba509 : PS4, Line 7: per nit: replace `per` with `for` https://review.coreboot.org/c/coreboot/+/82084/comment/4f60826e_8b2d2cca : PS4, Line 14: TEST=Verified boot functionality on google/rex. Given that this change only affects `intel/mtlrvp`, I don't think testing on `google/rex` matters (it wouldn't be affected). If you can get this tested on `intel/mtlrvp` (by you or anyone else), then please update the `TEST=` line accordingly. Otherwise, I would just remove it. -- To view, visit https://review.coreboot.org/c/coreboot/+/82084?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5292a0ffcd7524c55cd7aef37c2f59432b2af06a Gerrit-Change-Number: 82084 Gerrit-PatchSet: 4 Gerrit-Owner: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 09:56:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: mb/protectli/vault_cml: use combo v1/v2 FSP
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Krystian Hebel, Michał Żygowski, Piotr Król, Piotr Kubaj. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81963?usp=email ) Change subject: mb/protectli/vault_cml: use combo v1/v2 FSP ...................................................................... Patch Set 8: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/81963/comment/94c9e869_f17c9b55 : PS8, Line 9: Also switch configs to use combo v1/v2 FSP I think the only differences between these FSP binaries are in FSP-M, so you could technically use two FSP-M binaries but only a single FSP-S binary. No idea if that's worth the flash chip space savings, though. -- To view, visit https://review.coreboot.org/c/coreboot/+/81963?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1a6f6e873e4ec35b9777dc17c0495151348d1d88 Gerrit-Change-Number: 81963 Gerrit-PatchSet: 8 Gerrit-Owner: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 09:53:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: mb/protectli/vault_cml: use combo v1/v2 FSP
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Krystian Hebel, Michał Żygowski, Piotr Król, Piotr Kubaj. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81963?usp=email ) Change subject: mb/protectli/vault_cml: use combo v1/v2 FSP ...................................................................... Patch Set 8: (1 comment) File src/mainboard/protectli/vault_cml/Kconfig: https://review.coreboot.org/c/coreboot/+/81963/comment/63e6830e_a754250c : PS8, Line 27: default "VP46XX" This is overridden in SMBIOS table, c.f. `mainboard.c`. So this difference shouldn't impact runtime -- To view, visit https://review.coreboot.org/c/coreboot/+/81963?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1a6f6e873e4ec35b9777dc17c0495151348d1d88 Gerrit-Change-Number: 81963 Gerrit-PatchSet: 8 Gerrit-Owner: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 09:51:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: mb/protectli/vault_cml: use combo v1/v2 FSP
by Angel Pons (Code Review) 28 Apr '24

28 Apr '24

Attention is currently required from: Krystian Hebel, Michał Żygowski, Piotr Król, Piotr Kubaj. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81963?usp=email ) Change subject: mb/protectli/vault_cml: use combo v1/v2 FSP ...................................................................... Patch Set 8: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/81963?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1a6f6e873e4ec35b9777dc17c0495151348d1d88 Gerrit-Change-Number: 81963 Gerrit-PatchSet: 8 Gerrit-Owner: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Piotr Kubaj <pkubaj(a)anongoth.pl> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Sun, 28 Apr 2024 09:49:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2024