Martin Roth has posted comments on this change. ( https://review.coreboot.org/19723 )
Change subject: soc/amd/stoneyridge: Add CPU files ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/19723/3/src/northbridge/amd/pi/Kconfig File src/northbridge/amd/pi/Kconfig:
Line 19: default y if SOC_AMD_PI
Why are enabling northbridge/amd/pi under these circumstances? What's the l
Agreed - If we're moving things to SOC, why would we still need pieces in src/northbridge? Isn't the whole point of the SOC that it's all self-contained?
https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/Kconfig File src/soc/amd/common/Kconfig:
Line 13 As mentioned in the previous commit:
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.
My recommendation would be to move this Kconfig (or this section of the Kconfig) to soc/amd/common/pi and source src/amd/common/*/Kconfig from the top level Kconfig at an appropriate point.
https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/Makefile.inc File src/soc/amd/common/Makefile.inc:
Line 13 I'd also recommend we avoid having files in soc/amd/common, and move them all into subdirectories for the the same reason. If we have files here, people are going to want a Kconfig here, and as previously mentioned, that creates problems.
https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/cache_as_ram.inc File src/soc/amd/common/cache_as_ram.inc:
PS3, Line 149: change these tabs to spaces?
https://review.coreboot.org/#/c/19723/3/src/vendorcode/amd/Kconfig File src/vendorcode/amd/Kconfig:
PS3, Line 4: 7 Updating Sage's copyright?
https://review.coreboot.org/#/c/19723/3/src/vendorcode/amd/pi/Kconfig File src/vendorcode/amd/pi/Kconfig:
Line 29: if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || CPU_AMD_PI_00670F00_FP4 || CPU_AMD_PI_00670F00_FT4 || CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 Wrap the line?
if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || \ CPU_AMD_PI_00670F00_FP4 || CPU_AMD_PI_00670F00_FT4 || \ CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || \ SOC_AMD_STONEYRIDGE_FT4
-
Martin Roth (Code Review)