EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
mb/google/deltaur: Enable SPD_READ_BY_WORD
SPD_READ_BY_WORD can reduce iteration of SPD reading loop.
BUG=b:151702387 TEST=Check Memory SPD data is correct in console log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ib68e830c25d95e8498eead155b2fa881b0ae0313 --- M src/mainboard/google/deltaur/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/40645/1
diff --git a/src/mainboard/google/deltaur/Kconfig b/src/mainboard/google/deltaur/Kconfig index 74c8201..5ecad4f 100644 --- a/src/mainboard/google/deltaur/Kconfig +++ b/src/mainboard/google/deltaur/Kconfig @@ -16,6 +16,7 @@ select SOC_INTEL_TIGERLAKE select SYSTEM_TYPE_LAPTOP select MAINBOARD_USES_IFD_GBE_REGION if BOARD_GOOGLE_DELTAN + select SPD_READ_BY_WORD
if BOARD_GOOGLE_BASEBOARD_DELTAUR
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG@9 PS1, Line 9: SPD_READ_BY_WORD can reduce iteration of SPD reading loop. Is the boot time reduced?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG@9 PS1, Line 9: SPD_READ_BY_WORD can reduce iteration of SPD reading loop.
Is the boot time reduced?
Not sure the exactly time. In theory it can help little.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
I guess I'd prefer we check that it's actually going to help reduce boot time.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
Patch Set 1:
I guess I'd prefer we check that it's actually going to help reduce boot time.
And if so, I'd just select it at SoC level, or are there any downsides?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I guess I'd prefer we check that it's actually going to help reduce boot time.
And if so, I'd just select it at SoC level, or are there any downsides?
As long as everybody (SoC & memory) is SMBus compliant, it shouldn't matter 😄.
If it does result in faster boot, I think these are the first TGL boards in coreboot, so it would probably be OK to select it for the SoC. If anyone has issues, they can set it to n for their mainboard. Angel, Paul, what do you think?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
fine, I can add time stamp for this.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
This is not hurry, so I plan pending this 😄
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
Read by word takes 68,613 and Read by byte takes 116,784.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
This is one page time. We need two pages for one dime, if two dimms we need double it. We can save four times of this at most.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG@9 PS1, Line 9: SPD_READ_BY_WORD can reduce iteration of SPD reading loop.
Not sure the exactly time. In theory it can help little.
Thank you for your tests. Please update the commit message accordingly, and I think it should be selected on SoC level.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/1//COMMIT_MSG@9 PS1, Line 9: SPD_READ_BY_WORD can reduce iteration of SPD reading loop.
Thank you for your tests. […]
okay, let me move it.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: mb/google/deltaur: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 1:
Patch Set 1:
Read by word takes 68,613 and Read by byte takes 116,784.
Ratio looks about right, the amount of clocks required for 256 bytes of SPD is approximately:
256 x 4 x 9 bits = 9216 for byte-read 128 x 5 x 9 bits = 5760 for word-read 16 * (3 x 9 + 16 * 9) = 2736 16-byte block read
Looks like implementation of i2c_eeprom_read() does not have a strict block size restriction, although longer than 32 bytes might violate something in the SMBus timing specs. There is potential to half the time still from SPD_READ_BY_WORD.
Some older raminit match the SPD serial number and CRC against previous boots, to detect if DIMMs have been swapped. IMHO, the ability to skip SPD EEPROM reads when there are cached memory training results available would be a more valuable feature than being able to access SPD fast.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40645
to look at the new patch set (#2).
Change subject: soc/intel/common/block/smbus: Enable SPD_READ_BY_WORD ......................................................................
soc/intel/common/block/smbus: Enable SPD_READ_BY_WORD
Enable SPD_READ_BY_WORD can reduce reading time from 116,784ns to 68,613ns for each PAGE.
BUG=b:151702387 TEST=Check Memory SPD data is correct in console log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ib68e830c25d95e8498eead155b2fa881b0ae0313 --- M src/soc/intel/common/block/smbus/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/40645/2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: soc/intel/common/block/smbus: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 2:
SODIMM is switchable, I think detect is needed. For the Memorydown, MRC will skip training if we have MRC cache in there already.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: soc/intel/common/block/smbus: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/Kconfig:
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... PS2, Line 3: select SPD_READ_BY_WORD If this is just selected by default always, is there any use of this Kconfig anymore?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: soc/intel/common/block/smbus: Enable SPD_READ_BY_WORD ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/Kconfig:
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... PS2, Line 3: select SPD_READ_BY_WORD
If this is just selected by default always, is there any use of this Kconfig anymore?
I can check, but is any SODIMM only can read by byte? If not, we can default enable it.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Duncan Laurie, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40645
to look at the new patch set (#3).
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
src/lib: Enable SPD_READ_BY_WORD by default
Enable SPD_READ_BY_WORD can reduce reading time from 116,784ns to 68,613ns for each PAGE.
BUG=b:151702387 TEST=Check Memory SPD data is correct in console log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ib68e830c25d95e8498eead155b2fa881b0ae0313 --- M src/lib/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/40645/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/Kconfig:
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... PS2, Line 3: select SPD_READ_BY_WORD
I can check, but is any SODIMM only can read by byte? If not, we can default enable it.
I think this can be enabled for all boards. And this is only for SPD read not all SMBus read. Just did a quick check.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG@10 PS3, Line 10: 68,613ns for each PAGE. I think comma here is just confusing, is it a thousands or decimals separator? Unit does not look right to me either.
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/Kconfig:
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... PS2, Line 3: select SPD_READ_BY_WORD
I think this can be enabled for all boards. And this is only for SPD read not all SMBus read. […]
I checked SDRAM SPD specs from 1999 and it already specified EEPROM sequential read with unlimited byte count. Word read is case of sequential read with byte count of two, so this should be fine.
AFAIR one SPD specification for a memory technology (eg. SDRAM, DDR2, DDR4) always covers the different physical module types.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG@10 PS3, Line 10: 68,613ns for each PAGE.
I think comma here is just confusing, is it a thousands or decimals separator? Unit does not look ri […]
This is timestamp form cbmen -t...I think it is thousands separator. Unit is nano second. Correct me if I was wrong.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/Kconfig:
https://review.coreboot.org/c/coreboot/+/40645/2/src/soc/intel/common/block/... PS2, Line 3: select SPD_READ_BY_WORD
I checked SDRAM SPD specs from 1999 and it already specified EEPROM sequential read with unlimited b […]
Thanks for the checking. Make sense.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
Patch Set 1:
Patch Set 1:
Read by word takes 68,613 and Read by byte takes 116,784.
Ratio looks about right, the amount of clocks required for 256 bytes of SPD is approximately:
256 x 4 x 9 bits = 9216 for byte-read 128 x 5 x 9 bits = 5760 for word-read 16 * (3 x 9 + 16 * 9) = 2736 16-byte block read
Looks like implementation of i2c_eeprom_read() does not have a strict block size restriction, although longer than 32 bytes might violate something in the SMBus timing specs. There is potential to half the time still from SPD_READ_BY_WORD.
Some older raminit match the SPD serial number and CRC against previous boots, to detect if DIMMs have been swapped. IMHO, the ability to skip SPD EEPROM reads when there are cached memory training results available would be a more valuable feature than being able to access SPD fast.
Intel posted some patches to add an SPD cache as well. I believe it reads the SPD serial number only and if it matches what's in the cache, then it skips the rest of the SPD read (see patch train CB:40414). Good to see you back Kyosti 😊
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 1:
Patch Set 1:
Read by word takes 68,613 and Read by byte takes 116,784.
Ratio looks about right, the amount of clocks required for 256 bytes of SPD is approximately:
256 x 4 x 9 bits = 9216 for byte-read 128 x 5 x 9 bits = 5760 for word-read 16 * (3 x 9 + 16 * 9) = 2736 16-byte block read
Looks like implementation of i2c_eeprom_read() does not have a strict block size restriction, although longer than 32 bytes might violate something in the SMBus timing specs. There is potential to half the time still from SPD_READ_BY_WORD.
Some older raminit match the SPD serial number and CRC against previous boots, to detect if DIMMs have been swapped. IMHO, the ability to skip SPD EEPROM reads when there are cached memory training results available would be a more valuable feature than being able to access SPD fast.
Intel posted some patches to add an SPD cache as well. I believe it reads the SPD serial number only and if it matches what's in the cache, then it skips the rest of the SPD read (see patch train CB:40414). Good to see you back Kyosti 😊
We can add into TGL/CML later. But this still can help some, right?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG@10 PS3, Line 10: 68,613ns for each PAGE.
This is timestamp form cbmen -t...I think it is thousands separator. Unit is nano second. […]
For an SPD page in byte reads, 9000 bits 10us each would be 90ms. That would match the magnitude if that is a thousands separator and unit is microseconds.
You can check if overall cbmem -t record for entering payload makes sense in microseconds. If not, some scaling or timer frequency in coreboot runtime could be wrong.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Duncan Laurie, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40645
to look at the new patch set (#4).
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
src/lib: Enable SPD_READ_BY_WORD by default
Enable SPD_READ_BY_WORD can reduce reading time from 116,784us to 68,613us for each PAGE.
BUG=b:151702387 TEST=Check Memory SPD data is correct in console log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ib68e830c25d95e8498eead155b2fa881b0ae0313 --- M src/lib/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/40645/4
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40645/3//COMMIT_MSG@10 PS3, Line 10: 68,613ns for each PAGE.
For an SPD page in byte reads, 9000 bits 10us each would be 90ms. […]
Yes, sorry for my poor English... I confuse the unit. Unit is ms/1000..
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 4:
Any other suggestions here? It's always good to learn something new 😊
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Patch Set 4: Code-Review+1
Seems OK to me, at least I don't think it will break anybody's boards
EricR Lai has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40645 )
Change subject: src/lib: Enable SPD_READ_BY_WORD by default ......................................................................
Abandoned
-
Angel Pons (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)