Change in ...coreboot[master]: intel/apollolake: Add fixed resources for VTd to system resources

[Werner Zeh (Code Review)]

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30991

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

intel/apollolake: Add fixed resources for VTd to system resources

If the VTd feature is enabled, there will be up to two fixed resources
which are set up by the FSP. Add this resources to the list of system
resources so that the PCI enumerator will know them.

Change-Id: If96fc1c93746e3c7f510e5b3095ea3090e1b8807
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/systemagent.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/30991/1

diff --git a/src/soc/intel/apollolake/systemagent.c b/src/soc/intel/apollolake/systemagent.c
index c8f1330..6acc3b5 100644
--- a/src/soc/intel/apollolake/systemagent.c
+++ b/src/soc/intel/apollolake/systemagent.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2015 Intel Corp.
  * (Written by Andrey Petrov  for Intel Corp.)
  * (Written by Alexandru Gagniuc  for Intel Corp.)
+ * Copyright (C) 2018 Siemens AG
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -19,6 +20,7 @@
 #include <cpu/cpu.h>
 #include <console/console.h>
 #include <device/device.h>
+#include <device/pci_ops.h>
 #include <fsp/util.h>
 #include <intelblocks/systemagent.h>
 #include <soc/iomap.h>
@@ -40,6 +42,24 @@

sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources,
    		ARRAY_SIZE(soc_fixed_resources));
+
+	/* Add VTd resources if VTd is enabled. These resources has been
+	  set up by the FSP-S call. */
+	if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
+		return;
+
+	if (MCHBAR32(GFXVTBAR) & VTBAR_ENABLED) {
+		mmio_resource(dev, *index,
+			      (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB,
+			      VTBAR_SIZE / KiB);
+		(*index)++;
+	}
+	if (MCHBAR32(DEFVTBAR) & VTBAR_ENABLED) {
+		mmio_resource(dev, *index,
+			      (MCHBAR32(DEFVTBAR) & VTBAR_MASK) / KiB,
+			      VTBAR_SIZE / KiB);
+		(*index)++;
+	}
 }

int soc_get_uncore_prmmr_base_and_mask(uint64_t *prmrr_base,

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/30991/1/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/1/src/soc/intel/apollolake/systemagent... 
PS1, Line 47: 	  set up by the FSP-S call. */
one character to the right

[Werner Zeh (Code Review)]

Hello Patrick Rudolph, Mario Scheithauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

intel/apollolake: Add fixed resources for VTd to system resources

If the VTd feature is enabled, there will be up to two fixed resources
which are set up by the FSP. Add this resources to the list of system
resources so that the PCI enumerator will know them.

Change-Id: If96fc1c93746e3c7f510e5b3095ea3090e1b8807
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/systemagent.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/30991/2

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 2: Code-Review+2

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/30991/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30991/2//COMMIT_MSG@10 
PS2, Line 10: this
these

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
PS2, Line 46: has been
were

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
PS2, Line 53: (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB
Did you mean to silently ignore the possible division remainder? (same for the other divisions here)

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/30991/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30991/2//COMMIT_MSG@10 
PS2, Line 10: this
> these

Ack

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
PS2, Line 46: has been
> were

Ack

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
PS2, Line 53: (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB
> Did you mean to silently ignore the possible division remainder? (same for the other divisions here)

No, there is just this requirement of having this memory to be in KiB granularity. So there will be no rest.

[Werner Zeh (Code Review)]

Hello Patrick Rudolph, Mario Scheithauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

intel/apollolake: Add fixed resources for VTd to system resources

If the VTd feature is enabled, there will be up to two fixed resources
which are set up by the FSP. Add these resources to the list of system
resources so that the PCI enumerator will know them.

Change-Id: If96fc1c93746e3c7f510e5b3095ea3090e1b8807
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/systemagent.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/30991/3

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Looks fine to me, though I have to admit I don't know much about resource handling here :D

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/2/src/soc/intel/apollolake/systemagent... 
PS2, Line 53: (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB
> No, there is just this requirement of having this memory to be in KiB granularity. […]

I mean if the input is not KiB-granular, then you would lose the remainder. So the question is whether there is a _promise_ to have input KiB-granular. After checking the defines used here I see that's the case.

In C++ code I'd' request a few `static_assert`s here, but should be OK here.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 5: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
PS6, Line 53: 			      (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB,
Both BARs are 64-bit registers.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
PS6, Line 53: 			      (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB,
> Both BARs are 64-bit registers.

Yes, and we have just an "unsigned long" to pass to the mmio_resource(). Do you want to me to make sure that the upper 32 bits are cleared? Or is there a way to report a 64-bit (reduced by 10 bit due to KiB granularity) value?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
File src/soc/intel/apollolake/systemagent.c:

https://review.coreboot.org/#/c/30991/6/src/soc/intel/apollolake/systemagent... 
PS6, Line 53: 			      (MCHBAR32(GFXVTBAR) & VTBAR_MASK) / KiB,
> Yes, and we have just an "unsigned long" to pass to the mmio_resource(). […]

We are lucky here as the most significant 24 bits are defined to be
read-only zero. So when reading the 64-bit value, you'll actually
have only 40 bits, and minus 10 makes only 30 bits after converting
to KiB.

In case APL supports reading 64-bit values, this could be as simple
as changing 32 to 64 in this line (given a proper definition of
VTBAR_MASK).

[Werner Zeh (Code Review)]

Hello Alex Thiessen, Patrick Rudolph, Mario Scheithauer, Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

intel/apollolake: Add fixed resources for VTd to system resources

If the VTd feature is enabled, there will be up to two fixed resources
which are set up by the FSP. Add these resources to the list of system
resources so that the PCI enumerator will know them.

Change-Id: If96fc1c93746e3c7f510e5b3095ea3090e1b8807
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/systemagent.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/30991/7

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

Patch Set 8: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30991 )

Change subject: intel/apollolake: Add fixed resources for VTd to system resources
......................................................................

intel/apollolake: Add fixed resources for VTd to system resources

If the VTd feature is enabled, there will be up to two fixed resources
which are set up by the FSP. Add these resources to the list of system
resources so that the PCI enumerator will know them.

Change-Id: If96fc1c93746e3c7f510e5b3095ea3090e1b8807
Signed-off-by: Werner Zeh 
Reviewed-on: https://review.coreboot.org/c/30991
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/systemagent.c
1 file changed, 20 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/systemagent.c b/src/soc/intel/apollolake/systemagent.c
index c8f1330..fd91082 100644
--- a/src/soc/intel/apollolake/systemagent.c
+++ b/src/soc/intel/apollolake/systemagent.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2015 Intel Corp.
  * (Written by Andrey Petrov  for Intel Corp.)
  * (Written by Alexandru Gagniuc  for Intel Corp.)
+ * Copyright (C) 2019 Siemens AG
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -19,6 +20,7 @@
 #include <cpu/cpu.h>
 #include <console/console.h>
 #include <device/device.h>
+#include <device/pci_ops.h>
 #include <fsp/util.h>
 #include <intelblocks/systemagent.h>
 #include <soc/iomap.h>
@@ -40,6 +42,24 @@

sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources,
    		ARRAY_SIZE(soc_fixed_resources));
+
+	/* Add VTd resources if VTd is enabled. These resources were
+	   set up by the FSP-S call. */
+	if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
+		return;
+
+	if (MCHBAR32(GFXVTBAR) & VTBAR_ENABLED) {
+		mmio_resource(dev, *index,
+			      (MCHBAR64(GFXVTBAR) & VTBAR_MASK) / KiB,
+			      VTBAR_SIZE / KiB);
+		(*index)++;
+	}
+	if (MCHBAR32(DEFVTBAR) & VTBAR_ENABLED) {
+		mmio_resource(dev, *index,
+			      (MCHBAR64(DEFVTBAR) & VTBAR_MASK) / KiB,
+			      VTBAR_SIZE / KiB);
+		(*index)++;
+	}
 }

int soc_get_uncore_prmmr_base_and_mask(uint64_t *prmrr_base,