Martin Roth has posted comments on this change. ( https://review.coreboot.org/19722 )
Change subject: soc: Add AMD StoneyRidge southbridge code ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/#/c/19722/3/src/soc/amd/common/Kconfig File src/soc/amd/common/Kconfig:
Line 5 You might want to avoid the issues that are being seen currently in soc/intel/common and not have this Kconfig file here. The issue is that chips in soc/amd/[ab].* will be able to override defaults set in this file, but Kconfig files that get sourced later (soc/amd/[d-z].*) will NOT be able to override these defaults.
Because we're automatically sourcing soc/*/*/Kconfig, the order here can't be changed easily except by not having a Kconfig file in src/amd/common.
I'd recommend not having any files in this directory, but if we do, you could help things by adding a comment here:
# Caution: Do NOT add any default values to this file. # Default values here cannot be overridden by Kconfig files # that are sourced later in the soc/amd directory.
My recommendation would be to move this Kconfig file to soc/amd/common/util or soc/amd/common/devices and source src/amd/common/*/Kconfig from the top level Kconfig at an appropriate point.
https://review.coreboot.org/#/c/19722/3/src/soc/amd/common/amd_pci_util.h File src/soc/amd/common/amd_pci_util.h:
PS3, Line 16: AMD_PCI_UTIL_H Update to SOC_AMD_PCI_UTIL_H?
https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/chip.c File src/soc/amd/stoneyridge/chip.c:
Line 60: extra line
https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/hudson.h:
PS3, Line 17: STONEYRIDGE_H Should the name of the file be updated?
https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/pci_devs.h:
PS3, Line 74: #if IS_ENABLED(CONFIG_SOUTHBRIDGE_AMD_PI_BOLTON) : #define IDE_DEV 0x14 : #define IDE_FUNC 1 : #define IDE_DEVID 0x780C : #define IDE_DEVFN PCI_DEVFN(IDE_DEV,IDE_FUNC) : #endif remove?
PS3, Line 106: #if IS_ENABLED(CONFIG_SOUTHBRIDGE_AMD_PI_BOLTON) : #define SB_PCIE_DEV 0x15 : #define SB_PCIE_PORT1_FUNC 0 : #define SB_PCIE_PORT2_FUNC 1 : #define SB_PCIE_PORT3_FUNC 2 : #define SB_PCIE_PORT4_FUNC 3 : #define SB_PCIE_PORT1_DEVID 0x7820 : #define SB_PCIE_PORT2_DEVID 0x7821 : #define SB_PCIE_PORT3_DEVID 0x7822 : #define SB_PCIE_PORT4_DEVID 0x7823 : #define SB_PCIE_PORT1_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT1_FUNC) : #define SB_PCIE_PORT2_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT2_FUNC) : #define SB_PCIE_PORT3_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT3_FUNC) : #define SB_PCIE_PORT4_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT4_FUNC) : #endif remove?
https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/smi.h:
PS3, Line 8: _SOUTHBRIDGE_AMD_PI_STONEYRIDGE_SMI_H _SOC_AMD_PI_STONEYRIDGE_SMI_H
And could we be consistent about the naming on these? Some have no leading or trailing underscores, some have one, some have both.
https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smbus.c File src/soc/amd/stoneyridge/smbus.c:
PS3, Line 16: #ifndef _STONEYRIDGE_SMBUS_C_ : #define _STONEYRIDGE_SMBUS_C_ Let's not add another place where we're including a c file. These shouldn't be needed.
-
Martin Roth (Code Review)