Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34380 )
Change subject: src/drivers/ptt: Add PTT Support ......................................................................
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?
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?
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 somewhere else? Is it explicitly documented to be platform-agnostic?
And why are these definitions in a header 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?
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>.
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@13 PS6, Line 13: int index What is this?
https://review.coreboot.org/c/coreboot/+/34380/6/src/drivers/ptt/ptt.c@15 PS6, Line 15: PCH_DEV_CSE Missing NULL-check.
As this device may be hidden, it would be wise to document in the API when (in which phases of coreboot) ptt_active() can be called.
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.
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 reads. So this needs some check first if the ME interface is up at all.