Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47860 )
Change subject: soc/intel/tigerlake: Refactor TCSS port mux config ......................................................................
Patch Set 2:
(4 comments)
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
also another suggestion, could be another patch, this is already so much better than just raw constants, but would be nice to just specify GPP_A10 for example in the bias_control fields; that would mean decoding the bank and pin numbers though.
ya, i was looking for a way to decode the pin numbers. is that done on the FSP side?
I don't know of a great way, other than
if (gpio_num >= GPP_A0 && gpio_num <= GPP_A24) { bank = GPIO_BANK_A; } else if (gpio_num >= GPP_B0 && gpio_num <= GPP_B23) { bank = GPIO_BANK_B; }or the equivalent as a table + loop. I haven't looked much beyond the UPD itself TBH
We have most of the required code already present in soc/intel/common/block/gpio/gpio.c. I would recommend looking into that:
const struct pad_community *comm = gpio_get_community(pad); // pad would be what you input from the mainboard e.g. GPP_E10. int rel_offset = relative_pad_in_comm(comm, pad);
That should give you the relative offset that needs to be filled in. For the group(bank) encoding, has anyone checked with Intel how GPP_E translates to 0xe? I don't think there is any hardware register that defines this. I fear that we are leaking FSP assumptions/macros into coreboot here. Anyways, this can be handled by:
int group_idx = gpio_group_index(comm, rel_offset);
and then have a helper function that can return the bank # expected by FSP. Either you can have that implemented only for TGL now or update pad_group to add a field for fsp_bank_number. We should bring this up with FSP team as well because we cannot just keep expecting users to understand/know the internal assumptions of FSP w.r.t. macros or ids.
this is getting a bit messy. i got as far as computing the group offset, but not the group number encoding used here. i opened https://issuetracker.google.com/174116646 to get this resolved.
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/chi... PS1, Line 339: struct tcss_config {
suggestion: A little bit of documentation about using IOM_AUX_ORI_BIAS_CTRL for the bias_control* fi […]
Done
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/fsp... PS1, Line 135: TGL_LP_GPIO_ID
Can you please add a comment why this is required to be set even if there is no bias_control informa […]
Done
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/fsp... PS1, Line 139: params->TcssAuxOri |= IOM_TCSS_PORT_CTRL(i,
nit: blank line after continue
Done
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/early_tcss.h:
https://review.coreboot.org/c/coreboot/+/47860/1/src/soc/intel/tigerlake/inc... PS1, Line 102: TGL_LP_GPIO_ID
Can you please add a comment here that this is expected to be set by FSP and it represents the GPIO […]
Done
-
Caveh Jalali (Code Review)