Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38620 )
Change subject: mb/google/volteer: add volteer mainboard initial support ......................................................................
Patch Set 8:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl:
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 16: Scope (_SB.PCI0) : { : Device (IPU0) : { : Name (_ADR, 0x00050000) : Name (_DDN, "Camera and Imaging Subsystem") : } : } This should not be added to mainboard. It should be in SoC. In general, can you please add mipi_camera.asl in a follow-up CL?
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright 2020 Google LLC : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ Coreboot is moving towards SPDX headers.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 46: Scope (_SB) { : Device (PEPD) : { : Name (_HID, "INT33A1" /* Intel Power Engine */) // _HID: Hardware ID : Name (_CID, EisaId ("PNP0D80") /* System Power Management Controller */) // _CID: Compatible ID : Name (_UID, One) // _UID: Unique ID : Name (PPD0, Package (0x03) : { : "\_SB.PC00.SAT0", : Zero, : Package (0x02) : { : Zero, : Package (0x03) : { : 0xFF, : Zero, : 0x81 : } : } : }) : } : } This is not a mainboard device. This should not be added here.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 71: mainboard_ec_init(); Any reason why this has to be done as part of chip init? I believe this can be done as part of dev init. Please see hatch as an example.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 36: register "pch_isclk" = "1" I am really not sure if this is correct. AFAIU, camera clocks are enabled using the ASL. Any ways, at minimum can you please raise a bug to revisit this later to check if it is really correct for volteer?
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 53: register "sdcard_cd_gpio" = "GPP_E11" I don't think this should be required. It was useful for native SD controllers. But, volteer seems to be using PCIe-to-SD card which can use in-band wake.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 115: 1 It would be good to have a comment indicating that this is eDP.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 149: Please use tabs instead of spaces here and in the rest of the file.
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 186: .pch_thermal_trip = 75, This is not even used for TGL. Can you please add it when required?
https://review.coreboot.org/c/coreboot/+/38620/8/src/mainboard/google/voltee... PS8, Line 235: chip drivers/i2c/generic : register "hid" = ""10EC5682"" : register "name" = ""RT58"" : register "desc" = ""Realtek RT5682"" : register "irq" = "ACPI_IRQ_EDGE_HIGH(GPP_F8_IRQ)" : # Set the jd_src to RT5668_JD1 for jack detection : register "property_count" = "1" : register "property_list[0].type" = "ACPI_DP_TYPE_INTEGER" : register "property_list[0].name" = ""realtek,jd-src"" : register "property_list[0].integer" = "1" : device i2c 1a on end : end Note for future work: All the devices for audio, sar, etc. which are being added here should move to variant overridetree.cb. Not all variants would use the same devices.