Nick Vaccaro 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 is already in soc folder. Refer https://review.coreboot. […]
Done
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.
Done
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.
Done
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 i […]
Done
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. […]
https://b.corp.google.com/issues/148884060
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. […]
Done
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.
Done
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.
Done
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. […]
Done
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. […]
Ack