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 7:
(12 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?
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 31: PLTRST Same here and the rest of entries below. I am mostly referring to GPIOs that are acting as output from the SoC and are reset/enable lines. Is the intent that those should return to their default state on PLTRST so that it can power cycle or reset external devices? Do all these lines have external pulls to guarantee this happens?
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. This is also helpful in case any gpio has to be overriden by variant.
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 trigger is applied at the APIC.
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?
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.
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.
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 198: EDGE_SINGLE LEVEL
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 208: EDGE_SINGLE LEVEL
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 340: DN_20K Why does this have an internal PD?
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 411: EDGE_SINGLE LEVEL
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/39674/7/src/mainboard/google/deltau... PS7, Line 26: void variant_mainboard_post_init_params(FSPM_UPD *mupd); Not related to this change?