Hello Matt DeVillier, Angel Pons, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40726
to review the following change.
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
intel/gma: Don't bluntly enable I/O
The allocator should take care of this.
Change-Id: I4ec88ebe23b4dcab069f764decc8b9b0c6e6a142 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/northbridge/intel/gm45/gma.c M src/northbridge/intel/haswell/gma.c M src/northbridge/intel/i945/gma.c M src/northbridge/intel/ironlake/gma.c M src/northbridge/intel/pineview/gma.c M src/northbridge/intel/sandybridge/gma.c M src/northbridge/intel/x4x/gma.c M src/soc/intel/broadwell/igd.c M src/soc/intel/common/block/graphics/graphics.c 9 files changed, 11 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/40726/1
diff --git a/src/northbridge/intel/gm45/gma.c b/src/northbridge/intel/gm45/gma.c index 9039fad..45da91d 100644 --- a/src/northbridge/intel/gm45/gma.c +++ b/src/northbridge/intel/gm45/gma.c @@ -153,7 +153,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/northbridge/intel/haswell/gma.c b/src/northbridge/intel/haswell/gma.c index d857745..4dcdfe3 100644 --- a/src/northbridge/intel/haswell/gma.c +++ b/src/northbridge/intel/haswell/gma.c @@ -463,7 +463,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* Init graphics power management */ diff --git a/src/northbridge/intel/i945/gma.c b/src/northbridge/intel/i945/gma.c index d9cac3b..198e044 100644 --- a/src/northbridge/intel/i945/gma.c +++ b/src/northbridge/intel/i945/gma.c @@ -673,8 +673,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER - | PCI_COMMAND_IO | PCI_COMMAND_MEMORY); + pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER);
if (CONFIG(MAINBOARD_DO_NATIVE_VGA_INIT)) { int vga_disable = (pci_read_config16(dev, GGC) & 2) >> 1; @@ -718,10 +717,9 @@ u32 reg32; u8 val;
- /* IGD needs to be Bus Master, also enable IO access */ + /* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | - PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); + pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER);
if (get_option(&val, "tft_brightness") == CB_SUCCESS) pci_write_config8(dev, 0xf4, val); diff --git a/src/northbridge/intel/ironlake/gma.c b/src/northbridge/intel/ironlake/gma.c index defe5da..325857f 100644 --- a/src/northbridge/intel/ironlake/gma.c +++ b/src/northbridge/intel/ironlake/gma.c @@ -140,7 +140,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/northbridge/intel/pineview/gma.c b/src/northbridge/intel/pineview/gma.c index bdbf3a7..7189f95 100644 --- a/src/northbridge/intel/pineview/gma.c +++ b/src/northbridge/intel/pineview/gma.c @@ -223,7 +223,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
if (!CONFIG(MAINBOARD_DO_NATIVE_VGA_INIT)) { diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index 00f436a..53578f2 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -590,7 +590,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* Init graphics power management */ diff --git a/src/northbridge/intel/x4x/gma.c b/src/northbridge/intel/x4x/gma.c index e5da8b2..1b3d316 100644 --- a/src/northbridge/intel/x4x/gma.c +++ b/src/northbridge/intel/x4x/gma.c @@ -27,7 +27,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* configure GMBUSFREQ */ diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index dd7b1bd..f136d74 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -497,7 +497,7 @@
/* IGD needs to be Bus Master */ u32 reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c index 533ae31..6e7d27e 100644 --- a/src/soc/intel/common/block/graphics/graphics.c +++ b/src/soc/intel/common/block/graphics/graphics.c @@ -47,7 +47,7 @@
/* IGD needs to Bus Master */ u32 reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/*
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 2: Code-Review+1
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 2: Code-Review+1
tested ok on google/chell (SKL)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 4: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Damien Zammit, Matt DeVillier, Paul Menzel, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40726
to look at the new patch set (#5).
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
intel/gma: Don't bluntly enable I/O
The allocator should take care of this.
Change-Id: I4ec88ebe23b4dcab069f764decc8b9b0c6e6a142 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/northbridge/intel/gm45/gma.c M src/northbridge/intel/haswell/gma.c M src/northbridge/intel/i945/gma.c M src/northbridge/intel/ironlake/gma.c M src/northbridge/intel/pineview/gma.c M src/northbridge/intel/sandybridge/gma.c M src/northbridge/intel/x4x/gma.c M src/soc/intel/broadwell/igd.c M src/soc/intel/common/block/graphics/graphics.c 9 files changed, 11 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/40726/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
intel/gma: Don't bluntly enable I/O
The allocator should take care of this.
Change-Id: I4ec88ebe23b4dcab069f764decc8b9b0c6e6a142 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/40726 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/gm45/gma.c M src/northbridge/intel/haswell/gma.c M src/northbridge/intel/i945/gma.c M src/northbridge/intel/ironlake/gma.c M src/northbridge/intel/pineview/gma.c M src/northbridge/intel/sandybridge/gma.c M src/northbridge/intel/x4x/gma.c M src/soc/intel/broadwell/igd.c M src/soc/intel/common/block/graphics/graphics.c 9 files changed, 11 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/gm45/gma.c b/src/northbridge/intel/gm45/gma.c index e9bd722..b48b3c3 100644 --- a/src/northbridge/intel/gm45/gma.c +++ b/src/northbridge/intel/gm45/gma.c @@ -154,7 +154,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/northbridge/intel/haswell/gma.c b/src/northbridge/intel/haswell/gma.c index ae9e257..fa67fe1 100644 --- a/src/northbridge/intel/haswell/gma.c +++ b/src/northbridge/intel/haswell/gma.c @@ -464,7 +464,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* Init graphics power management */ diff --git a/src/northbridge/intel/i945/gma.c b/src/northbridge/intel/i945/gma.c index 181aee5..71ed4c4 100644 --- a/src/northbridge/intel/i945/gma.c +++ b/src/northbridge/intel/i945/gma.c @@ -674,8 +674,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER - | PCI_COMMAND_IO | PCI_COMMAND_MEMORY); + pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER);
if (CONFIG(MAINBOARD_DO_NATIVE_VGA_INIT)) { int vga_disable = (pci_read_config16(dev, GGC) & 2) >> 1; @@ -717,10 +716,9 @@ u32 reg32; u8 val;
- /* IGD needs to be Bus Master, also enable IO access */ + /* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | - PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); + pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER);
if (get_option(&val, "tft_brightness") == CB_SUCCESS) pci_write_config8(dev, 0xf4, val); diff --git a/src/northbridge/intel/ironlake/gma.c b/src/northbridge/intel/ironlake/gma.c index 6de64fb..6ba95d3 100644 --- a/src/northbridge/intel/ironlake/gma.c +++ b/src/northbridge/intel/ironlake/gma.c @@ -141,7 +141,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/northbridge/intel/pineview/gma.c b/src/northbridge/intel/pineview/gma.c index e2d0d18..7e5b236 100644 --- a/src/northbridge/intel/pineview/gma.c +++ b/src/northbridge/intel/pineview/gma.c @@ -224,7 +224,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
if (!CONFIG(MAINBOARD_DO_NATIVE_VGA_INIT)) { diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index 82e43fc..6e89b4e 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -591,7 +591,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* Init graphics power management */ diff --git a/src/northbridge/intel/x4x/gma.c b/src/northbridge/intel/x4x/gma.c index 004960f..69b6f71 100644 --- a/src/northbridge/intel/x4x/gma.c +++ b/src/northbridge/intel/x4x/gma.c @@ -28,7 +28,7 @@
/* IGD needs to be Bus Master */ reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
/* configure GMBUSFREQ */ diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index fbd45cb..0f83937 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -498,7 +498,7 @@
/* IGD needs to be Bus Master */ u32 reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c index 5110aad..6b035bc 100644 --- a/src/soc/intel/common/block/graphics/graphics.c +++ b/src/soc/intel/common/block/graphics/graphics.c @@ -47,7 +47,7 @@
/* IGD needs to Bus Master */ u32 reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO; + reg32 |= PCI_COMMAND_MASTER; pci_write_config32(dev, PCI_COMMAND, reg32);
if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40726 )
Change subject: intel/gma: Don't bluntly enable I/O ......................................................................
Patch Set 7:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4254 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4253 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4252 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4251
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)