Attention is currently required from: Sean Rhodes, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56088 )
Change subject: mainboard/starlabs: Add StarBook Mk V ......................................................................
Patch Set 109: Code-Review+1
(22 comments)
File Documentation/distributions.md:
https://review.coreboot.org/c/coreboot/+/56088/comment/e1e94fad_90f4a3f1 PS109, Line 29: offer singular: offer*s*
File Documentation/mainboard/starlabs/BiosLock.jpg:
PS109: Converting this image to WebP reduces its size by 37.5%
File Documentation/mainboard/starlabs/starbook_tgl.md:
https://review.coreboot.org/c/coreboot/+/56088/comment/aa8df81b_58eef390 PS109, Line 16: GOP driver is recommended Is there any other option?
https://review.coreboot.org/c/coreboot/+/56088/comment/b28d0620_fa13b253 PS109, Line 50: The below are optional I'm not sure if this is gramatically correct. How about:
The files listed below are optional:
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/56088/comment/b268c1b1_96ff4107 PS109, Line 45: string Declaring the type shouldn't be needed
https://review.coreboot.org/c/coreboot/+/56088/comment/1e0c8856_eca1a49f PS109, Line 52: int Declaring the type shouldn't be needed
https://review.coreboot.org/c/coreboot/+/56088/comment/60769edb_3eca4617 PS109, Line 56: int Declaring the type shouldn't be needed
https://review.coreboot.org/c/coreboot/+/56088/comment/1dd14ad3_d3e566e8 PS109, Line 66: string Declaring the type shouldn't be needed
File src/mainboard/starlabs/labtop/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/d4e837d6_07d55ee1 PS109, Line 8: Comet Huh, isn't this a TGL board?
https://review.coreboot.org/c/coreboot/+/56088/comment/d10252cd_36d3c6fc PS109, Line 25: "PRP00001" This device ID is invalid, one 0 too many. Could you please test if removing it (only keep "PNP0A05") still works?
Related LKML discussion: https://www.spinics.net/lists/linux-acpi/msg103981.html
File src/mainboard/starlabs/labtop/acpi/sleep.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/f76bc43f_dcce4a34 PS109, Line 3: // _PTS: Prepare To Sleep nit: Drop these comments?
File src/mainboard/starlabs/labtop/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/56088/comment/ca82aeb4_4ca07fcd PS109, Line 12: // OEM revision nit: remove?
File src/mainboard/starlabs/labtop/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/10346c5c_24432879 PS109, Line 15: u32 viddid Unused parameter
https://review.coreboot.org/c/coreboot/+/56088/comment/f213d64c_ba615cbf PS109, Line 23: %d Why not use `%08x` instead?
File src/mainboard/starlabs/labtop/mainboard.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/fdac0b7e_aee8328f PS109, Line 31: return "B5"; How about `return CONFIG_MAINBOARD_FAMILY;` instead?
File src/mainboard/starlabs/labtop/variants/tgl/board.fmd:
https://review.coreboot.org/c/coreboot/+/56088/comment/e32806fa_4ba6ad01 PS109, Line 6: SI_ALL@0x0 0x500000 { : SI_DESC@0x0 0x1000 : SI_ME@0x1000 : } Why is this part indented with spaces?
File src/mainboard/starlabs/labtop/variants/tgl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56088/comment/e56f8e07_8f2a8559 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. I'd remove this assignment to avoid confusion (the initial values will default to 0).
https://review.coreboot.org/c/coreboot/+/56088/comment/33dafb53_efd2fb32 PS109, Line 74: register "PcieRpEnable[0]" = "1" : register "PcieRpEnable[2]" = "1" The PCI devices for these RPs are disabled, why?
https://review.coreboot.org/c/coreboot/+/56088/comment/6cf345b6_da9a796c PS109, Line 82: register "PcieClkSrcUsage[0]" = "0x40" TGL-U doesn't have PEG (PCI Express Graphics, i.e. CPU PCIe ports).
https://review.coreboot.org/c/coreboot/+/56088/comment/c453b44d_3fa6f787 PS109, Line 236: device pci 1f.4 off end # SMBus Shouldn't SMBus be enabled? It's used to read the DIMM SPDs
File src/mainboard/starlabs/labtop/variants/tgl/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/56088/comment/c5eb76a5_1287258c PS109, Line 8: 0x10ec1200 Why is this value different from the one in the `AZALIA_SUBVENDOR` macro?
https://review.coreboot.org/c/coreboot/+/56088/comment/aa01cba3_21054afe PS109, Line 7: 0x10ec0256, // Codec Vendor / Device ID: Realtek ALC256 : 0x10ec1200, // Subsystem ID : 38, // Number of jacks (NID entries) Please use consistent comment style
-
Angel Pons (Code Review)