Change in ...coreboot[master]: soc/intel/common: Fix booting issue without default IGD enabled
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33449
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
soc/intel/common: Fix booting issue without default IGD enabled
This patch ensures to boot platform without onboard GFX (PCI B0:D2:F0) enabled.
TEST=Previously platform was dying at "GMADR is not programmed!" with IGD disabled.
Change-Id: I8c907ee25db4538a84890f2ccc3187afa86604b8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/graphics.c 1 file changed, 21 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/33449/1
diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c index 4cea21b..ed9ae00 100644 --- a/src/soc/intel/common/block/graphics/graphics.c +++ b/src/soc/intel/common/block/graphics/graphics.c @@ -33,15 +33,18 @@ pci_dev_init(dev); }
-static uintptr_t graphics_get_bar(unsigned long index) +static int is_graphics_disabled(struct device *dev) { - struct device *dev = SA_DEV_IGD; - struct resource *gm_res; - assert(dev != NULL); - /* Check if Graphics PCI device is disabled */ if (!dev || !dev->enabled) - return 0; + return 1; + + return 0; +} + +static uintptr_t graphics_get_bar(struct device *dev, unsigned long index) +{ + struct resource *gm_res;
gm_res = find_resource(dev, index); if (!gm_res) @@ -52,11 +55,16 @@
uintptr_t graphics_get_memory_base(void) { + uintptr_t memory_base; + struct device *dev = SA_DEV_IGD; + + if (is_graphics_disabled(dev)) + return 0; /* * GFX PCI config space offset 0x18 know as Graphics * Memory Range Address (GMADR) */ - uintptr_t memory_base = graphics_get_bar(PCI_BASE_ADDRESS_2); + memory_base = graphics_get_bar(dev, PCI_BASE_ADDRESS_2); if (!memory_base) die_with_post_code(POST_HW_INIT_FAILURE, "GMADR is not programmed!"); @@ -66,14 +74,18 @@
static uintptr_t graphics_get_gtt_base(void) { + static uintptr_t gtt_base; + struct device *dev = SA_DEV_IGD; + + if (is_graphics_disabled(dev)) + die("IGD is disabled!"); /* * GFX PCI config space offset 0x10 know as Graphics * Translation Table Memory Mapped Range Address * (GTTMMADR) */ - static uintptr_t gtt_base; if (!gtt_base) { - gtt_base = graphics_get_bar(PCI_BASE_ADDRESS_0); + gtt_base = graphics_get_bar(dev, PCI_BASE_ADDRESS_0); if (!gtt_base) die_with_post_code(POST_HW_INIT_FAILURE, "GTTMMADR is not programmed!");
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33449
to look at the new patch set (#2).
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
soc/intel/common: Fix booting issue without default IGD enabled
This patch ensures to boot platform without onboard GFX (PCI B0:D2:F0) enabled from mainboard devicetree.cb.
TEST=Previously platform was dying at "GMADR is not programmed!" with IGD disabled.
Change-Id: I8c907ee25db4538a84890f2ccc3187afa86604b8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/graphics.c 1 file changed, 21 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/33449/2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/graphics.c:
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... PS2, Line 81: die I guess die() is appropriate as the users of this function do not check for any errors.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/graphics.c:
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... PS2, Line 62: return 0; Why is it ok to return 0 here if we have to die() below? This seems to be the only effective change, that we return 0 here instead of running into the die_with_post_code() below.
This might make the calling code more error-prone.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
Patch Set 2:
I think the cause of this could be a configuration issue. If RUN_FSP_GOP is selected but the IGD disabled, it would run into this path. Checking if the IGD is actually enabled, in fill_lb_framebuffer() in drivers/intel/fsp2_0/graphics.c, might be a more elegant solution.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
Patch Set 2:
(2 comments)
Patch Set 2:
I think the cause of this could be a configuration issue. If RUN_FSP_GOP is selected but the IGD disabled, it would run into this path. Checking if the IGD is actually enabled, in fill_lb_framebuffer() in drivers/intel/fsp2_0/graphics.c, might be a more elegant solution.
yes, let me do that as well
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/graphics.c:
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... PS2, Line 62: return 0;
Why is it ok to return 0 here if we have to die() below? This seems […]
Looks like called of graphics_get_memory_base() function has non-zero value check so it might able to know if framebuffer is available or not
https://review.coreboot.org/#/c/33449/2/src/soc/intel/common/block/graphics/... PS2, Line 81: die
I guess die() is appropriate as the users of this function do not check for any errors.
yes, this code can't get call in case IGD is disable still just an additional check to block user.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
Patch Set 2:
Patch Set 2:
(2 comments)
Patch Set 2:
I think the cause of this could be a configuration issue. If RUN_FSP_GOP is selected but the IGD disabled, it would run into this path. Checking if the IGD is actually enabled, in fill_lb_framebuffer() in drivers/intel/fsp2_0/graphics.c, might be a more elegant solution.
yes, let me do that as well
merging this CL now and let see how this external GFX enable goes with correct sets of config (!CONFIG_RUN_FSP_GOP)
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33449 )
Change subject: soc/intel/common: Fix booting issue without default IGD enabled ......................................................................
soc/intel/common: Fix booting issue without default IGD enabled
This patch ensures to boot platform without onboard GFX (PCI B0:D2:F0) enabled from mainboard devicetree.cb.
TEST=Previously platform was dying at "GMADR is not programmed!" with IGD disabled.
Change-Id: I8c907ee25db4538a84890f2ccc3187afa86604b8 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33449 Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/graphics/graphics.c 1 file changed, 21 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved
diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c index 4cea21b..ed9ae00 100644 --- a/src/soc/intel/common/block/graphics/graphics.c +++ b/src/soc/intel/common/block/graphics/graphics.c @@ -33,15 +33,18 @@ pci_dev_init(dev); }
-static uintptr_t graphics_get_bar(unsigned long index) +static int is_graphics_disabled(struct device *dev) { - struct device *dev = SA_DEV_IGD; - struct resource *gm_res; - assert(dev != NULL); - /* Check if Graphics PCI device is disabled */ if (!dev || !dev->enabled) - return 0; + return 1; + + return 0; +} + +static uintptr_t graphics_get_bar(struct device *dev, unsigned long index) +{ + struct resource *gm_res;
gm_res = find_resource(dev, index); if (!gm_res) @@ -52,11 +55,16 @@
uintptr_t graphics_get_memory_base(void) { + uintptr_t memory_base; + struct device *dev = SA_DEV_IGD; + + if (is_graphics_disabled(dev)) + return 0; /* * GFX PCI config space offset 0x18 know as Graphics * Memory Range Address (GMADR) */ - uintptr_t memory_base = graphics_get_bar(PCI_BASE_ADDRESS_2); + memory_base = graphics_get_bar(dev, PCI_BASE_ADDRESS_2); if (!memory_base) die_with_post_code(POST_HW_INIT_FAILURE, "GMADR is not programmed!"); @@ -66,14 +74,18 @@
static uintptr_t graphics_get_gtt_base(void) { + static uintptr_t gtt_base; + struct device *dev = SA_DEV_IGD; + + if (is_graphics_disabled(dev)) + die("IGD is disabled!"); /* * GFX PCI config space offset 0x10 know as Graphics * Translation Table Memory Mapped Range Address * (GTTMMADR) */ - static uintptr_t gtt_base; if (!gtt_base) { - gtt_base = graphics_get_bar(PCI_BASE_ADDRESS_0); + gtt_base = graphics_get_bar(dev, PCI_BASE_ADDRESS_0); if (!gtt_base) die_with_post_code(POST_HW_INIT_FAILURE, "GTTMMADR is not programmed!");
-
Duncan Laurie (Code Review) -
Nico Huber (Code Review)
-
Subrata Banik (Code Review)