Change in coreboot[master]: Revert "soc/intel/cannonlake: Remove DMA support for PTT"

[Jeremy Soller (Code Review)]

Hello Aaron Durbin, Subrata Banik, Balaji Manigandan, Aamir Bohra, Paul Menzel, build bot (Jenkins), Hannah Williams, Lijian Zhao, Furquan Shaikh,

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

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

to review the following change.

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Revert "soc/intel/cannonlake: Remove DMA support for PTT"

This reverts commit d5018a8f78b9e1f0b7d3d1be298cba9716b10c6c.

Reason for revert: Breaks boot on Whiskey Lake-U System76 boards

Change-Id: Ib82f02c4a2b1cd2dbf95d4ca4a9edd314e78edd2
---
M src/soc/intel/cannonlake/include/soc/iomap.h
M src/soc/intel/cannonlake/memmap.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/35924/1

diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h
index 75f11c0..2a3608c 100644
--- a/src/soc/intel/cannonlake/include/soc/iomap.h
+++ b/src/soc/intel/cannonlake/include/soc/iomap.h
@@ -65,6 +65,10 @@

#define HECI1_BASE_ADDRESS	0xfeda2000

+/* PTT registers */
+#define PTT_TXT_BASE_ADDRESS	0xfed30800
+#define PTT_PRESENT		0x00070000
+
 #define VTD_BASE_ADDRESS	0xFED90000
 #define VTD_BASE_SIZE		0x00004000
 /*
diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c
index 64e07be..108a1b0 100644
--- a/src/soc/intel/cannonlake/memmap.c
+++ b/src/soc/intel/cannonlake/memmap.c
@@ -83,6 +83,22 @@
    return 0;
 }

+static bool is_ptt_enable(void)
+{
+	if ((read32((void *)PTT_TXT_BASE_ADDRESS) & PTT_PRESENT) ==
+			PTT_PRESENT)
+		return true;
+
+	return false;
+}
+
+/* Calculate PTT size */
+static size_t get_ptt_size(void)
+{
+	/* Allocate 4KB for PTT if enabled */
+	return is_ptt_enable() ? 4*KiB : 0;
+}
+
 /* Calculate ME Stolen size */
 static size_t get_imr_size(void)
 {
@@ -176,6 +192,9 @@
    /* Get Tracehub size */
    reserve_mem_base -= get_imr_size();

+	/* Get PTT size */
+	reserve_mem_base -= get_ptt_size();
+
    /* Traditional Area Size */
    reserve_mem_size = dram_base - reserve_mem_base;

[Jeremy Soller (Code Review)]

Hello Patrick Rudolph, Aaron Durbin, Subrata Banik, Balaji Manigandan, Aamir Bohra, Paul Menzel, build bot (Jenkins), Hannah Williams, Lijian Zhao, Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Revert "soc/intel/cannonlake: Remove DMA support for PTT"

This reverts commit d5018a8f78b9e1f0b7d3d1be298cba9716b10c6c.

Reason for revert: Breaks boot on Whiskey Lake-U System76 boards

Signed-off-by: Jeremy Soller 
CC: Matt DeVillier 
CC: Subrata Banik 
Change-Id: Ib82f02c4a2b1cd2dbf95d4ca4a9edd314e78edd2
---
M src/soc/intel/cannonlake/include/soc/iomap.h
M src/soc/intel/cannonlake/memmap.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/35924/2

[Patrick Rudolph (Code Review)]

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

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2: Code-Review+2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks boot on Whiskey Lake-U System76 boards
Why? What is the interaction? An FSP default setting for that board? Soft strap configuration? Fusing?

[Matt DeVillier (Code Review)]

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

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks boot on Whiskey Lake-U System76 boards
> Why? What is the interaction? An FSP default setting for that board? Soft strap configuration? Fusing?

without the mem allocated for PTT removed from reserved size, size mismatch occurs and boot halts

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks boot on Whiskey Lake-U System76 boards
> > Why? What is the interaction? An FSP default setting for that board? Soft strap configuration? Fusing?
>
> without the mem allocated for PTT removed from reserved size, size mismatch occurs and boot halts

What FSP is being used? And with what settings? Are you enabling PTT in the devicetree? Or is it hard coded in the default UPDs of the FSP being used?

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2: Code-Review+2

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2:

Because PTT_TXT_BASE_ADDRESS	0xfed30800 is changed or what?

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Because PTT_TXT_BASE_ADDRESS	0xfed30800 is changed or what?

asked some question here https://review.coreboot.org/c/coreboot/+/27176

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 2: Code-Review-1

I think it's important we update the reasoning and answer the questions I have prior to submission.

[Jeremy Soller (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Matt DeVillier, Paul Menzel, build bot (Jenkins), Hannah Williams, Lijian Zhao, Furquan Shaikh, Philipp Deppenwiese,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Revert "soc/intel/cannonlake: Remove DMA support for PTT"

This reverts commit d5018a8f78b9e1f0b7d3d1be298cba9716b10c6c.

Reason for revert: Breaks boot on Whiskey Lake-U boards

Both System76 and Purism have had memory initialization failures
when this patch is applied, with the following error message:
Failed to accommodate FSP reserved memory request!

It appears that an extra 4096 bytes needs to be reserved for the
FSP on these systems, and reinstating the PTT reservation does
this as expected. PTT is enabled for the System76 galp3-c in the
ME configuration, which is potentially why the behaviour is
different.

Signed-off-by: Jeremy Soller 
CC: Matt DeVillier 
CC: Subrata Banik 
Change-Id: Ib82f02c4a2b1cd2dbf95d4ca4a9edd314e78edd2
---
M src/soc/intel/cannonlake/include/soc/iomap.h
M src/soc/intel/cannonlake/memmap.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/35924/3

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 2:
>
> (1 comment)

I have updated the commit description and responded to Aaron's questions

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks boot on Whiskey Lake-U System76 boards
> > > Why? What is the interaction? An FSP default setting for that board? Soft strap configuration? F […]

The FSP version is the one from the current master branch of coreboot, Coffee Lake 7.0.64.40. No special UPDs are being set.

PTT is enabled in the ME configuration, not in the firmware.

Here is the settings file from Intel FIT tool showing PTT enabled:

<IntelPttConfiguration label="Intel(R) PTT Configuration">
    <PttSupported value="Yes" value_list="No,,Yes" label="Intel(R) PTT Supported" help_text="This setting permanently disables Intel(R) PTT in the firmware image." />
    <PttPwrUpState value="Enabled" value_list="Disabled,,Enabled" label="Intel(R) PTT initial power-up state" />
    <PttSupportedFpf value="Yes" value_list="No,,Yes" label="Intel(R) PTT Supported [FPF]" help_text="This setting will permanently disable Intel(R) PTT through platform FPFs.  Caution: Using this option will permanently disable Intel(R) PTT on the platform hardware." />
</IntelPttConfiguration>

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks boot on Whiskey Lake-U System76 boards
> The FSP version is the one from the current master branch of coreboot, Coffee Lake 7.0.64.40. […]

Thanks, Jeremy.

https://review.coreboot.org/c/coreboot/+/35924/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/3//COMMIT_MSG@20 
PS3, Line 20: potentially 
Not potentially. It is. These systems are using an ME with PTT enabled by way of FIT tool. Makes perfect sense with the whole story. Let's nuke potentially and explain this is expected given these systems' ME configuration.

[Jeremy Soller (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Matt DeVillier, Paul Menzel, build bot (Jenkins), Hannah Williams, Lijian Zhao, Furquan Shaikh, Philipp Deppenwiese,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Revert "soc/intel/cannonlake: Remove DMA support for PTT"

This reverts commit d5018a8f78b9e1f0b7d3d1be298cba9716b10c6c.

Reason for revert: Breaks boot on Whiskey Lake-U boards

Both System76 and Purism have had memory initialization failures
when this patch is applied, with the following error message:
Failed to accommodate FSP reserved memory request!

An extra 4096 bytes needs to be reserved for the FSP on these
systems, and reinstating the PTT reservation does this as
expected. PTT is enabled for the System76 galp3-c in the
ME configuration, which is why the behaviour is different.

Signed-off-by: Jeremy Soller 
CC: Matt DeVillier 
CC: Subrata Banik 
Change-Id: Ib82f02c4a2b1cd2dbf95d4ca4a9edd314e78edd2
---
M src/soc/intel/cannonlake/include/soc/iomap.h
M src/soc/intel/cannonlake/memmap.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/35924/4

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35924/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35924/3//COMMIT_MSG@20 
PS3, Line 20: potentially
> Not potentially. It is. These systems are using an ME with PTT enabled by way of FIT tool. […]

I reworded that last paragraph in changeset 4.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 4: Code-Review+1

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 4: Code-Review+2

We disabled PTT for WHL internal coreboot build, but that didn't mean WHL and CFL don't support PTT at all. Cast my +2 here.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Patch Set 4: Code-Review+1

Arthur Heymans is working on a solution to get rid of EBDA. As soon as this is ready, CB:36136 will remove the need for doing any calculations.

As a quickfix I'd be fine getting this merged

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35924 )

Change subject: Revert "soc/intel/cannonlake: Remove DMA support for PTT"
......................................................................

Revert "soc/intel/cannonlake: Remove DMA support for PTT"

This reverts commit d5018a8f78b9e1f0b7d3d1be298cba9716b10c6c.

Reason for revert: Breaks boot on Whiskey Lake-U boards

Both System76 and Purism have had memory initialization failures
when this patch is applied, with the following error message:
Failed to accommodate FSP reserved memory request!

An extra 4096 bytes needs to be reserved for the FSP on these
systems, and reinstating the PTT reservation does this as
expected. PTT is enabled for the System76 galp3-c in the
ME configuration, which is why the behaviour is different.

Signed-off-by: Jeremy Soller 
CC: Matt DeVillier 
CC: Subrata Banik 
Change-Id: Ib82f02c4a2b1cd2dbf95d4ca4a9edd314e78edd2
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35924
Reviewed-by: Aaron Durbin 
Reviewed-by: Lance Zhao 
Reviewed-by: Michael Niewöhner
Reviewed-by: Patrick Rudolph 
Reviewed-by: Matt DeVillier 
Reviewed-by: Philipp Deppenwiese 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/include/soc/iomap.h
M src/soc/intel/cannonlake/memmap.c
2 files changed, 23 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, but someone else must approve
  Philipp Deppenwiese: Looks good to me, approved
  Matt DeVillier: Looks good to me, but someone else must approve
  Patrick Rudolph: Looks good to me, approved
  Lance Zhao: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h
index 1ebaf3f..9cfb59e 100644
--- a/src/soc/intel/cannonlake/include/soc/iomap.h
+++ b/src/soc/intel/cannonlake/include/soc/iomap.h
@@ -68,6 +68,10 @@

#define HECI1_BASE_ADDRESS	0xfeda2000

+/* PTT registers */
+#define PTT_TXT_BASE_ADDRESS	0xfed30800
+#define PTT_PRESENT		0x00070000
+
 #define VTD_BASE_ADDRESS	0xFED90000
 #define VTD_BASE_SIZE		0x00004000
 /*
diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c
index f3286cc..7adaa30 100644
--- a/src/soc/intel/cannonlake/memmap.c
+++ b/src/soc/intel/cannonlake/memmap.c
@@ -37,6 +37,22 @@
    *size = sa_get_tseg_size();
 }

+static bool is_ptt_enable(void)
+{
+	if ((read32((void *)PTT_TXT_BASE_ADDRESS) & PTT_PRESENT) ==
+			PTT_PRESENT)
+		return true;
+
+	return false;
+}
+
+/* Calculate PTT size */
+static size_t get_ptt_size(void)
+{
+	/* Allocate 4KB for PTT if enabled */
+	return is_ptt_enable() ? 4*KiB : 0;
+}
+
 /* Calculate ME Stolen size */
 static size_t get_imr_size(void)
 {
@@ -130,6 +146,9 @@
    /* Get Tracehub size */
    reserve_mem_base -= get_imr_size();

+	/* Get PTT size */
+	reserve_mem_base -= get_ptt_size();
+
    /* Traditional Area Size */
    reserve_mem_size = dram_base - reserve_mem_base;