Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43087 )
Change subject: Added GBYT4 port ......................................................................
Patch Set 5: Code-Review+1
(48 comments)
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/bostentech/gb... File src/mainboard/bostentech/gbyt4/gpio.c:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/bostentech/gb... PS1, Line 208: Done
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... File src/mainboard/bostentech/gbyt4/gpio.c:
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 7: GPIO_FUNC(2, PULL_UP, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_UP, 20K), I'd say these shouldn't be pulling anything
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 10: GPIO_FUNC(0, PULL_DISABLE, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), Uh, function 0 is reserved? No idea
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 19: GPIO_FUNC(1, PULL_DOWN, 20K), I'd try removing the pull
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 24: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), I'd say these don't need pull down
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 40: GPIO_FUNC(2, PULL_DOWN, 20K), I'd say this doesn't need a pull
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 41: GPIO_FUNC(1, PULL_UP, 20K), SATA LED, does it work as intended with this pull?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 41: : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), These are PCIe clock request pins. I'd drop the pull and see if the Ethernet NICs still work.
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 46: GPIO_FUNC(0, PULL_UP, 20K), SD-related, probably NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 47: GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), : GPIO_FUNC(2, PULL_DOWN, 20K), HDA-related, if there's no audio on your board then this should be NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 55: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), MMC-related, unused?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 66: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), SD2, unused
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 72: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), SD3, unused
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 81: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), LPC pins, use GPIO_FUNC1
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 86: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), More LPC clock stuff, they can also use GPIO_FUNC1
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 89: GPIO_FUNC(1, PULL_UP, 20K), SERIRQ, probably doesn't need pullups
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 90: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), SMBus, should be GPIO_FUNC1
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 93: GPIO_FUNC(1, PULL_DOWN, 20K) Speaker
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 94: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), Strap pins, probably unused
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 96: GPIO_FUNC(1, PULL_UP, 20K), Integrated UART TXD, likely NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 97: GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), Probably NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 100: GPIO_FUNC(1, PULL_DOWN, 20K), Integrated UART RXD, probably NC as well
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 101: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), I2S, pretty sure it's NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 105: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), SPI controller for dedicated hardware, not for the SPI flash so NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 109: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), UART1, most likely not connected
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 113: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), UART2, most likely NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 117: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), I2C ports for touchpad/touchscreen, most likely NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 117: GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(2, PULL_UP, 20K), : GPIO_FUNC(0, PULL_UP, 20K), : GPIO_FUNC(0, PULL_UP, 20K), I2C ports, unused
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 133: GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), PWM pins, probably unused because they are wired as GPIOs
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 135: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), Platform clocks, most likely NC.
Out of curiosity, I checked my Q1900M: it can either use these pins or a regular crystal, and it's using the latter.
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 147: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), This looks wrong, function 1 is reserved for these two
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 146: GPIO_FUNC(0, PULL_UP, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(0, PULL_UP, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), I'd say these are all NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 151: GPIO_FUNC(1, PULL_DOWN, 20K), GPIO_FUNC1 (PMC_SUSCLK1)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 152: GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), Most likely NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 154: GPIO_FUNC(0, PULL_DISABLE, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 156: GPIO_FUNC(0, PULL_UP, 20K), Probably NC
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 157: GPIO_FUNC(0, PULL_DOWN, 20K), GPIO_FUNC0 (SUSPWRDNACK)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 158: GPIO_FUNC(0, PULL_DOWN, 20K), GPIO_FUNC0 (SUSCLK)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 159: GPIO_FUNC(0, PULL_DOWN, 20K), NC, probably?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 160: GPIO_FUNC(2, PULL_DISABLE, 20K), GPIO_FUNC2 (USB_ULPI_RST#)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 161: GPIO_FUNC(0, PULL_UP, 20K), GPIO_FUNC0 (PMC_WAKE_PCIE[0])
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 162: GPIO_FUNC(0, PULL_UP, 20K), This is the power button that needs the pullup, right?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 163: GPIO_FUNC(1, PULL_UP, 20K), NC, most likely
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 164: GPIO_FUNC(0, PULL_DOWN, 20K), GPIO_FUNC0 (PMC_SUS_STAT#)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 165: GPIO_FUNC(0, PULL_UP, 20K), : GPIO_FUNC(0, PULL_UP, 20K), GPIO_FUNC0 (USB_OC0 and USB_OC1)
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 167: GPIO_FUNC(1, PULL_UP, 20K), NC?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 168: GPIO_FUNC(0, PULL_DISABLE, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_DOWN, 20K), : GPIO_FUNC(0, PULL_UP, 20K), : GPIO_FUNC(2, PULL_UP, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), : GPIO_FUNC(0, PULL_DISABLE, 20K), XDP stuff, most likely NC?
https://review.coreboot.org/c/coreboot/+/43087/5/src/mainboard/bostentech/gb... PS5, Line 177: GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_DISABLE, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), : GPIO_FUNC(1, PULL_UP, 20K), : GPIO_FUNC(1, PULL_DOWN, 20K), This is most likely NC as well, it's USB ULPI and would only be used in tablets, if at all...
http://web.archive.org/web/20131212023536/http://www.ulpi.org/