Attention is currently required from: Sathyanarayana Nujella, Varshit B Pandya, Subrata Banik, Francois Toguo Fotso, Maulik V Vaghela, Paul Menzel, Angel Pons, Ronak Kanabar, Jairaj Arava, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51353 )
Change subject: soc/intel/alderlake: Update iDisp Link UPD settings ......................................................................
Patch Set 7:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51353/comment/42bf8816_b00d4c93 PS6, Line 9: These UPD values are automatically set by the FSP.
Not quite sure I understand the rational in "..common code should set UPDs as follows.." .There are no UPD settings in common code that I know of, rather they are done in fsp_param.c, which are always specific to each SOC, not common.
Used the wrong terminology. I meant "ADL SoC code" -- common from mainboard standpoint.
Additionally, the suggestion to set the default PchHdaIDispLinkTmode -> 4T also won't work. For TGL it was the default value, and may have worked. But for ADL we had to switch to 8T for it to work properly. Please see the first set of this CL.
What your latest patchset does is -- not configure the UPD at all and rely on FSP defaults. Hence my comment about setting PchHdaIDispLinkTmode in ADL SoC code in coreboot (i.e. fsp_params.c) to set whatever default is expected.
For me, it still makes sense to use the devicetree in mainboard to set the proper register values which are passed to UPDs in fsp_param.c (as originally done in patchset 1 & 2), and drop the subsequent changes (in patchset 3 onward)
That would be my first preference as well i.e. let the mainboard decide what it wants to configure here because it seems like the UPD is dependent on mainboard design. Honestly, I still don't have a clear understanding under what circumstances mainboard would choose 4T v/s 8T etc. and that is because there is no good documentation either in FSP headers or Intel datasheets to explain the impact of mainboard design on these settings. Anyways, +2 for the proposal that we should go back to patchset #2 and let mainboard configure this in devicetree.
Also, if you have any more details that you can add to describe what determines the setting of UPDs, it would be good to add this information to chip.h file.
-
Furquan Shaikh (Code Review)