Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39674 )
Change subject: mb/google/deltaur: Add initial GPIO configuration ......................................................................
Patch Set 10:
(13 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
The EEs provided us a GPIO table with the listed reset source for each. […]
Interesting. Humm.. It would be good to understand how this pin actually gets used.
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 31: PLTRST
PLTRST also controls power to at least the touch screen, SSD, and WLAN. […]
That sounds mostly fine.
Thinking out loud: Difference in reset config between DEEP and PLTRST is S3/S4/S5 i.e. DEEP retains state across trip to S3/S4/S5. On the other hand, PLTRST results in GPIO pad config resetting to default on entry into S3/S4/S5.
When you say power to these devices(WLAN,SSD,touchscreen) is controlled by PLTRST#, does it mean that PLTRST# is like a master switch for enabling the VRs for them? If yes, then this should work okay. It would be good to ensure that we still do not violate any power sequencing.
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 208: EDGE_SINGLE
Also I think this is the one that should be PAD_CFG_GPI_IRQ_WAKE
Correct.
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 340: DN_20K
The schematic has it labeled that way, "WEAK INTERNAL PD 20K". […]
So, the strapping pins actually could have some internal pull auto configured when they get sampled. This is to guarantee some default state of the straps in case there are no external pulls. However, that should be independent of what coreboot is configuring.
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 65: TOUCH_SCREEN_PD Does PD mean power down?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 97: D3_RST I am not sure what the usage is. Is PLTRST the right setting for this?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 106: DN_20K I don't think we need an internal pull here?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 125: 0 Assuming you plan to do this as a follow-up?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 126: SBIOS_TX What is this used for?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 201: EDGE_SINGLE LEVEL?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 250: UP_20K Required?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 254: UP_20K Same here.
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 262: DN_20K I don't think this is needed. Same for other straps.