[coreboot-gerrit] Change in coreboot[master]: mb/google/kahlee/variants/baseboard/gpio.c:move all non-critical gpios

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/25395 )

Change subject: mb/google/kahlee/variants/baseboard/gpio.c:move all non-critical gpios
......................................................................


Patch Set 1:

(14 comments)

https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c
File src/mainboard/google/kahlee/variants/baseboard/gpio.c:

https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a255
PS1, Line 255: 
             : 
I think we want this set up early.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a258
PS1, Line 258: 
             : 
This one's like the wifi - we probably want it set up early.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a267
PS1, Line 267: 
             : 
Why move the LPC/EC setup to late?  We're using the EC early, so shouldn't we make sure it's set up early?
74, 87, 88


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a285
PS1, Line 285: 
             : 
I'd think we'd want this set up early too, to match with GPIO4.  It probably defaults  to clkreq, so it would only be a problem if it were set to some other function in the OS.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a306
PS1, Line 306: 
             : 
Is this CS0 or CS1?  the comment doesn't match the code. I know this is unrelated to your change.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a324
PS1, Line 324: 
             : 
Don't move the config straps.  We need those to read memory.
131, 132, 129, 142


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@31
PS1, Line 31: 	 * Even though one would assume WLAN can be programmed later,
            : 	 * doing so will cause WIFI to fail.
That's a bad assumption.  The PCIe enables need to be set up early so that AGESA can detect them.  This has already been debugged.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@42
PS1, Line 42: 	/* GPIO_11 - TOUCHSCREEN_INT_3V3_ODL, SCI */
            : 	PAD_GPI(GPIO_11, PULL_UP),
All of the touchscreen interrupts should be able to be moved
11, 19, 20, 85.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@45
PS1, Line 45: 	/* GPIO_14 - APU_HP_INT_ODL, SCI */
            : 	PAD_GPI(GPIO_14, PULL_UP),
Headphone can be moved to late.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@69
PS1, Line 69: 	/* GPIO_95 - SD_CLK */
            : 	/* GPIO_96 - SD_CMD */
            : 	/* GPIO_97 - SD_D0 */
            : 	/* GPIO_98 - SD_D1 */
            : 	/* GPIO_99 - SD_D2 */
            : 	/* GPIO_100 - SD_D3 */
            : 	/* GPIO_117 - PCH_SPI_CLK_R */
            : 	/* GPIO_120 - PCH_SPI_MISO */
            : 	/* GPIO_121 - PCH_SPI_MOSI */
Go ahead and delete the comments please.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@88
PS1, Line 88: 	/* GPIO_140 - I2S_BCLK_R (init to func0, used for I2S) */
            : 	PAD_NF(GPIO_140, UART1_CTS_L, PULL_NONE),
            : 
            : 	/* GPIO_141 - I2S2_DATA_MIC2 (init to func0, used for I2S) */
            : 	PAD_NF(GPIO_141, UART1_RXD, PULL_NONE),
These can be moved.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@134
PS1, Line 134: 	/* GPIO_84 - HUB_RST (Active High) */
             : 	PAD_GPO(GPIO_84, LOW),
The USB hub reset line can be moved to late.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@140
PS1, Line 140: /* GPIO_91 - DMIC_CLK1_EN */
             : 	PAD_GPO(GPIO_91, HIGH),
The Microphone init can safely be moved to late.


https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@165
PS1, Line 165: 	/* GPIO_141 - I2S2_DATA_MIC2 (init to func0, used for I2S) */
             : 	PAD_NF(GPIO_141, UART1_RXD, PULL_NONE),
This microphone init can be move to late too.



-- 
To view, visit https://review.coreboot.org/25395
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icbc9370050d619800026035caaac3e89536a460a
Gerrit-Change-Number: 25395
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-CC: Martin Roth <martinroth at google.com>
Gerrit-Comment-Date: Wed, 28 Mar 2018 01:02:44 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180328/4a9770be/attachment.html>