Change in coreboot[master]: soc/intel/fsp_broadwell_de: Populate SMBIOS tables with memory inform...
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34681 )
Change subject: soc/intel/fsp_broadwell_de: Populate SMBIOS tables with memory information ......................................................................
Patch Set 10:
(10 comments)
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/Kconfig:
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 94: default 512
the code in spd_bin.c uses this to initialize arrays. DDR4 is always 2 pages and DDR3 is 1. […]
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 98: depends on !FSP_MEMORY_DOWN
Also on SMBIOS value?
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 99: default n
I will remove this as an option.
Ack
https://review.coreboot.org/c/coreboot/+/34681/4/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/Kconfig:
https://review.coreboot.org/c/coreboot/+/34681/4/src/soc/intel/fsp_broadwell... PS4, Line 96: config SAVE_DRAM_INFO
I don't see why it should be board specific.
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/memory.c:
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 4: * Copyright (C) 2019 Facebook, Inc
Inc.
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 35: are
Remove?
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 38: .addr_map = { SPD_SLAVE_ADDR(0, 0),
Only one space after {
Ack
https://review.coreboot.org/c/coreboot/+/34681/2/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/memory.c:
https://review.coreboot.org/c/coreboot/+/34681/2/src/soc/intel/fsp_broadwell... PS2, Line 54: continue;
thanks, great catch
Ack
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34681/1/src/soc/intel/fsp_broadwell... PS1, Line 124: if (CONFIG(SAVE_DRAM_INFO))
Shouldn’t we condition this on, if SMBIOS table are created or not?
Ack
https://review.coreboot.org/c/coreboot/+/34681/4/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34681/4/src/soc/intel/fsp_broadwell... PS4, Line 124: if (CONFIG(SAVE_DRAM_INFO))
!CONFIG(FSP_MEMORY_DOWN)?
Ack
-
Andrey Petrov (Code Review)