Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 44:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformResource.h:
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 24: #define LPC_FUNCTION 0 is this still used?
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 30: SMRR_PHYSBASE_MSR why aren't those define in msr.h?
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformResource.c:
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 20: #include <southbridge/intel/common/pmutil.h> that's only correct for SOUTHBRIDGE_INTEL_COMMON_PMCLIB for other platforms this should include <soc/pm.h>
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 26: uint32_t m_tseg_base; can be removed. those are only written and read once.
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 31: uint64_t uint64; can be removed. It's only used once and the same operation can be done with simple arithmethics
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 113: #ifndef ACPI_BASE_ADDRESS please remove
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 181: Status = add_pi_resource((void *)&rsc_lpc_bridge_pci, 1); return code not checked
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 198: Status = add_pi_resource((void *)&rsc_msr_tpl, 1); return code not checked
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformSmm.c:
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 90: #define SMM_CODE_SEG 0x8 that's not the right place. The other segment defines are in src/arch/x86/include/arch/*_segs.h
https://review.coreboot.org/c/coreboot/+/33234/44/src/security/intel/stm/Stm... PS44, Line 181: load_stm_image(mseg); return code not checked