[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: Add cache as ram init and teardown code

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/18381 )

Change subject: soc/intel/common/block: Add cache as ram init and teardown code
......................................................................


Patch Set 36:

(9 comments)

Some nits.  Feel free to fix these in a follow-on patch if you want.

https://review.coreboot.org/#/c/18381/36/src/soc/intel/common/block/cpu/Kconfig
File src/soc/intel/common/block/cpu/Kconfig:

PS36, Line 13: mode
remove "mode" so we're not saying "non-evict mode mode"


PS36, Line 13: block
blocked


PS36, Line 20: set up portion
set up a portion


PS36, Line 21:  use remainder
use the remainder


PS36, Line 27: 	  Current limitation NEM mode is that code and data size is derive
             : 	  from the need not to spill out any modified line: as in NEM mode
             : 	  there is no memory behind, the modified data will be lost and NEM
             : 	  results will be inconsistent, hence NEM ENHANCED mode ensure to
             : 	  have, some “magic” way to guarantee that modified data is always
             : 	  kept in cache while clean data is replaced.
Maybe:
 A current limitation of NEM (Non-Evict mode) is that code and
 data sizes are derived from the requirement to not write out
 any modified cache line.

 With NEM, if there is no physical memory behind the cached area,
 the modified data will be lost and NEM results will be inconsistent.
 ENHANCED NEM guarantees that modified data is always kept in the cache
 while clean data is replaced.


https://review.coreboot.org/#/c/18381/36/src/soc/intel/common/block/cpu/car/cache_as_ram.S
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:

PS36, Line 108:         /* Configure CAR region as write-back (WB) */
              :         mov     $MTRR_PHYS_BASE(0), %ecx
              :         mov     $CONFIG_DCACHE_RAM_BASE, %eax
              :         or      $MTRR_TYPE_WRBACK, %eax
              :         xor     %edx,%edx
              :         
indent with tabs


PS36, Line 124:         /* Configure CAR region as write-back (WB) */
              :         mov     $MTRR_PHYS_BASE(0), %ecx
              :         mov     $CONFIG_DCACHE_RAM_BASE, %eax
              :         or      $MTRR_TYPE_WRBACK, %eax
              :         xor     %edx,%edx
              :         wrmsr
indent with tabs


https://review.coreboot.org/#/c/18381/36/src/soc/intel/skylake/Kconfig
File src/soc/intel/skylake/Kconfig:

PS36, Line 269: Current limitation NEM mode is that code and data size is derive
              : 	  from the need not to spill out any modified line: as in NEM mode
              : 	  there is no memory behind, the modified data will be lost and NEM
              : 	  results will be inconsistent, hence NEM ENHANCED mode ensure to
              : 	  have, some “magic” way to guarantee that modified data is always
              : 	  kept in cache while clean data is replaced.
see previous comment


PS36, Line 280: Tear Down
lowercase - tear down


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffd0c3e3ca81a3d283d5f1da115222a222e6b157
Gerrit-PatchSet: 36
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: Yes