Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
nb/intel/nehalem: Disable PEG and IGD based on devicetree
Tested on Thinkpad X201: PEG device hidden.
Change-Id: Ib378458a55e18cc02fc49b3e6d6939d31dd4aa65 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/devicetree.cb M src/mainboard/packardbell/ms2290/devicetree.cb M src/northbridge/intel/nehalem/nehalem.h M src/northbridge/intel/nehalem/northbridge.c 4 files changed, 27 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/35744/1
diff --git a/src/mainboard/lenovo/x201/devicetree.cb b/src/mainboard/lenovo/x201/devicetree.cb index bf74d71..de6d568 100644 --- a/src/mainboard/lenovo/x201/devicetree.cb +++ b/src/mainboard/lenovo/x201/devicetree.cb @@ -48,6 +48,7 @@ device pci 00.0 on # Host bridge subsystemid 0x17aa 0x2193 end + device pci 01.0 off end # PEG device pci 02.0 on # VGA controller subsystemid 0x17aa 0x215a end diff --git a/src/mainboard/packardbell/ms2290/devicetree.cb b/src/mainboard/packardbell/ms2290/devicetree.cb index bb4e854..c98f9a3 100644 --- a/src/mainboard/packardbell/ms2290/devicetree.cb +++ b/src/mainboard/packardbell/ms2290/devicetree.cb @@ -48,6 +48,7 @@ device pci 00.0 on # Host bridge subsystemid 0x1025 0x0379 end + device pci 01.0 off end # PEG device pci 02.0 on # VGA controller subsystemid 0x1025 0x0379 end diff --git a/src/northbridge/intel/nehalem/nehalem.h b/src/northbridge/intel/nehalem/nehalem.h index 22bf596..d7646e4 100644 --- a/src/northbridge/intel/nehalem/nehalem.h +++ b/src/northbridge/intel/nehalem/nehalem.h @@ -67,8 +67,6 @@ #define D0F0_MCHBAR_HI 0x4c #define D0F0_GGC 0x52 #define D0F0_DEVEN 0x54 -/* Note: Intel's datasheet is broken. Assume the following values are correct */ -#define DEVEN_PEG60 (1 << 13) #define DEVEN_IGD (1 << 3) #define DEVEN_PEG10 (1 << 1) #define DEVEN_HOST (1 << 0) diff --git a/src/northbridge/intel/nehalem/northbridge.c b/src/northbridge/intel/nehalem/northbridge.c index 7b9283f..fea6c7b 100644 --- a/src/northbridge/intel/nehalem/northbridge.c +++ b/src/northbridge/intel/nehalem/northbridge.c @@ -224,9 +224,34 @@ DMIBAR32(0x88) = reg32; }
+/* Disable unused PEG devices based on devicetree */ +static void disable_peg_igd(void) +{ + struct device *dev; + u32 reg; + + dev = pcidev_on_root(0, 0); + reg = pci_read_config32(dev, D0F0_DEVEN); + + dev = pcidev_on_root(1, 0); + if (!dev || !dev->enabled) { + printk(BIOS_DEBUG, "Disabling PEG10.\n"); + reg &= ~DEVEN_PEG10; + } + dev = pcidev_on_root(2, 0); + if (!dev || !dev->enabled) { + printk(BIOS_DEBUG, "Disabling IGD.\n"); + reg &= ~DEVEN_IGD; + } + dev = pcidev_on_root(0, 0); + pci_write_config32(dev, D0F0_DEVEN, reg); +} + static void northbridge_init(struct device *dev) { northbridge_dmi_init(dev); + + disable_peg_igd(); }
static struct pci_operations intel_pci_ops = {
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... PS4, Line 247: pci_write_config32(dev, D0F0_DEVEN, reg); I don't like read-modify-write split like this for register D0F0_DEVEN, and having to re-initialise dev. I assume for this hardware there is no write-once registers involved?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... PS4, Line 247: pci_write_config32(dev, D0F0_DEVEN, reg);
I don't like read-modify-write split like this for register D0F0_DEVEN, and having to re-initialise dev. I assume for this hardware there is no write-once registers involved?
"All the bits in this register are Intel TXT Lockable." We don't seem to do that at the moment, but I don't like disabling/hiding devices after the resource allocation has been done. I think moving this before the raminit might be appropriate? Early ramstage like gm45 does it, is also an option.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... PS4, Line 247: pci_write_config32(dev, D0F0_DEVEN, reg);
I don't like read-modify-write split like this for register D0F0_DEVEN, and having to re-initialis […]
Just use a different variable name for 0:0.0. You could even use some pci_update() variant here if you really wanted to.
I think chip_ops->init would run before PCI enumeration.
Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35744
to look at the new patch set (#6).
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
nb/intel/nehalem: Disable PEG and IGD based on devicetree
Tested on Thinkpad X201: PEG device hidden.
Change-Id: Ib378458a55e18cc02fc49b3e6d6939d31dd4aa65 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/devicetree.cb M src/mainboard/packardbell/ms2290/devicetree.cb M src/northbridge/intel/nehalem/nehalem.h M src/northbridge/intel/nehalem/northbridge.c 4 files changed, 26 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/35744/6
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/4/src/northbridge/intel/nehal... PS4, Line 247: pci_write_config32(dev, D0F0_DEVEN, reg);
Just use a different variable name for 0:0.0. You could even use some pci_update() variant here if you really wanted to.
I think chip_ops->init would run before PCI enumeration.
Yes chip_ops->init seems like the right place to do even though this will result in some leftover devices from the devicetree when scanning PCI.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/7/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/7/src/northbridge/intel/nehal... PS7, Line 240: PEG10 Is there any other PEG?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35744/7/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35744/7/src/northbridge/intel/nehal... PS7, Line 240: PEG10
Is there any other PEG?
I couldn't find one, but there potentially is on desktop variants?
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35744
to look at the new patch set (#8).
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
nb/intel/nehalem: Disable PEG and IGD based on devicetree
Tested on Thinkpad X201: PEG device hidden.
Change-Id: Ib378458a55e18cc02fc49b3e6d6939d31dd4aa65 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/devicetree.cb M src/mainboard/packardbell/ms2290/devicetree.cb M src/northbridge/intel/nehalem/nehalem.h M src/northbridge/intel/nehalem/northbridge.c 4 files changed, 26 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/35744/8
Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35744 )
Change subject: nb/intel/nehalem: Disable PEG and IGD based on devicetree ......................................................................
nb/intel/nehalem: Disable PEG and IGD based on devicetree
Tested on Thinkpad X201: PEG device hidden.
Change-Id: Ib378458a55e18cc02fc49b3e6d6939d31dd4aa65 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/35744 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/lenovo/x201/devicetree.cb M src/mainboard/packardbell/ms2290/devicetree.cb M src/northbridge/intel/nehalem/nehalem.h M src/northbridge/intel/nehalem/northbridge.c 4 files changed, 26 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/lenovo/x201/devicetree.cb b/src/mainboard/lenovo/x201/devicetree.cb index bf74d71..de6d568 100644 --- a/src/mainboard/lenovo/x201/devicetree.cb +++ b/src/mainboard/lenovo/x201/devicetree.cb @@ -48,6 +48,7 @@ device pci 00.0 on # Host bridge subsystemid 0x17aa 0x2193 end + device pci 01.0 off end # PEG device pci 02.0 on # VGA controller subsystemid 0x17aa 0x215a end diff --git a/src/mainboard/packardbell/ms2290/devicetree.cb b/src/mainboard/packardbell/ms2290/devicetree.cb index bb4e854..c98f9a3 100644 --- a/src/mainboard/packardbell/ms2290/devicetree.cb +++ b/src/mainboard/packardbell/ms2290/devicetree.cb @@ -48,6 +48,7 @@ device pci 00.0 on # Host bridge subsystemid 0x1025 0x0379 end + device pci 01.0 off end # PEG device pci 02.0 on # VGA controller subsystemid 0x1025 0x0379 end diff --git a/src/northbridge/intel/nehalem/nehalem.h b/src/northbridge/intel/nehalem/nehalem.h index f3b9dbb..53743ec 100644 --- a/src/northbridge/intel/nehalem/nehalem.h +++ b/src/northbridge/intel/nehalem/nehalem.h @@ -67,8 +67,6 @@ #define D0F0_MCHBAR_HI 0x4c #define D0F0_GGC 0x52 #define D0F0_DEVEN 0x54 -/* Note: Intel's datasheet is broken. Assume the following values are correct */ -#define DEVEN_PEG60 (1 << 13) #define DEVEN_IGD (1 << 3) #define DEVEN_PEG10 (1 << 1) #define DEVEN_HOST (1 << 0) diff --git a/src/northbridge/intel/nehalem/northbridge.c b/src/northbridge/intel/nehalem/northbridge.c index 7b9283f..6ec76e1 100644 --- a/src/northbridge/intel/nehalem/northbridge.c +++ b/src/northbridge/intel/nehalem/northbridge.c @@ -229,6 +229,28 @@ northbridge_dmi_init(dev); }
+/* Disable unused PEG devices based on devicetree before PCI enumeration */ +static void nehalem_init(void *const chip_info) +{ + u32 deven_mask = UINT32_MAX; + const struct device *dev; + + dev = pcidev_on_root(1, 0); + if (!dev || !dev->enabled) { + printk(BIOS_DEBUG, "Disabling PEG10.\n"); + deven_mask &= ~DEVEN_PEG10; + } + dev = pcidev_on_root(2, 0); + if (!dev || !dev->enabled) { + printk(BIOS_DEBUG, "Disabling IGD.\n"); + deven_mask &= ~DEVEN_IGD; + } + const struct device *const d0f0 = pcidev_on_root(0, 0); + if (d0f0) + pci_update_config32(d0f0, D0F0_DEVEN, deven_mask, 0); + +} + static struct pci_operations intel_pci_ops = { .set_subsystem = pci_dev_set_subsystem, }; @@ -269,5 +291,6 @@
struct chip_operations northbridge_intel_nehalem_ops = { CHIP_NAME("Intel i7 (Nehalem) integrated Northbridge") - .enable_dev = enable_dev, + .enable_dev = enable_dev, + .init = nehalem_init, };
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)