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);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48408/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48408/1//COMMIT_MSG@9 PS1, Line 9: Do not combine the host bridge device ID with the CPU stepping. Why not? I mean, the change looks ok, but you say it like it was an error. Though, it looks like somebody did it on purpose.
If you think it was not a good idea because the code turned out to be confusing, just write that?
Hello Felix Singer, build bot (Jenkins), Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48408
to look at the new patch set (#3).
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 because it is confusing. Although Sandy/Ivy Bridge processors incorporate both CPU and northbridge components into the same die, it is best to treat them separately. Plus, this change enables moving CPU stepping macros from northbridge code into the CPU scope, which is done in a follow-up.
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/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48408/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48408/1//COMMIT_MSG@9 PS1, Line 9: Do not combine the host bridge device ID with the CPU stepping.
Why not? I mean, the change looks ok, but you say it like it was […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 3:
(3 comments)
Thanks for updating the commit message!
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... File src/cpu/intel/model_206ax/model_206ax.h:
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... PS3, Line 95: } Actually, this code is not model specific.
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/northbridge.c:
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... PS3, Line 277: is_sandy && I read `!is_sandy ||`?
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... PS3, Line 282: is_sandy && Same here?
Hello Felix Singer, build bot (Jenkins), Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48408
to look at the new patch set (#4).
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 because it is confusing. Although Sandy/Ivy Bridge processors incorporate both CPU and northbridge components into the same die, it is best to treat them separately. Plus, this change enables moving CPU stepping macros from northbridge code into the CPU scope, which is done in a follow-up.
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/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/northbridge.c:
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... PS3, Line 277: is_sandy &&
I read `!is_sandy ||`?
Derp. Thanks.
https://review.coreboot.org/c/coreboot/+/48408/3/src/northbridge/intel/sandy... PS3, Line 282: is_sandy &&
Same here?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... File src/cpu/intel/model_206ax/model_206ax.h:
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... PS3, Line 95: }
Actually, this code is not model specific.
Yeah, where can I place this instead?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... File src/cpu/intel/model_206ax/model_206ax.h:
https://review.coreboot.org/c/coreboot/+/48408/3/src/cpu/intel/model_206ax/m... PS3, Line 95: }
Yeah, where can I place this instead?
There are already some functions around, e.g. one that decodes the CPUID signature into family, modding, stepping, IIRC. It's not handy if one just needs the stepping, though. In `arch/x86/include/arch/cpu.h`.
Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
Change subject: nb/intel/sandybridge: Clean up stepping logic ......................................................................
Removed Verified+1 by build bot (Jenkins) no-reply@coreboot.org
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48408 )
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 because it is confusing. Although Sandy/Ivy Bridge processors incorporate both CPU and northbridge components into the same die, it is best to treat them separately. Plus, this change enables moving CPU stepping macros from northbridge code into the CPU scope, which is done in a follow-up.
Change-Id: I27ad609eb53b96987ad5445301b5392055fa4ea1 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48408 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- 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(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
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..4d87878 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);
-
Angel Pons (Code Review) -
Nico Huber (Code Review)