Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SND/HSW ......................................................................
nb/intel/*: Fill in SMBIOS type 16 on SND/HSW
Fill in the maximum DRAM capacity and slot count read from CAPID_A registers on Sandy Bridge and Haswell.
While the register isn't part of the Core Series datasheet, it can be found in the corresponding "Intel Open Source Graphics Programmer's Reference" datasheets.
Change-Id: I6e2346de1ffe52e8685276acbdbf25755f4cc162 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/haswell/hostbridge_regs.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/hostbridge_regs.h M src/northbridge/intel/sandybridge/raminit.c 4 files changed, 64 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/43971/1
diff --git a/src/northbridge/intel/haswell/hostbridge_regs.h b/src/northbridge/intel/haswell/hostbridge_regs.h index f5fa54a..70e15b7 100644 --- a/src/northbridge/intel/haswell/hostbridge_regs.h +++ b/src/northbridge/intel/haswell/hostbridge_regs.h @@ -60,7 +60,11 @@ #define SKPAD 0xdc /* Scratchpad Data */
#define CAPID0_A 0xe4 +#define CAPID_ECCDIS (1 << 25) #define VTD_DISABLE (1 << 23) +#define CAPID_DDPCD (1 << 14) +#define CAPID_PDCD (1 << 12) +#define CAPID_DDRSZ(x) (((x) >> 19) & 0x3)
#define CAPID0_B 0xe8
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 5d67954..9b192d6 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -170,6 +170,20 @@ report_memory_config(); }
+static uint32_t soc_systemagent_max_chan_capacity_mib(u8 capid0_a_ddrsz) +{ + switch (capid0_a_ddrsz) { + case 1: + return 8192; + case 2: + return 2048; + case 3: + return 512; + default: + return 16384; + } +} + void setup_sdram_meminfo(struct pei_data *pei_data) { u32 addr_decode_ch[2]; @@ -221,4 +235,12 @@ } } mem_info->dimm_cnt = dimm_cnt; + const uint32_t capida = pci_read_config32(HOST_BRIDGE, CAPID0_A); + const u8 ddrsz = CAPID_DDRSZ(capida); + + mem_info->ecc_capable = !(capida & CAPID_ECCDIS); + mem_info->max_capacity_mib = soc_systemagent_max_chan_capacity_mib(ddrsz) * + (!(capida & CAPID_PDCD) + 1); + mem_info->number_of_devices = (!(capida & CAPID_DDPCD) + 1) * + (!(capida & CAPID_PDCD) + 1); } diff --git a/src/northbridge/intel/sandybridge/hostbridge_regs.h b/src/northbridge/intel/sandybridge/hostbridge_regs.h index 00d37d4..2d2fcff 100644 --- a/src/northbridge/intel/sandybridge/hostbridge_regs.h +++ b/src/northbridge/intel/sandybridge/hostbridge_regs.h @@ -49,6 +49,11 @@ #define TOLUD 0xbc /* Top of Low Used Memory */
#define CAPID0_A 0xe4 /* Capabilities Register A */ +#define CAPID_ECCDIS (1 << 25) +#define CAPID_DDPCD (1 << 14) +#define CAPID_PDCD (1 << 12) +#define CAPID_DDRSZ(x) (((x) >> 19) & 0x3) + #define CAPID0_B 0xe8 /* Capabilities Register B */
#define SKPAD 0xdc /* Scratchpad Data */ diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 422067b..64571dc 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -54,6 +54,36 @@ memset(&ctrl->info.dimm[channel][0], 0, sizeof(ctrl->info.dimm[0])); }
+static uint32_t soc_systemagent_max_chan_capacity_mib(u8 capid0_a_ddrsz) +{ + switch (capid0_a_ddrsz) { + case 1: + return 8192; + case 2: + return 2048; + case 3: + return 512; + default: + return 16384; + } +} + +/* Fill cbmem with information for SMBIOS type 16 */ +static void fill_smbios16(void) +{ + struct memory_info *m = cbmem_find(CBMEM_ID_MEMINFO); + if (m == NULL) + return; + + const uint32_t capida = pci_read_config32(HOST_BRIDGE, CAPID0_A); + + m->ecc_capable = !(capida & CAPID_ECCDIS); + m->max_capacity_mib = soc_systemagent_max_chan_capacity_mib(CAPID_DDRSZ(capida)) * + (!(capida & CAPID_PDCD) + 1); + m->number_of_devices = (!(capida & CAPID_DDPCD) + 1) * + (!(capida & CAPID_PDCD) + 1); +} + /* Fill cbmem with information for SMBIOS type 17 */ static void fill_smbios17(ramctr_timing *ctrl) { @@ -385,8 +415,10 @@ system_reset(); }
- if (!s3resume) + if (!s3resume) { + fill_smbios16(); fill_smbios17(&ctrl); + } }
void perform_raminit(int s3resume)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SND/HSW ......................................................................
Patch Set 1: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG@7 PS1, Line 7: SND SNB
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG@9 PS1, Line 9: CAPID_A CAPID0_A
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... PS1, Line 173: soc_systemagent_max_chan_capacity_mib Can we call this `northbridge_max_chan_capacity_mib` instead, please? This is not a SoC
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 59: switch (capid0_a_ddrsz) { nit: I didn't check if this field from CAPID0_A was redefined between SNB and IVB. I'll check.
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 80: two spaces
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 82: (!(capida & CAPID_PDCD) + 1) Why not encapsulate these too, like on soc/intel?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SND/HSW ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... PS1, Line 238: pci_read_config32 needs pci_ops.h
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43971
to look at the new patch set (#2).
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW
Fill in the maximum DRAM capacity and slot count read from CAPID0_A registers on Sandy Bridge and Haswell.
While the register isn't part of the Core Series datasheet, it can be found in the corresponding "Intel Open Source Graphics Programmer's Reference" datasheets.
Change-Id: I6e2346de1ffe52e8685276acbdbf25755f4cc162 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/haswell/hostbridge_regs.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/hostbridge_regs.h M src/northbridge/intel/sandybridge/raminit.c 4 files changed, 96 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/43971/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG@7 PS1, Line 7: SND
SNB
Done
https://review.coreboot.org/c/coreboot/+/43971/1//COMMIT_MSG@9 PS1, Line 9: CAPID_A
CAPID0_A
Done
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... PS1, Line 173: soc_systemagent_max_chan_capacity_mib
Can we call this `northbridge_max_chan_capacity_mib` instead, please? This is not a SoC
Done
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/haswe... PS1, Line 238: pci_read_config32
needs pci_ops. […]
Done
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 80:
two spaces
Done
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 82: (!(capida & CAPID_PDCD) + 1)
Why not encapsulate these too, like on soc/intel?
Done
Angel Pons has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW
Fill in the maximum DRAM capacity and slot count read from CAPID0_A registers on Sandy Bridge and Haswell.
While the register isn't part of the Core Series datasheet, it can be found in the corresponding "Intel Open Source Graphics Programmer's Reference" datasheets.
Change-Id: I6e2346de1ffe52e8685276acbdbf25755f4cc162 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/haswell/hostbridge_regs.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/hostbridge_regs.h M src/northbridge/intel/sandybridge/raminit.c 4 files changed, 109 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/43971/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/43971/1/src/northbridge/intel/sandy... PS1, Line 59: switch (capid0_a_ddrsz) {
nit: I didn't check if this field from CAPID0_A was redefined between SNB and IVB. I'll check.
Ack
Angel Pons has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW
Fill in the maximum DRAM capacity and slot count read from CAPID0_A registers on Sandy Bridge and Haswell.
While the register isn't part of the Core Series datasheet, it can be found in the corresponding "Intel Open Source Graphics Programmer's Reference" datasheets.
Note that the values for DDRSZ (maximum allowed memory size per channel) need to be halved when only one DIMM per channel is supported. On mobile platforms, all but quad-core processors are subject to this restriction.
Tested on Lenovo X230: On Linux, verify that `dmidecode -t 16` reports the actual maximum capacity (16 GiB) instead of the currently-installed capacity (4 GiB) or the max capacity assuming two DIMMs per channel is possible (32 GiB).
Change-Id: I6e2346de1ffe52e8685276acbdbf25755f4cc162 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/haswell/hostbridge_regs.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/hostbridge_regs.h M src/northbridge/intel/sandybridge/raminit.c 4 files changed, 112 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/43971/6
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43971 )
Change subject: nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW ......................................................................
nb/intel/*: Fill in SMBIOS type 16 on SNB/HSW
Fill in the maximum DRAM capacity and slot count read from CAPID0_A registers on Sandy Bridge and Haswell.
While the register isn't part of the Core Series datasheet, it can be found in the corresponding "Intel Open Source Graphics Programmer's Reference" datasheets.
Note that the values for DDRSZ (maximum allowed memory size per channel) need to be halved when only one DIMM per channel is supported. On mobile platforms, all but quad-core processors are subject to this restriction.
Tested on Lenovo X230: On Linux, verify that `dmidecode -t 16` reports the actual maximum capacity (16 GiB) instead of the currently-installed capacity (4 GiB) or the max capacity assuming two DIMMs per channel is possible (32 GiB).
Change-Id: I6e2346de1ffe52e8685276acbdbf25755f4cc162 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43971 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Christian Walter christian.walter@9elements.com --- M src/northbridge/intel/haswell/hostbridge_regs.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/sandybridge/hostbridge_regs.h M src/northbridge/intel/sandybridge/raminit.c 4 files changed, 112 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Christian Walter: Looks good to me, approved
diff --git a/src/northbridge/intel/haswell/hostbridge_regs.h b/src/northbridge/intel/haswell/hostbridge_regs.h index f5fa54a..70e15b7 100644 --- a/src/northbridge/intel/haswell/hostbridge_regs.h +++ b/src/northbridge/intel/haswell/hostbridge_regs.h @@ -60,7 +60,11 @@ #define SKPAD 0xdc /* Scratchpad Data */
#define CAPID0_A 0xe4 +#define CAPID_ECCDIS (1 << 25) #define VTD_DISABLE (1 << 23) +#define CAPID_DDPCD (1 << 14) +#define CAPID_PDCD (1 << 12) +#define CAPID_DDRSZ(x) (((x) >> 19) & 0x3)
#define CAPID0_B 0xe8
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 5d67954..0b59692 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -10,6 +10,7 @@ #include <memory_info.h> #include <mrc_cache.h> #include <device/pci_def.h> +#include <device/pci_ops.h> #include <device/dram/ddr3.h> #include <smbios.h> #include <spd.h> @@ -170,6 +171,45 @@ report_memory_config(); }
+static bool nb_supports_ecc(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_ECCDIS); +} + +static uint16_t nb_slots_per_channel(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_DDPCD) + 1; +} + +static uint16_t nb_number_of_channels(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_PDCD) + 1; +} + +static uint32_t nb_max_chan_capacity_mib(const uint32_t capid0_a) +{ + uint32_t ddrsz; + + /* Values from documentation, which assume two DIMMs per channel */ + switch (CAPID_DDRSZ(capid0_a)) { + case 1: + ddrsz = 8192; + break; + case 2: + ddrsz = 2048; + break; + case 3: + ddrsz = 512; + break; + default: + ddrsz = 16384; + break; + } + + /* Account for the maximum number of DIMMs per channel */ + return (ddrsz / 2) * nb_slots_per_channel(capid0_a); +} + void setup_sdram_meminfo(struct pei_data *pei_data) { u32 addr_decode_ch[2]; @@ -221,4 +261,12 @@ } } mem_info->dimm_cnt = dimm_cnt; + + const uint32_t capid0_a = pci_read_config32(HOST_BRIDGE, CAPID0_A); + + const uint16_t channels = nb_number_of_channels(capid0_a); + + mem_info->ecc_capable = nb_supports_ecc(capid0_a); + mem_info->max_capacity_mib = channels * nb_max_chan_capacity_mib(capid0_a); + mem_info->number_of_devices = channels * nb_slots_per_channel(capid0_a); } diff --git a/src/northbridge/intel/sandybridge/hostbridge_regs.h b/src/northbridge/intel/sandybridge/hostbridge_regs.h index 00d37d4..2d2fcff 100644 --- a/src/northbridge/intel/sandybridge/hostbridge_regs.h +++ b/src/northbridge/intel/sandybridge/hostbridge_regs.h @@ -49,6 +49,11 @@ #define TOLUD 0xbc /* Top of Low Used Memory */
#define CAPID0_A 0xe4 /* Capabilities Register A */ +#define CAPID_ECCDIS (1 << 25) +#define CAPID_DDPCD (1 << 14) +#define CAPID_PDCD (1 << 12) +#define CAPID_DDRSZ(x) (((x) >> 19) & 0x3) + #define CAPID0_B 0xe8 /* Capabilities Register B */
#define SKPAD 0xdc /* Scratchpad Data */ diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 422067b..2728037 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -54,8 +54,47 @@ memset(&ctrl->info.dimm[channel][0], 0, sizeof(ctrl->info.dimm[0])); }
-/* Fill cbmem with information for SMBIOS type 17 */ -static void fill_smbios17(ramctr_timing *ctrl) +static bool nb_supports_ecc(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_ECCDIS); +} + +static uint16_t nb_slots_per_channel(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_DDPCD) + 1; +} + +static uint16_t nb_number_of_channels(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_PDCD) + 1; +} + +static uint32_t nb_max_chan_capacity_mib(const uint32_t capid0_a) +{ + uint32_t ddrsz; + + /* Values from documentation, which assume two DIMMs per channel */ + switch (CAPID_DDRSZ(capid0_a)) { + case 1: + ddrsz = 8192; + break; + case 2: + ddrsz = 2048; + break; + case 3: + ddrsz = 512; + break; + default: + ddrsz = 16384; + break; + } + + /* Account for the maximum number of DIMMs per channel */ + return (ddrsz / 2) * nb_slots_per_channel(capid0_a); +} + +/* Fill cbmem with information for SMBIOS type 16 and type 17 */ +static void setup_sdram_meminfo(ramctr_timing *ctrl) { int channel, slot; const u16 ddr_freq = (1000 << 8) / ctrl->tCK; @@ -66,6 +105,19 @@ if (ret != CB_SUCCESS) printk(BIOS_ERR, "RAMINIT: Failed to add SMBIOS17\n"); } + + /* The 'spd_add_smbios17' function allocates this CBMEM area */ + struct memory_info *m = cbmem_find(CBMEM_ID_MEMINFO); + if (m == NULL) + return; + + const uint32_t capid0_a = pci_read_config32(HOST_BRIDGE, CAPID0_A); + + const uint16_t channels = nb_number_of_channels(capid0_a); + + m->ecc_capable = nb_supports_ecc(capid0_a); + m->max_capacity_mib = channels * nb_max_chan_capacity_mib(capid0_a); + m->number_of_devices = channels * nb_slots_per_channel(capid0_a); }
/* Return CRC16 match for all SPDs */ @@ -386,7 +438,7 @@ }
if (!s3resume) - fill_smbios17(&ctrl); + setup_sdram_meminfo(&ctrl); }
void perform_raminit(int s3resume)
-
Angel Pons (Code Review) -
Christian Walter (Code Review)
-
Patrick Rudolph (Code Review)