Hello Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48550
to review the following change.
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code
......................................................................
nb/intel/sandybridge: Fix blunder in MR2 shadow code
Commit 7f1363d9b4 (nb/intel/sandybridge: Program MR2 shadow register)
has a bug where the system locks up and power cycles when booting Linux,
but is still able to pass memtest86+ with flying colors. The issue will
occur when the following conditions are true:
- CPU is Ivy Bridge
- Memory speed is not greater than 1066 MHz (DDR3-2133 or slower)
- System contains dual-rank DIMMs
- The second rank of the dual-rank DIMMs is mirrored
- All DIMMs support Extended Temperature Range
- At least one of the DIMMs does not support Auto Self-Refresh
If all of these conditions are met, the final value of the MR2 Shadow
registers configures the memory controller to issue a MRS command to
update MR2 before entering self-refresh mode, but indicates that rank
mirroring is not required (the first rank on a DIMM is never mirrored).
Before the memory controller enters self-refresh, it sends MRS commands
to all ranks to update MR2, but the missing address and bank mirroring
means DRAM chips on mirrored ranks instead clobber MR1 with junk data.
With garbage in MR1, the mirrored ranks no longer function properly,
which ultimately leads to all hell breaking loose (undefined behavior).
The condition is backwards, since only odd ranks can be mirrored. To
avoid this problem completely, simply remove the condition. The final
register value will still be correct, since the bits are always ORed.
Tested on Asus P8Z77-V LX2, fixes booting Linux with dual-rank DIMMs.
Change-Id: Iceff741eb85fab0ae846e50af0080e5ff405404c
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M src/northbridge/intel/sandybridge/raminit_common.c
1 file changed, 6 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/48550/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c
index 399ba5a..80c5186 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.c
+++ b/src/northbridge/intel/sandybridge/raminit_common.c
@@ -815,13 +815,12 @@
reg32 |= mr2reg & ~(3 << 6);
- if (rank & 1) {
- if (srt)
- reg32 |= 1 << (rank / 2 + 6);
- } else {
- if (ctrl->rank_mirror[channel][rank])
- reg32 |= 1 << (rank / 2 + 14);
- }
+ if (srt)
+ reg32 |= 1 << (rank / 2 + 6);
+
+ if (ctrl->rank_mirror[channel][rank])
+ reg32 |= 1 << (rank / 2 + 14);
+
MCHBAR32(TC_MR2_SHADOW_ch(channel)) = reg32;
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/48550
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iceff741eb85fab0ae846e50af0080e5ff405404c
Gerrit-Change-Number: 48550
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newchange
Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39306
to review the following change.
Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................
cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
---
M src/drivers/pc80/rtc/option.c
M src/include/cbfs.h
M src/lib/cbfs.c
M src/northbridge/intel/haswell/raminit.c
M src/soc/intel/broadwell/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 60 insertions(+), 89 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/1
diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c
index 4789b15..1f9abb5 100644
--- a/src/drivers/pc80/rtc/option.c
+++ b/src/drivers/pc80/rtc/option.c
@@ -60,36 +60,28 @@
return CB_SUCCESS;
}
-static enum cb_err locate_cmos_layout(struct region_device *rdev)
+static struct cmos_option_table *get_cmos_layout(struct region_device *rdev)
{
- uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT;
- MAYBE_STATIC_BSS struct cbfsf fh = {};
+ MAYBE_STATIC_BSS struct cmos_option_table ct = NULL;
/*
* In case VBOOT is enabled and this function is called from SMM,
* we have multiple CMOS layout files and to locate them we'd need to
* include VBOOT into SMM...
*
- * Support only one CMOS layout in the 'COREBOOT' region for now.
+ * Support only one CMOS layout in the RO CBFS for now.
*/
- if (!region_device_sz(&(fh.data))) {
- if (cbfs_locate_file_in_region(&fh, "COREBOOT", "cmos_layout.bin",
- &cbfs_type)) {
- printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. "
- "Options are disabled\n");
- return CB_CMOS_LAYOUT_NOT_FOUND;
- }
- }
-
- cbfs_file_data(rdev, &fh);
-
- return CB_SUCCESS;
+ if (!ct)
+ ct = cbfs_ro_map("cmos_layout.bin", NULL);
+ if (!ct)
+ printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. "
+ "Options are disabled\n");
+ return ct;
}
enum cb_err cmos_get_option(void *dest, const char *name)
{
struct cmos_option_table *ct;
- struct region_device rdev;
struct cmos_entries *ce;
size_t namelen;
int found = 0;
@@ -97,16 +89,9 @@
/* Figure out how long name is */
namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
- if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+ ct = get_cmos_layout();
+ if (!ct)
return CB_CMOS_LAYOUT_NOT_FOUND;
- }
- ct = rdev_mmap_full(&rdev);
- if (!ct) {
- printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. "
- "Options are disabled\n");
-
- return CB_CMOS_LAYOUT_NOT_FOUND;
- }
/* find the requested entry record */
ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
@@ -119,19 +104,15 @@
}
if (!found) {
printk(BIOS_DEBUG, "No CMOS option '%s'.\n", name);
- rdev_munmap(&rdev, ct);
return CB_CMOS_OPTION_NOT_FOUND;
}
- if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC)) {
- rdev_munmap(&rdev, ct);
+ if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC))
return CB_CMOS_CHECKSUM_INVALID;
- }
- if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) {
- rdev_munmap(&rdev, ct);
+}
+ if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS)
return CB_CMOS_ACCESS_ERROR;
- }
- rdev_munmap(&rdev, ct);
+}
return CB_SUCCESS;
}
@@ -179,7 +160,6 @@
enum cb_err cmos_set_option(const char *name, void *value)
{
struct cmos_option_table *ct;
- struct region_device rdev;
struct cmos_entries *ce;
unsigned long length;
size_t namelen;
@@ -188,16 +168,9 @@
/* Figure out how long name is */
namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
- if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+ ct = get_cmos_layout();
+ if (!ct)
return CB_CMOS_LAYOUT_NOT_FOUND;
- }
- ct = rdev_mmap_full(&rdev);
- if (!ct) {
- printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. "
- "Options are disabled\n");
-
- return CB_CMOS_LAYOUT_NOT_FOUND;
- }
/* find the requested entry record */
ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
@@ -210,7 +183,6 @@
}
if (!found) {
printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
- rdev_munmap(&rdev, ct);
return CB_CMOS_OPTION_NOT_FOUND;
}
@@ -219,18 +191,13 @@
length = MAX(strlen((const char *)value) * 8, ce->length - 8);
/* make sure the string is null terminated */
if (set_cmos_value(ce->bit + ce->length - 8, 8, &(u8[]){0})
- != CB_SUCCESS) {
- rdev_munmap(&rdev, ct);
+ != CB_SUCCESS)
return CB_CMOS_ACCESS_ERROR;
- }
}
- if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) {
- rdev_munmap(&rdev, ct);
+ if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS)
return CB_CMOS_ACCESS_ERROR;
- }
- rdev_munmap(&rdev, ct);
return CB_SUCCESS;
}
diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index cc61b16..8cd5713 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -36,9 +36,12 @@
NOTE: Since this may return a direct pointer to memory-mapped hardware,
compressed files are NOT transparently decompressed (unlike cbfs_load()). */
void *cbfs_map(const char *name, size_t *size);
-/* Removes a mapping previously allocated with cbfs_map(). Should try to unmap
- mappings in strict LIFO order where possible, since mapping backends often
- don't support more complicated cases. */
+/* Like cbfs_map(), except that it will always read from the read-only CBFS
+ ("COREBOOT" FMAP region), even when CONFIG_VBOOT is enabled. */
+void *cbfs_ro_map(const char *name, size_t *size);
+/* Removes a previously allocated CBFS mapping. Should try to unmap mappings in
+ strict LIFO order where possible, since mapping backends often don't support
+ more complicated cases. */
int cbfs_unmap(void *mapping);
/* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/
int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name,
@@ -47,6 +50,9 @@
success or 0 on error. File will get decompressed as necessary. Same
decompression requirements as cbfs_load_and_decompress(). */
size_t cbfs_load(const char *name, void *buf, size_t buf_size);
+/* Like cbfs_load(), except that it will always read from the read-only CBFS
+ ("COREBOOT" FMAP region), even when CONFIG_VBOOT is enabled. */
+size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size);
/* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes
* large |buffer|, decompressing it according to |compression| in the process.
* Returns the decompressed file size, or 0 on error.
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index c3660aa..ba50c34 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -79,9 +79,9 @@
return 0;
}
-void *cbfs_map(const char *name, size_t *size)
+static void *_cbfs_map(const char *name, size_t *size, bool force_ro)
{
- const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
+ const struct cbfs_boot_device *cbd = cbfs_get_boot_device(force_ro);
union cbfs_mdata mdata;
size_t data_offset;
@@ -96,6 +96,16 @@
return rdev_mmap(&cbd->rdev, data_offset, data_size);
}
+void *cbfs_map(const char *name, size_t *size)
+{
+ return _cbfs_map(name, size, false);
+}
+
+void *cbfs_ro_map(const char *name, size_t *size)
+{
+ return _cbfs_map(name, size, true);
+}
+
int cbfs_unmap(void *mapping)
{
/* This works because munmap() only works on the root rdev and never
@@ -215,7 +225,8 @@
return prog_entry(&stage);
}
-size_t cbfs_load(const char *name, void *buf, size_t buf_size)
+static size_t _cbfs_load(const char *name, void *buf, size_t buf_size,
+ bool force_ro)
{
const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
union cbfs_mdata mdata;
@@ -238,6 +249,16 @@
compression);
}
+size_t cbfs_load(const char *name, void *buf, size_t buf_size)
+{
+ return _cbfs_load(name, buf, buf_size, false);
+}
+
+size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size)
+{
+ return _cbfs_load(name, buf, buf_size, true);
+}
+
size_t cbfs_prog_stage_section(struct prog *pstage, uintptr_t *base)
{
struct cbfs_stage stage;
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c
index 8267833..00211bd 100644
--- a/src/northbridge/intel/haswell/raminit.c
+++ b/src/northbridge/intel/haswell/raminit.c
@@ -118,8 +118,6 @@
void sdram_initialize(struct pei_data *pei_data)
{
unsigned long entry;
- uint32_t type = CBFS_TYPE_MRC;
- struct cbfsf f;
printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");
@@ -144,12 +142,11 @@
/*
* Locate and call UEFI System Agent binary. The binary needs to be at
* a fixed offset in the flash and can therefore only reside in the
- * COREBOOT fmap region
+ * COREBOOT fmap region. We don't care about leaking the mapping.
*/
- if (cbfs_locate_file_in_region(&f, "COREBOOT", "mrc.bin", &type) < 0)
+ entry = (unsigned long)cbfs_ro_map("mrc.bin", NULL);
+ if (!entry)
die("mrc.bin not found!");
- /* We don't care about leaking the mapping */
- entry = (unsigned long)rdev_mmap_full(&f.data);
if (entry) {
int rv;
asm volatile (
diff --git a/src/soc/intel/broadwell/romstage/raminit.c b/src/soc/intel/broadwell/romstage/raminit.c
index 3759c1f..bcf7148 100644
--- a/src/soc/intel/broadwell/romstage/raminit.c
+++ b/src/soc/intel/broadwell/romstage/raminit.c
@@ -43,8 +43,6 @@
struct memory_info *mem_info;
pei_wrapper_entry_t entry;
int ret;
- struct cbfsf f;
- uint32_t type = CBFS_TYPE_MRC;
broadwell_fill_pei_data(pei_data);
@@ -76,15 +74,10 @@
pei_data->saved_data_size = 0;
}
- /* Determine if mrc.bin is in the cbfs. */
- if (cbfs_locate_file_in_region(&f, "COREBOOT", "mrc.bin", &type) < 0)
- die("mrc.bin not found!");
/* We don't care about leaking the mapping */
- entry = (pei_wrapper_entry_t)rdev_mmap_full(&f.data);
- if (entry == NULL) {
- printk(BIOS_DEBUG, "Couldn't find mrc.bin\n");
- return;
- }
+ entry = cbfs_ro_map("mrc.bin", NULL);
+ if (entry == NULL)
+ die("mrc.bin not found!");
printk(BIOS_DEBUG, "Starting Memory Reference Code\n");
diff --git a/src/soc/intel/quark/romstage/romstage.c b/src/soc/intel/quark/romstage/romstage.c
index 4ac580e..3e08663 100644
--- a/src/soc/intel/quark/romstage/romstage.c
+++ b/src/soc/intel/quark/romstage/romstage.c
@@ -63,29 +63,16 @@
void *locate_rmu_file(size_t *rmu_file_len)
{
- struct cbfsf fh;
size_t fsize;
void *rmu_data;
- uint32_t type;
/* Locate the rmu.bin file in the read-only region of the flash */
- type = CBFS_TYPE_RAW;
- if (cbfs_locate_file_in_region(&fh, "COREBOOT", "rmu.bin", &type))
+ rmu_data = cbfs_ro_map("rmu.bin", &fsize);
+ if (!rmu_data)
return NULL;
- /* Get the file size */
- fsize = region_device_sz(&fh.data);
if (rmu_file_len != NULL)
*rmu_file_len = fsize;
- /* Get the data address */
- rmu_data = rdev_mmap(&fh.data, 0, fsize);
-
- /* Since the SPI flash is directly mapped into memory, we do not need
- * the mapping provided by the rdev service. Unmap the file to prevent
- * a memory leak. Return/leak the SPI flash address for the rmu.bin
- * file data which will be directly accessed by FSP MemoryInit.
- */
- rdev_munmap(&fh.data, rmu_data);
return rmu_data;
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/39306
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Gerrit-Change-Number: 39306
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newchange
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48594 )
Change subject: mb/clevo/kbl-u: drop duplicated configuration of UART pads
......................................................................
Set Ready For Review
--
To view, visit https://review.coreboot.org/c/coreboot/+/48594
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I95565a74e19d693a7d5ead81e72592cc4ca2038c
Gerrit-Change-Number: 48594
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Sat, 12 Dec 2020 00:57:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48593 )
Change subject: mb/clevo/cml-u: drop duplicated configuration of UART pads
......................................................................
Set Ready For Review
--
To view, visit https://review.coreboot.org/c/coreboot/+/48593
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I05a459b0af79c75c31b1bb26ea1a1a40857ef9bf
Gerrit-Change-Number: 48593
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Sat, 12 Dec 2020 00:54:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment