New subject: Change in coreboot[master]: nb/intel/haswell: Do some cosmetic fixes

15 Jan 2020

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38434 )
Change subject: [WIP] nb/intel/haswell: Do some cosmetic fixes
......................................................................
[WIP] nb/intel/haswell: Do some cosmetic fixes
- Reformat some lines of code
- Put names to the MCHBAR registers
- Rewrite a few comments
- Rewrite some hex constants
It does not change the binary of Asrock B85M Pro4.
Change-Id: I926289304acb834f9b13cd7902801798f8ee478a
Signed-off-by: Angel Pons th3fanbus@gmail.com
---
M src/northbridge/intel/haswell/acpi.c
M src/northbridge/intel/haswell/early_init.c
M src/northbridge/intel/haswell/finalize.c
M src/northbridge/intel/haswell/gma.c
M src/northbridge/intel/haswell/haswell.h
A src/northbridge/intel/haswell/mchbar_regs.h
M src/northbridge/intel/haswell/memmap.c
M src/northbridge/intel/haswell/minihd.c
M src/northbridge/intel/haswell/northbridge.c
M src/northbridge/intel/haswell/raminit.c
10 files changed, 376 insertions(+), 353 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/38434/1

diff --git a/src/northbridge/intel/haswell/acpi.c b/src/northbridge/intel/haswell/acpi.c
index d92e858..c6aaa86 100644
--- a/src/northbridge/intel/haswell/acpi.c
+++ b/src/northbridge/intel/haswell/acpi.c
@@ -38,35 +38,35 @@
pciexbar_reg = pci_read_config32(dev, PCIEXBAR);
-	// MMCFG not supported or not enabled.
+	/* MMCFG not supported or not enabled. */
    if (!(pciexbar_reg & (1 << 0)))
    	return current;
mask = (1UL << 31) | (1 << 30) | (1 << 29) | (1 << 28);
    switch ((pciexbar_reg >> 1) & 3) {
-	case 0: // 256MB
+	case 0: /* 256MB */
    	pciexbar = pciexbar_reg & mask;
    	max_buses = 256;
    	break;
-	case 1: // 128M
+	case 1: /* 128M */
    	mask |= (1 << 27);
    	pciexbar = pciexbar_reg & mask;
    	max_buses = 128;
    	break;
-	case 2: // 64M
+	case 2: /* 64M */
    	mask |= (1 << 27) | (1 << 26);
    	pciexbar = pciexbar_reg & mask;
    	max_buses = 64;
    	break;
-	default: // RSVD
+	default: /* RSVD */
    	return current;
    }
if (!pciexbar)
    	return current;
-	current += acpi_create_mcfg_mmconfig((acpi_mcfg_mmconfig_t *) current,
-			pciexbar, 0x0, 0x0, max_buses - 1);
+	current += acpi_create_mcfg_mmconfig((acpi_mcfg_mmconfig_t *) current, pciexbar, 0, 0,
+					     max_buses - 1);
return current;
 }
@@ -80,8 +80,8 @@
    const bool vtvc0en = MCHBAR32(VTVC0BAR) & 0x1;
/* iGFX has to be enabled; GFXVTBAR set, enabled, in 32-bit space */
-	if (igfx_dev && igfx_dev->enabled && gfxvtbar
-			&& gfxvten && !MCHBAR32(GFXVTBAR + 4)) {
+	if (igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten && !MCHBAR32(GFXVTBAR + 4)) {
+
    	const unsigned long tmp = current;
current += acpi_create_dmar_drhd(current, 0, 0, gfxvtbar);
@@ -92,24 +92,23 @@
/* VTVC0BAR has to be set, enabled, and in 32-bit space */
    if (vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) {
+
    	const unsigned long tmp = current;
-		current += acpi_create_dmar_drhd(current,
-				DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar);
-		current += acpi_create_dmar_ds_ioapic(current,
-				2, PCH_IOAPIC_PCI_BUS, PCH_IOAPIC_PCI_SLOT, 0);
+		current += acpi_create_dmar_drhd(current, DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar);
+		current += acpi_create_dmar_ds_ioapic(current, 2, PCH_IOAPIC_PCI_BUS,
+						      PCH_IOAPIC_PCI_SLOT, 0);
+
    	size_t i;
    	for (i = 0; i < 8; ++i)
-			current += acpi_create_dmar_ds_msi_hpet(current,
-					0, PCH_HPET_PCI_BUS,
-					PCH_HPET_PCI_SLOT, i);
+			current += acpi_create_dmar_ds_msi_hpet(current, 0, PCH_HPET_PCI_BUS,
+								PCH_HPET_PCI_SLOT, i);
    	acpi_dmar_drhd_fixup(tmp, current);
    }
return current;
 }
-unsigned long northbridge_write_acpi_tables(struct device *const dev,
-					    unsigned long current,
+unsigned long northbridge_write_acpi_tables(struct device *const dev, unsigned long current,
    				    struct acpi_rsdp *const rsdp)
 {
    /* Create DMAR table only if we have VT-d capability. */
diff --git a/src/northbridge/intel/haswell/early_init.c b/src/northbridge/intel/haswell/early_init.c
index 666bda2..5caede5 100644
--- a/src/northbridge/intel/haswell/early_init.c
+++ b/src/northbridge/intel/haswell/early_init.c
@@ -28,21 +28,21 @@
 {
    printk(BIOS_DEBUG, "Setting up static northbridge registers...");
    /* Set up all hardcoded northbridge BARs */
-	pci_write_config32(PCI_DEV(0, 0x00, 0), EPBAR, DEFAULT_EPBAR | 1);
-	pci_write_config32(PCI_DEV(0, 0x00, 0), EPBAR + 4, (0LL+DEFAULT_EPBAR) >> 32);
-	pci_write_config32(PCI_DEV(0, 0x00, 0), MCHBAR, DEFAULT_MCHBAR | 1);
-	pci_write_config32(PCI_DEV(0, 0x00, 0), MCHBAR + 4, (0LL+DEFAULT_MCHBAR) >> 32);
-	pci_write_config32(PCI_DEV(0, 0x00, 0), DMIBAR, (uintptr_t)DEFAULT_DMIBAR | 1);
-	pci_write_config32(PCI_DEV(0, 0x00, 0), DMIBAR + 4, (0LL+(uintptr_t)DEFAULT_DMIBAR) >> 32);
+	pci_write_config32(PCI_DEV(0, 0, 0), EPBAR,  DEFAULT_EPBAR  | 1);
+	pci_write_config32(PCI_DEV(0, 0, 0), EPBAR  + 4, (0LL + DEFAULT_EPBAR)  >> 32);
+	pci_write_config32(PCI_DEV(0, 0, 0), MCHBAR, DEFAULT_MCHBAR | 1);
+	pci_write_config32(PCI_DEV(0, 0, 0), MCHBAR + 4, (0LL + DEFAULT_MCHBAR) >> 32);
+	pci_write_config32(PCI_DEV(0, 0, 0), DMIBAR, DEFAULT_DMIBAR | 1);
+	pci_write_config32(PCI_DEV(0, 0, 0), DMIBAR + 4, (0LL + DEFAULT_DMIBAR) >> 32);
/* Set C0000-FFFFF to access RAM on both reads and writes */
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM0, 0x30);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM1, 0x33);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM2, 0x33);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM3, 0x33);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM4, 0x33);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM5, 0x33);
-	pci_write_config8(PCI_DEV(0, 0x00, 0), PAM6, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM0, 0x30);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM1, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM2, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM3, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM4, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM5, 0x33);
+	pci_write_config8(PCI_DEV(0, 0, 0), PAM6, 0x33);
printk(BIOS_DEBUG, " done.\n");
 }
@@ -55,8 +55,7 @@
printk(BIOS_DEBUG, "Initializing IGD...\n");
-	igd_enabled = !!(pci_read_config32(PCI_DEV(0, 0, 0), DEVEN)
-			 & DEVEN_D2EN);
+	igd_enabled = !!(pci_read_config32(PCI_DEV(0, 0, 0), DEVEN) & DEVEN_D2EN);
ggc = pci_read_config16(PCI_DEV(0, 0, 0), GGC);
    ggc &= ~0x3f8;
@@ -64,8 +63,7 @@
    	ggc |= GGC_GTT_2MB | GGC_IGD_MEM_IN_32MB_UNITS(1);
    	ggc &= ~GGC_DISABLE_VGA_IO_DECODE;
    } else {
-		ggc |= GGC_GTT_0MB | GGC_IGD_MEM_IN_32MB_UNITS(0) |
-		       GGC_DISABLE_VGA_IO_DECODE;
+		ggc |= GGC_GTT_0MB | GGC_IGD_MEM_IN_32MB_UNITS(0) | GGC_DISABLE_VGA_IO_DECODE;
    }
    pci_write_config16(PCI_DEV(0, 0, 0), GGC, ggc);
@@ -104,10 +102,9 @@
    printk(BIOS_DEBUG, "Started PEG1%d link training.\n", PCI_FUNC(dev));
/*
-	 * The PEG device is hidden while the MRC runs. This is because the
-	 * MRC makes configurations that are not ideal if it sees a VGA
-	 * device in a PEG slot, and it locks registers preventing changes
-	 * to these configurations.
+	 * Hide the PEG device while the MRC runs. This is because the MRC makes
+	 * configurations that are not ideal if it sees a VGA device in a PEG slot,
+	 * and it locks registers preventing changes to these configurations.
     */
    pci_update_config32(PCI_DEV(0, 0, 0), DEVEN, ~mask, 0);
    peg_hidden[PCI_FUNC(dev)] = true;
@@ -135,8 +132,10 @@
if (deven & DEVEN_D1F2EN)
    	start_peg2_link_training(PCI_DEV(0, 1, 2));
+
    if (deven & DEVEN_D1F1EN)
    	start_peg2_link_training(PCI_DEV(0, 1, 1));
+
    if (deven & DEVEN_D1F0EN)
    	start_peg2_link_training(PCI_DEV(0, 1, 0));
 }
@@ -146,27 +145,27 @@
    u32 reg32;
/* Erratum workarounds */
-	reg32 = MCHBAR32(0x5f00);
-	reg32 |= (1 << 9)|(1 << 10);
-	MCHBAR32(0x5f00) = reg32;
+	reg32 = MCHBAR32(SAPMCTL);
+	reg32 |= (1 << 9) | (1 << 10);
+	MCHBAR32(SAPMCTL) = reg32;
/* Enable SA Clock Gating */
-	reg32 = MCHBAR32(0x5f00);
-	MCHBAR32(0x5f00) = reg32 | 1;
+	reg32 = MCHBAR32(SAPMCTL);
+	MCHBAR32(SAPMCTL) = reg32 | 1;
/* GPU RC6 workaround for sighting 366252 */
-	reg32 = MCHBAR32(0x5d14);
+	reg32 = MCHBAR32(SSKPD + 4);
    reg32 |= (1UL << 31);
-	MCHBAR32(0x5d14) = reg32;
+	MCHBAR32(SSKPD + 4) = reg32;
-	/* VLW */
+	/* VLW (Virtual Legacy Wire?) */
    reg32 = MCHBAR32(0x6120);
    reg32 &= ~(1 << 0);
    MCHBAR32(0x6120) = reg32;
-	reg32 = MCHBAR32(0x5418);
+	reg32 = MCHBAR32(INTRDIRCTL);
    reg32 |= (1 << 4) | (1 << 5);
-	MCHBAR32(0x5418) = reg32;
+	MCHBAR32(INTRDIRCTL) = reg32;
 }
static void haswell_setup_iommu(void)
@@ -176,20 +175,21 @@
    if (capid0_a & VTD_DISABLE)
    	return;
-	/* setup BARs: zeroize top 32 bits; set enable bit */
+	/* Setup BARs: zeroize top 32 bits; set enable bit */
    MCHBAR32(GFXVTBAR + 4) = GFXVT_BASE_ADDRESS >> 32;
-	MCHBAR32(GFXVTBAR) = GFXVT_BASE_ADDRESS | 1;
+	MCHBAR32(GFXVTBAR)     = GFXVT_BASE_ADDRESS | 1;
    MCHBAR32(VTVC0BAR + 4) = VTVC0_BASE_ADDRESS >> 32;
-	MCHBAR32(VTVC0BAR) = VTVC0_BASE_ADDRESS | 1;
+	MCHBAR32(VTVC0BAR)     = VTVC0_BASE_ADDRESS | 1;
-	/* set L3HIT2PEND_DIS, lock GFXVTBAR policy cfg registers */
+	/* Set L3HIT2PEND_DIS, lock GFXVTBAR policy cfg registers */
    u32 reg32;
    reg32 = read32((void *)(GFXVT_BASE_ADDRESS + ARCHDIS));
-	write32((void *)(GFXVT_BASE_ADDRESS + ARCHDIS),
-			reg32 | DMAR_LCKDN | L3HIT2PEND_DIS);
-	/* clear SPCAPCTRL */
+	write32((void *)(GFXVT_BASE_ADDRESS + ARCHDIS), reg32 | DMAR_LCKDN | L3HIT2PEND_DIS);
+
+	/* Clear SPCAPCTRL */
    reg32 = read32((void *)(VTVC0_BASE_ADDRESS + ARCHDIS)) & ~SPCAPCTRL;
-	/* set GLBIOTLBINV, GLBCTXTINV; lock VTVC0BAR policy cfg registers */
+
+	/* Set GLBIOTLBINV, GLBCTXTINV; lock VTVC0BAR policy cfg registers */
    write32((void *)(VTVC0_BASE_ADDRESS + ARCHDIS),
    		reg32 | DMAR_LCKDN | GLBIOTLBINV | GLBCTXTINV);
 }
diff --git a/src/northbridge/intel/haswell/finalize.c b/src/northbridge/intel/haswell/finalize.c
index ca36634..e147bcc 100644
--- a/src/northbridge/intel/haswell/finalize.c
+++ b/src/northbridge/intel/haswell/finalize.c
@@ -33,19 +33,19 @@
    pci_or_config32(PCI_DEV_HSW, 0xb8, 1 << 0);	/* TSEGMB */
    pci_or_config32(PCI_DEV_HSW, 0xbc, 1 << 0);	/* TOLUD */
-	MCHBAR32_OR(0x5500, 1 << 0);	/* PAVP */
-	MCHBAR32_OR(0x5f00, 1UL << 31);	/* SA PM */
-	MCHBAR32_OR(0x6020, 1 << 0);	/* UMA GFX */
-	MCHBAR32_OR(0x63fc, 1 << 0);	/* VTDTRK */
-	MCHBAR32_OR(0x6800, 1UL << 31);
-	MCHBAR32_OR(0x7000, 1UL << 31);
-	MCHBAR32_OR(0x77fc, 1 << 0);
+	MCHBAR32_OR(MMIO_PAVP_MSG, 1 << 0);	/* PAVP */
+	MCHBAR32_OR(SAPMCTL, 1UL << 31);	/* SA PM */
+	MCHBAR32_OR(UMAGFXCTL, 1 << 0);		/* UMA GFX */
+	MCHBAR32_OR(VTDTRKLCK, 1 << 0);		/* VTDTRK */
+	MCHBAR32_OR(REQLIM, 1UL << 31);
+	MCHBAR32_OR(DMIVCLIM, 1UL << 31);
+	MCHBAR32_OR(CRDTLCK, 1 << 0);
/* Memory Controller Lockdown */
-	MCHBAR8(0x50fc) = 0x8f;
+	MCHBAR8(MC_LOCK) = 0x8f;
/* Read+write the following */
-	MCHBAR32(0x6030) = MCHBAR32(0x6030);
-	MCHBAR32(0x6034) = MCHBAR32(0x6034);
-	MCHBAR32(0x6008) = MCHBAR32(0x6008);
+	MCHBAR32(VDMBDFBARKVM)  = MCHBAR32(VDMBDFBARKVM);
+	MCHBAR32(VDMBDFBARPAVP) = MCHBAR32(VDMBDFBARPAVP);
+	MCHBAR32(HDAUDRID)      = MCHBAR32(HDAUDRID);
 }
diff --git a/src/northbridge/intel/haswell/gma.c b/src/northbridge/intel/haswell/gma.c
index 3132c20..b2dc880 100644
--- a/src/northbridge/intel/haswell/gma.c
+++ b/src/northbridge/intel/haswell/gma.c
@@ -97,9 +97,9 @@
    { 0 },
 };
-/* some vga option roms are used for several chipsets but they only have one
- * PCI ID in their header. If we encounter such an option rom, we need to do
- * the mapping ourselves
+/*
+ * Some vga option roms are used for several chipsets but they only have one PCI ID in their
+ * header. If we encounter such an option rom, we need to do the mapping ourselves.
  */
u32 map_oprom_vendev(u32 vendev)
@@ -129,39 +129,41 @@
    return new_vendev;
 }
-/* GTT is the Global Translation Table for the graphics pipeline.
- * It is used to translate graphics addresses to physical
- * memory addresses. As in the CPU, GTTs map 4K pages.
- * The setgtt function adds a further bit of flexibility:
- * it allows you to set a range (the first two parameters) to point
- * to a physical address (third parameter);the physical address is
- * incremented by a count (fourth parameter) for each GTT in the
- * range.
- * Why do it this way? For ultrafast startup,
- * we can point all the GTT entries to point to one page,
- * and set that page to 0s:
- * memset(physbase, 0, 4096);
- * setgtt(0, 4250, physbase, 0);
- * this takes about 2 ms, and is a win because zeroing
- * the page takes a up to 200 ms.
- * This call sets the GTT to point to a linear range of pages
- * starting at physbase.
+/** FIXME: Seems to be outdated. */
+/*
+ * GTT is the Global Translation Table for the graphics pipeline. It is used to translate
+ * graphics addresses to physical memory addresses. As in the CPU, GTTs map 4K pages.
+ *
+ * The setgtt function adds a further bit of flexibility: it allows you to set a range (the
+ * first two parameters) to point to a physical address (third parameter); the physical address
+ * is incremented by a count (fourth parameter) for each GTT in the range.
+ *
+ * Why do it this way? For ultrafast startup, we can point all the GTT entries to point to one
+ * page, and set that page to 0s:
+ *
+ *   memset(physbase, 0, 4096);
+ *   setgtt(0, 4250, physbase, 0);
+ *
+ * this takes about 2 ms, and is a win because zeroing the page takes up to 200 ms.
+ *
+ * This call sets the GTT to point to a linear range of pages starting at physbase.
  */
#define GTT_PTE_BASE (2 << 20)
-void
-set_translation_table(int start, int end, u64 base, int inc)
+void set_translation_table(int start, int end, u64 base, int inc)
 {
    int i;
for (i = start; i < end; i++){
-		u64 physical_address = base + i*inc;
+		u64 physical_address = base + i * inc;
+
    	/* swizzle the 32:39 bits to 4:11 */
    	u32 word = physical_address | ((physical_address >> 28) & 0xff0) | 1;
-		/* note: we've confirmed by checking
-		 * the values that mrc does no
-		 * useful setup before we run this.
+
+		/*
+		 * Note: we've confirmed by checking the values that mrc does no useful
+		 * setup before we run this.
    	 */
    	gtt_write(GTT_PTE_BASE + i * 4, word);
    	gtt_read(GTT_PTE_BASE + i * 4);
@@ -211,6 +213,7 @@
    	data = gtt_read(reg);
    	if ((data & mask) == value)
    		return 1;
+
    	udelay(10);
    }
@@ -261,10 +264,13 @@
/* Wait for Mailbox Ready */
    gtt_poll(0x138124, (1UL << 31), (0UL << 31));
+
    /* Mailbox Data - RC6 VIDS */
    gtt_write(0x138128, 0x00000000);
+
    /* Mailbox Command */
    gtt_write(0x138124, 0x80000004);
+
    /* Wait for Mailbox Ready */
    gtt_poll(0x138124, (1UL << 31), (0UL << 31));
@@ -291,7 +297,7 @@
    	gtt_write(CURBASE_IVB(pipe), 0x00000000);
    }
-	/* Disable primary plane and set surface base address*/
+	/* Disable primary plane and set surface base address */
    for (plane = PLANE_A; plane <= PLANE_C; plane++) {
    	gtt_write(DSPCNTR(plane), DISPLAY_PLANE_DISABLE);
    	gtt_write(DSPSURF(plane), 0x00000000);
@@ -491,10 +497,9 @@
    intel_gma_restore_opregion();
 }
-const struct i915_gpu_controller_info *
-intel_gma_get_controller_info(void)
+const struct i915_gpu_controller_info *intel_gma_get_controller_info(void)
 {
-	struct device *dev = pcidev_on_root(0x2, 0);
+	struct device *dev = pcidev_on_root(2, 0);
    if (!dev) {
    	return NULL;
    }
@@ -512,9 +517,8 @@
    drivers_intel_gma_displays_ssdt_generate(gfx);
 }
-static unsigned long
-gma_write_acpi_tables(struct device *const dev, unsigned long current,
-		      struct acpi_rsdp *const rsdp)
+static unsigned long gma_write_acpi_tables(struct device *const dev, unsigned long current,
+					   struct acpi_rsdp *const rsdp)
 {
    igd_opregion_t *opregion = (igd_opregion_t *)current;
    global_nvs_t *gnvs;
@@ -538,19 +542,19 @@
 }
static struct pci_operations gma_pci_ops = {
-	.set_subsystem    = pci_dev_set_subsystem,
+	.set_subsystem = pci_dev_set_subsystem,
 };
static struct device_operations gma_func0_ops = {
-	.read_resources		= pci_dev_read_resources,
-	.set_resources		= pci_dev_set_resources,
-	.enable_resources	= pci_dev_enable_resources,
-	.init			= gma_func0_init,
+	.read_resources           = pci_dev_read_resources,
+	.set_resources            = pci_dev_set_resources,
+	.enable_resources         = pci_dev_enable_resources,
+	.init                     = gma_func0_init,
    .acpi_fill_ssdt_generator = gma_ssdt,
-	.scan_bus		= 0,
-	.enable			= 0,
-	.ops_pci		= &gma_pci_ops,
-	.write_acpi_tables	= gma_write_acpi_tables,
+	.scan_bus                 = NULL,
+	.enable                   = NULL,
+	.ops_pci                  = &gma_pci_ops,
+	.write_acpi_tables        = gma_write_acpi_tables,
 };
static const unsigned short pci_device_ids[] = {
@@ -570,7 +574,7 @@
 };
static const struct pci_driver pch_lpc __pci_driver = {
-	.ops	 = &gma_func0_ops,
-	.vendor	 = PCI_VENDOR_ID_INTEL,
+	.ops     = &gma_func0_ops,
+	.vendor  = PCI_VENDOR_ID_INTEL,
    .devices = pci_device_ids,
 };
diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index fce9416..bb20b35 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -26,13 +26,9 @@
 #define IED_SIZE	CONFIG_IED_REGION_SIZE
/* Northbridge BARs */
-#define DEFAULT_MCHBAR		0xfed10000	/* 16 KB */
-#ifndef __ACPI__
-#define DEFAULT_DMIBAR		((u8 *)0xfed18000)	/* 4 KB */
-#else
-#define DEFAULT_DMIBAR		0xfed18000	/* 4 KB */
-#endif
-#define DEFAULT_EPBAR		0xfed19000	/* 4 KB */
+#define DEFAULT_MCHBAR		0xfed10000		/* 16 KB */
+#define DEFAULT_DMIBAR		0xfed18000		/* 4 KB */
+#define DEFAULT_EPBAR		0xfed19000		/* 4 KB */
#define GFXVT_BASE_ADDRESS	0xfed90000ULL
 #define GFXVT_BASE_SIZE		0x1000
@@ -85,11 +81,11 @@
 #define  G_SMRAME	(1 << 3)
 #define  C_BASE_SEG	((0 << 2) | (1 << 1) | (0 << 0))
-#define MESEG_BASE	0x70	/* Management Engine Base. */
-#define MESEG_LIMIT	0x78	/* Management Engine Limit. */
-#define REMAPBASE	0x90	/* Remap base. */
-#define REMAPLIMIT	0x98	/* Remap limit. */
-#define TOM		0xa0	/* Top of DRAM in memory controller space. */
+#define MESEG_BASE	0x70	/* Management Engine Base */
+#define MESEG_LIMIT	0x78	/* Management Engine Limit */
+#define REMAPBASE	0x90	/* Remap base */
+#define REMAPLIMIT	0x98	/* Remap limit */
+#define TOM		0xa0	/* Top of DRAM in memory controller space */
 #define TOUUD		0xa8	/* Top of Upper Usable DRAM */
 #define BDSM		0xb0	/* Base Data Stolen Memory */
 #define BGSM		0xb4	/* Base GTT Stolen Memory */
@@ -117,26 +113,27 @@
  * MCHBAR
  */
-#define MCHBAR8(x) *((volatile u8 *)(DEFAULT_MCHBAR + x))
-#define MCHBAR16(x) *((volatile u16 *)(DEFAULT_MCHBAR + x))
-#define MCHBAR32(x) *((volatile u32 *)(DEFAULT_MCHBAR + x))
-#define MCHBAR32_OR(x, or) MCHBAR32(x) = (MCHBAR32(x) | (or))
+#define MCHBAR8(x)  (*((volatile u8  *)(DEFAULT_MCHBAR + (x))))
+#define MCHBAR16(x) (*((volatile u16 *)(DEFAULT_MCHBAR + (x))))
+#define MCHBAR32(x) (*((volatile u32 *)(DEFAULT_MCHBAR + (x))))
+#define MCHBAR8_AND(x,  and) (MCHBAR8(x)  = MCHBAR8(x)  & (and))
+#define MCHBAR16_AND(x, and) (MCHBAR16(x) = MCHBAR16(x) & (and))
+#define MCHBAR32_AND(x, and) (MCHBAR32(x) = MCHBAR32(x) & (and))
+#define MCHBAR8_OR(x,  or) (MCHBAR8(x)  = MCHBAR8(x)  | (or))
+#define MCHBAR16_OR(x, or) (MCHBAR16(x) = MCHBAR16(x) | (or))
+#define MCHBAR32_OR(x, or) (MCHBAR32(x) = MCHBAR32(x) | (or))
+#define MCHBAR8_AND_OR(x,  and, or) (MCHBAR8(x)  = (MCHBAR8(x)  & (and)) | (or))
+#define MCHBAR16_AND_OR(x, and, or) (MCHBAR16(x) = (MCHBAR16(x) & (and)) | (or))
+#define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))
-#define BIOS_RESET_CPL	0x5da8	/* 8bit */
-#define GFXVTBAR	0x5400
-#define VTVC0BAR	0x5410
-
-/* Some power MSRs are also represented in MCHBAR */
-#define MCH_PKG_POWER_LIMIT_LO	0x59a0
-#define MCH_PKG_POWER_LIMIT_HI	0x59a4
-#define MCH_DDR_POWER_LIMIT_LO	0x58e0
-#define MCH_DDR_POWER_LIMIT_HI	0x58e4
+/* As there are many registers, define them on a separate file */
+#include "mchbar_regs.h"
/*
  * EPBAR - Egress Port Root Complex Register Block
  */
-#define EPBAR8(x) *((volatile u8 *)(DEFAULT_EPBAR + x))
+#define EPBAR8(x)  *((volatile u8  *)(DEFAULT_EPBAR + x))
 #define EPBAR16(x) *((volatile u16 *)(DEFAULT_EPBAR + x))
 #define EPBAR32(x) *((volatile u32 *)(DEFAULT_EPBAR + x))
@@ -167,7 +164,7 @@
  * DMIBAR
  */
-#define DMIBAR8(x) *((volatile u8 *)(DEFAULT_DMIBAR + x))
+#define DMIBAR8(x)  *((volatile u8  *)(DEFAULT_DMIBAR + x))
 #define DMIBAR16(x) *((volatile u16 *)(DEFAULT_DMIBAR + x))
 #define DMIBAR32(x) *((volatile u32 *)(DEFAULT_DMIBAR + x))
@@ -215,9 +212,9 @@
 #include <device/device.h>
struct acpi_rsdp;
-unsigned long northbridge_write_acpi_tables(struct device *device,
-		unsigned long start, struct acpi_rsdp *rsdp);
+unsigned long northbridge_write_acpi_tables(struct device *device, unsigned long start,
+					    struct acpi_rsdp *rsdp);
-#endif
-#endif
+#endif /* __ASSEMBLER__ */
+#endif /* __ACPI__ */
 #endif /* __NORTHBRIDGE_INTEL_HASWELL_HASWELL_H__ */
diff --git a/src/northbridge/intel/haswell/mchbar_regs.h b/src/northbridge/intel/haswell/mchbar_regs.h
new file mode 100644
index 0000000..edddf9d
--- /dev/null
+++ b/src/northbridge/intel/haswell/mchbar_regs.h
@@ -0,0 +1,58 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __HASWELL_MCHBAR_REGS_H__
+#define __HASWELL_MCHBAR_REGS_H__
+
+/* Register definitions */
+#define MAD_CHNL		0x5000 /* Address Decoder Channel Configuration */
+#define MAD_DIMM_CH0		0x5004 /* Address Decode Channel 0 */
+#define MAD_DIMM_CH1		0x5008 /* Address Decode Channel 1 */
+#define MAD_DIMM_CH2		0x500c /* Address Decode Channel 2 (unused on HSW) */
+#define MC_INIT_STATE_G		0x5030
+#define MRC_REVISION		0x5034 /* MRC Revision */
+
+#define MC_LOCK			0x50fc /* Memory Controlller Lock register */
+
+#define GFXVTBAR		0x5400 /* Base address for IGD */
+#define EDRAMBAR		0x5408 /* Base address for eDRAM */
+#define VTVC0BAR		0x5410 /* Base address for PEG, USB, SATA, etc. */
+#define INTRDIRCTL		0x5418 /* Interrupt Redirection Control (PAIR) */
+#define GDXCBAR			0x5420 /* Generic Debug eXternal Connection */
+
+/* PAVP message register. Bit 0 locks PAVP settings, and bits [31..20] are an offset. */
+#define MMIO_PAVP_MSG		0x5500
+
+/* Some power MSRs are also represented in MCHBAR */
+#define MCH_PKG_POWER_LIMIT_LO	0x59a0
+#define MCH_PKG_POWER_LIMIT_HI	0x59a4
+
+#define MCH_DDR_POWER_LIMIT_LO	0x58e0
+#define MCH_DDR_POWER_LIMIT_HI	0x58e4
+
+#define SSKPD			0x5d10 /* 64-bit scratchpad register */
+#define BIOS_RESET_CPL		0x5da8 /* 8bit */
+
+#define MC_BIOS_DATA		0x5e04 /* Miscellaneous information for BIOS */
+#define SAPMCTL			0x5f00
+
+#define HDAUDRID		0x6008
+#define UMAGFXCTL		0x6020
+#define VDMBDFBARKVM		0x6030
+#define VDMBDFBARPAVP		0x6034
+#define VTDTRKLCK		0x63fc
+#define REQLIM			0x6800
+#define DMIVCLIM		0x7000
+#define CRDTLCK			0x77fc
+
+#endif /* __HASWELL_MCHBAR_REGS_H__ */
diff --git a/src/northbridge/intel/haswell/memmap.c b/src/northbridge/intel/haswell/memmap.c
index 74d9292..bde7ecf 100644
--- a/src/northbridge/intel/haswell/memmap.c
+++ b/src/northbridge/intel/haswell/memmap.c
@@ -13,7 +13,7 @@
  * GNU General Public License for more details.
  */
-// Use simple device model for this file even in ramstage
+/* Use simple device model for this file even in ramstage */
 #define __SIMPLE_DEVICE__
#include <arch/romstage.h>
@@ -30,7 +30,7 @@
     * Base of TSEG is top of usable DRAM below 4GiB. The register has
     * 1 MiB alignment.
     */
-	uintptr_t tom = pci_read_config32(PCI_DEV(0,0,0), TSEG);
+	uintptr_t tom = pci_read_config32(PCI_DEV(0, 0, 0), TSEG);
    return tom & ~((1 << 20) - 1);
 }
@@ -53,7 +53,6 @@
     * above top of the ram. This satisfies MTRR alignment requirement
     * with different TSEG size configurations.
     */
-	top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8*MiB);
-	postcar_frame_add_mtrr(pcf, top_of_ram - 8*MiB, 16*MiB,
-			MTRR_TYPE_WRBACK);
+	top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8 * MiB);
+	postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
 }
diff --git a/src/northbridge/intel/haswell/minihd.c b/src/northbridge/intel/haswell/minihd.c
index 52b158e..c59d02e 100644
--- a/src/northbridge/intel/haswell/minihd.c
+++ b/src/northbridge/intel/haswell/minihd.c
@@ -25,30 +25,30 @@
static const u32 minihd_verb_table[] = {
    /* coreboot specific header */
-	0x80862807,	// Codec Vendor / Device ID: Intel Haswell Mini-HD
-	0x80860101,	// Subsystem ID
-	0x00000004,	// Number of jacks
+	0x80862807,	/* Codec Vendor / Device ID: Intel Haswell Mini-HD */
+	0x80860101,	/* Subsystem ID */
+	4,		/* Number of jacks */
/* Enable 3rd Pin and Converter Widget */
    0x00878101,
/* Pin Widget 5 - PORT B */
-	0x00571C10,
-	0x00571D00,
-	0x00571E56,
-	0x00571F18,
+	0x00571c10,
+	0x00571d00,
+	0x00571e56,
+	0x00571f18,
/* Pin Widget 6 - PORT C */
-	0x00671C20,
-	0x00671D00,
-	0x00671E56,
-	0x00671F18,
+	0x00671c20,
+	0x00671d00,
+	0x00671e56,
+	0x00671f18,
/* Pin Widget 7 - PORT D */
-	0x00771C30,
-	0x00771D00,
-	0x00771E56,
-	0x00771F18,
+	0x00771c30,
+	0x00771d00,
+	0x00771e56,
+	0x00771f18,
/* Disable 3rd Pin and Converter Widget */
    0x00878100,
@@ -94,15 +94,14 @@
    if (codec_mask) {
    	for (i = 3; i >= 0; i--) {
    		if (codec_mask & (1 << i))
-				hda_codec_init(base, i,
-					       sizeof(minihd_verb_table),
+				hda_codec_init(base, i, sizeof(minihd_verb_table),
    				       minihd_verb_table);
    	}
    }
 }
static struct pci_operations minihd_pci_ops = {
-	.set_subsystem    = pci_dev_set_subsystem,
+	.set_subsystem = pci_dev_set_subsystem,
 };
static struct device_operations minihd_ops = {
@@ -110,7 +109,7 @@
    .set_resources		= pci_dev_set_resources,
    .enable_resources	= pci_dev_enable_resources,
    .init			= minihd_init,
-	.scan_bus		= 0,
+	.scan_bus		= NULL,
    .ops_pci		= &minihd_pci_ops,
 };
diff --git a/src/northbridge/intel/haswell/northbridge.c b/src/northbridge/intel/haswell/northbridge.c
index 79ab747..a688e1c 100644
--- a/src/northbridge/intel/haswell/northbridge.c
+++ b/src/northbridge/intel/haswell/northbridge.c
@@ -31,11 +31,9 @@
 #include "chip.h"
 #include "haswell.h"
-static int get_pcie_bar(struct device *dev, unsigned int index, u32 *base,
-			u32 *len)
+static int get_pcie_bar(struct device *dev, unsigned int index, u32 *base, u32 *len)
 {
-	u32 pciexbar_reg;
-	u32 mask;
+	u32 pciexbar_reg, mask;
*base = 0;
    *len = 0;
@@ -46,18 +44,18 @@
    	return 0;
switch ((pciexbar_reg >> 1) & 3) {
-	case 0: // 256MB
+	case 0: /* 256MB */
    	mask = (1UL << 31) | (1 << 30) | (1 << 29) | (1 << 28);
    	*base = pciexbar_reg & mask;
    	*len = 256 * 1024 * 1024;
    	return 1;
-	case 1: // 128M
+	case 1: /* 128M */
    	mask = (1UL << 31) | (1 << 30) | (1 << 29) | (1 << 28);
    	mask |= (1 << 27);
    	*base = pciexbar_reg & mask;
    	*len = 128 * 1024 * 1024;
    	return 1;
-	case 2: // 64M
+	case 2: /* 64M */
    	mask = (1UL << 31) | (1 << 30) | (1 << 29) | (1 << 28);
    	mask |= (1 << 27) | (1 << 26);
    	*base = pciexbar_reg & mask;
@@ -89,50 +87,47 @@
    return NULL;
 }
-	/* TODO We could determine how many PCIe busses we need in
-	 * the bar. For now that number is hardcoded to a max of 64.
-	 */
+/*
+ * TODO: We could determine how many PCIe busses we need in the bar.
+ *       For now, that number is hardcoded to a max of 64.
+ */
 static struct device_operations pci_domain_ops = {
-	.read_resources   = pci_domain_read_resources,
-	.set_resources    = pci_domain_set_resources,
-	.enable_resources = NULL,
-	.init             = NULL,
-	.scan_bus         = pci_domain_scan_bus,
-	.acpi_name	  = northbridge_acpi_name,
+	.read_resources    = pci_domain_read_resources,
+	.set_resources     = pci_domain_set_resources,
+	.enable_resources  = NULL,
+	.init              = NULL,
+	.scan_bus          = pci_domain_scan_bus,
+	.acpi_name         = northbridge_acpi_name,
    .write_acpi_tables = northbridge_write_acpi_tables,
 };
static int get_bar(struct device *dev, unsigned int index, u32 *base, u32 *len)
 {
-	u32 bar;
+	u32 bar = pci_read_config32(dev, index);
-	bar = pci_read_config32(dev, index);
-
-	/* If not enabled don't report it. */
+	/* If not enabled don't report it */
    if (!(bar & 0x1))
    	return 0;
-	/* Knock down the enable bit. */
+	/* Knock down the enable bit */
    *base = bar & ~1;
return 1;
 }
-/* There are special BARs that actually are programmed in the MCHBAR. These
- * Intel special features, but they do consume resources that need to be
- * accounted for. */
-static int get_bar_in_mchbar(struct device *dev, unsigned int index,
-			     u32 *base, u32 *len)
+/*
+ * There are special BARs that actually are programmed in the MCHBAR. These Intel special
+ * features, but they do consume resources that need to be accounted for.
+ */
+static int get_bar_in_mchbar(struct device *dev, unsigned int index, u32 *base, u32 *len)
 {
-	u32 bar;
+	u32 bar = MCHBAR32(index);
-	bar = MCHBAR32(index);
-
-	/* If not enabled don't report it. */
+	/* If not enabled don't report it */
    if (!(bar & 0x1))
    	return 0;
-	/* Knock down the enable bit. */
+	/* Knock down the enable bit */
    *base = bar & ~1;
return 1;
@@ -141,26 +136,22 @@
 struct fixed_mmio_descriptor {
    unsigned int index;
    u32 size;
-	int (*get_resource)(struct device *dev, unsigned int index,
-			    u32 *base, u32 *size);
+	int (*get_resource)(struct device *dev, unsigned int index, u32 *base, u32 *size);
    const char *description;
 };
-#define SIZE_KB(x) ((x)*1024)
+#define SIZE_KB(x) ((x) * 1024)
 struct fixed_mmio_descriptor mc_fixed_resources[] = {
    { PCIEXBAR, SIZE_KB(0),  get_pcie_bar,      "PCIEXBAR" },
    { MCHBAR,   SIZE_KB(32), get_bar,           "MCHBAR"   },
    { DMIBAR,   SIZE_KB(4),  get_bar,           "DMIBAR"   },
    { EPBAR,    SIZE_KB(4),  get_bar,           "EPBAR"    },
-	{ 0x5420,   SIZE_KB(4),  get_bar_in_mchbar, "GDXCBAR"  },
-	{ 0x5408,   SIZE_KB(16), get_bar_in_mchbar, "EDRAMBAR" },
+	{ GDXCBAR,  SIZE_KB(4),  get_bar_in_mchbar, "GDXCBAR"  },
+	{ EDRAMBAR, SIZE_KB(16), get_bar_in_mchbar, "EDRAMBAR" },
 };
 #undef SIZE_KB
-/*
- * Add all known fixed MMIO ranges that hang off the host bridge/memory
- * controller device.
- */
+/* Add all known fixed MMIO ranges that hang off the host bridge/memory controller device. */
 static void mc_add_fixed_mmio_resources(struct device *dev)
 {
    int i;
@@ -173,14 +164,13 @@
size = mc_fixed_resources[i].size;
    	index = mc_fixed_resources[i].index;
-		if (!mc_fixed_resources[i].get_resource(dev, index,
-							&base, &size))
+		if (!mc_fixed_resources[i].get_resource(dev, index, &base, &size))
    		continue;
resource = new_resource(dev, mc_fixed_resources[i].index);
-		resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
-				  IORESOURCE_STORED | IORESOURCE_RESERVE |
-				  IORESOURCE_ASSIGNED;
+		resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED |
+				  IORESOURCE_RESERVE | IORESOURCE_ASSIGNED;
+
    	resource->base = base;
    	resource->size = size;
    	printk(BIOS_DEBUG, "%s: Adding %s @ %x 0x%08lx-0x%08lx.\n",
@@ -205,10 +195,10 @@
  * |     Usage DRAM           |
  * +--------------------------+ 0
  *
- * Some of the base registers above can be equal making the size of those
- * regions 0. The reason is because the memory controller internally subtracts
- * the base registers from each other to determine sizes of the regions. In
- * other words, the memory map is in a fixed order no matter what.
+ * Some of the base registers above can be equal, making the size of the regions within 0.
+ * This is because the memory controller internally subtracts the base registers from each
+ * other to determine sizes of the regions. In other words, the memory map regions are always
+ * in a fixed order, no matter what sizes they have.
  */
struct map_entry {
@@ -218,14 +208,13 @@
    const char *description;
 };
-static void read_map_entry(struct device *dev, struct map_entry *entry,
-			   uint64_t *result)
+static void read_map_entry(struct device *dev, struct map_entry *entry, uint64_t *result)
 {
    uint64_t value;
    uint64_t mask;
-	/* All registers are on a 1MiB granularity. */
-	mask = ((1ULL<<20)-1);
+	/* All registers have a 1MiB granularity */
+	mask = ((1ULL << 20) - 1);
    mask = ~mask;
value = 0;
@@ -252,12 +241,9 @@
    	.description = desc_,  \
    }
-#define MAP_ENTRY_BASE_64(reg_, desc_) \
-	MAP_ENTRY(reg_, 1, 0, desc_)
-#define MAP_ENTRY_LIMIT_64(reg_, desc_) \
-	MAP_ENTRY(reg_, 1, 1, desc_)
-#define MAP_ENTRY_BASE_32(reg_, desc_) \
-	MAP_ENTRY(reg_, 0, 0, desc_)
+#define MAP_ENTRY_BASE_32(reg_, desc_)	MAP_ENTRY(reg_, 0, 0, desc_)
+#define MAP_ENTRY_BASE_64(reg_, desc_)	MAP_ENTRY(reg_, 1, 0, desc_)
+#define MAP_ENTRY_LIMIT_64(reg_, desc_)	MAP_ENTRY(reg_, 1, 1, desc_)
enum {
    TOM_REG,
@@ -270,21 +256,21 @@
    BGSM_REG,
    BDSM_REG,
    TSEG_REG,
-	// Must be last.
-	NUM_MAP_ENTRIES
+	/* Must be last */
+	NUM_MAP_ENTRIES,
 };
static struct map_entry memory_map[NUM_MAP_ENTRIES] = {
-	[TOM_REG] = MAP_ENTRY_BASE_64(TOM, "TOM"),
-	[TOUUD_REG] = MAP_ENTRY_BASE_64(TOUUD, "TOUUD"),
-	[MESEG_BASE_REG] = MAP_ENTRY_BASE_64(MESEG_BASE, "MESEG_BASE"),
+	[TOM_REG]         = MAP_ENTRY_BASE_64(TOM, "TOM"),
+	[TOUUD_REG]       = MAP_ENTRY_BASE_64(TOUUD, "TOUUD"),
+	[MESEG_BASE_REG]  = MAP_ENTRY_BASE_64(MESEG_BASE, "MESEG_BASE"),
    [MESEG_LIMIT_REG] = MAP_ENTRY_LIMIT_64(MESEG_LIMIT, "MESEG_LIMIT"),
-	[REMAP_BASE_REG] = MAP_ENTRY_BASE_64(REMAPBASE, "REMAP_BASE"),
+	[REMAP_BASE_REG]  = MAP_ENTRY_BASE_64(REMAPBASE, "REMAP_BASE"),
    [REMAP_LIMIT_REG] = MAP_ENTRY_LIMIT_64(REMAPLIMIT, "REMAP_LIMIT"),
-	[TOLUD_REG] = MAP_ENTRY_BASE_32(TOLUD, "TOLUD"),
-	[BDSM_REG] = MAP_ENTRY_BASE_32(BDSM, "BDSM"),
-	[BGSM_REG] = MAP_ENTRY_BASE_32(BGSM, "BGSM"),
-	[TSEG_REG] = MAP_ENTRY_BASE_32(TSEG, "TESGMB"),
+	[TOLUD_REG]       = MAP_ENTRY_BASE_32(TOLUD, "TOLUD"),
+	[BDSM_REG]        = MAP_ENTRY_BASE_32(BDSM, "BDSM"),
+	[BGSM_REG]        = MAP_ENTRY_BASE_32(BGSM, "BGSM"),
+	[TSEG_REG]        = MAP_ENTRY_BASE_32(TSEG, "TESGMB"),
 };
static void mc_read_map_entries(struct device *dev, uint64_t *values)
@@ -302,51 +288,45 @@
    	printk(BIOS_DEBUG, "MC MAP: %s: 0x%llx\n",
    	       memory_map[i].description, values[i]);
    }
-	/* One can validate the BDSM and BGSM against the GGC. */
+	/* One can validate the BDSM and BGSM against the GGC */
    printk(BIOS_DEBUG, "MC MAP: GGC: 0x%x\n", pci_read_config16(dev, GGC));
 }
static void mc_add_dram_resources(struct device *dev, int *resource_cnt)
 {
-	unsigned long base_k, size_k;
-	unsigned long touud_k;
-	unsigned long index;
+	unsigned long base_k, size_k, touud_k, index;
    struct resource *resource;
    uint64_t mc_values[NUM_MAP_ENTRIES];
-	/* Read in the MAP registers and report their values. */
+	/* Read in the MAP registers and report their values */
    mc_read_map_entries(dev, &mc_values[0]);
    mc_report_map_entries(dev, &mc_values[0]);
/*
     * These are the host memory ranges that should be added:
-	 * - 0 -> 0xa0000: cacheable
-	 * - 0xc0000 -> TSEG : cacheable
-	 * - TESG -> BGSM: cacheable with standard MTRRs and reserved
-	 * - BGSM -> TOLUD: not cacheable with standard MTRRs and reserved
-	 * - 4GiB -> TOUUD: cacheable
+	 * - 0 -> 0xa0000:    cacheable
+	 * - 0xc0000 -> TSEG: cacheable
+	 * - TSEG -> BGSM:    cacheable with standard MTRRs and reserved
+	 * - BGSM -> TOLUD:   not cacheable with standard MTRRs and reserved
+	 * - 4GiB -> TOUUD:   cacheable
     *
-	 * The default SMRAM space is reserved so that the range doesn't
-	 * have to be saved during S3 Resume. Once marked reserved the OS
-	 * cannot use the memory. This is a bit of an odd place to reserve
-	 * the region, but the CPU devices don't have dev_ops->read_resources()
-	 * called on them.
+	 * The default SMRAM space is reserved so that the range doesn't have to be saved
+	 * during S3 Resume. Once marked reserved the OS cannot use the memory. This is a
+	 * bit of an odd place to reserve the region, but the CPU devices don't have
+	 * dev_ops->read_resources() called on them.
     *
-	 * The range 0xa0000 -> 0xc0000 does not have any resources
-	 * associated with it to handle legacy VGA memory. If this range
-	 * is not omitted the mtrr code will setup the area as cacheable
-	 * causing VGA access to not work.
+	 * The range 0xa0000 -> 0xc0000 does not have any resources associated with it to
+	 * handle legacy VGA memory. If this range is not omitted the mtrr code will setup
+	 * the area as cacheable, causing VGA access to not work.
     *
-	 * The TSEG region is mapped as cacheable so that one can perform
-	 * SMRAM relocation faster. Once the SMRR is enabled the SMRR takes
-	 * precedence over the existing MTRRs covering this region.
+	 * The TSEG region is mapped as cacheable so that one can perform SMRAM relocation
+	 * faster. Once the SMRR is enabled, the SMRR takes precedence over the existing
+	 * MTRRs covering this region.
     *
-	 * It should be noted that cacheable entry types need to be added in
-	 * order. The reason is that the current MTRR code assumes this and
-	 * falls over itself if it isn't.
+	 * It should be noted that cacheable entry types need to be added in order. The reason
+	 * is that the current MTRR code assumes this and falls over itself if it isn't.
     *
-	 * The resource index starts low and should not meet or exceed
-	 * PCI_BASE_ADDRESS_0.
+	 * The resource index starts low and should not meet or exceed PCI_BASE_ADDRESS_0.
     */
    index = *resource_cnt;
@@ -364,18 +344,16 @@
    resource = new_resource(dev, index++);
    resource->base = mc_values[TSEG_REG];
    resource->size = mc_values[BGSM_REG] - resource->base;
-	resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
-			  IORESOURCE_STORED | IORESOURCE_RESERVE |
-			  IORESOURCE_ASSIGNED | IORESOURCE_CACHEABLE;
+	resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED |
+			  IORESOURCE_RESERVE | IORESOURCE_ASSIGNED | IORESOURCE_CACHEABLE;
-	/* BGSM -> TOLUD. If the IGD is disabled, BGSM can equal TOLUD */
+	/* BGSM -> TOLUD. If the IGD is disabled, BGSM can equal TOLUD. */
    if (mc_values[BGSM_REG] != mc_values[TOLUD_REG]) {
    	resource = new_resource(dev, index++);
    	resource->base = mc_values[BGSM_REG];
    	resource->size = mc_values[TOLUD_REG] - resource->base;
-		resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
-				  IORESOURCE_STORED | IORESOURCE_RESERVE |
-				  IORESOURCE_ASSIGNED;
+		resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED |
+				  IORESOURCE_RESERVE | IORESOURCE_ASSIGNED;
    }
/* 4GiB -> TOUUD */
@@ -391,12 +369,12 @@
     * 0xc0000 - 0xfffff: RAM
     */
    mmio_resource(dev, index++, (0xa0000 >> 10), (0xc0000 - 0xa0000) >> 10);
-	reserved_ram_resource(dev, index++, (0xc0000 >> 10),
-			      (0x100000 - 0xc0000) >> 10);
+	reserved_ram_resource(dev, index++, (0xc0000 >> 10), (0x100000 - 0xc0000) >> 10);
+
 #if CONFIG(CHROMEOS_RAMOOPS)
    reserved_ram_resource(dev, index++,
    		CONFIG_CHROMEOS_RAMOOPS_RAM_START >> 10,
-			CONFIG_CHROMEOS_RAMOOPS_RAM_SIZE >> 10);
+			CONFIG_CHROMEOS_RAMOOPS_RAM_SIZE  >> 10);
 #endif
    *resource_cnt = index;
 }
@@ -404,31 +382,27 @@
 static void mc_read_resources(struct device *dev)
 {
    int index = 0;
-	const bool vtd_capable =
-		!(pci_read_config32(dev, CAPID0_A) & VTD_DISABLE);
+	const bool vtd_capable = !(pci_read_config32(dev, CAPID0_A) & VTD_DISABLE);
-	/* Read standard PCI resources. */
+	/* Read standard PCI resources */
    pci_dev_read_resources(dev);
-	/* Add all fixed MMIO resources. */
+	/* Add all fixed MMIO resources */
    mc_add_fixed_mmio_resources(dev);
-	/* Add VT-d MMIO resources if capable */
+	/* Add VT-d MMIO resources, if capable */
    if (vtd_capable) {
-		mmio_resource(dev, index++, GFXVT_BASE_ADDRESS / KiB,
-			GFXVT_BASE_SIZE / KiB);
-		mmio_resource(dev, index++, VTVC0_BASE_ADDRESS / KiB,
-			VTVC0_BASE_SIZE / KiB);
+		mmio_resource(dev, index++, GFXVT_BASE_ADDRESS / KiB, GFXVT_BASE_SIZE / KiB);
+		mmio_resource(dev, index++, VTVC0_BASE_ADDRESS / KiB, VTVC0_BASE_SIZE / KiB);
    }
-	/* Calculate and add DRAM resources. */
+	/* Calculate and add DRAM resources */
    mc_add_dram_resources(dev, &index);
 }
/*
- * The Mini-HD audio device is disabled whenever the IGD is. This is
- * because it provides audio over the integrated graphics port(s), which
- * requires the IGD to be functional.
+ * The Mini-HD audio device is disabled whenever the IGD is. This is because it provides
+ * audio over the integrated graphics port(s), which requires the IGD to be functional.
  */
 static void disable_devices(void)
 {
@@ -446,7 +420,7 @@
    	{ PCI_DEVFN(7, 0), DEVEN_D7EN, ""device 7"" },
    };
-	struct device *host_dev = pcidev_on_root(0x0, 0);
+	struct device *host_dev = pcidev_on_root(0, 0);
    u32 deven;
    size_t i;
@@ -470,29 +444,29 @@
 {
    u8 bios_reset_cpl, pair;
-	/* Enable Power Aware Interrupt Routing */
-	pair = MCHBAR8(0x5418);
+	/* Enable Power Aware Interrupt Routing. */
+	pair = MCHBAR8(INTRDIRCTL);
    pair &= ~0x7;	/* Clear 2:0 */
    pair |= 0x4;	/* Fixed Priority */
-	MCHBAR8(0x5418) = pair;
+	MCHBAR8(INTRDIRCTL) = pair;
disable_devices();
/*
-	 * Set bits 0+1 of BIOS_RESET_CPL to indicate to the CPU
-	 * that BIOS has initialized memory and power management
+	 * Set bits 0 + 1 of BIOS_RESET_CPL to indicate to the CPU
+	 * that BIOS has initialized memory and power management.
     */
    bios_reset_cpl = MCHBAR8(BIOS_RESET_CPL);
    bios_reset_cpl |= 3;
    MCHBAR8(BIOS_RESET_CPL) = bios_reset_cpl;
    printk(BIOS_DEBUG, "Set BIOS_RESET_CPL\n");
-	/* Configure turbo power limits 1ms after reset complete bit */
+	/* Configure turbo power limits 1ms after reset complete bit. */
    mdelay(1);
    set_power_limits(28);
-	/* Set here before graphics PM init */
-	MCHBAR32(0x5500) = 0x00100001;
+	/* Set here before graphics PM init. */
+	MCHBAR32(MMIO_PAVP_MSG) = 0x00100001;
 }
static struct pci_operations intel_pci_ops = {
@@ -500,13 +474,13 @@
 };
static struct device_operations mc_ops = {
-	.read_resources   = mc_read_resources,
-	.set_resources    = pci_dev_set_resources,
-	.enable_resources = pci_dev_enable_resources,
-	.init             = northbridge_init,
+	.read_resources           = mc_read_resources,
+	.set_resources            = pci_dev_set_resources,
+	.enable_resources         = pci_dev_enable_resources,
+	.init                     = northbridge_init,
    .acpi_fill_ssdt_generator = generate_cpu_entries,
-	.scan_bus         = 0,
-	.ops_pci          = &intel_pci_ops,
+	.scan_bus                 = NULL,
+	.ops_pci                  = &intel_pci_ops,
 };
static const unsigned short mc_pci_device_ids[] = {
@@ -528,12 +502,12 @@
    .set_resources    = DEVICE_NOOP,
    .enable_resources = DEVICE_NOOP,
    .init             = mp_cpu_bus_init,
-	.scan_bus         = 0,
+	.scan_bus         = NULL,
 };
static void enable_dev(struct device *dev)
 {
-	/* Set the operations if it is a special bus type */
+	/* Set the operations if it is a special bus type. */
    if (dev->path.type == DEVICE_PATH_DOMAIN) {
    	dev->ops = &pci_domain_ops;
    } else if (dev->path.type == DEVICE_PATH_CPU_CLUSTER) {
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c
index 8267833..86dfb88 100644
--- a/src/northbridge/intel/haswell/raminit.c
+++ b/src/northbridge/intel/haswell/raminit.c
@@ -28,6 +28,7 @@
 #include <spd.h>
 #include <security/vboot/vboot_common.h>
 #include <commonlib/region.h>
+
 #include "raminit.h"
 #include "pei_data.h"
 #include "haswell.h"
@@ -37,27 +38,27 @@
 void save_mrc_data(struct pei_data *pei_data)
 {
    /* Save the MRC S3 restore data to cbmem */
-	mrc_cache_stash_data(MRC_TRAINING_DATA, MRC_CACHE_VERSION,
-			pei_data->mrc_output, pei_data->mrc_output_len);
+	mrc_cache_stash_data(MRC_TRAINING_DATA, MRC_CACHE_VERSION, pei_data->mrc_output,
+			     pei_data->mrc_output_len);
 }
static void prepare_mrc_cache(struct pei_data *pei_data)
 {
    struct region_device rdev;
-	// preset just in case there is an error
+	/* Preset just in case there is an error */
    pei_data->mrc_input = NULL;
    pei_data->mrc_input_len = 0;
if (mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev))
-		/* error message printed in find_current_mrc_cache */
+		/* Error message printed in find_current_mrc_cache */
    	return;
pei_data->mrc_input = rdev_mmap_full(&rdev);
    pei_data->mrc_input_len = region_device_sz(&rdev);
-	printk(BIOS_DEBUG, "%s: at %p, size %x\n",
-	       __func__, pei_data->mrc_input, pei_data->mrc_input_len);
+	printk(BIOS_DEBUG, "%s: at %p, size %x\n", __func__, pei_data->mrc_input,
+	       pei_data->mrc_input_len);
 }
static const char *ecc_decoder[] = {
@@ -67,41 +68,41 @@
    "active"
 };
-/*
- * Dump in the log memory controller configuration as read from the memory
- * controller registers.
- */
+/* Print out the memory controller configuration, as per the values in its registers. */
 static void report_memory_config(void)
 {
-	u32 addr_decoder_common, addr_decode_ch[2];
+	u32 addr_decoder_common, addr_decode_chan[2];
    int i;
-	addr_decoder_common = MCHBAR32(0x5000);
-	addr_decode_ch[0] = MCHBAR32(0x5004);
-	addr_decode_ch[1] = MCHBAR32(0x5008);
+	addr_decoder_common = MCHBAR32(MAD_CHNL);
+	addr_decode_chan[0] = MCHBAR32(MAD_DIMM_CH0);
+	addr_decode_chan[1] = MCHBAR32(MAD_DIMM_CH1);
printk(BIOS_DEBUG, "memcfg DDR3 clock %d MHz\n",
-	       (MCHBAR32(0x5e04) * 13333 * 2 + 50)/100);
+	       (MCHBAR32(MC_BIOS_DATA) * 13333 * 2 + 50) / 100);
+
    printk(BIOS_DEBUG, "memcfg channel assignment: A: %d, B % d, C % d\n",
-	       addr_decoder_common & 3,
+	       (addr_decoder_common >> 0) & 3,
           (addr_decoder_common >> 2) & 3,
           (addr_decoder_common >> 4) & 3);
-	for (i = 0; i < ARRAY_SIZE(addr_decode_ch); i++) {
-		u32 ch_conf = addr_decode_ch[i];
-		printk(BIOS_DEBUG, "memcfg channel[%d] config (%8.8x):\n",
-		       i, ch_conf);
-		printk(BIOS_DEBUG, "   ECC %s\n",
-		       ecc_decoder[(ch_conf >> 24) & 3]);
+	for (i = 0; i < ARRAY_SIZE(addr_decode_chan); i++) {
+		u32 ch_conf = addr_decode_chan[i];
+
+		printk(BIOS_DEBUG, "memcfg channel[%d] config (%8.8x):\n", i, ch_conf);
+		printk(BIOS_DEBUG, "   ECC %s\n", ecc_decoder[(ch_conf >> 24) & 3]);
    	printk(BIOS_DEBUG, "   enhanced interleave mode %s\n",
    	       ((ch_conf >> 22) & 1) ? "on" : "off");
+
    	printk(BIOS_DEBUG, "   rank interleave %s\n",
    	       ((ch_conf >> 21) & 1) ? "on" : "off");
+
    	printk(BIOS_DEBUG, "   DIMMA %d MB width %s %s rank%s\n",
    	       ((ch_conf >> 0) & 0xff) * 256,
    	       ((ch_conf >> 19) & 1) ? "x16" : "x8 or x32",
    	       ((ch_conf >> 17) & 1) ? "dual" : "single",
    	       ((ch_conf >> 16) & 1) ? "" : ", selected");
+
    	printk(BIOS_DEBUG, "   DIMMB %d MB width %s %s rank%s\n",
    	       ((ch_conf >> 8) & 0xff) * 256,
    	       ((ch_conf >> 20) & 1) ? "x16" : "x8 or x32",
@@ -123,18 +124,14 @@
printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");
-	/*
-	 * Do not pass MRC data in for recovery mode boot,
-	 * Always pass it in for S3 resume.
-	 */
+	/* Do not pass MRC data in for recovery mode boot, always pass it in for S3 resume */
    if (!vboot_recovery_mode_enabled() || pei_data->boot_mode == 2)
    	prepare_mrc_cache(pei_data);
-	/* If MRC data is not found we cannot continue S3 resume. */
+	/* If MRC data is not found, we cannot continue S3 resume */
    if (pei_data->boot_mode == 2 && !pei_data->mrc_input) {
    	post_code(POST_RESUME_FAILURE);
-		printk(BIOS_DEBUG, "Giving up in sdram_initialize: "
-		       "No MRC data\n");
+		printk(BIOS_DEBUG, "Giving up in sdram_initialize: No MRC data\n");
    	system_reset();
    }
@@ -142,21 +139,20 @@
    pei_data->tx_byte = do_putchar;
/*
-	 * 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
+	 * 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.
     */
    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 = (unsigned long)rdev_mmap_full(&f.data);
    if (entry) {
    	int rv;
-		asm volatile (
-			      "call *%%ecx\n\t"
+		asm volatile ("call *%%ecx\n\t"
    		      :"=a" (rv) : "c" (entry), "a" (pei_data));
-		/* mrc.bin reconfigures USB, so reinit it to have debug */
+		/* The mrc.bin reconfigures USB, so usbdebug needs to be reinitialized */
    	if (CONFIG(USBDEBUG_IN_PRE_RAM))
    		usbdebug_hw_init(true);
@@ -178,13 +174,11 @@
    	die("UEFI PEI System Agent not found.\n");
    }
-	/* For reference print the System Agent version
-	 * after executing the UEFI PEI stage.
-	 */
-	u32 version = MCHBAR32(0x5034);
+	/* For reference, print the System Agent version after executing the UEFI PEI stage */
+	u32 version = MCHBAR32(MRC_REVISION);
    printk(BIOS_DEBUG, "System Agent Version %d.%d.%d Build %d\n",
-		version >> 24, (version >> 16) & 0xff,
-		(version >> 8) & 0xff, version & 0xff);
+	       (version >> 24) & 0xff, (version >> 16) & 0xff,
+	       (version >>  8) & 0xff, (version >>  0) & 0xff);
report_memory_config();
 }
@@ -194,22 +188,21 @@
    u32 addr_decode_ch[2];
    struct memory_info* mem_info;
    struct dimm_info *dimm;
-	int ddr_frequency;
-	int dimm_size;
-	int ch, d_num;
+	int ddr_frequency, dimm_size, ch, d_num;
    int dimm_cnt = 0;
mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(struct memory_info));
    if (!mem_info)
    	die("Failed to add memory info to CBMEM.\n");
+
    memset(mem_info, 0, sizeof(struct memory_info));
-	/* FIXME: Do we need to read MCHBAR32(0x5000) ? */
-	MCHBAR32(0x5000);
-	addr_decode_ch[0] = MCHBAR32(0x5004);
-	addr_decode_ch[1] = MCHBAR32(0x5008);
+	/* FIXME: Do we need to read MCHBAR32(MAD_CHNL) ? (Answer: Nope) */
+	MCHBAR32(MAD_CHNL);
+	addr_decode_ch[0] = MCHBAR32(MAD_DIMM_CH0);
+	addr_decode_ch[1] = MCHBAR32(MAD_DIMM_CH1);
-	ddr_frequency = (MCHBAR32(0x5e04) * 13333 * 2 + 50) / 100;
+	ddr_frequency = (MCHBAR32(MC_BIOS_DATA) * 13333 * 2 + 50) / 100;
for (ch = 0; ch < ARRAY_SIZE(addr_decode_ch); ch++) {
    	u32 ch_conf = addr_decode_ch[ch];
@@ -233,7 +226,7 @@
    				SPD_DIMM_PART_LEN);
    			dimm->mod_id =
    				(pei_data->spd_data[dimm_cnt][SPD_DIMM_MOD_ID2] << 8) |
-					(pei_data->spd_data[dimm_cnt][SPD_DIMM_MOD_ID1] & 0xFF);
+					(pei_data->spd_data[dimm_cnt][SPD_DIMM_MOD_ID1] & 0xff);
    			dimm->mod_type = SPD_SODIMM;
    			dimm->bus_width = 0x3;	/* 64-bit */
    			dimm_cnt++;
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38434
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I926289304acb834f9b13cd7902801798f8ee478a
Gerrit-Change-Number: 38434
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons th3fanbus@gmail.com
Gerrit-MessageType: newchange



    

Change in coreboot[master]: [WIP] nb/intel/haswell: Do some cosmetic fixes