Attention is currently required from: Alicja Michalska.
Felix Singer 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 1:
(25 comments)
Patchset:
PS1: Nice work!
File src/mainboard/erying/Kconfig:
PS1: Missing ``` ## SPDX-License-Identifier: GPL-2.0-only ```
File src/mainboard/erying/Kconfig.name:
PS1: Missing
``` ## SPDX-License-Identifier: GPL-2.0-only ```
File src/mainboard/erying/tgl/Kconfig:
PS1: Missing
``` ## SPDX-License-Identifier: GPL-2.0-only ```
https://review.coreboot.org/c/coreboot/+/80853/comment/4622f3ec_0bfa3477 : PS1, Line 24: config DIMM_SPD_SIZE : default 512 512 is the default, remove.
File src/mainboard/erying/tgl/Kconfig.name:
PS1: Missing
``` ## SPDX-License-Identifier: GPL-2.0-only ```
File src/mainboard/erying/tgl/bootblock.c:
PS1: Use tabs instead of spaces.
File src/mainboard/erying/tgl/devicetree.cb:
PS1: Remove execute bit.
https://review.coreboot.org/c/coreboot/+/80853/comment/69a9ef84_2d57e672 : PS1, Line 6: .tdp_pl1_override = 45, : . Add one more tab
https://review.coreboot.org/c/coreboot/+/80853/comment/e05eb8d7_87353fd4 : PS1, Line 11: .tdp_pl1_override = 45, : . Add one more tab
https://review.coreboot.org/c/coreboot/+/80853/comment/dd999b1e_c0981d7f : PS1, Line 16: # FSP configuration Seems superfluous
https://review.coreboot.org/c/coreboot/+/80853/comment/d50b5f30_4814f869 : 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.
https://review.coreboot.org/c/coreboot/+/80853/comment/4196317a_e4f1bca8 : PS1, Line 26: # Power Seems superfluous
https://review.coreboot.org/c/coreboot/+/80853/comment/9dff9ecf_aec08b53 : PS1, Line 35: device ref system_agent on end Equivalent to chipset dt, remove.
https://review.coreboot.org/c/coreboot/+/80853/comment/4271d6dc_0351e190 : 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. That's why we introduced the alias names. It's useful to document the slot type (x16, x8, ..) though.
https://review.coreboot.org/c/coreboot/+/80853/comment/3c4654a1_5ac30b8c : PS1, Line 43: # [02.0] TigerLake-H GT1 Most of the comments seem superfluous since the alias names are used. Remove.
https://review.coreboot.org/c/coreboot/+/80853/comment/2274fc9e_9b1e89de : 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
https://review.coreboot.org/c/coreboot/+/80853/comment/ab4ad050_809d14a1 : 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
https://review.coreboot.org/c/coreboot/+/80853/comment/fc8dfb4a_db1e86d7 : PS1, Line 96: device ref heci1 off end Equivalent to chipset dt, remove.
https://review.coreboot.org/c/coreboot/+/80853/comment/0a32c419_59fb1312 : PS1, Line 222: register "PchHdaDspEnable" = "0" Avoid setting boolean options to 0, as it's the default. It does make sense for other value assignments though, for example root port clock numbers.
https://review.coreboot.org/c/coreboot/+/80853/comment/5f68b1b0_d20f7961 : 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.
Here: Off, remove.
File src/mainboard/erying/tgl/gpio.h:
https://review.coreboot.org/c/coreboot/+/80853/comment/8620382a_bb24a0a8 : PS1, Line 32: /* GPIO */ Remove comments for GPIOs which are NC.
File src/mainboard/erying/tgl/ramstage.c:
PS1: Remove execute bit.
File src/mainboard/erying/tgl/romstage_fsp_params.c:
PS1: Remove execute bit.
https://review.coreboot.org/c/coreboot/+/80853/comment/e1dd45a1_4e386e9c : 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 should avoid that or reconfigure them later in the ramstage. Have a look at mb/clevo/cml-u/ramstage.c for reference.