Change in coreboot[master]: soc/intel/common/block: Move smihandler common functions into common ...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/26138 )
Change subject: soc/intel/common/block: Move smihandler common functions into common code ......................................................................
Patch Set 42:
(2 comments)
https://review.coreboot.org/c/coreboot/+/26138/41/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/26138/41/src/soc/intel/common/block... PS41, Line 70: tco_sts
So, there is a slight change in behavior now for APL. Previously, this function would do nothing, but now: a) It checks bit 8 in tco_sts, but that bit is marked reserved. As per EDS, it is RO and reads as 0, so I am guessing this has
no side-effects?
i don't think this big is RO. its RW/1C, As per EDS
BIOSWR_STS: Intel PCH sets this bit to 1 and generates an SMI# to indicate an illegal attempt to write to the BIOS located in the FWH that is accessed over the LPC. This occurs when either: a) The BIOSWP bit is changed from 0 to 1 and the LE bit is also set, or b) Any write is attempted to the BIOS and the BIOSWP bit is also set. This bit doesn’t get set to 1 when: 1) a or b above occurs on eSPI controller. 2) a or b above occurs on SPI Flash controller. Note: On write cycles attempted to the 4MB lower alias to the BIOS space, the BIOSWR_STS bit will not be set.
b) But, CONFIG(SPI_FLASH_SMM) could be true and so now this would always end up calling fast_spi_enable_wp(). Are you changing this intentionally?
i don't think this function is getting called unconditionally for APL/GLK, i can double confirm the same
https://review.coreboot.org/c/coreboot/+/26138/41/src/soc/intel/common/block... PS41, Line 499:
nit: extra blank line not required.
isn't this fixed at latest patch set ?
-
Subrata Banik (Code Review)