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 10:
(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 340: DN_20K
So, the strapping pins actually could have some internal pull auto configured when they get sampled. […]
Maybe that's what they're referring to on the schematic... sounds reasonable.
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?
I believe so; once it's inverted, the signal changes name to "DISP_ON" and goes to the display connector (I still haven't seen the schematic for the I/O board yet...)
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 97: D3_RST
This could be D3 cold power sequence pin. We not implement yet.. […]
I think Eric is right, it's connected to the reset signals for the SSD and card reader, so if it needs to keep state in D3 cold then DEEP is appropriate.
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?
I'm starting to think the notes about the internal PD/PU on the strap pins were unrelated to the pull to be programmed. Removing.
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 125: 0
C8 should be high here for the power sequence.
Done
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 126: SBIOS_TX
What is this used for?
I don't know; it goes to the OEM's JTAG connector; it has a weak pull up to 3.3V. I'm guessing it is their BIOS UART output?
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 201: EDGE_SINGLE
LEVEL?
Done
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 250: UP_20K
Required?
Done
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 254: UP_20K
Same here.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39674/10/src/mainboard/google/delta... PS10, Line 483: * mupd->FspmConfig.DisableDimmChannel0 = gpio_get(DDR_CHA_EN) ? 0 : 3;
Header get update in https://review.coreboot.org/c/coreboot/+/39797. […]
Ack. I will update this patch and we can add the memory init in another one. I think Intel was working on the patches needed for DRAM.
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?
Done