Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.0 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is symantec only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 10 files changed, 24 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/1
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 6c92d03..a49e91e 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -281,8 +281,8 @@
memset(t, 0, sizeof(struct smbios_type17)); t->memory_type = dimm->ddr_type; - t->clock_speed = dimm->ddr_frequency; - t->speed = dimm->ddr_frequency; + t->clock_speed = dimm->ddr_speed_mts; + t->speed = dimm->ddr_speed_mts; t->type = SMBIOS_MEMORY_DEVICE; if (dimm->dimm_size < 0x7fff) { t->size = dimm->dimm_size; diff --git a/src/device/dram/ddr3.c b/src/device/dram/ddr3.c index 461fb65..1d529b1 100644 --- a/src/device/dram/ddr3.c +++ b/src/device/dram/ddr3.c @@ -536,7 +536,7 @@ dimm = &mem_info->dimm[mem_info->dimm_cnt]; if (info->size_mb) { dimm->ddr_type = MEMORY_TYPE_DDR3; - dimm->ddr_frequency = selected_freq; + dimm->ddr_speed_mts = selected_freq; dimm->dimm_size = info->size_mb; dimm->channel_num = channel; dimm->rank_per_dimm = info->ranks; diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c index 429a9d2..41690e1 100644 --- a/src/device/dram/ddr4.c +++ b/src/device/dram/ddr4.c @@ -211,7 +211,7 @@ dimm = &mem_info->dimm[mem_info->dimm_cnt]; if (info->size_mb) { dimm->ddr_type = MEMORY_TYPE_DDR4; - dimm->ddr_frequency = selected_freq; + dimm->ddr_speed_mts = selected_freq; dimm->dimm_size = info->size_mb; dimm->channel_num = channel; dimm->rank_per_dimm = info->ranks; diff --git a/src/include/memory_info.h b/src/include/memory_info.h index f4a2009..26b42f4 100644 --- a/src/include/memory_info.h +++ b/src/include/memory_info.h @@ -28,7 +28,12 @@ * See the smbios.h smbios_memory_type enum. */ uint16_t ddr_type; - uint16_t ddr_frequency; + /* + * This field used to be called ddr_frequency, however it is actually + * being used used to store ddr speed in MT/s. SMBIOS version 3.0 made + * this change offical. + */ + uint16_t ddr_speed_mts; uint8_t rank_per_dimm; uint8_t channel_num; uint8_t dimm_num; diff --git a/src/mainboard/google/cyan/spd/spd.c b/src/mainboard/google/cyan/spd/spd.c index 1c08d90..e62b0a5 100644 --- a/src/mainboard/google/cyan/spd/spd.c +++ b/src/mainboard/google/cyan/spd/spd.c @@ -122,10 +122,10 @@ hob_ptr = get_next_guid_hob(&memory_info_hob_guid, hob_list_ptr); if (hob_ptr != NULL) { memory_info_hob = (FSP_SMBIOS_MEMORY_INFO *)(hob_ptr + 1); - dimm->ddr_frequency = memory_info_hob->MemoryFrequencyInMHz; + dimm->ddr_speed_mts = memory_info_hob->MemoryFrequencyInMHz; } else { printk(BIOS_ERR, "Can't get memory info hob pointer\n"); - dimm->ddr_frequency = 0; + dimm->ddr_speed_mts = 0; }
/* Parse the SPD data to determine the DIMM information */ diff --git a/src/soc/amd/common/block/pi/amd_late_init.c b/src/soc/amd/common/block/pi/amd_late_init.c index 25aaea3..0daad16 100644 --- a/src/soc/amd/common/block/pi/amd_late_init.c +++ b/src/soc/amd/common/block/pi/amd_late_init.c @@ -28,11 +28,11 @@ dimm->ddr_type = dmi17->MemoryType;
/* - * dimm_info uses ddr_frequency for setting both config speed and max + * dimm_info uses ddr_speed_mts for setting both config speed and max * speed. Lets use config speed so we don't get the false impression * that the RAM is running faster than it actually is. */ - dimm->ddr_frequency = dmi17->ConfigSpeed; + dimm->ddr_speed_mts = dmi17->ConfigSpeed;
dimm->rank_per_dimm = dmi17->Attributes;
@@ -55,7 +55,7 @@ "CBMEM_ID_MEMINFO:\n" " dimm_size: %u\n" " ddr_type: 0x%hx\n" - " ddr_frequency: %hu\n" + " ddr_speed_mts: %hu\n" " rank_per_dimm: %hhu\n" " channel_num: %hhu\n" " dimm_num: %hhu\n" @@ -67,7 +67,7 @@ " module_part_number(%zu): %s\n", dimm->dimm_size, dimm->ddr_type, - dimm->ddr_frequency, + dimm->ddr_speed_mts, dimm->rank_per_dimm, dimm->channel_num, dimm->dimm_num, diff --git a/src/soc/amd/picasso/dmi.c b/src/soc/amd/picasso/dmi.c index d3101b7..a3b8a1a 100644 --- a/src/soc/amd/picasso/dmi.c +++ b/src/soc/amd/picasso/dmi.c @@ -28,14 +28,12 @@ dimm->ddr_type = dmi17->MemoryType;
/** - * Based on the name, ddr_frequency should hold the memory clock - * frequency in Mhz. However it is interpreted as MT/s in SMBIOS - * downstream. So multiply by 2 to translate to memory speed in MT/s. - * ddr_frequency is used for setting both config speed and max + * Multiply by 2 to translate MHz to memory speed in MT/s. + * ddr_speed_mts is used for setting both config speed and max * speed. Using config speed so we don't get the false impression * that the RAM is running faster than it actually is. */ - dimm->ddr_frequency = dmi17->ConfigSpeed*2; + dimm->ddr_speed_mts = 2 * dmi17->ConfigSpeed;
dimm->rank_per_dimm = dmi17->Attributes;
@@ -58,7 +56,7 @@ "CBMEM_ID_MEMINFO:\n" " dimm_size: %u\n" " ddr_type: 0x%hx\n" - " ddr_frequency: %hu\n" + " ddr_speed_mts: %hu\n" " rank_per_dimm: %hhu\n" " channel_num: %hhu\n" " dimm_num: %hhu\n" @@ -70,7 +68,7 @@ " module_part_number(%zu): %s\n", dimm->dimm_size, dimm->ddr_type, - dimm->ddr_frequency, + dimm->ddr_speed_mts, dimm->rank_per_dimm, dimm->channel_num, dimm->dimm_num, diff --git a/src/soc/intel/broadwell/include/soc/pei_data.h b/src/soc/intel/broadwell/include/soc/pei_data.h index 39c48d3..85b9b1d 100644 --- a/src/soc/intel/broadwell/include/soc/pei_data.h +++ b/src/soc/intel/broadwell/include/soc/pei_data.h @@ -82,7 +82,7 @@ * See the smbios.h smbios_memory_device_type enum. */ uint16_t ddr_type; - uint16_t ddr_frequency; + uint16_t ddr_speed_mts; uint8_t rank_per_dimm; uint8_t channel_num; uint8_t dimm_num; diff --git a/src/soc/intel/common/smbios.c b/src/soc/intel/common/smbios.c index 5e3560c..07d2d93 100644 --- a/src/soc/intel/common/smbios.c +++ b/src/soc/intel/common/smbios.c @@ -39,7 +39,7 @@ } dimm->dimm_size = dimm_capacity; dimm->ddr_type = ddr_type; - dimm->ddr_frequency = frequency; + dimm->ddr_speed_mts = frequency; dimm->rank_per_dimm = rank_per_dimm; dimm->channel_num = channel_id; dimm->dimm_num = dimm_id; diff --git a/src/soc/intel/denverton_ns/hob_mem.c b/src/soc/intel/denverton_ns/hob_mem.c index 326e716..3642b9e 100644 --- a/src/soc/intel/denverton_ns/hob_mem.c +++ b/src/soc/intel/denverton_ns/hob_mem.c @@ -62,7 +62,7 @@ dimm_info->SizeInMb; mem_info->dimm[index].ddr_type = memory_info_hob->MemoryType; - mem_info->dimm[index].ddr_frequency = + mem_info->dimm[index].ddr_speed_mts = memory_info_hob->MemoryFrequencyInMHz; mem_info->dimm[index].channel_num = channel_info->ChannelId;
Hello David Guckian, Vanessa Eusebio, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#2).
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.0 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is symantec only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 10 files changed, 21 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 2: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/2/src/include/memory_info.h File src/include/memory_info.h:
https://review.coreboot.org/c/coreboot/+/44549/2/src/include/memory_info.h@3... PS2, Line 34: * this change offical. 'offical' may be misspelled - perhaps 'official'?
Hello build bot (Jenkins), Raul Rangel, David Guckian, Vanessa Eusebio, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#3).
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.0 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is symantec only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/drivers/intel/fsp1_1/romstage.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/broadwell/romstage/raminit.c M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 12 files changed, 26 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 3:
src/northbridge/intel/haswell/raminit.c:243:9: error: 'struct dimm_info' has no member named 'ddr_frequency' dimm->ddr_frequency = ddr_frequency;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 3:
+Tim and +Karthik as well.
Have you verified all call sites to ensure that they are providing the correct input? i.e. MT/s instead of MHz?
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 3:
Patch Set 3:
src/northbridge/intel/haswell/raminit.c:243:9: error: 'struct dimm_info' has no member named 'ddr_frequency' dimm->ddr_frequency = ddr_frequency;
Looks like my sublime project was setup to filter out intel folders so this was missed.
Hello build bot (Jenkins), Raul Rangel, David Guckian, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Huang Jin, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#4).
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.0 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is symantec only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/drivers/intel/fsp1_1/romstage.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/northbridge/intel/haswell/raminit.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/broadwell/romstage/raminit.c M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 13 files changed, 27 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 4: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@11 PS4, Line 11: 0 we are at 3.0.0, so this should be specified in MHz and not MT/s.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 4:
(1 comment)
Patch Set 3:
+Tim and +Karthik as well.
Have you verified all call sites to ensure that they are providing the correct input? i.e. MT/s instead of MHz?
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@11 PS4, Line 11: 0
we are at 3.0.0, so this should be specified in MHz and not MT/s.
It's a mess. Starting in 3.1 (not 3.0 like I stated) of the spec, this field was retroactively reinterpreted as MT/s because "This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed". So it sounds like this field has always been mislabeled.
Arguably this a bug in dmidecode, and it should interpret this field differently when the version is 3.0 or earlier. Here's the patch that changed the units in dmidecode: https://github.com/mirror/dmidecode/commit/1aef508ce487804fcfd12da9afc5a73cc...
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@11 PS4, Line 11: 0
It's a mess. Starting in 3.1 (not 3. […]
Yes, totally agree. I guess the best option would be to bump SMBIOS version to 3.1, so that it's clear the unit is now MT/s. I tried to do the same in CB:44013, but as it's not clear if the blobs provide units in MT/s or Mhz. Tests showed that the advertised unit is wrong, so all platforms need to be tested 1 by 1.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@18 PS4, Line 18: symantec semantic
Hello build bot (Jenkins), Raul Rangel, David Guckian, Patrick Rudolph, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Angel Pons, Huang Jin, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#5).
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.0 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is semantic only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/drivers/intel/fsp1_1/romstage.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/northbridge/intel/haswell/raminit.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/broadwell/romstage/raminit.c M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 13 files changed, 27 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/5
Hello build bot (Jenkins), Raul Rangel, David Guckian, Patrick Rudolph, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Angel Pons, Huang Jin, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#6).
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
memory_info: Rename ddr_frequency to ddr_speed_mts
This is correcting a legacy misnaming of this field. From SMBIOS 3.1 spec: Memory speed is expressed in megatransfers per second (MT/s). Previous revisions (3.0.0 and earlier) of this specification used MHz to indicate clock speed. With double data rate memory, clock speed is distinct from transfer rate, since data is transferred on both the rising and the falling edges of the clock signal. This maintains backward compatibility with observed DDR implementations prior to this revision, which already reported transfer rate instead of clock speed, e.g., DDR4-2133 (PC4-17000) memory was reported as 2133 instead of 1066.
This change is semantic only, no logical change.
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/drivers/intel/fsp1_1/romstage.c M src/include/memory_info.h M src/mainboard/google/cyan/spd/spd.c M src/northbridge/intel/haswell/raminit.c M src/soc/amd/common/block/pi/amd_late_init.c M src/soc/amd/picasso/dmi.c M src/soc/intel/broadwell/include/soc/pei_data.h M src/soc/intel/broadwell/romstage/raminit.c M src/soc/intel/common/smbios.c M src/soc/intel/denverton_ns/hob_mem.c 13 files changed, 27 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@18 PS4, Line 18: symantec
semantic
Done
(lol, Symantec used to be a software company which made Norton, an antivirus software)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 6:
Where are we at on this?
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: Rename ddr_frequency to ddr_speed_mts ......................................................................
Patch Set 6:
Patch Set 6:
Where are we at on this?
Seems like the right path forward is to bump sbmios from 3.0 to 3.1. Patrick started this in https://review.coreboot.org/c/coreboot/+/44013. Let's push that forward.
Hello build bot (Jenkins), Raul Rangel, David Guckian, Patrick Rudolph, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Angel Pons, Huang Jin, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#7).
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
memory_info: add max_speed_mts and configured_speed_mts
ddr_frequency is ambiguous and is interpreted differently in several places. Instead of renaming this field, this deprecates it and adds two new fields with unambiguous naming, max_speed_mts and configured_speed_mts. smbios.c falls back to using ddr_frequency when either of these fields are 0.
The same value was being used for both configured memory speed and max memory speed in SMBIOS type 17, which is not accurate when configured speed is not the max speed.
BUG=b:167218112 TEST=Boot ezkinil and observe dmidecode -t17 dmidecode -t17 # dmidecode 3.2 Getting SMBIOS data from sysfs. SMBIOS 3.0 present.
Handle 0x000B, DMI type 17, 40 bytes Memory Device Array Handle: 0x000A Error Information Handle: Not Provided Total Width: 64 bits Data Width: 64 bits Size: 4096 MB Form Factor: SODIMM Set: None Locator: Channel-0-DIMM-0 Bank Locator: BANK 0 Type: DDR4 Type Detail: Synchronous Speed: 3200 MT/s Manufacturer: Unknown (0) Serial Number: 00000000 Asset Tag: Not Specified Part Number: MT40A512M16TB-062E:J Rank: 1 Configured Memory Speed: 2400 MT/s Minimum Voltage: Unknown Maximum Voltage: Unknown Configured Voltage: Unknown
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/include/memory_info.h M src/soc/amd/picasso/dmi.c 3 files changed, 27 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44549/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/44549/7/src/arch/x86/smbios.c@284 PS7, Line 284: if(dimm->configured_speed_mts != 0) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/44549/7/src/arch/x86/smbios.c@288 PS7, Line 288: if(dimm->max_speed_mts != 0) space required before the open parenthesis '('
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 7:
Hey everyone, here's a new proposal for resolving this issue. Instead of renaming ddr_frequency, deprecate it and replace it with two new fields, max_speed_mts and configured_speed_mts.
Hello build bot (Jenkins), Raul Rangel, David Guckian, Patrick Rudolph, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Angel Pons, Huang Jin, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#8).
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
memory_info: add max_speed_mts and configured_speed_mts
ddr_frequency is ambiguous and is interpreted differently in several places. Instead of renaming this field, this deprecates it and adds two new fields with unambiguous naming, max_speed_mts and configured_speed_mts. smbios.c falls back to using ddr_frequency when either of these fields are 0.
The same value was being used for both configured memory speed and max memory speed in SMBIOS type 17, which is not accurate when configured speed is not the max speed.
BUG=b:167218112 TEST=Boot ezkinil and observe dmidecode -t17 dmidecode -t17 # dmidecode 3.2 Getting SMBIOS data from sysfs. SMBIOS 3.0 present.
Handle 0x000B, DMI type 17, 40 bytes Memory Device Array Handle: 0x000A Error Information Handle: Not Provided Total Width: 64 bits Data Width: 64 bits Size: 4096 MB Form Factor: SODIMM Set: None Locator: Channel-0-DIMM-0 Bank Locator: BANK 0 Type: DDR4 Type Detail: Synchronous Speed: 3200 MT/s Manufacturer: Unknown (0) Serial Number: 00000000 Asset Tag: Not Specified Part Number: MT40A512M16TB-062E:J Rank: 1 Configured Memory Speed: 2400 MT/s Minimum Voltage: Unknown Maximum Voltage: Unknown Configured Voltage: Unknown
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/include/memory_info.h M src/soc/amd/picasso/dmi.c 3 files changed, 27 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8: Code-Review+1
This ass
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8: Code-Review+2
I like it!
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44549/4//COMMIT_MSG@11 PS4, Line 11: 0
Yes, totally agree. I guess the best option would be to bump SMBIOS version to 3. […]
Since ddr_frequency is ambiguous I propose we deprecate the field and add max_speed_mts and configured_speed_mts. This should be compatible with efforts to move to SMBIOS 3.1/3.2.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8:
Patch Set 8: Code-Review+1
This ass
?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8:
I'm very sorry, somehow gerrit cropped my text and left this nonsense. I wouldn't insult someone. I like the idea, this way all platforms can be validated one by one.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8:
Patch Set 8:
Patch Set 8: Code-Review+1
This ass
?
lol. Sounds good.
Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8: Code-Review+1
Good to see these two values finally being distinguished.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8: Code-Review-1
I think the Picasso change should go in a separate CL on top of this one; ddr_frequency still exists, so the dmi.c code won't break
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 8:
Patch Set 8: Code-Review-1
I think the Picasso change should go in a separate CL on top of this one; ddr_frequency still exists, so the dmi.c code won't break
Makes sense
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Rudolph, Jack Rosenthal, Angel Pons, Patrick Rudolph, David Guckian, Lee Leahy, Tim Wawrzynczak, Vanessa Eusebio, Huang Jin, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44549
to look at the new patch set (#9).
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
memory_info: add max_speed_mts and configured_speed_mts
ddr_frequency is ambiguous and is interpreted differently in several places. Instead of renaming this field, this deprecates it and adds two new fields with unambiguous naming, max_speed_mts and configured_speed_mts. smbios.c falls back to using ddr_frequency when either of these fields are 0.
The same value was being used for both configured memory speed and max memory speed in SMBIOS type 17, which is not accurate when configured speed is not the max speed.
BUG=b:167218112 TEST=Boot ezkinil, no change to dmidecode -t17
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/arch/x86/smbios.c M src/include/memory_info.h 2 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/44549/9
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 9: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 9: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 9: Code-Review+2
Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
Patch Set 9: Code-Review+1
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44549 )
Change subject: memory_info: add max_speed_mts and configured_speed_mts ......................................................................
memory_info: add max_speed_mts and configured_speed_mts
ddr_frequency is ambiguous and is interpreted differently in several places. Instead of renaming this field, this deprecates it and adds two new fields with unambiguous naming, max_speed_mts and configured_speed_mts. smbios.c falls back to using ddr_frequency when either of these fields are 0.
The same value was being used for both configured memory speed and max memory speed in SMBIOS type 17, which is not accurate when configured speed is not the max speed.
BUG=b:167218112 TEST=Boot ezkinil, no change to dmidecode -t17
Change-Id: Iaa75401f9fc33642dbdce6c69bd9b20f96d1cc25 Signed-off-by: Rob Barnes robbarnes@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44549 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Jack Rosenthal jrosenth@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/smbios.c M src/include/memory_info.h 2 files changed, 22 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Raul Rangel: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Jack Rosenthal: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 6c92d03..7516305 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -281,8 +281,14 @@
memset(t, 0, sizeof(struct smbios_type17)); t->memory_type = dimm->ddr_type; - t->clock_speed = dimm->ddr_frequency; - t->speed = dimm->ddr_frequency; + if (dimm->configured_speed_mts != 0) + t->clock_speed = dimm->configured_speed_mts; + else + t->clock_speed = dimm->ddr_frequency; + if (dimm->max_speed_mts != 0) + t->speed = dimm->max_speed_mts; + else + t->speed = dimm->ddr_frequency; t->type = SMBIOS_MEMORY_DEVICE; if (dimm->dimm_size < 0x7fff) { t->size = dimm->dimm_size; diff --git a/src/include/memory_info.h b/src/include/memory_info.h index f4a2009..d9d9c37 100644 --- a/src/include/memory_info.h +++ b/src/include/memory_info.h @@ -28,6 +28,10 @@ * See the smbios.h smbios_memory_type enum. */ uint16_t ddr_type; + /* + * ddr_frequency is deprecated. + * Use max_speed_mts and configured_speed_mts instead. + */ uint16_t ddr_frequency; uint8_t rank_per_dimm; uint8_t channel_num; @@ -79,6 +83,16 @@ * Voltage Level */ uint16_t vdd_voltage; + /* + * Max speed in MT/s + * If the value is 0, ddr_frequency should be used instead. + */ + uint16_t max_speed_mts; + /* + * Configured speed in MT/s + * If the value is 0, ddr_frequency should be used instead. + */ + uint16_t configured_speed_mts; } __packed;
struct memory_info {
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Jack Rosenthal (Code Review)
-
Patrick Rudolph (Code Review)
-
Raul Rangel (Code Review)
-
Rob Barnes (Code Review)
-
Tim Wawrzynczak (Code Review)