Máté Kukri has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC failes to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/44092/1
diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c index 7e6bcab..836e911 100644 --- a/src/soc/intel/baytrail/romstage/raminit.c +++ b/src/soc/intel/baytrail/romstage/raminit.c @@ -115,11 +115,15 @@ populate_smbios_tables(dram_data, speed, num_channels); }
+#define SPD_SIZE 256 +static u8 spd_buf[SPD_SIZE * NUM_CHANNELS]; + void raminit(struct mrc_params *mp, int prev_sleep_state) { int ret; mrc_wrapper_entry_t mrc_entry; struct region_device rdev; + size_t i;
/* Fill in default entries. */ mp->version = MRC_PARAMS_VER; @@ -158,8 +162,18 @@ */ mrc_entry = (void *)(uintptr_t)CONFIG_MRC_BIN_ADDRESS;
- if (mp->mainboard.dram_info_location == DRAM_INFO_SPD_SMBUS) + if (mp->mainboard.dram_info_location == DRAM_INFO_SPD_SMBUS) { + /* Workaround for broken SMBus support in the MRC */ enable_smbus(); + mp->mainboard.dram_info_location = DRAM_INFO_SPD_MEM; + for (i = 0; i < NUM_CHANNELS; ++i) + if (mp->mainboard.spd_addrs[i]) { + i2c_eeprom_read(mp->mainboard.spd_addrs[i], + 0, SPD_SIZE, &spd_buf[SPD_SIZE * i]); + mp->mainboard.dram_data[i] = + &spd_buf[SPD_SIZE * i]; + } + }
ret = mrc_entry(mp);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 1: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 119: static u8 spd_buf[SPD_SIZE * NUM_CHANNELS] static u8 spd_buf[NUM_CHANNELS][SPD_SIZE];
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 169: for (i = 0; i < NUM_CHANNELS; ++i) nit: add braces around this big for-loop
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 172: &spd_buf[SPD_SIZE * i] spd_buf[i]
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 173: mp->mainboard.dram_data[i] = : &spd_buf[SPD_SIZE * i]; This always needs to be set to `spd_buf[0]` even for channel 1. Otherwise, it doesn't work.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 173: mp->mainboard.dram_data[i] = : &spd_buf[SPD_SIZE * i];
This always needs to be set to `spd_buf[0]` even for channel 1. Otherwise, it doesn't work.
Um, I could've explained myself better. Keep this inside the if-block, but always assign `spd_buf[0]` to it.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44092/1//COMMIT_MSG@9 PS1, Line 9: failes fails
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44092
to look at the new patch set (#2).
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC failes to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/44092/2
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44092
to look at the new patch set (#3).
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC fails to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/44092/3
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44092/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44092/1//COMMIT_MSG@9 PS1, Line 9: failes
fails
Done
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 119: static u8 spd_buf[SPD_SIZE * NUM_CHANNELS]
static u8 spd_buf[NUM_CHANNELS][SPD_SIZE];
Done
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 169: for (i = 0; i < NUM_CHANNELS; ++i)
nit: add braces around this big for-loop
Done
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 172: &spd_buf[SPD_SIZE * i]
spd_buf[i]
Done
https://review.coreboot.org/c/coreboot/+/44092/1/src/soc/intel/baytrail/roms... PS1, Line 173: mp->mainboard.dram_data[i] = : &spd_buf[SPD_SIZE * i];
Um, I could've explained myself better. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... PS3, Line 173: spd_buf spd_buf[i] ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... PS3, Line 173: spd_buf
spd_buf[i] ?
See comments on PS1, MRC expects both pointers to be the same (I tested this on Q1900M).
Please add a comment explaining that MRC expects the SPD data to be passed in like that, though. Otherwise, people might be tempted to "fix" this.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... PS3, Line 163: CONFIG_MRC_BIN_ADDRESS [IGNORE]sidenote: Hmm it should probably be checked in the Makefile.inc that the entry of the elf file in cbfs matches this.
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... PS3, Line 173: spd_buf
See comments on PS1, MRC expects both pointers to be the same (I tested this on Q1900M). […]
Oh this definitely needs a comment. It seems like this MRC binary has some pretty problematic 'bugs/unimplemented features'.
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44092
to look at the new patch set (#4).
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC fails to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/44092/4
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/3/src/soc/intel/baytrail/roms... PS3, Line 173: spd_buf
Oh this definitely needs a comment. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : } So a DIMM in only channel 1 would never boot? Same for different DIMM's? I'd print some warning about that.
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : }
So a DIMM in only channel 1 would never boot? Same for different DIMM's? I'd print some warning abou […]
A single DIMM in channel 1 would surely fail. (I assume) Different DIMMs might work if the one in channel 1 can work with the same timings as the one in channel 0, even if the default its SPD reports does not match. Just a sidenote: This (https://imgur.com/6yuhHRR) is the only code inside the MRC I can find that directly touches the passed in SPD pointers, and to me it looks like different pointers would work, but I don't have a two channel board to test. And Angel said it won't work on his Q1900 so I went with this fix.
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : }
A single DIMM in channel 1 would surely fail. […]
(The 0xF0 and 0xF1 are fake SPD addresses the MRC's main function sets as markers for in memory SPD for channel 0 and 1.)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : }
(The 0xF0 and 0xF1 are fake SPD addresses the MRC's main function sets as markers for in memory SPD […]
Actually, the data buffers can be different, but the data for channel 0 needs to be at index 0, and the data for channel 1 needs to be at index 1. Using the same buffer avoids wasting space. I was given this snippet when I had problems getting the MRC to accept my SPDs: https://gist.github.com/Th3Fanbus/e01265a1f1c5b75360edb6c92b5ceeaa
I can try if different DIMMs and CH1-only setups work on Q1900M. I had quite a bit of trouble making RAM work because Baytrail MRC.bin does not like UDIMMs (I had to patch the SPDs to pretend they're SO-DIMMs) and requires 1.35V operation (I had to patch that too). Now that I put all the pieces together, it retrieves the SPDs from SMBus reliably.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : }
Actually, the data buffers can be different, but the data for channel 0 needs to be at index 0, and […]
Reading it again, I'd reword the comment:
Note: MRC looks for Channel 1 SPD at array index 1
Hello build bot (Jenkins), Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44092
to look at the new patch set (#5).
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC fails to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/44092/5
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44092/4/src/soc/intel/baytrail/roms... PS4, Line 170: if (mp->mainboard.spd_addrs[i]) { : i2c_eeprom_read(mp->mainboard.spd_addrs[i], : 0, SPD_SIZE, spd_buf[i]); : /* NOTE: the MRC expects both SPD pointers : to match */ : mp->mainboard.dram_data[i] = spd_buf; : }
Reading it again, I'd reword the comment: […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44092 )
Change subject: soc/intel/baytrail: Add MRC SMBus workaround ......................................................................
soc/intel/baytrail: Add MRC SMBus workaround
- The Bay Trail MRC fails to read the SPDs from SMBus. - Instead the SPDs are read into a buffer and the buffer is passed to the MRC.
Change-Id: I7f560d950cb4e4d118f3ee17e6e19e14cd0cc193 Signed-off-by: Mate Kukri kukri.mate@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44092 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/baytrail/romstage/raminit.c 1 file changed, 17 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c index 7e6bcab..6ff6c03 100644 --- a/src/soc/intel/baytrail/romstage/raminit.c +++ b/src/soc/intel/baytrail/romstage/raminit.c @@ -115,11 +115,15 @@ populate_smbios_tables(dram_data, speed, num_channels); }
+#define SPD_SIZE 256 +static u8 spd_buf[NUM_CHANNELS][SPD_SIZE]; + void raminit(struct mrc_params *mp, int prev_sleep_state) { int ret; mrc_wrapper_entry_t mrc_entry; struct region_device rdev; + size_t i;
/* Fill in default entries. */ mp->version = MRC_PARAMS_VER; @@ -158,8 +162,20 @@ */ mrc_entry = (void *)(uintptr_t)CONFIG_MRC_BIN_ADDRESS;
- if (mp->mainboard.dram_info_location == DRAM_INFO_SPD_SMBUS) + if (mp->mainboard.dram_info_location == DRAM_INFO_SPD_SMBUS) { + /* Workaround for broken SMBus support in the MRC */ enable_smbus(); + mp->mainboard.dram_info_location = DRAM_INFO_SPD_MEM; + for (i = 0; i < NUM_CHANNELS; ++i) { + if (mp->mainboard.spd_addrs[i]) { + i2c_eeprom_read(mp->mainboard.spd_addrs[i], + 0, SPD_SIZE, spd_buf[i]); + /* NOTE: MRC looks for Channel 1 SPD at array + index 1 */ + mp->mainboard.dram_data[i] = spd_buf; + } + } + }
ret = mrc_entry(mp);
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Máté Kukri (Code Review)