Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB

[Christian Walter (Code Review)]

Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB

Change the TPM ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/1

diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c
index 8ab993e..18c517a 100644
--- a/src/arch/x86/acpi.c
+++ b/src/arch/x86/acpi.c
@@ -386,10 +386,14 @@

/* Hard to detect for coreboot. Just set it to 0 */
    tpm2->platform_class = 0;
-	/* Must be set to 0 for TIS interface support */
-	tpm2->control_area = 0;
-	/* coreboot only supports the TIS interface driver. */
-	tpm2->start_method = 6;
+	if (CONFIG(CRB_TPM)) {
+		tpm2->control_area = 0xfed40000;
+		tpm2->start_method = 7;
+	} else {
+		/* Must be set to 0 for TIS interface support */
+		tpm2->control_area = 0;
+		tpm2->start_method = 6;
+	}
    memset(tpm2->msp, 0, sizeof(tpm2->msp));

/* Fill the log area size and start address fields. */

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 7:

(1 comment)

This change is ready for review.

https://review.coreboot.org/c/coreboot/+/34333/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34333/5//COMMIT_MSG@9 
PS5, Line 9: Change the TPM ACPI Table to support CRB Interface when
> it's the TPM2 ACPI table, as it's only used for TPM2, but not present for TPM1. […]

Ack

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c@90 
PS7, Line 90: 		printk(BIOS_ERR,
            : 		       "ACPI: Error: Could not add ACPI table, "
            : 		       "too many tables.\n");
printk(BIOS_ERR, "ACPI: Error: Could not add ACPI table, too many tables.\n");

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c@388 
PS7, Line 388: 0xfed40040
TPM32(0x40)?

[Patrick Rudolph (Code Review)]

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

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 7:

too many unrelated changes

[Paul Menzel (Code Review)]

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

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34333/7//COMMIT_MSG@9 
PS7, Line 9: Change the TPM2 ACPI Table to support CRB Interface when
           : selected.
Should fit on one line?

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 8:
> Patch Set 7:
>
> too many unrelated changes

You may have to split current change on 2:
one for code formatting and the 2nd for CRB.

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM ACPI Table to support CRB
......................................................................

Patch Set 8:
> Patch Set 8:
> > Patch Set 7:
> >
> > too many unrelated changes
>
> You may have to split current change on 2:
> one for code formatting and the 2nd for CRB.

Why? It's just formatting..

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 121 insertions(+), 156 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/9

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c@90 
PS7, Line 90: 		printk(BIOS_ERR,
            : 		       "ACPI: Error: Could not add ACPI table, "
            : 		       "too many tables.\n");
> printk(BIOS_ERR, "ACPI: Error: Could not add ACPI table, too many tables. […]

ACK

https://review.coreboot.org/c/coreboot/+/34333/7/src/arch/x86/acpi.c@388 
PS7, Line 388: 0xfed40040
> TPM32(0x40)?

Ack

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 119 insertions(+), 156 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/10

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 120 insertions(+), 156 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/12

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c@53 
PS12, Line 53: <northbridge/intel/x4x/iomap.h>
no, sorry, it was just a question :)
but if you want, you can make TPM(x) common...

anyway you 'll have to split this change on 2 parts: the 1st for code re-formation 
and an other change for CRB

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c@53 
PS12, Line 53: <northbridge/intel/x4x/iomap.h>
> no, sorry, it was just a question :) […]

code formatting* I mean :)

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 120 insertions(+), 156 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/13

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when
selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 9 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/16

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 18:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/12/src/arch/x86/acpi.c@53 
PS12, Line 53: <northbridge/intel/x4x/iomap.h>
> code formatting* I mean :)

I split it up - but did not used the define anymore.

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 20: Code-Review+1

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Patrick Rudolph (Code Review)]

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 20:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/20/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/20/src/arch/x86/acpi.c@391 
PS20, Line 391: 		tpm2->control_area = 0xfed40040;
CONFIG_CRB_TPM_BASE_ADDRESS + 0x40

Change in coreboot[master]: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Paul Menzel (Code Review)]

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

Change subject: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 22:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@7 
PS22, Line 7: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
Please remove `src`.

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@7 
PS22, Line 7: Change
Redundant, as a commit often changes something.
> arch/x86/acpi: Hook up TPM2 CRB

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@10 
PS22, Line 10: selected.
Sentence should fit on one line.

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
---
M src/arch/x86/acpi.c
1 file changed, 9 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34333/23

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 22:

(5 comments)

https://review.coreboot.org/c/coreboot/+/34333/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34333/7//COMMIT_MSG@9 
PS7, Line 9: Change the TPM2 ACPI Table to support CRB Interface when
           : selected.
> Should fit on one line?

Ack

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@7 
PS22, Line 7: Change
> Redundant, as a commit often changes something. […]

True - but I guess there is a difference if you CHANGE a functionality, or add/remove it. So to be clear - i"ll keep it there.

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@7 
PS22, Line 7: src/arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
> Please remove `src`.

Ack

https://review.coreboot.org/c/coreboot/+/34333/22//COMMIT_MSG@10 
PS22, Line 10: selected.
> Sentence should fit on one line.

Ack

https://review.coreboot.org/c/coreboot/+/34333/20/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/20/src/arch/x86/acpi.c@391 
PS20, Line 391: 		tpm2->control_area = 0xfed40040;
> CONFIG_CRB_TPM_BASE_ADDRESS + 0x40

Ack

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Philipp Deppenwiese (Code Review)]

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

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 23: Code-Review+2

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Patrick Rudolph (Code Review)]

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

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 23:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/23/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/23/src/arch/x86/acpi.c@391 
PS23, Line 391: 		tpm2->control_area = CONFIG_CRB_TPM_BASE_ADDRESS + 0x40;
+ 0x40 seems Intel PTT specific.
The Tcg doesn't specify the offset, does it?

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

Patch Set 23:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34333/23/src/arch/x86/acpi.c 
File src/arch/x86/acpi.c:

https://review.coreboot.org/c/coreboot/+/34333/23/src/arch/x86/acpi.c@391 
PS23, Line 391: 		tpm2->control_area = CONFIG_CRB_TPM_BASE_ADDRESS + 0x40;
> 0x40 seems Intel PTT specific. […]

No. The TCG PC Client Platform TPM Profile (PTP) Specs p. 98 states that TPM_CRB_CTRL_REQ_0 should have the offset 0040h, which is the first struct of the control structure.

Change in coreboot[master]: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34333 )

Change subject: arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB
......................................................................

arch/x86/acpi.c: Change TPM2 ACPI Table to support CRB

Change the TPM2 ACPI Table to support CRB Interface when selected.

Change-Id: Ide3af348fd4676f2d04e1d0b9ad83f9124e09dcc
Signed-off-by: Christian Walter 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34333
Tested-by: build bot (Jenkins) 
Reviewed-by: Philipp Deppenwiese 
---
M src/arch/x86/acpi.c
1 file changed, 9 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c
index e4ccd37..fdcbcd3 100644
--- a/src/arch/x86/acpi.c
+++ b/src/arch/x86/acpi.c
@@ -386,10 +386,15 @@

/* Hard to detect for coreboot. Just set it to 0 */
    tpm2->platform_class = 0;
-	/* Must be set to 0 for TIS interface support */
-	tpm2->control_area = 0;
-	/* coreboot only supports the TIS interface driver. */
-	tpm2->start_method = 6;
+	if (CONFIG(CRB_TPM)) {
+		/* Must be set to 7 for CRB Support */
+		tpm2->control_area = CONFIG_CRB_TPM_BASE_ADDRESS + 0x40;
+		tpm2->start_method = 7;
+	} else {
+		/* Must be set to 0 for FIFO interface support */
+		tpm2->control_area = 0;
+		tpm2->start_method = 6;
+	}
    memset(tpm2->msp, 0, sizeof(tpm2->msp));

/* Fill the log area size and start address fields. */