Change in coreboot[master]: soc/intel/cannonlake: Fix DMAR when no iGPU is present

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43994 )

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

soc/intel/cannonlake: Fix DMAR when no iGPU is present

Don't emit RMRR for the iGPU if it's not present. This is done on
other platforms as well.

Fixes an DMAR error seen in dmesg on platforms without iGPU.

Change-Id: Iafe86e6938a120b707aaae935cb8168f790bb22f
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/acpi.c
1 file changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43994/1

diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c
index 3f3ba10..3111989 100644
--- a/src/soc/intel/cannonlake/acpi.c
+++ b/src/soc/intel/cannonlake/acpi.c
@@ -293,6 +293,13 @@
    	current += acpi_create_dmar_ds_pci(current, 0, 2, 0);

acpi_dmar_drhd_fixup(tmp, current);
+
+		/* Add RMRR entry */
+		tmp = current;
+		current += acpi_create_dmar_rmrr(current, 0,
+			sa_get_gsm_base(), sa_get_tolud_base() - 1);
+		current += acpi_create_dmar_ds_pci(current, 0, 2, 0);
+		acpi_dmar_rmrr_fixup(tmp, current);
    }

struct device *const ipu_dev = pcidev_path_on_root(SA_DEVFN_IPU);
@@ -326,13 +333,6 @@
    	acpi_dmar_drhd_fixup(tmp, current);
    }

-	/* Add RMRR entry */
-	const unsigned long tmp = current;
-	current += acpi_create_dmar_rmrr(current, 0,
-		sa_get_gsm_base(), sa_get_tolud_base() - 1);
-	current += acpi_create_dmar_ds_pci(current, 0, 2, 0);
-	acpi_dmar_rmrr_fixup(tmp, current);
-
    return current;
 }

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
Wait... Apparently VT-d spec says that RMRRs need to be added after all DRHDs have been added. So we need to keep this where it was before.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43994 )

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
> Wait... Apparently VT-d spec says that RMRRs need to be added after all DRHDs have been added. […]

You are right. Checked Document D51397-011.
Then the following platforms are wrong: APL, SDY, BDW, SKL

[Patrick Rudolph (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Philipp Deppenwiese, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

soc/intel/cannonlake: Fix DMAR when no iGPU is present

Don't emit RMRR for the iGPU if it's not present. This is done on
other platforms as well.

Fixes an DMAR error seen in dmesg on platforms without iGPU.

Change-Id: Iafe86e6938a120b707aaae935cb8168f790bb22f
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/acpi.c
1 file changed, 9 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43994/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43994 )

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
> You are right. Checked Document D51397-011. […]

Done

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 3: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
> Done

What's SDY?

https://review.coreboot.org/c/coreboot/+/43994/3/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/3/src/soc/intel/cannonlake/ac... 
PS3, Line 330: igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten
Instead of repeating this, put it in a variable?

const bool igfx_enabled = igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten;

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Matt DeVillier, Angel Pons, Sridhar Siricilla, Bernardo Perez Priego, Patrick Rudolph, Philipp Deppenwiese, Philipp Deppenwiese, Christian Walter, Meera Ravindranath, Brandon Breitenstein, Werner Zeh,

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

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

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

soc/intel/cannonlake: Fix DMAR when no iGPU is present

Don't emit RMRR for the iGPU if it's not present. This is done on
other platforms as well.

Fixes an DMAR error seen in dmesg on platforms without iGPU.

Change-Id: Iafe86e6938a120b707aaae935cb8168f790bb22f
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/acpi.c
1 file changed, 11 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/43994/4

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43994 )

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
> What's SDY?

Sandybridge

https://review.coreboot.org/c/coreboot/+/43994/3/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/3/src/soc/intel/cannonlake/ac... 
PS3, Line 330: igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten
> Instead of repeating this, put it in a variable? […]

Done

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/43994/2/src/soc/intel/cannonlake/ac... 
PS2, Line 297: 		/* Add RMRR entry */
> Sandybridge

Ah, the "official" spelling would be SNB

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43994 )

Change subject: soc/intel/cannonlake: Fix DMAR when no iGPU is present
......................................................................

soc/intel/cannonlake: Fix DMAR when no iGPU is present

Don't emit RMRR for the iGPU if it's not present. This is done on
other platforms as well.

Fixes an DMAR error seen in dmesg on platforms without iGPU.

Change-Id: Iafe86e6938a120b707aaae935cb8168f790bb22f
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43994
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/acpi.c
1 file changed, 11 insertions(+), 8 deletions(-)

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

diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c
index 3f3ba10..f061c30 100644
--- a/src/soc/intel/cannonlake/acpi.c
+++ b/src/soc/intel/cannonlake/acpi.c
@@ -285,8 +285,8 @@
    struct device *const igfx_dev = pcidev_path_on_root(SA_DEVFN_IGD);
    uint64_t gfxvtbar = MCHBAR64(GFXVTBAR) & VTBAR_MASK;
    bool gfxvten = MCHBAR32(GFXVTBAR) & VTBAR_ENABLED;
-
-	if (igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten) {
+	const bool emit_igd = igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten;
+	if (emit_igd) {
    	unsigned long tmp = current;

current += acpi_create_dmar_drhd(current, 0, 0, gfxvtbar);
@@ -326,12 +326,15 @@
    	acpi_dmar_drhd_fixup(tmp, current);
    }

-	/* Add RMRR entry */
-	const unsigned long tmp = current;
-	current += acpi_create_dmar_rmrr(current, 0,
-		sa_get_gsm_base(), sa_get_tolud_base() - 1);
-	current += acpi_create_dmar_ds_pci(current, 0, 2, 0);
-	acpi_dmar_rmrr_fixup(tmp, current);
+	/* Add RMRR entry after all DRHD entries */
+	if (emit_igd) {
+		const unsigned long tmp = current;
+
+		current += acpi_create_dmar_rmrr(current, 0,
+			sa_get_gsm_base(), sa_get_tolud_base() - 1);
+		current += acpi_create_dmar_ds_pci(current, 0, 2, 0);
+		acpi_dmar_rmrr_fixup(tmp, current);
+	}

return current;
 }