Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38512 )
Change subject: soc/intel/skylake: Only reserve TPM area for !CONFIG_TPM_CR50 device ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3:
Patch Set 1 looks good to me btw. (unless the PCH doesn't decode it unconditionally. if it does, we should also reserve the range within coreboot).
But why increase the reservation size? I don't see
https://review.coreboot.org/c/coreboot/+/38512/3/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/38512/3/src/soc/intel/skylake/acpi/... PS3, Line 185: #if !CONFIG(TPM_CR50)
Yes, if we wanted to advertise memory-mapped TPM support. But isn't it that we advertise here what is decoded (no matter if there is a device behind it)?
For instance, if the PCH does positively decode it, the OS would have to avoid that range for resource allocation. It can only do that if we tell it to.
I couldn't find any clue in the EDS, if the decoding is conditional. Should we risk anything just to make some log look nicer?
Yes, there's currently nothing tying the actual hardware configuration to the advertised resources in ACPI currently. We should probably generate all this on the fly, tbh. I think the decoding is conditional from my recollection (but it might just be HPET that I'm thinking of).
As far as OS assignment, you are correct that the OS needs to avoid that range if the chipset is decoding it since the transaction won't be routed correctly.