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/+/48408
to review the following change.
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
nb/intel/sandybridge: Clean up stepping logic
Do not combine the host bridge device ID with the CPU stepping.
Change-Id: I27ad609eb53b96987ad5445301b5392055fa4ea1 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/cpu/intel/model_206ax/model_206ax.h M src/northbridge/intel/sandybridge/gma.c M src/northbridge/intel/sandybridge/northbridge.c M src/northbridge/intel/sandybridge/sandybridge.h 4 files changed, 47 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/48408/1
diff --git a/src/cpu/intel/model_206ax/model_206ax.h b/src/cpu/intel/model_206ax/model_206ax.h index eb340ad..b6e2d65 100644 --- a/src/cpu/intel/model_206ax/model_206ax.h +++ b/src/cpu/intel/model_206ax/model_206ax.h @@ -3,6 +3,7 @@ #ifndef _CPU_INTEL_MODEL_206AX_H #define _CPU_INTEL_MODEL_206AX_H
+#include <arch/cpu.h> #include <stdint.h>
/* SandyBridge/IvyBridge bus clock is fixed at 100MHz */ @@ -88,4 +89,9 @@ int cpu_config_tdp_levels(void); int get_platform_id(void);
+static inline u8 cpu_stepping(void) +{ + return cpuid_eax(1) & 0xf; +} + #endif diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index f75dfda..86e12b0 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -4,6 +4,7 @@ #include <device/mmio.h> #include <console/console.h> #include <bootmode.h> +#include <cpu/intel/model_206ax/model_206ax.h> #include <delay.h> #include <device/device.h> #include <device/pci.h> @@ -301,6 +302,8 @@
static void gma_pm_init_pre_vbios(struct device *dev) { + const bool is_sandy = is_sandybridge(); + u32 reg32;
printk(BIOS_DEBUG, "GT Power Management Init\n"); @@ -309,7 +312,7 @@ if (!gtt_res || !gtt_res->base) return;
- if (bridge_silicon_revision() < IVB_STEP_C0) { + if (is_sandy || cpu_stepping() < IVB_STEP_C0) { /* 1: Enable force wake */ gtt_write(0xa18c, 0x00000001); gtt_poll(0x130090, (1 << 0), (1 << 0)); @@ -319,14 +322,14 @@ gtt_poll(0x130040, (1 << 0), (1 << 0)); }
- if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) { + if (is_sandy) { /* 1d: Set GTT+0x42004 [15:14]=11 (SnB C1+) */ reg32 = gtt_read(0x42004); reg32 |= (1 << 14) | (1 << 15); gtt_write(0x42004, reg32); }
- if (bridge_silicon_revision() >= IVB_STEP_A0) { + if (!is_sandy) { /* Display Reset Acknowledge Settings */ reg32 = gtt_read(0x45010); reg32 |= (1 << 1) | (1 << 0); @@ -335,7 +338,7 @@
/* 2: Get GT SKU from GTT+0x911c[13] */ reg32 = gtt_read(0x911c); - if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) { + if (is_sandy) { if (reg32 & (1 << 13)) { printk(BIOS_DEBUG, "SNB GT1 Power Meter Weights\n"); gtt_write_powermeter(snb_pm_gt1); @@ -384,13 +387,12 @@ reg32 = gtt_read(0xa180); reg32 |= (1 << 26) | (1 << 31); /* (bit 20=1 for SNB step D1+ / IVB A0+) */ - if (bridge_silicon_revision() >= SNB_STEP_D1) + if (!is_sandy || cpu_stepping() >= SNB_STEP_D1) reg32 |= (1 << 20); gtt_write(0xa180, reg32);
/* 6a: for SnB step D2+ only */ - if (((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) && - (bridge_silicon_revision() >= SNB_STEP_D2)) { + if (is_sandy && cpu_stepping() >= SNB_STEP_D2) { reg32 = gtt_read(0x9400); reg32 |= (1 << 7); gtt_write(0x9400, reg32); @@ -402,16 +404,16 @@ gtt_poll(0x941c, (1 << 1), (0 << 1)); }
- if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) { + if (is_sandy) { + /* 6b: Clocking reset controls */ + gtt_write(0x9424, 0x00000000); + } else { reg32 = gtt_read(0x907c); reg32 |= (1 << 16); gtt_write(0x907c, reg32);
/* 6b: Clocking reset controls */ gtt_write(0x9424, 0x00000001); - } else { - /* 6b: Clocking reset controls */ - gtt_write(0x9424, 0x00000000); }
/* 7 */ @@ -503,7 +505,7 @@ printk(BIOS_DEBUG, "GT Power Management Init (post VBIOS)\n");
/* 15: Deassert Force Wake */ - if (bridge_silicon_revision() < IVB_STEP_C0) { + if (is_sandybridge() || cpu_stepping() < IVB_STEP_C0) { gtt_write(0xa18c, gtt_read(0xa18c) & ~1); gtt_poll(0x130090, (1 << 0), (0 << 0)); } else { diff --git a/src/northbridge/intel/sandybridge/northbridge.c b/src/northbridge/intel/sandybridge/northbridge.c index 541bf73..942d135 100644 --- a/src/northbridge/intel/sandybridge/northbridge.c +++ b/src/northbridge/intel/sandybridge/northbridge.c @@ -4,31 +4,26 @@ #include <acpi/acpi.h> #include <commonlib/helpers.h> #include <device/pci_ops.h> -#include <stdint.h> #include <delay.h> #include <cpu/intel/model_206ax/model_206ax.h> #include <cpu/x86/msr.h> #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> +#include <types.h> #include "chip.h" #include "sandybridge.h" #include <cpu/intel/smm_reloc.h>
-static int bridge_revision_id = -1; - /* IGD UMA memory */ static uint64_t uma_memory_base = 0; static uint64_t uma_memory_size = 0;
-int bridge_silicon_revision(void) +bool is_sandybridge(void) { - if (bridge_revision_id < 0) { - uint8_t stepping = cpuid_eax(1) & 0x0f; - uint8_t bridge_id = pci_read_config16(pcidev_on_root(0, 0), PCI_DEVICE_ID); - bridge_revision_id = (bridge_id & 0xf0) | stepping; - } - return bridge_revision_id; + const uint16_t bridge_id = pci_read_config16(pcidev_on_root(0, 0), PCI_DEVICE_ID); + + return (bridge_id & BASE_REV_MASK) == BASE_REV_SNB; }
/* Reserve everything between A segment and 1MB: @@ -115,7 +110,7 @@ CONFIG_CHROMEOS_RAMOOPS_RAM_SIZE >> 10); #endif
- if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) { + if (is_sandybridge()) { /* Required for SandyBridge sighting 3715511 */ bad_ram_resource(dev, index++, 0x20000000 >> 10, 0x00200000 >> 10); bad_ram_resource(dev, index++, 0x40000000 >> 10, 0x00200000 >> 10); @@ -259,6 +254,8 @@
static void northbridge_dmi_init(struct device *dev) { + const bool is_sandy = is_sandybridge(); + u32 reg32;
/* Clear error status bits */ @@ -266,7 +263,7 @@ DMIBAR32(DMICESTS) = 0xffffffff;
/* Steps prior to DMI ASPM */ - if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) { + if (is_sandy) { reg32 = DMIBAR32(0x250); reg32 &= ~((1 << 22) | (1 << 20)); reg32 |= (1 << 21); @@ -277,12 +274,12 @@ reg32 |= (1 << 29); DMIBAR32(DMILLTC) = reg32;
- if (bridge_silicon_revision() >= SNB_STEP_D0) { + if (is_sandy && cpu_stepping() >= SNB_STEP_D0) { reg32 = DMIBAR32(0x1f8); reg32 |= (1 << 16); DMIBAR32(0x1f8) = reg32;
- } else if (bridge_silicon_revision() >= SNB_STEP_D1) { + } else if (is_sandy && cpu_stepping() >= SNB_STEP_D1) { reg32 = DMIBAR32(0x1f8); reg32 &= ~(1 << 26); reg32 |= (1 << 16); @@ -294,7 +291,7 @@ }
/* Enable ASPM on SNB link, should happen before PCH link */ - if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) { + if (is_sandy) { reg32 = DMIBAR32(0xd04); reg32 |= (1 << 4); DMIBAR32(0xd04) = reg32; @@ -376,7 +373,10 @@ bridge_type = MCHBAR32(SAPMTIMERS); bridge_type &= ~0xff;
- if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) { + if (is_sandybridge()) { + /* 20h for Sandybridge */ + bridge_type |= 0x20; + } else { /* Enable Power Aware Interrupt Routing */ u8 pair = MCHBAR8(INTRDIRCTL); pair &= ~0x0f; /* Clear 3:0 */ @@ -385,9 +385,6 @@
/* 30h for IvyBridge */ bridge_type |= 0x30; - } else { - /* 20h for Sandybridge */ - bridge_type |= 0x20; } MCHBAR32(SAPMTIMERS) = bridge_type;
diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h index 8f9d118..e5f2132 100644 --- a/src/northbridge/intel/sandybridge/sandybridge.h +++ b/src/northbridge/intel/sandybridge/sandybridge.h @@ -9,22 +9,22 @@ #define BASE_REV_MASK 0x50
/* SandyBridge CPU stepping */ -#define SNB_STEP_D0 (BASE_REV_SNB + 5) /* Also J0 */ -#define SNB_STEP_D1 (BASE_REV_SNB + 6) -#define SNB_STEP_D2 (BASE_REV_SNB + 7) /* Also J1/Q0 */ +#define SNB_STEP_D0 5 /* Also J0 */ +#define SNB_STEP_D1 6 +#define SNB_STEP_D2 7 /* Also J1/Q0 */
/* IvyBridge CPU stepping */ -#define IVB_STEP_A0 (BASE_REV_IVB + 0) -#define IVB_STEP_B0 (BASE_REV_IVB + 2) -#define IVB_STEP_C0 (BASE_REV_IVB + 4) -#define IVB_STEP_K0 (BASE_REV_IVB + 5) -#define IVB_STEP_D0 (BASE_REV_IVB + 6) +#define IVB_STEP_A0 0 +#define IVB_STEP_B0 2 +#define IVB_STEP_C0 4 +#define IVB_STEP_K0 5 +#define IVB_STEP_D0 6
#include "memmap.h"
/* Everything below this line is ignored in the DSDT */ #ifndef __ACPI__ -#include <stdint.h> +#include <types.h>
/* Chipset types */ enum platform_type { @@ -87,8 +87,9 @@
#ifndef __ASSEMBLER__
+bool is_sandybridge(void); + void intel_sandybridge_finalize_smm(void); -int bridge_silicon_revision(void); void systemagent_early_init(void); void sandybridge_init_iommu(void); void sandybridge_late_initialization(void);