Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34380 )
Change subject: src/drivers/ptt: Add PTT Support ......................................................................
Patch Set 3:
(12 comments)
https://review.coreboot.org/c/coreboot/+/34380/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34380/3//COMMIT_MSG@10 PS3, Line 10: integrated TPM is enabled/active. Corrected spelling: Add function which checks if …
Better in my opinion: Add function to check if …
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/Kconfig File src/drivers/ptt/Kconfig:
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/Kconfig@1 PS3, Line 1: config INTEL_PTT Rephrase: HAVE_INTEL_PTT? ACTIVATE_INTEL_PTT?
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/Kconfig@5 PS3, Line 5: Intel Platform Trust Technology like Intel iTPM Please elaborate.
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.h File src/drivers/ptt/ptt.h:
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.h@7 PS3, Line 7: << Please add spaces around the operator. Maybe run the file through clang-format?
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.h@7 PS3, Line 7: PTT_ENABLE_BIT PTT_ENABLED? No idea, what is common in coreboot.
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.h@8 PS3, Line 8: Please document the function.
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c File src/drivers/ptt/ptt.c:
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@12 PS3, Line 12: Register register
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@13 PS3, Line 13: dump_status That’s quite generic and nothing is dumped.
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@13 PS3, Line 13: static uint32_t dump_status(int index, int reg_addr) Can this be moved to some common code?
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@17 PS3, Line 17: return reg; Just one line?
return pci_read_config32(PCH_DEV_CSE, reg_addr);
Maybe the function is not needed yet?
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@29 PS3, Line 29: // Check if PTT establishment bit is valid Please indent the comment line correctly.
https://review.coreboot.org/c/coreboot/+/34380/3/src/drivers/ptt/ptt.c@32 PS3, Line 32: Bit bit