[coreboot-gerrit] Change in coreboot[master]: src/soc/amd/stoneyridge: cleanup southbridge.c
Martin Roth (Code Review)
gerrit at coreboot.org
Mon Mar 5 17:49:42 CET 2018
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/southbridge.h
File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/include/soc/southbridge.h@34
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/southbridge.h@368
PS1, Line 368:
Tabs instead of spaces please
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c
File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@300
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.c@310
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.c@320
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/bootblock/bootblock.c#n48
https://review.coreboot.org/#/c/24998/1/src/soc/amd/stoneyridge/southbridge.c@476
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.c@551
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?
--
To view, visit https://review.coreboot.org/24998
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: I45c1d1d7edc864000282c7ca4e2b8f2a14ea9eac
Gerrit-Change-Number: 24998
Gerrit-PatchSet: 1
Gerrit-Owner: Garrett Kirkendall <garrett.kirkendall at amd.corp-partner.google.com>
Gerrit-Reviewer: Marshall Dawson <marshall.dawson at amd.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Mon, 05 Mar 2018 16:49:42 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180305/5dcb2ace/attachment.html>
More information about the coreboot-gerrit
mailing list