Attention is currently required from: Jes Klinke, Philipp Deppenwiese, Patrick Rudolph, Christian Walter, Werner Zeh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63285 )
Change subject: Factor TI50/CR50 config ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Currently, I see both CRB_TPM and MAINBOARD_HAS_CRB_TPM in Kconfig. I am not sure how they are used differently, maybe the latter declares that the support exist, and the former can be used to actually enable it in coreboot.
Right, but I don't think there's really a point in those being split (same as for I2C and SPI). The original intent for these splits was that MAINBOARD_HAS_xxx describes the hardware truth of the mainboard, and the other option describes whether the driver is actually enabled. But this only makes sense when the Kconfigs are built to allow for a difference in those choices. You can see this work correctly in MAINBOARD_HAS_TPM2 vs TPM2/NO_TPM... the former describes what the board has and is not user-visible in menuconfig, and the latter can be independently set by the user to decide whether they actually want to enable it. But then this pattern seems to have been copied incorrectly to other areas without really understanding the logic behind this distinction... SPI_TPM/I2C_TPM/CRB_TPM are not menuconfig visible, and they are directly and solely decided by the respective MAINBOARD_HAS Kconfigs. Having two Kconfigs with a direct 1:1 relationship to each other is pointless, that might as well be a single config. So I believe merging them makes sense. (+Adding some people who might have different opinions.)
Since the MAINBOARD_HAS_xxx symbols appear a lot in many board Kconfigs, if you don't want to go through the churn of updating all of those, we could also say that we only keep the MAINBOARD_HAS_I2C_TPM/MAINBOARD_HAS_SPI_TPM and get rid of the I2C_TPM/SPI_TPM options instead? That should make for a smaller patch overall. (Although I'm not sure how much sense this makes to be honest, the option we select when we have a Winbond flash chip on the board is also just called SPI_FLASH_WINBOND, not MAINBOARD_HAS_SPI_FLASH_WINBOND. I think the MAINBOAR_HAS only makes sense if a meaningfully separate option without that exists as well.)
It will be a major refactoring, and I an worried that it will be hard for me to verify that I am not accidentally changing something unintended for an existing board.
You can never test all boards with large refactorings like this but that's no reason not to make them. This is mostly an "either it builds or it doesn't" issue, so I think with careful reviews and Jenkins we should be fine.
-
Julius Werner (Code Review)