<p><a href="https://review.coreboot.org/24998">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24998/1//COMMIT_MSG">Commit Message:</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/24998/1//COMMIT_MSG@10">Patch Set #1, Line 10:</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;"> Add code to enable decode of FCH ACPI<br>MMIO regions. Factor out to functions, device power-on<br>code for AMBA and UART. Add FCH I2C device power-on code. Add code for<br>FCH to decode TPM memory and IO regions.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">A lot of these changes aren't really cleanup. Most of them should go in individual commits.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also typically you'd want to make this into a list format:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * Limit dependency on vendorcode header files and add<br> appropriate definitions to southbridge.h.<br> * Factor out to functions, device power-on code for AMBA<br> and UART.</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/southbridge.h">File src/soc/amd/stoneyridge/include/soc/southbridge.h:</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/24998/1/src/soc/amd/stoneyridge/include/soc/southbridge.h@34">Patch Set #1, Line 34:</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;">/* FCH ACPI MMIO register base addresses */<br>#define ACPI_MMIO_BASE 0xFED80000ul<br>#define FCH_ACPI_MMIO_SMBUS_PCI_BASE ACPI_MMIO_BASE + 0x000<br>#define FCH_ACPI_MMIO_SMI_BASE ACPI_MMIO_BASE + 0x200<br>#define FCH_ACPI_MMIO_PMIO_BASE ACPI_MMIO_BASE + 0x300<br>#define FCH_ACPI_MMIO_PMIO2_BASE ACPI_MMIO_BASE + 0x400<br>#define FCH_ACPI_MMIO_BIOS_RAM_BASE ACPI_MMIO_BASE + 0x500<br>#define FCH_ACPI_MMIO_CMOS_RAM_BASE ACPI_MMIO_BASE + 0x600<br>#define FCH_ACPI_MMIO_CMOS_BASE ACPI_MMIO_BASE + 0x700<br>#define FCH_ACPI_MMIO_ASF_BASE ACPI_MMIO_BASE + 0x900<br>#define FCH_ACPI_MMIO_SMBUS_BASE ACPI_MMIO_BASE + 0xA00<br>#define FCH_ACPI_MMIO_WATCHDOG_BASE ACPI_MMIO_BASE + 0xB00<br>#define FCH_ACPI_MMIO_HPET_BASE ACPI_MMIO_BASE + 0xC00<br>#define FCH_ACPI_MMIO_IOMUX_BASE ACPI_MMIO_BASE + 0xD00<br>#define FCH_ACPI_MMIO_MISC_BASE ACPI_MMIO_BASE + 0xE00<br>#define FCH_ACPI_MMIO_GFX_DAC_BASE ACPI_MMIO_BASE + 0x1400<br>#define FCH_ACPI_MMIO_GPIO_BANK0_BASE ACPI_MMIO_BASE + 0x1500<br>#define FCH_ACPI_MMIO_GPIO_BANK1_BASE ACPI_MMIO_BASE + 0x1600<br>#define FCH_ACPI_MMIO_GPIO_BANK2_BASE ACPI_MMIO_BASE + 0x1700<br>#define FCH_ACPI_MMIO_XHCI_BASE ACPI_MMIO_BASE + 0x1C00<br>#define FCH_ACPI_MMIO_ACDC_BASE ACPI_MMIO_BASE + 0x1D00<br>#define FCH_ACPI_MMIO_AOAC_BASE ACPI_MMIO_BASE + 0x1E00<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of these should go in stoneyridge/include/soc/iomap.h. Some of these are already defined:</p><p style="white-space: pre-wrap; word-wrap: break-word;">#define AMD_SB_ACPI_MMIO_ADDR 0xfed80000<br>#define APU_SMI_BASE 0xfed80200<br>#define PM_MMIO_BASE 0xfed80300<br>#define BIOSRAM_MMIO_BASE 0xfed80500<br>#define XHCI_ACPI_PM_MMIO_BASE 0xfed81c00<br>#define APU_UART0_BASE 0xfedc6000<br>#define APU_UART1_BASE 0xfedc8000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/southbridge.h@368">Patch Set #1, Line 368:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Tabs instead of spaces please</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c">File src/soc/amd/stoneyridge/southbridge.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/24998/1/src/soc/amd/stoneyridge/southbridge.c@300">Patch Set #1, Line 300:</a> <code style="font-family:monospace,monospace">void configure_stoneyridge_i2c(void)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add in a separate commit please.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@310">Patch Set #1, Line 310:</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;"> /* Wait for the I2C devices to indicate power and clock OK */<br> do {<br> udelay(100);<br> status = is_aoac_device_enabled(AOAC_D3_STATE_I2C0);<br> status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C1);<br> status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C2);<br> status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C3);<br> } while (!(status));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do we need to delay here? Are these devices used immediately?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@320">Patch Set #1, Line 320:</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;">void sb_acpi_mmio_decode(void)<br>{<br> u8 byte;<br><br> /* Enable ACPI MMIO range 0xFED80000 - 0xFED81FFF */<br> outb(PM_ISA_CONTROL, PM_INDEX);<br> byte = inb(PM_DATA);<br> byte |= MMIO_EN;<br> outb(PM_ISA_CONTROL, PM_INDEX);<br> outb(byte, PM_DATA);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">While adding this is fine, i don't think it's needed here since it's one of the first chipset specific things we do in bootblock. It should also go into a separate commit.</p><p style="white-space: pre-wrap; word-wrap: break-word;">https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/stoneyridge/bootblock/bootblock.c#n48</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@476">Patch Set #1, Line 476:</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;">void sb_tpm_decode(void)<br>{<br> /* Enable TPM decoding to FCH */<br> u32 value = pci_read_config32(SOC_LPC_DEV,<br> LPC_TRUSTED_PLATFORM_MODULE);<br> value |= (TPM_12_EN | TPM_LEGACY_EN);<br> pci_write_config32(SOC_LPC_DEV, LPC_TRUSTED_PLATFORM_MODULE, value);<br>}<br><br>void sb_tpm_decode_spi(void)<br>{<br> /* Enable TPM decoding to FCH */<br> sb_tpm_decode();<br><br> /* Make sure SPI base register is programmed */<br> sb_spibase();<br><br> /* Route TPM accesses to SPI */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can we do these in a separate commit?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@551">Patch Set #1, Line 551:</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;"> sb_set_spi100(SPI_SPEED_33M,<br> SPI_SPEED_33M,<br> SPI_SPEED_16M,<br> SPI_SPEED_16M);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can we do this in a separate commit?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/24998">change 24998</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/24998"/><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: I45c1d1d7edc864000282c7ca4e2b8f2a14ea9eac </div>
<div style="display:none"> Gerrit-Change-Number: 24998 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Garrett Kirkendall <garrett.kirkendall@amd.corp-partner.google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Marshall Dawson <marshall.dawson@amd.corp-partner.google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 05 Mar 2018 16:49:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>