Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39674 )
Change subject: mb/google/deltaur: Add initial GPIO configuration ......................................................................
Patch Set 7:
(11 comments)
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 25: PLTRST
What is the reason behind configuring this as PLTRST?
The EEs provided us a GPIO table with the listed reset source for each. Although the schematic and the GPIO table disagree about whether this is an input or output. From the schematic, this looks like an output to EC, presumably so it can power sequence the CNVi?
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 31: PLTRST
Same here and the rest of entries below. […]
PLTRST also controls power to at least the touch screen, SSD, and WLAN. No point in saving the state beyond PLTRST any way then.
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 56: programmed in early table
We should still configure all pads in ramstage. […]
Done
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 81: EDGE_SINGLE
We have been using LEVEL at the pad so that the signal just goes unmodified to the APIC and the trig […]
Done
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 134: 0
SSD power being set to 0? Do you intend to change this later?
I think I must have misread a # at the end 😊
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 147: early table
Please see my comment above about configuring all GPIOs in this table.
Done
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 196: PAD_CFG_GPI_SCI_LOW
Why SCI? Touchscreen is not a wake source. This can simply be PAD_CFG_GPI_APIC.
Done
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 198: EDGE_SINGLE
LEVEL
Done
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 208: EDGE_SINGLE
LEVEL
Also I think this is the one that should be PAD_CFG_GPI_IRQ_WAKE
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 340: DN_20K
Why does this have an internal PD?
The schematic has it labeled that way, "WEAK INTERNAL PD 20K". All of the Strapping Pins are labeled so as to use the internal pulls. I'm not sure why, as I don't think the strapping pins matter (for strapping purposes) by the time we get to ramstage 😊 (coincidence?)
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 411: EDGE_SINGLE
LEVEL
Whoops