Attention is currently required from: Raul Rangel, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74095 )
Change subject: mb/google/myst: First pass GPIO configuration for Myst
......................................................................
Patch Set 7:
(3 comments)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74095/comment/1c6087f3_85a84e7e
PS7, Line 60: HIGH)
> Active High. So please leave it as LOW.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/0bb73a4a_66b54522
PS7, Line 64: HIGH
> Active High. So please leave it as LOW.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/12f3cc69_2d51c1b1
PS7, Line 75: /* SOC_MEM_VID_C1 */
: PAD_GPI(GPIO_42, PULL_NONE),
> This should also be configured by SMU and not touched by coreboot.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/74095
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia019704c7b027f14d46281e0de0ffdbc4906a20b
Gerrit-Change-Number: 74095
Gerrit-PatchSet: 7
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 17:11:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74095 )
Change subject: mb/google/myst: First pass GPIO configuration for Myst
......................................................................
Patch Set 7:
(10 comments)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74095/comment/09e3917c_fbe3b5fc
PS7, Line 17: GPIO_3
> GPIO_7
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/4b534e5e_5de1d8e4
PS7, Line 17: PAD_WAKE(GPIO_3, PULL_NONE, EDGE_LOW, S0i3),
> As per the schematics, SOC_PEN_DETECT_ODL is in AGPIO7 and this is unused.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/47c7eba6_db5d4c0d
PS7, Line 22: EN_PWR_WWAN_X
> Leave it LOW so that on SKUs without WWAN module, we are not leaking power. […]
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/1cda60e7_f7976723
PS7, Line 30: /* Unused */
: PAD_NC(GPIO_10),
> Coreboot should not touch it. This is handled by SMU and should be left as is.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/29f30215_3004e344
PS7, Line 32: WWAN_RST_L
> This is listed as just `WWAN_RST`, no `_L`.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/2a8486d5_d8e61c41
PS7, Line 33: HIGH
> Update this to `LOW`, since this is not active low (assuming documentation is correct).
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/88cec3fd_bc82c7e3
PS7, Line 39: EC_SOC_WAKE_R_L
> `EC_SOC_WAKE_R_ODL`
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/bcea0ee9_c8583bd1
PS7, Line 52: PULL_UP
> There is an external pull-up in the schematics. So I dont think an internal pull-up is required.
Done
https://review.coreboot.org/c/coreboot/+/74095/comment/ce4a214f_e8fc0326
PS7, Line 80: /* ESPI1_DATA2 */
: PAD_NF(GPIO_68, SPI1_DAT2, PULL_NONE),
: /* ESPI1_DATA3 */
: PAD_NF(GPIO_69, SPI1_DAT3, PULL_NONE),
> These are not defined in go/myst-gpios.
They are defined in go/myst-gpios as AGPIO68/AGPIO69 Cumulative GPIO's 31 and 32
https://review.coreboot.org/c/coreboot/+/74095/comment/c06464ba_11c8bc0b
PS7, Line 116: /* SPI_SOC_DO_TCHSCR_DO_R */
: PAD_NF(GPIO_105, SPI2_DAT1, PULL_NONE),
> Not defined in go/myst-gpios.
Defined as EGPIO105 at cumulative GPIO 57
--
To view, visit https://review.coreboot.org/c/coreboot/+/74095
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia019704c7b027f14d46281e0de0ffdbc4906a20b
Gerrit-Change-Number: 74095
Gerrit-PatchSet: 7
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 17:10:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jon Murphy, Paul Menzel, Karthik Ramasubramanian, Mark Hasemeyer.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74106 )
Change subject: mb/google/myst: Add ACPI configuration for USB ports
......................................................................
Patch Set 12:
(2 comments)
File src/mainboard/google/myst/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/74106/comment/c291f711_206c3960
PS12, Line 9: device ref iommu on end
ref1
https://review.coreboot.org/c/coreboot/+/74106/comment/90573791_05268722
PS12, Line 89: device ref iommu on end
> It is on Skyrim? https://source.corp.google. […]
You have it above already. See `ref1`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/74106
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iecb256cad7b2daea1fddfc8323e88ff5c38d1e51
Gerrit-Change-Number: 74106
Gerrit-PatchSet: 12
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 17:06:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jon Murphy <jpmurphy(a)google.com>
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jon Murphy, Tim Van Patten, Mark Hasemeyer.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74095 )
Change subject: mb/google/myst: First pass GPIO configuration for Myst
......................................................................
Patch Set 7:
(3 comments)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74095/comment/3d5080b9_c6cbd415
PS7, Line 60: HIGH)
Active High. So please leave it as LOW.
https://review.coreboot.org/c/coreboot/+/74095/comment/b07eb643_276f6550
PS7, Line 64: HIGH
Active High. So please leave it as LOW.
https://review.coreboot.org/c/coreboot/+/74095/comment/7f47989b_00a5422c
PS7, Line 75: /* SOC_MEM_VID_C1 */
: PAD_GPI(GPIO_42, PULL_NONE),
This should also be configured by SMU and not touched by coreboot.
--
To view, visit https://review.coreboot.org/c/coreboot/+/74095
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia019704c7b027f14d46281e0de0ffdbc4906a20b
Gerrit-Change-Number: 74095
Gerrit-PatchSet: 7
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 17:06:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74113 )
Change subject: mb/google/myst: Configure WLAN
......................................................................
Patch Set 16:
(1 comment)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74113/comment/4e000c18_21d3ad3c
PS16, Line 194: WLAN_AUX_RESET_L
> nit: `WLAN_AUX_RST_L` […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/74113
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5059be0bc011978e74ab4245e6ae037aa177ef9b
Gerrit-Change-Number: 74113
Gerrit-PatchSet: 16
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 17:03:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Paul Menzel, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74106 )
Change subject: mb/google/myst: Add ACPI configuration for USB ports
......................................................................
Patch Set 12:
(1 comment)
File src/mainboard/google/myst/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/74106/comment/2f3943c2_95de2d85
PS12, Line 89: device ref iommu on end
> Do we need this line also? Skyrim does not have it.
It is on Skyrim? https://source.corp.google.com/chromeos_public/src/third_party/coreboot/src…
You also asked about adding it?
https://review.coreboot.org/c/coreboot/+/74112/8..16/src/mainboard/google/m…
--
To view, visit https://review.coreboot.org/c/coreboot/+/74106
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iecb256cad7b2daea1fddfc8323e88ff5c38d1e51
Gerrit-Change-Number: 74106
Gerrit-PatchSet: 12
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 16:57:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jon Murphy, Karthik Ramasubramanian, Mark Hasemeyer.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74113 )
Change subject: mb/google/myst: Configure WLAN
......................................................................
Patch Set 16: Code-Review+1
(1 comment)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74113/comment/fdbdde70_4077dc5f
PS16, Line 194: WLAN_AUX_RESET_L
nit: `WLAN_AUX_RST_L`
Match the spreadsheet naming for both comments.
--
To view, visit https://review.coreboot.org/c/coreboot/+/74113
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5059be0bc011978e74ab4245e6ae037aa177ef9b
Gerrit-Change-Number: 74113
Gerrit-PatchSet: 16
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 16:51:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jon Murphy, Tim Van Patten, Mark Hasemeyer.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74095 )
Change subject: mb/google/myst: First pass GPIO configuration for Myst
......................................................................
Patch Set 7:
(1 comment)
File src/mainboard/google/myst/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74095/comment/840b78cb_64c8b5f9
PS7, Line 52: PULL_UP
There is an external pull-up in the schematics. So I dont think an internal pull-up is required.
--
To view, visit https://review.coreboot.org/c/coreboot/+/74095
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia019704c7b027f14d46281e0de0ffdbc4906a20b
Gerrit-Change-Number: 74095
Gerrit-PatchSet: 7
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 16:44:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment