Change in coreboot[master]: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device

[Michał Żygowski (Code Review)]

Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device

According TCG PC Client Platform Firmware Profile Specification Revision
1.04 Chapter 8.1 the TPM device object should have the _CID and _HID value
set to MSFT0101 for TPM2.

FreeBSD also detects TPM2 device only MSFT0101 _HID and _CID only.

TEST=boot FreeBSD 12.1 on PC Engines apu2 and check in dmesg that TPM2.0
is detected

Signed-off-by: Michał Żygowski 
Change-Id: I45123f272038e664b834cabd9d8525baca0eb583
---
M src/drivers/pc80/tpm/tis.c
1 file changed, 9 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/39699/1

diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c
index a35ef83..0c8e9c3 100644
--- a/src/drivers/pc80/tpm/tis.c
+++ b/src/drivers/pc80/tpm/tis.c
@@ -896,11 +896,16 @@
    acpigen_write_scope(path);
    acpigen_write_device(acpi_device_name(dev));

-	acpigen_write_name("_HID");
-	acpigen_emit_eisaid("PNP0C31");
+	if (CONFIG(TPM2)) {
+		acpigen_write_name_string("_HID", "MSFT0101");
+		acpigen_write_name_string("_CID", "MSFT0101");
+	} else {
+		acpigen_write_name("_HID");
+		acpigen_emit_eisaid("PNP0C31");

-	acpigen_write_name("_CID");
-	acpigen_emit_eisaid("PNP0C31");
+		acpigen_write_name("_CID");
+		acpigen_emit_eisaid("PNP0C31");
+	}

acpi_device_write_uid(dev);

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
the TPM spec says that either _HID or _CID must be MSFT0101.
I'd set _CID to MSFT0101 and _HID to PNP0C31.

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> the TPM spec says that either _HID or _CID must be MSFT0101. […]

Hmm, maybe you are looking on an outdated spec:

```
8.1 ACPI Device Object for TPM
A TPM device object MUST contain
a. a _HID object with a value that is formatted according to the ACPI Specification
and contains the manufacturer ID of the TPM vendor and a unique identifier and
is in the format “AAA####” or “NNNN####”, and
b. a _CID object with the value of “MSFT0101” or evaluate to a package where the
2515 value “MSFT0101” is one of the IDs within the package, or
c. A TPM device object MUST contain a _HID object with the value of “MSFT0101”
```

Point b says CID must be MSFT0101. Point c says HID must be MSFT0101

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Hmm, maybe you are looking on an outdated spec: […]

The detail here is the ", or" at the end of b.
So as long as either b. or c. is implemented the condition is true IMHO.

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> The detail here is the ", or" at the end of b. […]

Right, I missed that or and assumed all 3 conditions must be met. Will test with MSFT0101 CID only.

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Right, I missed that or and assumed all 3 conditions must be met. Will test with MSFT0101 CID only.

Tested following configurations:
HID and CID MSFT0101 - Linux and FreeBSD 12.1 works well
HID PNP0C31, CID MSFT0101 - Linux works well, FreeBSD 12.1 does not work (not probed as TPM 2.0)
HID MSFT0101, CID PNP0C31 - it doesn't work anywhere
HID and CID PNP0C31 - Linux works well, FreeBSD 12.1 does not work (not probed as TPM 2.0)

I guess the only option is to go with both HID and CID MSFT0101.

[Michał Żygowski (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device

According TCG PC Client Platform Firmware Profile Specification Revision
1.04 Chapter 8.1 the TPM device object should have the _CID and _HID
values set to MSFT0101 for TPM2.

FreeBSD also detects TPM2 device using MSFT0101 _HID and _CID only.

TEST=boot FreeBSD 12.1 on PC Engines apu2 and check in dmesg that TPM2.0
is detected

Signed-off-by: Michał Żygowski 
Change-Id: I45123f272038e664b834cabd9d8525baca0eb583
---
M src/drivers/pc80/tpm/tis.c
1 file changed, 9 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/39699/2

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Tested following configurations: […]

Does it still work on Windows10?

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Does it still work on Windows10?

Unfortunately I have no way to test on Windows 10.

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3: Code-Review+2

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Unfortunately I have no way to test on Windows 10.

Ack

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c 
File src/drivers/pc80/tpm/tis.c:

https://review.coreboot.org/c/coreboot/+/39699/1/src/drivers/pc80/tpm/tis.c@... 
PS1, Line 900: 		acpigen_write_name_string("_HID", "MSFT0101");
> Ack

If a thing with "MSFT" on its name isn't working on Windows, I think it's not our problem :^)

[Michał Żygowski (Code Review)]

Michał Żygowski has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device

According TCG PC Client Platform Firmware Profile Specification Revision
1.04 Chapter 8.1 the TPM device object should have the _CID and _HID
values set to MSFT0101 for TPM2.

FreeBSD also detects TPM2 device using MSFT0101 _HID and _CID only.

TEST=boot FreeBSD 12.1 on PC Engines apu2 and check in dmesg that TPM2.0
is detected

Signed-off-by: Michał Żygowski 
Change-Id: I45123f272038e664b834cabd9d8525baca0eb583
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39699
Reviewed-by: Patrick Rudolph 
Reviewed-by: Philipp Deppenwiese 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/drivers/pc80/tpm/tis.c
1 file changed, 9 insertions(+), 4 deletions(-)

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

diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c
index 98d31c0..1081410 100644
--- a/src/drivers/pc80/tpm/tis.c
+++ b/src/drivers/pc80/tpm/tis.c
@@ -886,11 +886,16 @@
    acpigen_write_scope(path);
    acpigen_write_device(acpi_device_name(dev));

-	acpigen_write_name("_HID");
-	acpigen_emit_eisaid("PNP0C31");
+	if (CONFIG(TPM2)) {
+		acpigen_write_name_string("_HID", "MSFT0101");
+		acpigen_write_name_string("_CID", "MSFT0101");
+	} else {
+		acpigen_write_name("_HID");
+		acpigen_emit_eisaid("PNP0C31");

-	acpigen_write_name("_CID");
-	acpigen_emit_eisaid("PNP0C31");
+		acpigen_write_name("_CID");
+		acpigen_emit_eisaid("PNP0C31");
+	}

acpi_device_write_uid(dev);

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39699 )

Change subject: drivers/pc80/tpm/tis.c: change the _HID and _CID for TPM2 device
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2154
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2153
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2152

Please note: This test is under development and might not be accurate at all!