[coreboot-gerrit] Change in coreboot[master]: vendorcode/amd/pi/00670F00: Clarify CAR disable

[Marshall Dawson (Code Review)]

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

Change subject: vendorcode/amd/pi/00670F00: Clarify CAR disable
......................................................................


Patch Set 5:

(5 comments)

https://review.coreboot.org/#/c/18492/5/src/vendorcode/amd/pi/00670F00/binaryPI/gcccar.inc
File src/vendorcode/amd/pi/00670F00/binaryPI/gcccar.inc:

Line 399: * AMD_DISABLE_STACK_FAMILY_HOOK_F15 Macro - Stackless
> We are not stackless.
| We are not stackless.

As far as I'm aware, the term "stackless" has historically referred to code that does not use a stack.  It does not mean an empty stack, like in your reworked romstage code.

I think the comment is accurate.


Line 406: *   coreboot CAR globals. This should NOT be used with S3 resume IF the
> No need to preserve CAR_GLOBALs.
> No need to preserve CAR_GLOBALs.

I don't think this is an accurate statement.  When AmdInitPost exits, 0-0xd0000000 is WB.  Then when car_migrate_variables() is called, the info is in cache and we have no reason to believe it would have been sent to DRAM yet.

If you're referring to the old copies of CAR_GLOBALs at CONFIG_DCACHE_RAM_BASE, that may be a nuanced difference.  The stack itself is preserved in-place however.  Do you have a better suggestion here?


Line 646:     # Send cache to memory. Preserve stack and coreboot CAR globals.
> No need to preserve CAR_GLOBALs.
Same as above.


Line 650:     wbinvd                                # Clear the cache tag RAMs
> This used to say
How do you propose we modify the comment?  It's not inaccurate, but perhaps it could be removed.  It's fairly obvious what wbinvd does.


Line 1298: * AMD_DISABLE_STACK:  Copy CAR data to DRAM and destroy the stack inside the
> > > 'flush cachelines' or 'invalidate with write-back'
Your account of the regions written to DRAM looks accurate.

The disable_cache_as_ram() is functional for non-S3-supporting systems.  The wbinvd approach retains the information that we need.  We can look at streamlining the functionality later.  I'm not willing to address a performance hit here though.

We're trying to accurately describe the process in this patch.  If you have suggestions on how to best do that, we welcome it.  Otherwise, should we abandon these comments?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77e53262212e00bce9145b0bc3909ad8651f2328
Gerrit-PatchSet: 5
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: Zheng Bao <fishbaozi at gmail.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes