Attention is currently required from: Felix Singer, Michał Żygowski, 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 2:
(28 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80853/comment/3dbdee87_7b187349 : PS1, Line 25: even though I force-disabled it, I'm still getting AERs
That's because AER is still enabled: […]
No, of course not. I want to fix the issue, not hide it 😜
https://review.coreboot.org/c/coreboot/+/80853/comment/41fa55d5_cc0e8bc1 : PS1, Line 35: haven't enabled
singular: hasn’t enabled […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/b38ad7ae_1c89ab7a : PS1, 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 : : Adjusting LaneEq's didn't change anything, all settings are configured : in (mostly) the same way as they were on stock firmware.
Without proper `PcieClkSrcClkReq` settings, the most you can get is ASPM L0s. […]
Yes, that's true. I have disabled all ASPM I could find, though:
``` config PCIEXP_ASPM default n
config PCIEXP_CLK_PM default n
config PCIEXP_L1_SUB_STATE default n ```
https://review.coreboot.org/c/coreboot/+/80853/comment/b968ac9d_95c010f0 : PS1, Line 50: Starting to suspect Intel's FSP might be buggy, as I haven't had those : issues when I initially started working on this project when 4.20 tree : was current.
Would be nice if you could bisect in some way.
Will try. I had suffered a lot of health issues in recent months (especially in March), so I didn't feel well enough to dedicate time to this port.
Definitely want to get to the bottom of it now that I'm feeling better though 😊
File src/mainboard/erying/Kconfig:
PS1:
Missing […]
Done
File src/mainboard/erying/Kconfig.name:
PS1:
Missing […]
Done
File src/mainboard/erying/tgl/Kconfig:
PS1:
Missing […]
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/c847a34a_4953491e : PS1, Line 24: config DIMM_SPD_SIZE : default 512
512 is the default, remove.
Acknowledged
File src/mainboard/erying/tgl/Kconfig.name:
PS1:
Missing […]
Done
File src/mainboard/erying/tgl/bootblock.c:
PS1:
Use tabs instead of spaces.
Done
File src/mainboard/erying/tgl/devicetree.cb:
PS1:
Remove execute bit.
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/74a41f13_35096ce3 : PS1, Line 6: .tdp_pl1_override = 45, : .
Add one more tab
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/6a8bdd13_64ae4869 : PS1, Line 11: .tdp_pl1_override = 45, : .
Add one more tab
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/a339ab75_567eb5f6 : PS1, Line 16: # FSP configuration
Seems superfluous
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/45ad9a4d_c6bd5db3 : PS1, Line 22: # IT8613E doesn't support s0ix specification, only S3 is possible. : # Sleep is broken, supposedly RAM loses power in S3 state. : register "s0ix_enable" = "0"
Rather add that information to the commit message and later to the documentation instead of here.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/960f7b91_3739c9a4 : PS1, Line 26: # Power
Seems superfluous
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/19a9478b_ce99cdd9 : PS1, Line 35: device ref system_agent on end
Equivalent to chipset dt, remove.
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/e32321f5_a22f0b50 : PS1, Line 37: # [01.0] PCI-E x16 4.0 (SoC)
Please don't document the PCI numbers. It leads to copy/paste issues and thus wrong documentation. […]
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/c5bd97c4_00097a92 : PS1, Line 43: # [02.0] TigerLake-H GT1
Most of the comments seem superfluous since the alias names are used. Remove.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80853/comment/2fdc3b62_860fe864 : PS1, Line 70: [0] = USB2_PORT_MID(OC0), /* Rear, bottom right */ : [1] = USB2_PORT_MID(OC0), /* Rear, bottom left */ : [2] = USB2_PORT_MID(OC2), /* NIC left */ : [3] = USB2_PORT_MID(OC2), /* NIC right */ : [4] = USB2_PORT_MID(OC2), /* Front Panel 1 */ : [5] = USB2_PORT_MID(OC2), /* Front Panel 2 */ : [8] = USB2_PORT_MID(OC0), /* Front Panel 1 (USB3) */ : [9] = USB2_PORT_MID(OC0), /* Front Panel 2 (USB3) */ : [10] = USB2_PORT_MID(OC0), /* Rear, top left */ :
Add one more tab
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/ca6fa7f3_beacb4ed : PS1, Line 83: [0] = USB3_PORT_DEFAULT(OC0), /* Rear, bottom right */ : [1] = USB3_PORT_DEFAULT(OC0), /* Rear, bottom left */ : [2] = USB3_PORT_DEFAULT(OC0), /* Front Panel 1 */ : [3] = USB3_PORT_DEFAULT(OC0), /* Front Panel 2 */ : [4] = USB3_PORT_DEFAULT(OC0), /* Rear, top left */ :
Add one more tab
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/4d122c7b_d6d161ee : PS1, Line 96: device ref heci1 off end
Equivalent to chipset dt, remove.
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/2d516bc7_a42e957b : PS1, Line 222: register "PchHdaDspEnable" = "0"
Avoid setting boolean options to 0, as it's the default. […]
ACK, I left it while debugging and forgot to remove it.
https://review.coreboot.org/c/coreboot/+/80853/comment/20c252de_a20fe757 : PS1, Line 225: device ref fast_spi off end
Avoid adding devices which have the same state as the definitions in the chipset devices tree. […]
Done
File src/mainboard/erying/tgl/gpio.h:
https://review.coreboot.org/c/coreboot/+/80853/comment/3f92f3ca_de961a86 : PS1, Line 32: /* GPIO */
Remove comments for GPIOs which are NC.
Done
File src/mainboard/erying/tgl/ramstage.c:
PS1:
Remove execute bit.
Done
File src/mainboard/erying/tgl/romstage_fsp_params.c:
PS1:
Remove execute bit.
Done
https://review.coreboot.org/c/coreboot/+/80853/comment/a14aefff_46f08293 : PS1, Line 99: gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table));
Is there a reason you do GPIO configuration before FSP-M runs? FSP might reconfigure GPIOs, so you s […]
Not really, will take a look. Thanks!
-
Alicja Michalska (Code Review)