Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Paul Menzel, Eric Lai, Angel Pons, Patrick Rudolph, EricR Lai. John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62723 )
Change subject: soc/intel/common: Abstract the common TCSS functions ......................................................................
Patch Set 11:
(3 comments)
Patchset:
PS11:
Just think can we gather all the TCSS things in chip.h together. […]
It seems TCSS things in chip.h are individually categorized. i.e, tcss_d3_hot_disable and tcss_d3_cold_disable are associated with s0ix. It seems not worth to group them together which would cause more code format change.
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/62723/comment/8ed9e0b9_91e6295f PS11, Line 895: const config_t *config = config_of_soc();
can we make this outside of `switch` case itself, may be later ?
If the *config is set outside of 'switch', it would be useless if platform Kconfig does not have the SOC_INTEL_COMMON_BLOCK_TCSS at the moment. Yes, we might consider to move if further expansion functions(besides TCSS) also refer to config_of_soc().
File src/soc/intel/common/block/usb4/usb4.c:
https://review.coreboot.org/c/coreboot/+/62723/comment/3d78e08e_c10a1564 PS11, Line 34: if (config->tcss_ops.valid_tbt_auth) { : if (!config->tcss_ops.valid_tbt_auth()) : return; : }
can't we combine this both ?
It seems logically clear NOT to combine them together. If platform presents NULL function pointer, then the valid_tbt_auth() function won't be executed. Otherwise if combining, the NULL pointer function valid_tbt_auth() will be unconditionally executed.
-
John Zhao (Code Review)