[coreboot-gerrit] Change in coreboot[master]: soc: Add AMD StoneyRidge southbridge code

[Martin Roth (Code Review)]

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/hudson.h
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/pci_devs.h
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/smi.h
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.


-- 
To view, visit https://review.coreboot.org/19722
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88a868e654ad127be70ecc506f6b90b784f8d1b
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes