[coreboot-gerrit] Change in coreboot[master]: soc/amd/stoneyridge: Add northbridge support

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/19724 )

Change subject: soc/amd/stoneyridge: Add northbridge support
......................................................................


Patch Set 3:

(5 comments)

Tried to answer some questions. lots of cleanup in follow-on patches. Maybe we will squash it all down.

https://review.coreboot.org/#/c/19724/3/src/soc/amd/common/amd_late_init.c
File src/soc/amd/common/amd_late_init.c:

PS3, Line 23: #include <agesawrapper.h>
            : #include <agesawrapper_call.h>
> These includes should use "" if you are wanting to pick up the files in the
common/ is in the include path, so I left it <>. I don't understand what you mean by why they are directly under common. Do you think common/agesa/ or common/include/ or somewhere else?


https://review.coreboot.org/#/c/19724/3/src/soc/amd/common/def_callouts.c
File src/soc/amd/common/def_callouts.c:

PS3, Line 20: #include <AGESA.h>
            : #include <amdlib.h>
            : #include <Ids.h>
            : #include <agesawrapper.h>
            : #include <BiosCallOuts.h>
> All these headers should be namespaced in some matter. Why are these bare a
They are in src/vendorcode/amd/pi/00670F00 and added by the vendorcode makefile.


Line 84: 		LibAmdIoWrite (AccessWidth8, 0xCf9, &Value, StdHeader);
> Is this really necessary? We can't use io_write8() ?
Yes, we need to clean out the AMD functions and use coreboot functions. This is the reason some functions live in  fixme.c and need to get fixed and moved.


https://review.coreboot.org/#/c/19724/3/src/soc/amd/common/dimmSpd.h
File src/soc/amd/common/dimmSpd.h:

Line 20: AmdMemoryReadSPD (IN UINT32 Func, IN UINT32 Data, IN OUT AGESA_READ_SPD_PARAMS *SpdData);
> IN and OUT ?
agesa defines.


https://review.coreboot.org/#/c/19724/3/src/soc/amd/stoneyridge/northbridge.c
File src/soc/amd/stoneyridge/northbridge.c:

Line 731: 
> I have no words for this file.
There is a lot of legacy here and still requires a lot more cleanup. The multinode HT init is overly complicated and didn't/doesn't fit well in coreboot. The PI removes the need for a lot of that logic in coreboot, but I still think there are a lot of problems putting the CPUs in PCI space and IO and MEMORY bus routing when there is more than a subtractive PCI bus on CPU bus.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie86b4d744900f23502068517ece5bcea6c128993
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: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes