Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32374 )
Change subject: intel: Add functions to retrieve top of usable DRAM ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/#/c/32374/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32374/4//COMMIT_MSG@12 PS4, Line 12: available. .. top of usable memory above 4GiB ? Naming it with _base does not sound correct.
https://review.coreboot.org/#/c/32374/4//COMMIT_MSG@14 PS4, Line 14: Returns the memory address of usable DRAM below 4GiB .. top of usable memory below 4GiB? Naming it with _base does not sound correct.
https://review.coreboot.org/#/c/32374/4/src/northbridge/intel/e7505/systemag... File src/northbridge/intel/e7505/systemagent_early.c:
https://review.coreboot.org/#/c/32374/4/src/northbridge/intel/e7505/systemag... PS4, Line 27: return 0; Platform has PAE and supports 8/16 GiB of DDR1 so this should be implemented too.
https://review.coreboot.org/#/c/32374/4/src/northbridge/intel/fsp_rangeley/s... File src/northbridge/intel/fsp_rangeley/systemagent_early.c:
https://review.coreboot.org/#/c/32374/4/src/northbridge/intel/fsp_rangeley/s... PS4, Line 27: return 0; I would assume the platform can support > 4GiB.
https://review.coreboot.org/#/c/32374/4/src/soc/intel/broadwell/include/soc/... File src/soc/intel/broadwell/include/soc/systemagent.h:
https://review.coreboot.org/#/c/32374/4/src/soc/intel/broadwell/include/soc/... PS4, Line 140: #ifndef __PRE_RAM__ Why the guard? And how is _gsm_base() related?
https://review.coreboot.org/#/c/32374/4/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/systemagent_memrange.h:
https://review.coreboot.org/#/c/32374/4/src/soc/intel/common/block/include/i... PS4, Line 24: uint64_t sa_get_touud_base(void); I don't see why we would want to define these in soc/intel scope, or even arch/x86 scope, if the intention is to have an API at the end of romstage to tell where we have usable DRAM. IMO these are arch-agnostic.
https://review.coreboot.org/#/c/32374/4/src/soc/intel/fsp_broadwell_de/inclu... File src/soc/intel/fsp_broadwell_de/include/soc/broadwell_de.h:
https://review.coreboot.org/#/c/32374/4/src/soc/intel/fsp_broadwell_de/inclu... PS4, Line 31: #define TOHM 0xd4 /* 64bit */ Are these acronyms from the datasheet? Elsewhere we have TOLUD and TOUUD for the same purpose?