Attention is currently required from: Angel Pons, Felix Held. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56088 )
Change subject: mainboard/starlabs: Add StarBook Mk V ......................................................................
Patch Set 109:
(21 comments)
File Documentation/distributions.md:
https://review.coreboot.org/c/coreboot/+/56088/comment/97fa8858_9be5d3de PS109, Line 29: offer
singular: offer*s*
Done
File Documentation/mainboard/starlabs/BiosLock.jpg:
PS109:
Converting this image to WebP reduces its size by 37. […]
Safari doesn't support WebP though, which is granted a minority, but better to have it visible for all.
File Documentation/mainboard/starlabs/starbook_tgl.md:
https://review.coreboot.org/c/coreboot/+/56088/comment/1fea8f14_967a33fe PS109, Line 50: The below are optional
I'm not sure if this is gramatically correct. How about: […]
Done
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/56088/comment/d08e5912_ae89985b PS109, Line 45: string
Declaring the type shouldn't be needed
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/d7dfcfac_a1325506 PS109, Line 52: int
Declaring the type shouldn't be needed
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/c8f776f5_2a58ec41 PS109, Line 56: int
Declaring the type shouldn't be needed
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/815a099e_edbf00f6 PS109, Line 66: string
Declaring the type shouldn't be needed
Done
File src/mainboard/starlabs/labtop/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/6bbae148_4a5bef9f PS109, Line 8: Comet
Huh, isn't this a TGL board?
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/35dd7970_df402179 PS109, Line 25: "PRP00001"
This device ID is invalid, one 0 too many. […]
It does!
File src/mainboard/starlabs/labtop/acpi/sleep.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/f9ef8cc0_6455a344 PS109, Line 3: // _PTS: Prepare To Sleep
nit: Drop these comments?
Done
File src/mainboard/starlabs/labtop/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/4d535917_3544e410 PS109, Line 12: // OEM revision
nit: remove?
Done
File src/mainboard/starlabs/labtop/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/97d7773a_8d4db3e3 PS109, Line 15: u32 viddid
Unused parameter
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/7e83b0c4_60da7399 PS109, Line 23: %d
Why not use `%08x` instead?
Done
File src/mainboard/starlabs/labtop/mainboard.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/82068bc3_a4fd1858 PS109, Line 31: return "B5";
How about `return CONFIG_MAINBOARD_FAMILY;` instead?
Done
File src/mainboard/starlabs/labtop/variants/tgl/board.fmd:
https://review.coreboot.org/c/coreboot/+/56088/comment/09420b11_ac3aa9b9 PS109, Line 6: SI_ALL@0x0 0x500000 { : SI_DESC@0x0 0x1000 : SI_ME@0x1000 : }
Why is this part indented with spaces?
Done
File src/mainboard/starlabs/labtop/variants/tgl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56088/comment/40eca644_dd3d63c4 PS109, Line 3: register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{ : .tdp_pl1_override = 15, : .tdp_pl2_override = 15, : }" : : register "power_limits_config[POWER_LIMITS_U_4_CORE]" = "{ : .tdp_pl1_override = 15, : .tdp_pl2_override = 15, : }"
I see `devtree_update()` overrides these values. […]
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/ef6ebc79_1e84e262 PS109, Line 74: register "PcieRpEnable[0]" = "1" : register "PcieRpEnable[2]" = "1"
The PCI devices for these RPs are disabled, why?
Left over from debugging TCSS/D3C stuff. They can go.
https://review.coreboot.org/c/coreboot/+/56088/comment/bfef92ea_1220162a PS109, Line 82: register "PcieClkSrcUsage[0]" = "0x40"
TGL-U doesn't have PEG (PCI Express Graphics, i.e. CPU PCIe ports).
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/d9fe0380_eb934496 PS109, Line 236: device pci 1f.4 off end # SMBus
Shouldn't SMBus be enabled? It's used to read the DIMM SPDs
Done
File src/mainboard/starlabs/labtop/variants/tgl/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/a155e073_a5884b9f PS109, Line 8: 0x10ec1200
Why is this value different from the one in the `AZALIA_SUBVENDOR` macro?
Done
https://review.coreboot.org/c/coreboot/+/56088/comment/40ac83d2_edf617bf PS109, Line 7: 0x10ec0256, // Codec Vendor / Device ID: Realtek ALC256 : 0x10ec1200, // Subsystem ID : 38, // Number of jacks (NID entries)
Please use consistent comment style
Done