<p><a href="https://review.coreboot.org/25395">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c">File src/mainboard/google/kahlee/variants/baseboard/gpio.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a255">Patch Set #1, Line 255:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we want this set up early.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a258">Patch Set #1, Line 258:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This one's like the wifi - we probably want it set up early.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a267">Patch Set #1, Line 267:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?<br>74, 87, 88</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a285">Patch Set #1, Line 285:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a306">Patch Set #1, Line 306:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this CS0 or CS1? the comment doesn't match the code. I know this is unrelated to your change.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@a324">Patch Set #1, Line 324:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Don't move the config straps. We need those to read memory.<br>131, 132, 129, 142</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@31">Patch Set #1, Line 31:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * Even though one would assume WLAN can be programmed later,<br> * doing so will cause WIFI to fail.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@42">Patch Set #1, Line 42:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_11 - TOUCHSCREEN_INT_3V3_ODL, SCI */<br> PAD_GPI(GPIO_11, PULL_UP),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of the touchscreen interrupts should be able to be moved<br>11, 19, 20, 85.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@45">Patch Set #1, Line 45:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_14 - APU_HP_INT_ODL, SCI */<br> PAD_GPI(GPIO_14, PULL_UP),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Headphone can be moved to late.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@69">Patch Set #1, Line 69:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_95 - SD_CLK */<br> /* GPIO_96 - SD_CMD */<br> /* GPIO_97 - SD_D0 */<br> /* GPIO_98 - SD_D1 */<br> /* GPIO_99 - SD_D2 */<br> /* GPIO_100 - SD_D3 */<br> /* GPIO_117 - PCH_SPI_CLK_R */<br> /* GPIO_120 - PCH_SPI_MISO */<br> /* GPIO_121 - PCH_SPI_MOSI */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Go ahead and delete the comments please.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@88">Patch Set #1, Line 88:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_140 - I2S_BCLK_R (init to func0, used for I2S) */<br> PAD_NF(GPIO_140, UART1_CTS_L, PULL_NONE),<br><br> /* GPIO_141 - I2S2_DATA_MIC2 (init to func0, used for I2S) */<br> PAD_NF(GPIO_141, UART1_RXD, PULL_NONE),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These can be moved.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@134">Patch Set #1, Line 134:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_84 - HUB_RST (Active High) */<br> PAD_GPO(GPIO_84, LOW),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The USB hub reset line can be moved to late.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@140">Patch Set #1, Line 140:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/* GPIO_91 - DMIC_CLK1_EN */<br> PAD_GPO(GPIO_91, HIGH),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The Microphone init can safely be moved to late.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25395/1/src/mainboard/google/kahlee/variants/baseboard/gpio.c@165">Patch Set #1, Line 165:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* GPIO_141 - I2S2_DATA_MIC2 (init to func0, used for I2S) */<br> PAD_NF(GPIO_141, UART1_RXD, PULL_NONE),<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This microphone init can be move to late too.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/25395">change 25395</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/25395"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Icbc9370050d619800026035caaac3e89536a460a </div>
<div style="display:none"> Gerrit-Change-Number: 25395 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Spiegel <richard.spiegel@silverbackltd.com> </div>
<div style="display:none"> Gerrit-CC: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 28 Mar 2018 01:02:44 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>