Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34380 )
Change subject: src/drivers/intel/ptt: Add PTT Support ......................................................................
Patch Set 7:
(9 comments)
We need ptt_active before we run the first tpm2_process_command command, though after the tis_init. We need to check once, if this bit is set, so that we can use the iTPM. It is either in the Ramstage, or in the Verstage. In both stages, the MEI should be available.
Patch Set 6:
(8 comments)
place under src/drivers/intel/ptt instead ?
I agree.
Though, I'm not sure how feasible it is to make it platform independent. When do we need ptt_active()? will the MEI always be available when we need it?
You need
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/Kconfig File src/drivers/ptt/Kconfig:
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/Kconfig@1 PS6, Line 1: config HAVE_INTEL_PTT
place under src/drivers/intel/ptt instead ?
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.h File src/drivers/ptt/ptt.h:
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.h@3 PS6, Line 3: LICENSE
Where is it?
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.h@6 PS6, Line 6: #define PCI_ME_HFSTS4 0x64 : #define PTT_ENABLE (1 << 19)
Bit 19 isn't documented in any ME BWG I could find. I assume it is documented […]
It is documented in the Intel Document 560297. Removed the defines and put them into the .c file
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c File src/drivers/ptt/ptt.c:
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@6 PS6, Line 6: #include <arch/early_variables.h>
why?
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@8 PS6, Line 8: #include <device/pci_ops.h>
Missing <stdint.h> and <console/console.h>.
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@13 PS6, Line 13: int index
What is this?
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@15 PS6, Line 15: PCH_DEV_CSE
Missing NULL-check. […]
How to do the NULL Check? reg_addr could be NULL.. and the other one is a define.. I'll update the documentation.
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@27 PS6, Line 27: // Check if PTT establishment bit is valid
Comment seems redundant, especially with the debug message.
Ack
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@29 PS6, Line 29: if ((fwsts4 & PTT_ENABLE_BIT) == 0) {
If something is wrong with the ME, the MEI device might return 0xff for all […]
Ack