Change in coreboot[master]: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return int need to mask the reset bits for the statement.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/1
diff --git a/src/soc/intel/common/block/smbus/smbuslib.c b/src/soc/intel/common/block/smbus/smbuslib.c index 4db2c6e..705a636 100644 --- a/src/soc/intel/common/block/smbus/smbuslib.c +++ b/src/soc/intel/common/block/smbus/smbuslib.c @@ -40,7 +40,7 @@
static void get_spd(u8 *spd, u8 addr) { - if (do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) == 0xff) { + if ((do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) & 0xff) == 0xff) { printk(BIOS_INFO, "No memory dimm at address %02X\n", addr << 1); /* Make sure spd is zeroed if dimm doesn't exist. */ @@ -68,7 +68,10 @@ for (i = 0 ; i < CONFIG_DIMM_MAX; i++) { get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE], blk->addr_map[i]); - blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; + if (spd_data[i * CONFIG_DIMM_SPD_SIZE] != 0) + blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; + else + blk->spd_array[i] = NULL; }
update_spd_len(blk);
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) What is the value that gets returned by do_smbus_read_byte()? It looks like we should be updating the return type of do_smbus_read_* functions to ensure that we don't have to add these masks every where they are used.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
What is the value that gets returned by do_smbus_read_byte()? It looks like we should be updating th […]
I thought about that as well. I can add it tomorrow. But not sure what the impact to others board if they use the SMBus.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
I thought about that as well. I can add it tomorrow. […]
CL here https://review.coreboot.org/c/coreboot/+/40574. Will verify tomorrow, then remove this line 😄
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
CL here https://review.coreboot.org/c/coreboot/+/40574. […]
So, it looks like do_smbus_read_byte() is expected to return CB_ERR(-1) if SMBus transaction fails. I think what needs to be done here is:
if ((do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) < 0) { ... }
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
So, it looks like do_smbus_read_byte() is expected to return CB_ERR(-1) if SMBus transaction fails. […]
I would try this to see it works or not. Thanks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
I would try this to see it works or not. Thanks.
Agreed, this looks semantically incorrect.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
Agreed, this looks semantically incorrect.
This works < 0... will update later.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
This works < 0... will update later.
Oh, for the 00 address will return 0xff only. I will tweak this a little bit.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#2).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... PS2, Line 44: dimm_present == 0xff Under what circumstances is this returned? smbus read is fine but it reads as all 1s? Can you please add a comment?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... PS2, Line 44: dimm_present == 0xff
Under what circumstances is this returned? smbus read is fine but it reads as all 1s? Can you please […]
If address is 00 it will get 0xff, I don't know why thought. I can add comment here.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#3).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 10 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/2/src/soc/intel/common/block/... PS2, Line 44: dimm_present == 0xff
If address is 00 it will get 0xff, I don't know why thought. I can add comment here.
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/1/src/soc/intel/common/block/... PS1, Line 43: do_smbus_read_byte(SMBUS_IO_BASE, addr, 0)
Oh, for the 00 address will return 0xff only. I will tweak this a little bit.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... PS3, Line 45: If address is 0, it will return 0xff. If that is known, why not check: if (addr == 0) { ... }
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... PS3, Line 45: If address is 0, it will return 0xff.
If that is known, why not check: […]
oh,yes... I can add in the caller 😊
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#4).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/4/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/4/src/soc/intel/common/block/... PS4, Line 73: if(blk->addr_map[i] == 0) { space required before the open parenthesis '('
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#5).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/5
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/3/src/soc/intel/common/block/... PS3, Line 45: If address is 0, it will return 0xff.
oh,yes... […]
How about this? Looks good?
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#6).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte return CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/6
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40558/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40558/6//COMMIT_MSG@9 PS6, Line 9: return returns
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... PS6, Line 46: < 0 Please check for CB_ERR.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... PS6, Line 46: < 0
Please check for CB_ERR.
What do you mean? Do you mean != CB_ERR ??
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... PS6, Line 46: < 0
What do you mean? Do you mean != CB_ERR ??
@Furquan, will this return other CB_ERR? or only return -1?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... PS6, Line 46: < 0
@Furquan, will this return other CB_ERR? or only return -1?
@Paul, we can't do check CB_ERR only. This is from smbus_def.h. So Furquan is right <0 if better. #define SMBUS_ERROR CB_ERR #define SMBUS_WAIT_UNTIL_READY_TIMEOUT CB_I2C_BUSY #define SMBUS_WAIT_UNTIL_DONE_TIMEOUT CB_I2C_TIMEOUT #define SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT CB_I2C_NO_DEVICE
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/6/src/soc/intel/common/block/... PS6, Line 46: < 0
@Paul, we can't do check CB_ERR only. This is from smbus_def.h. So Furquan is right <0 if better. […]
Done
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#7).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns CB_ERR(-1) if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/7
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40558/6//COMMIT_MSG@9 PS6, Line 9: return
returns
Done
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#8).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... PS8, Line 50: memset(spd, 0, CONFIG_DIMM_SPD_SIZE); nit: I think you can avoid doing this memset and instead return a -1 which can be checked by the caller.
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... PS8, Line 79: if (spd_data[i * CONFIG_DIMM_SPD_SIZE] != 0) : blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; : else : blk->spd_array[i] = NULL; Continuing my comment above: This can be changed to: if (get_spd(...) < 0) blk->spd_array[i] = NULL; else blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE];
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#9).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40558/9/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/9/src/soc/intel/common/block/... PS9, Line 75: if (get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE],blk->addr_map[i]) < 0) space required after that ',' (ctx:VxV)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... PS8, Line 50: memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
nit: I think you can avoid doing this memset and instead return a -1 which can be checked by the cal […]
Done
https://review.coreboot.org/c/coreboot/+/40558/8/src/soc/intel/common/block/... PS8, Line 79: if (spd_data[i * CONFIG_DIMM_SPD_SIZE] != 0) : blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; : else : blk->spd_array[i] = NULL;
Continuing my comment above: This can be changed to: […]
okay, this is better 🎉
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#10).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/10
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 10: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#11).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630,b:151702387 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/11
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 11:
Patch Set 10: Code-Review+2
if (get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE], blk->addr_map[i]) < 0) blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; else blk->spd_array[i] = NULL; This should be reverse right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 11: -Code-Review
Patch Set 11:
Patch Set 10: Code-Review+2
if (get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE], blk->addr_map[i]) < 0) blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; else blk->spd_array[i] = NULL; This should be reverse right?
You are right. Thanks for catching that.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 11:
whoops, you are right. I want add ! in my mind but forgot.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Varun Joshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40558
to look at the new patch set (#12).
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630,b:151702387 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/40558/12
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 12: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 12: Code-Review+2
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present
Set SPD array NULL if no DIMM present. do_smbus_read_byte returns negative value if SMBus transaction fails.
BUG=b:154445630,b:151702387 TEST=Check SPD is NULL if no DIMM in the slot.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ie81adbfab5bb1d5c557fe549a158cb68e26b1162 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40558 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/smbus/smbuslib.c 1 file changed, 15 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/common/block/smbus/smbuslib.c b/src/soc/intel/common/block/smbus/smbuslib.c index 4db2c6e..5f60ddc 100644 --- a/src/soc/intel/common/block/smbus/smbuslib.c +++ b/src/soc/intel/common/block/smbus/smbuslib.c @@ -38,14 +38,14 @@ } }
-static void get_spd(u8 *spd, u8 addr) +/* return -1 if SMBus errors otherwise return 0 */ +static int get_spd(u8 *spd, u8 addr) { - if (do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) == 0xff) { + /* If address is not 0, it will return CB_ERR(-1) if no dimm */ + if (do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) < 0) { printk(BIOS_INFO, "No memory dimm at address %02X\n", addr << 1); - /* Make sure spd is zeroed if dimm doesn't exist. */ - memset(spd, 0, CONFIG_DIMM_SPD_SIZE); - return; + return -1; } smbus_read_spd(spd, addr);
@@ -58,6 +58,7 @@ /* Restore to page 0 */ do_smbus_write_byte(SMBUS_IO_BASE, SPD_PAGE_0, 0, 0); } + return 0; }
static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE]; @@ -66,9 +67,15 @@ { u8 i; for (i = 0 ; i < CONFIG_DIMM_MAX; i++) { - get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE], - blk->addr_map[i]); - blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; + if (blk->addr_map[i] == 0) { + blk->spd_array[i] = NULL; + continue; + } + + if (get_spd(&spd_data[i * CONFIG_DIMM_SPD_SIZE], blk->addr_map[i]) == 0) + blk->spd_array[i] = &spd_data[i * CONFIG_DIMM_SPD_SIZE]; + else + blk->spd_array[i] = NULL; }
update_spd_len(blk);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40558 )
Change subject: soc/intel/common/block/smbus: Set SPD array NULL if no DIMM present ......................................................................
Patch Set 14:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2786 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2785 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2784 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2783
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
build bot (Jenkins) (Code Review)
-
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Varun Joshi (Code Review)