Change in coreboot[master]: soc/intel/gma: Move display and opregion init to common code

[Nico Huber (Code Review)]

Hello Matt DeVillier, Angel Pons, Patrick Rudolph,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to review the following change.

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 39 insertions(+), 194 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/1

diff --git a/src/soc/intel/apollolake/graphics.c b/src/soc/intel/apollolake/graphics.c
index 99c3c88..8deb548 100644
--- a/src/soc/intel/apollolake/graphics.c
+++ b/src/soc/intel/apollolake/graphics.c
@@ -13,42 +13,11 @@
  * GNU General Public License for more details.
  */

-#include <stdint.h>
-#include <arch/acpi.h>
-#include <bootmode.h>
-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
 #include <intelblocks/graphics.h>
-#include <drivers/intel/gma/opregion.h>
-#include <drivers/intel/gma/libgfxinit.h>
 #include <types.h>

uintptr_t fsp_soc_get_igd_bar(void)
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *const dev)
-{
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
-}
diff --git a/src/soc/intel/cannonlake/graphics.c b/src/soc/intel/cannonlake/graphics.c
index 9046119..1ca5b76 100644
--- a/src/soc/intel/cannonlake/graphics.c
+++ b/src/soc/intel/cannonlake/graphics.c
@@ -14,15 +14,9 @@
  */

#include <arch/acpi.h>
-#include <bootmode.h>
-#include <console/console.h>
 #include <fsp/util.h>
 #include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
 #include <drivers/intel/gma/i915_reg.h>
-#include <drivers/intel/gma/libgfxinit.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -46,32 +40,4 @@
    			DDI_BUF_IS_IDLE);
    	graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
    }
-
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig option and input
-	 * VBT file.
-	 *
-	 * In case of non-FSP solution, SoC need to select another
-	 * Kconfig to perform GFX initialization.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
 }
diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c
index 0b2d635..533ae31 100644
--- a/src/soc/intel/common/block/graphics/graphics.c
+++ b/src/soc/intel/common/block/graphics/graphics.c
@@ -14,11 +14,14 @@
  */

#include <assert.h>
+#include <bootmode.h>
 #include <console/console.h>
 #include <device/mmio.h>
 #include <device/pci.h>
 #include <device/pci_ids.h>
 #include <drivers/intel/gma/i915.h>
+#include <drivers/intel/gma/libgfxinit.h>
+#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <soc/pci_devs.h>

@@ -28,9 +31,7 @@
    /*
     * User needs to implement SoC override in case wishes
     * to perform certain specific graphics initialization
-	 * along with pci_dev_init(dev)
     */
-	pci_dev_init(dev);
 }

__weak const struct i915_gpu_controller_info *
@@ -39,6 +40,40 @@
    return NULL;
 }

+static void gma_init(struct device *const dev)
+{
+	/* SoC specific configuration. */
+	graphics_soc_init(dev);
+
+	/* IGD needs to Bus Master */
+	u32 reg32 = pci_read_config32(dev, PCI_COMMAND);
+	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
+	pci_write_config32(dev, PCI_COMMAND, reg32);
+
+	/*
+	 * GFX PEIM module inside FSP binary is taking care of graphics
+	 * initialization based on RUN_FSP_GOP Kconfig option and input
+	 * VBT file.
+	 *
+	 * In case of non-FSP solution, SoC need to select another
+	 * Kconfig to perform GFX initialization.
+	 */
+	if (CONFIG(RUN_FSP_GOP)) {
+		/* nothing to do */
+	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
+		if (!acpi_is_wakeup_s3() && display_init_required()) {
+			int lightup_ok;
+			gma_gfxinit(&lightup_ok);
+			gfx_set_init_done(lightup_ok);
+		}
+	} else {
+		/* Initialize PCI device, load/execute BIOS Option ROM */
+		pci_dev_init(dev);
+	}
+
+	intel_gma_init_igd_opregion();
+}
+
 static void gma_generate_ssdt(struct device *device)
 {
    const struct i915_gpu_controller_info *gfx = intel_igd_get_controller_info(device);
@@ -129,7 +164,7 @@
    .read_resources		= pci_dev_read_resources,
    .set_resources		= pci_dev_set_resources,
    .enable_resources	= pci_dev_enable_resources,
-	.init			= graphics_soc_init,
+	.init			= gma_init,
    .ops_pci		= &pci_dev_ops_pci,
 #if CONFIG(HAVE_ACPI_TABLES)
    .acpi_fill_ssdt		= gma_generate_ssdt,
diff --git a/src/soc/intel/icelake/graphics.c b/src/soc/intel/icelake/graphics.c
index 79a1125..8deb548 100644
--- a/src/soc/intel/icelake/graphics.c
+++ b/src/soc/intel/icelake/graphics.c
@@ -13,12 +13,7 @@
  * GNU General Public License for more details.
  */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -26,29 +21,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
-}
diff --git a/src/soc/intel/jasperlake/graphics.c b/src/soc/intel/jasperlake/graphics.c
index 79a1125..8deb548 100644
--- a/src/soc/intel/jasperlake/graphics.c
+++ b/src/soc/intel/jasperlake/graphics.c
@@ -13,12 +13,7 @@
  * GNU General Public License for more details.
  */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -26,29 +21,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
-}
diff --git a/src/soc/intel/skylake/graphics.c b/src/soc/intel/skylake/graphics.c
index 5646522..97cdc44 100644
--- a/src/soc/intel/skylake/graphics.c
+++ b/src/soc/intel/skylake/graphics.c
@@ -1,17 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* This file is part of the coreboot project. */

-#include <bootmode.h>
 #include <commonlib/helpers.h>
 #include <console/console.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
+#include <device/mmio.h>
 #include <device/resource.h>
 #include <drivers/intel/gma/i915.h>
 #include <drivers/intel/gma/i915_reg.h>
-#include <drivers/intel/gma/libgfxinit.h>
 #include <intelblocks/graphics.h>
-#include <drivers/intel/gma/opregion.h>
 #include <soc/ramstage.h>
 #include <types.h>

@@ -96,34 +92,6 @@
    	ddi_buf_ctl |= DDI_A_4_LANES;
    	graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
    }
-
-	/* IGD needs to Bus Master */
-	u32 reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig option and input
-	 * VBT file.
-	 *
-	 * In case of non-FSP solution, SoC need to select another
-	 * Kconfig to perform GFX initialization.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
 }

const struct i915_gpu_controller_info *
diff --git a/src/soc/intel/tigerlake/graphics.c b/src/soc/intel/tigerlake/graphics.c
index 979b4a0..90d3a02 100644
--- a/src/soc/intel/tigerlake/graphics.c
+++ b/src/soc/intel/tigerlake/graphics.c
@@ -19,12 +19,7 @@
  * Chapter number: 4
  */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -32,29 +27,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-
-	intel_gma_init_igd_opregion();
-}

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to look at the new patch set (#2).

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 40 insertions(+), 194 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/2

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 2: Code-Review+1

tested ok on google/chell (SKL)

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to look at the new patch set (#3).

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 40 insertions(+), 193 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/3

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to look at the new patch set (#4).

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 40 insertions(+), 193 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/4

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 4: Code-Review+2

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 4: Code-Review+2

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Arthur Heymans, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to look at the new patch set (#5).

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M 3rdparty/amd_blobs
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
8 files changed, 42 insertions(+), 192 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/5

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40725/5/3rdparty/amd_blobs 
File 3rdparty/amd_blobs:

PS5: 
Oops!

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Arthur Heymans, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40725

to look at the new patch set (#6).

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 41 insertions(+), 191 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40725/6

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 6: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

Patch Set 6: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

soc/intel/gma: Move display and opregion init to common code

Change-Id: I359b529df44db7d63c5a7922cb1ebd8e130d0c43
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40725
Reviewed-by: Arthur Heymans 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/graphics.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/graphics/graphics.c
M src/soc/intel/icelake/graphics.c
M src/soc/intel/jasperlake/graphics.c
M src/soc/intel/skylake/graphics.c
M src/soc/intel/tigerlake/graphics.c
7 files changed, 41 insertions(+), 191 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Arthur Heymans: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/graphics.c b/src/soc/intel/apollolake/graphics.c
index 2070baf..c8d282d 100644
--- a/src/soc/intel/apollolake/graphics.c
+++ b/src/soc/intel/apollolake/graphics.c
@@ -1,42 +1,10 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */

-#include <stdint.h>
-#include <acpi/acpi.h>
-#include <bootmode.h>
-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
 #include <intelblocks/graphics.h>
-#include <drivers/intel/gma/opregion.h>
-#include <drivers/intel/gma/libgfxinit.h>
 #include <types.h>

uintptr_t fsp_soc_get_igd_bar(void)
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *const dev)
-{
-	intel_gma_init_igd_opregion();
-
-	if (CONFIG(RUN_FSP_GOP))
-		return;
-
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
-}
diff --git a/src/soc/intel/cannonlake/graphics.c b/src/soc/intel/cannonlake/graphics.c
index 1db4625..cd5e773 100644
--- a/src/soc/intel/cannonlake/graphics.c
+++ b/src/soc/intel/cannonlake/graphics.c
@@ -1,15 +1,9 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */

#include <acpi/acpi.h>
-#include <bootmode.h>
-#include <console/console.h>
 #include <fsp/util.h>
 #include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
 #include <drivers/intel/gma/i915_reg.h>
-#include <drivers/intel/gma/libgfxinit.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -22,8 +16,6 @@
 {
    uint32_t ddi_buf_ctl;

-	intel_gma_init_igd_opregion();
-
    /*
     * Enable DDI-A (eDP) 4-lane operation if the link is not up yet.
     * This will allow the kernel to use 4-lane eDP links properly
@@ -35,29 +27,4 @@
    			DDI_BUF_IS_IDLE);
    	graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
    }
-
-	/* IGD needs to Bus Master */
-	pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY |
-			PCI_COMMAND_IO);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig option and input
-	 * VBT file.
-	 *
-	 * In case of non-FSP solution, SoC need to select another
-	 * Kconfig to perform GFX initialization.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
 }
diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c
index ae45d67..5110aad 100644
--- a/src/soc/intel/common/block/graphics/graphics.c
+++ b/src/soc/intel/common/block/graphics/graphics.c
@@ -1,11 +1,14 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */

#include <assert.h>
+#include <bootmode.h>
 #include <console/console.h>
 #include <device/mmio.h>
 #include <device/pci.h>
 #include <device/pci_ids.h>
 #include <drivers/intel/gma/i915.h>
+#include <drivers/intel/gma/libgfxinit.h>
+#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <soc/pci_devs.h>

@@ -15,9 +18,7 @@
    /*
     * User needs to implement SoC override in case wishes
     * to perform certain specific graphics initialization
-	 * along with pci_dev_init(dev)
     */
-	pci_dev_init(dev);
 }

__weak const struct i915_gpu_controller_info *
@@ -26,6 +27,41 @@
    return NULL;
 }

+static void gma_init(struct device *const dev)
+{
+	intel_gma_init_igd_opregion();
+
+	/* SoC specific configuration. */
+	graphics_soc_init(dev);
+
+	/*
+	 * GFX PEIM module inside FSP binary is taking care of graphics
+	 * initialization based on RUN_FSP_GOP Kconfig option and input
+	 * VBT file.
+	 *
+	 * In case of non-FSP solution, SoC need to select another
+	 * Kconfig to perform GFX initialization.
+	 */
+	if (CONFIG(RUN_FSP_GOP))
+		return;
+
+	/* IGD needs to Bus Master */
+	u32 reg32 = pci_read_config32(dev, PCI_COMMAND);
+	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
+	pci_write_config32(dev, PCI_COMMAND, reg32);
+
+	if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
+		if (!acpi_is_wakeup_s3() && display_init_required()) {
+			int lightup_ok;
+			gma_gfxinit(&lightup_ok);
+			gfx_set_init_done(lightup_ok);
+		}
+	} else {
+		/* Initialize PCI device, load/execute BIOS Option ROM */
+		pci_dev_init(dev);
+	}
+}
+
 static void gma_generate_ssdt(const struct device *device)
 {
    const struct i915_gpu_controller_info *gfx = intel_igd_get_controller_info(device);
@@ -116,7 +152,7 @@
    .read_resources		= pci_dev_read_resources,
    .set_resources		= pci_dev_set_resources,
    .enable_resources	= pci_dev_enable_resources,
-	.init			= graphics_soc_init,
+	.init			= gma_init,
    .ops_pci		= &pci_dev_ops_pci,
 #if CONFIG(HAVE_ACPI_TABLES)
    .acpi_fill_ssdt		= gma_generate_ssdt,
diff --git a/src/soc/intel/icelake/graphics.c b/src/soc/intel/icelake/graphics.c
index 3118495..c8d282d 100644
--- a/src/soc/intel/icelake/graphics.c
+++ b/src/soc/intel/icelake/graphics.c
@@ -1,11 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -13,28 +8,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	intel_gma_init_igd_opregion();
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP))
-		return;
-
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/* Initialize PCI device, load/execute BIOS Option ROM */
-	pci_dev_init(dev);
-}
diff --git a/src/soc/intel/jasperlake/graphics.c b/src/soc/intel/jasperlake/graphics.c
index 3118495..c8d282d 100644
--- a/src/soc/intel/jasperlake/graphics.c
+++ b/src/soc/intel/jasperlake/graphics.c
@@ -1,11 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -13,28 +8,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	intel_gma_init_igd_opregion();
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP))
-		return;
-
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/* Initialize PCI device, load/execute BIOS Option ROM */
-	pci_dev_init(dev);
-}
diff --git a/src/soc/intel/skylake/graphics.c b/src/soc/intel/skylake/graphics.c
index dab7e14..d887097 100644
--- a/src/soc/intel/skylake/graphics.c
+++ b/src/soc/intel/skylake/graphics.c
@@ -1,16 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

-#include <bootmode.h>
+#include <acpi/acpi.h>
 #include <commonlib/helpers.h>
 #include <console/console.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
+#include <device/mmio.h>
 #include <device/resource.h>
 #include <drivers/intel/gma/i915.h>
 #include <drivers/intel/gma/i915_reg.h>
-#include <drivers/intel/gma/libgfxinit.h>
 #include <intelblocks/graphics.h>
-#include <drivers/intel/gma/opregion.h>
 #include <soc/ramstage.h>
 #include <types.h>

@@ -83,8 +80,6 @@
 {
    u32 ddi_buf_ctl;

-	intel_gma_init_igd_opregion();
-
    graphics_setup_panel(dev);

/*
@@ -97,32 +92,6 @@
    	ddi_buf_ctl |= DDI_A_4_LANES;
    	graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
    }
-
-	/* IGD needs to Bus Master */
-	u32 reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig option and input
-	 * VBT file.
-	 *
-	 * In case of non-FSP solution, SoC need to select another
-	 * Kconfig to perform GFX initialization.
-	 */
-	if (CONFIG(RUN_FSP_GOP)) {
-		/* nothing to do */
-	} else if (CONFIG(MAINBOARD_USE_LIBGFXINIT)) {
-		if (!acpi_is_wakeup_s3() && display_init_required()) {
-			int lightup_ok;
-			gma_gfxinit(&lightup_ok);
-			gfx_set_init_done(lightup_ok);
-		}
-	} else {
-		/* Initialize PCI device, load/execute BIOS Option ROM */
-		pci_dev_init(dev);
-	}
 }

const struct i915_gpu_controller_info *
diff --git a/src/soc/intel/tigerlake/graphics.c b/src/soc/intel/tigerlake/graphics.c
index ea90d44..11aea72 100644
--- a/src/soc/intel/tigerlake/graphics.c
+++ b/src/soc/intel/tigerlake/graphics.c
@@ -6,12 +6,7 @@
  * Chapter number: 4
  */

-#include <console/console.h>
 #include <fsp/util.h>
-#include <device/device.h>
-#include <device/pci.h>
-#include <device/pci_ops.h>
-#include <drivers/intel/gma/opregion.h>
 #include <intelblocks/graphics.h>
 #include <types.h>

@@ -19,28 +14,3 @@
 {
    return graphics_get_memory_base();
 }
-
-void graphics_soc_init(struct device *dev)
-{
-	intel_gma_init_igd_opregion();
-
-	/*
-	 * GFX PEIM module inside FSP binary is taking care of graphics
-	 * initialization based on RUN_FSP_GOP Kconfig
-	 * option and input VBT file. Hence no need to load/execute legacy VGA
-	 * OpROM in order to initialize GFX.
-	 *
-	 * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
-	 * Kconfig to perform GFX initialization through VGA OpRom.
-	 */
-	if (CONFIG(RUN_FSP_GOP))
-		return;
-
-	/* IGD needs to Bus Master */
-	uint32_t reg32 = pci_read_config32(dev, PCI_COMMAND);
-	reg32 |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-	pci_write_config32(dev, PCI_COMMAND, reg32);
-
-	/* Initialize PCI device, load/execute BIOS Option ROM */
-	pci_dev_init(dev);
-}

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40725 )

Change subject: soc/intel/gma: Move display and opregion init to common code
......................................................................

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/4230
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4229
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4228
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4227

Please note: This test is under development and might not be accurate at all!