Attention is currently required from: Cliff Huang, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jingyuan Liang, Kapil Porwal, Kyoung Il Kim, Paul Menzel, Subrata Banik, Tarun.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81333?usp=email )
Change subject: mb/google/rex: Add Intel Touch for controller 1 for Rex ......................................................................
Patch Set 4:
(3 comments)
File src/mainboard/google/rex/variants/rex0/fw_config.c:
https://review.coreboot.org/c/coreboot/+/81333/comment/e236a133_2e14ab9a : PS4, Line 10: #if CONFIG(SOC_INTEL_TOUCH)
Why is the preprocessor magic needed?
I'd say it would make much more sense to put the "touch" stuff in some `touch.c` file, which would be conditionally built.
File src/mainboard/google/rex/variants/rex0/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/81333/comment/cbc20a30_18041c5e : PS2, Line 33: THC NOTE for P1 and P2:reworked
Please add a space after the colon.
Done
https://review.coreboot.org/c/coreboot/+/81333/comment/19da916c_3a90bfb5 : PS2, Line 84: register "thc_configuration" = "THC_CONFIG_DOUBLE_CONTROLLER"
First, the name 'thc_configuration' is chosen instead of thc_config since 'config' has been used for […]
Looks like the requirement for THC 0 to always be enabled is because of PCI: function 0 of a multi-function device always needs to exist. https://github.com/coreboot/coreboot/blob/fce08d78837431a3309d2b8705555fd81b... shows that THC 0 is function 0, whereas THC 1 is function 1; as I expected.
Given that the THCs (Touch Host Controllers?) have corresponding PCI devices, one could use the PCI devices themselves to know if a given controller is enabled. This would only require mapping ports to controllers (one port maps to only one controller). Something like this should work:
``` // In chip.h enum touch_port_to_ctrlr { TO_THC_DISABLED = 0, TO_THC_0, TO_THC_1, };
// In chip config enum touch_port_to_ctrlr port_to_thc_map[2]; ```
This would allow mapping any port to any THC, and should directly map to FSP UPDs as on TGL (UPD names may differ): https://github.com/coreboot/coreboot/blob/fce08d78837431a3309d2b8705555fd81b...
Now I wonder, would this work for Rex use-case, or do any enabled THCs require having at least one port assigned?
``` device ref thc_0 on register "port_to_thc_map[0]" = "TO_THC_DISABLED" end device ref thc_1 on register "port_to_thc_map[1]" = "TO_THC_1" end ```