[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 4:

(4 comments)

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

Line 419: *   * MSRC001_0015[INVD_WBINVD]=1
> If we believe this comment, we enter with invd->wbinvd conversion enabled.
I think you're reading the comment incorrectly.  These are the list of tasks the AMD_DISABLE_STACK code must perform.  They are not prerequisites to the macro.

The INVD_WBINVD bit must be 0 for CAR and gets changed to 1 for normal operation.


Line 649:     #--------------------------------------------------------------------------
> Ah, right. I followed the jmp 1f's incorrectly.
Yes, positive/confirmed.


PS4, Line 925: * 80000h                                        40000h                                      00000h
             : *     +----------+----------+----------+----------+----------+----------+----------+----------+
             : * 64K |          |          |          |          |          |          |          |          |  64K  ea
             : *  ea +----------+----------+----------+----------+----------+----------+----------+----------+
             : *     |                             MTRR 0000_0250 MTRRfix64K_00000                           |
             : *     +----------+----------+----------+----------+----------+----------+----------+----------+
             : *     |  7 ,  6  |  5 ,  4  |  3 ,  2  |  1 ,  0  |     0    |          |          |          | <-node
             : *     |7..1,7..1 |7..1,7..1 |7..1,7..1 |7..1,7..1 |     0    |          |          |          | <-core
             : *     +----------+----------+----------+----------+----------+----------+----------+----------+
             : *
             : * C0000h                       B0000h                      A0000h                      90000h                      80000h
             : *     +------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+
             : *16K  |      |      |      |      |      |      |      |      |      |      |      |      |      |      |      |      |
             : * ea  +------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+
             : *     |              MTRR 0259 MTRRfix16K_A0000               |             MTRR 0258 MTRRfix16K_80000                |
             : *     +------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+
             : *     | > Dis|play B|uffer |   <  |      |      |      |      |   7  |  6   |  5   |  4   |  3   |  2   |  1   |      | <-node
             : *     | >   T| e  m |p o r |a r y |  B u |f f e |r   A |r e a<|   0  |  0   |  0   |  0   |  0   |  0   |  0   |      | <-core
             : *     +------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+------+
These are out of date for F15h.


Line 1299: *                     should only be executed on the BSP
> The comment "destroy the stack" obviously conflicts with the new comment "p
I think it's a reasonable request to update the comment since this patch was to "Clarify CAR disable".

> Assuming that DRAM is not configured at end of verstage, where is your backing store? The way I see it, any wbinvd there would really become invd, And why would you call disable_cache_as_ram() between verstage and romstage in the first place, if you need to preserve some leave some HOB or CAR_GLOBALS?

The topic of verstage isn't 100% relevant for this patch, as the split is 11 patches later, and this patch only "clarifies" what's currently happening.

But since you asked... this code will never run at the end of verstage.  CAR/stack will be maintained through the verstage-to-romstage transition.  AMD_DISABLE_STACK is only used at the end of romstage, once the DRAM backing is in place.


-- 
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: 4
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)
Gerrit-HasComments: Yes