[coreboot-gerrit] Change in coreboot[master]: amd/pi/00670F00: Add memmap file

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18428 )

Change subject: amd/pi/00670F00: Add memmap file
......................................................................


Patch Set 10:

(1 comment)

https://review.coreboot.org/#/c/18428/10/src/northbridge/amd/pi/00670F00/memmap.c
File src/northbridge/amd/pi/00670F00/memmap.c:

Line 65:  * also adjust TOM/TOM2 for the C6 storage, as well as the audio controller.
> CarrizoPI build seems to set CstateModeDisabled (changed from the default o
> CarrizoPI build seems to set CstateModeDisabled (changed from the default of CstateModeC6).

I only have the binaryPI directory that was reportedly used for Carrizo, and I see the same thing.  For ST, we copied over the Addendum/StoneyFp4Options.c file for buildOpts.c, and the CStateModeDisabled line is commented.

In poking around with this, I was surprised that 16MB is always allocated regardless of C6 en/disable.  The logic is basically if(C6en || PSP_present).  You should be able to confirm this in AmdAgesaPkg/Proc/Mem/NB/CZ/mnmctcz.c.

I still think using TOM/TOM2 is OK.  The carve off adjusts NBPtr->RefPtr->SysLimit, and this value is used to set TOM/TOM2 in Mem/NB/mnmct.c.

> My concern is that if C6 shifts uma_base by 16MiB, you will have excessive MTRR usage since currently CFG_UMA_ALIGNMENT is set to 4MiB.
Inspired by what board OemPostParams() could still do, I found out that there is runtime option CfgUmaAlignment and if it behaves as I expect it to, it will enforce the alignment for uma_base. But code here ignores any applied alignment.

That's a good thought about the alignment.  ST is also compiled for UMA_4MB_ALIGNED.  I can't think of a good way to detect that setting in coreboot.  It could be worth a cautionary comment though.  I don't believe changing the setting will affect the calculation unless 2GB DRAM or less is present, however.  4GB is the smallest DDR module I have to test with and the C6 region is above 4GB.  I looked at a Gizmo and TOM is set to 3F000000 (1GB - 16MB).

I had considered maybe adding a check that could report an error if the calculation for UMA base/size doesn't match what InitPost returns.

> you will have excessive MTRR usage...

Do you mean that we'll be burning memory?  Or, are you concerned that an additional variable pair(s) might be required?

> So confirm with C6 enabled things still work and please paste /proc/mtrr outputs for both C6 enabled and disabled.

This isn't as interesting as you are hoping because the 16MB is always carved off when the PSP is present:
 reg00: base=0x000000000 (    0MB), size= 2048MB, count=1: write-back
 reg01: base=0x080000000 ( 2048MB), size= 1024MB, count=1: write-back
 reg02: base=0x0c0000000 ( 3072MB), size=  256MB, count=1: write-back
 reg06: base=0x0ff800000 ( 4088MB), size=    8MB, count=1: write-protect


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ddaa8359536081752fb8e47e49f4d5958416620
Gerrit-PatchSet: 10
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: 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)
Gerrit-HasComments: Yes