Martin Roth has posted comments on this change. ( https://review.coreboot.org/24998 )
Change subject: src/soc/amd/stoneyridge: cleanup southbridge.c ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/#/c/24998/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/24998/1//COMMIT_MSG@10 PS1, Line 10: Add code to enable decode of FCH ACPI : MMIO regions. Factor out to functions, device power-on : code for AMBA and UART. Add FCH I2C device power-on code. Add code for : FCH to decode TPM memory and IO regions. A lot of these changes aren't really cleanup. Most of them should go in individual commits.
Also typically you'd want to make this into a list format:
* Limit dependency on vendorcode header files and add appropriate definitions to southbridge.h. * Factor out to functions, device power-on code for AMBA and UART.
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 34: /* FCH ACPI MMIO register base addresses */ : #define ACPI_MMIO_BASE 0xFED80000ul : #define FCH_ACPI_MMIO_SMBUS_PCI_BASE ACPI_MMIO_BASE + 0x000 : #define FCH_ACPI_MMIO_SMI_BASE ACPI_MMIO_BASE + 0x200 : #define FCH_ACPI_MMIO_PMIO_BASE ACPI_MMIO_BASE + 0x300 : #define FCH_ACPI_MMIO_PMIO2_BASE ACPI_MMIO_BASE + 0x400 : #define FCH_ACPI_MMIO_BIOS_RAM_BASE ACPI_MMIO_BASE + 0x500 : #define FCH_ACPI_MMIO_CMOS_RAM_BASE ACPI_MMIO_BASE + 0x600 : #define FCH_ACPI_MMIO_CMOS_BASE ACPI_MMIO_BASE + 0x700 : #define FCH_ACPI_MMIO_ASF_BASE ACPI_MMIO_BASE + 0x900 : #define FCH_ACPI_MMIO_SMBUS_BASE ACPI_MMIO_BASE + 0xA00 : #define FCH_ACPI_MMIO_WATCHDOG_BASE ACPI_MMIO_BASE + 0xB00 : #define FCH_ACPI_MMIO_HPET_BASE ACPI_MMIO_BASE + 0xC00 : #define FCH_ACPI_MMIO_IOMUX_BASE ACPI_MMIO_BASE + 0xD00 : #define FCH_ACPI_MMIO_MISC_BASE ACPI_MMIO_BASE + 0xE00 : #define FCH_ACPI_MMIO_GFX_DAC_BASE ACPI_MMIO_BASE + 0x1400 : #define FCH_ACPI_MMIO_GPIO_BANK0_BASE ACPI_MMIO_BASE + 0x1500 : #define FCH_ACPI_MMIO_GPIO_BANK1_BASE ACPI_MMIO_BASE + 0x1600 : #define FCH_ACPI_MMIO_GPIO_BANK2_BASE ACPI_MMIO_BASE + 0x1700 : #define FCH_ACPI_MMIO_XHCI_BASE ACPI_MMIO_BASE + 0x1C00 : #define FCH_ACPI_MMIO_ACDC_BASE ACPI_MMIO_BASE + 0x1D00 : #define FCH_ACPI_MMIO_AOAC_BASE ACPI_MMIO_BASE + 0x1E00 All of these should go in stoneyridge/include/soc/iomap.h. Some of these are already defined:
#define AMD_SB_ACPI_MMIO_ADDR 0xfed80000 #define APU_SMI_BASE 0xfed80200 #define PM_MMIO_BASE 0xfed80300 #define BIOSRAM_MMIO_BASE 0xfed80500 #define XHCI_ACPI_PM_MMIO_BASE 0xfed81c00 #define APU_UART0_BASE 0xfedc6000 #define APU_UART1_BASE 0xfedc8000
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 368: Tabs instead of spaces please
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 300: void configure_stoneyridge_i2c(void) Add in a separate commit please.
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 310: /* Wait for the I2C devices to indicate power and clock OK */ : do { : udelay(100); : status = is_aoac_device_enabled(AOAC_D3_STATE_I2C0); : status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C1); : status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C2); : status &= is_aoac_device_enabled(AOAC_D3_STATE_I2C3); : } while (!(status)); Do we need to delay here? Are these devices used immediately?
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 320: void sb_acpi_mmio_decode(void) : { : u8 byte; : : /* Enable ACPI MMIO range 0xFED80000 - 0xFED81FFF */ : outb(PM_ISA_CONTROL, PM_INDEX); : byte = inb(PM_DATA); : byte |= MMIO_EN; : outb(PM_ISA_CONTROL, PM_INDEX); : outb(byte, PM_DATA); : } 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.
https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/stoneyridge/b...
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 476: void sb_tpm_decode(void) : { : /* Enable TPM decoding to FCH */ : u32 value = pci_read_config32(SOC_LPC_DEV, : LPC_TRUSTED_PLATFORM_MODULE); : value |= (TPM_12_EN | TPM_LEGACY_EN); : pci_write_config32(SOC_LPC_DEV, LPC_TRUSTED_PLATFORM_MODULE, value); : } : : void sb_tpm_decode_spi(void) : { : /* Enable TPM decoding to FCH */ : sb_tpm_decode(); : : /* Make sure SPI base register is programmed */ : sb_spibase(); : : /* Route TPM accesses to SPI */ Can we do these in a separate commit?
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 551: sb_set_spi100(SPI_SPEED_33M, : SPI_SPEED_33M, : SPI_SPEED_16M, : SPI_SPEED_16M); Can we do this in a separate commit?
-
Martin Roth (Code Review)