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

[Marc Jones (Code Review)]

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

Change subject: soc: Add AMD StoneyRidge southbridge code
......................................................................


Patch Set 3:

(8 comments)

Thanks for the comments. There are lots of things to fix. I'll see what should be done directly here and what should be follow-on and where in the stack they sit. Some may make sense after the cpu and northbridge are also added to soc/

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/int
OK, I did not realize the issue with soc/intel/common.

I was trying to not add more clutter to src/amd/common/*/Kconfig at this point, so would it be more correct to source from the soc/amd/stoneyridge/Kconfig?


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?
I think the file will get renamed southbridge.c. I want to rename all the hudson function names, too.


https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/lpc.c
File src/soc/amd/stoneyridge/lpc.c:

PS3, Line 314: 	/* Set WideIO for as many IOs found (fall through is on purpose) */
             : 	switch (var_num) {
             : 	case 3:
             : 		pci_write_config16(dev, 0x90, reg_var[2]);
             : 		/* fall through */
             : 	case 2:
             : 		pci_write_config16(dev, 0x66, reg_var[1]);
             : 		/* fall through */
             : 	case 1:
             : 		pci_write_config16(dev, 0x64, reg_var[0]);
             : 		break;
             : 	}
> There was some other code you had doing this too. Is this code overriding t
the G EC required earlier setup. This needs cleanup.


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'
I fixed the c include but missed this. Thanks.


PS3, Line 175:  alink_ab
> What's alink?
It the bus that links the northbridge and the southbridge.


https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smbus_spd.c
File src/soc/amd/stoneyridge/smbus_spd.c:

PS3, Line 20: #include <Porting.h>
            : #include <AGESA.h>
            : #include <amdlib.h>
> Where are these residing?
These are vendorcode/


Line 24: #include <northbridge/amd/pi/dimmSpd.h>
> What's with the northbridge stuff here?
Updated with the northbridge is brought in to soc/


Line 51: 		if ((status & 1) == 1) continue;               // HostBusy set, keep waiting
> > 80 throughou tfile
and the comment style needs update.


-- 
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