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 11: Code-Review+1
(6 comments)
Overall looks okay to me. Just added some comments about reset configuration v/s state in S3.
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 126: SBIOS_TX
I don't know; it goes to the OEM's JTAG connector; it has a weak pull up to 3.3V. […]
Oh okay. Might have to revisit this later based on the usage.
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... PS11, Line 25: PLTRST Thinking about this again. I don't think you want to disable CNVi power in S3. It can be a wake source.
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... PS11, Line 95: nit: missing )
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... PS11, Line 135: PLTRST This will cause SSD power to be lost in S3 as well. I don't think that is the intent?
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... PS11, Line 192: PLTRST Isn't ISH active in S3? Wouldn't you want these configurations to hold true in that case as well?
https://review.coreboot.org/c/coreboot/+/39674/11/src/mainboard/google/delta... PS11, Line 201: NONE INVERT