Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33036 )
Change subject: sb/intel/common: Add a common interface to set final OPs settings ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
There are probably better ways to handle non-default configurations. I remember some discussion, with Stefan I guess, that there are ways to derive the settings from descriptor information.
The thing with retrofitted ports is that you sometimes just don't know what flash chips the board may ship with (or the user may have soldered to it). So without any runtime detection of the chip, this won't always work.
https://review.coreboot.org/#/c/33036/2/src/southbridge/intel/common/spi.h File src/southbridge/intel/common/spi.h:
PS2: I think the (function) names should reflect that this is about the `opmenu` or `swseq` and not the SPI controller in general.
https://review.coreboot.org/#/c/33036/2/src/southbridge/intel/common/spi.h@3... PS2, Line 36: void intel_southbridge_override_spi(struct intel_spi_config *spi_config); If this is going to be implemented by the mainboard code, it should be named accordingly, imho. e.g. mainboard_override_spi().
https://review.coreboot.org/#/c/33036/2/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/33036/2/src/southbridge/intel/common/spi.c@1... PS2, Line 1054: /* WRSR: Write Status Register */ aligning these comments would look much better, I guess