Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47653 )
Change subject: soc/intel/tigerlake: Define TCSS AUX pin bias control ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/47653/3/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/early_tcss.h:
PS3: Probably should rename the file to tcss.h since it is not just used for early_tcss support any more.
https://review.coreboot.org/c/coreboot/+/47653/3/src/soc/intel/tigerlake/inc... PS3, Line 106: 0x09000000 What is this value? And how was it determined?
https://review.coreboot.org/c/coreboot/+/47653/3/src/soc/intel/tigerlake/inc... PS3, Line 108: group I am guessing this is pad group? A comment here would be very helpful.
https://review.coreboot.org/c/coreboot/+/47653/3/src/soc/intel/tigerlake/inc... PS3, Line 108: pin Is this relative offset of the pad within the group?
https://review.coreboot.org/c/coreboot/+/47653/3/src/soc/intel/tigerlake/inc... PS3, Line 109: (((group) & 0xff) << 16) | ((pin) & 0xff)) In my opinion, rather than doing this in all macros and expecting mainboard to know group # pin #, it would be better to have clearly named fields in chip.h instead of IomTypeCPortPadConfig (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...):
Probably something like this: int tcss_bias_ctrl_pad[MAX_TCSS_PORTS];
And the mainboard only needs to set something like: register "tcss_bias_ctrl_pad[0]" = "GPP_E10" register "tcss_bias_ctrl_pad[1]" = "GPP_E13"
and the rest of the calculations to convert this into the values expected by FSP can be handled by the SoC code.
In fact, you can also make this more organized by combining the information into a structure for pad and orientation:
struct tcss_config { bool bias_control_enable; int bias_control_pad; uint8_t orientation; bool enable_internal_mux; } [MAX_TCSS_PORTS];
And mainboard can have: register "tcss_config[0]" = "{ .bias_control_enable = true, .bias_control_pad = GPP_E10, .orientation = IOM_TCSS_ORI_NORMAL, .enable_internal_mux = true, }"
And the variants that don't need this don't really need to do anything since bias_control_enable and enable_internal_mux will default to false.