Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/41343 )
Change subject: haswell: Make VGA on FDI work ......................................................................
Patch Set 9:
(9 comments)
Nice coding!
Just the CLKOUT_DP disable path seems odd.
https://review.coreboot.org/c/libgfxinit/+/41343/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/libgfxinit/+/41343/8//COMMIT_MSG@26 PS8, Line 26: However, this was not : the case, because one of the link ends programmed the link width for DP : unconditionally, so one end of FDI would always end up using x1 width. : The link is controlled by DDI_BUF_CTL which should have been correct already. It's the PIPE_DDI_FUNC_CTL that was wrong. Suggestion:
However, when configuring the display pipe, the wrong link width was assumed because `DP.Lane_Count` was used unconditonally instead of the `FDI.Lane_Count`.
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... File common/haswell/hw-gfx-gma-pch-lynxpoint.adb:
PS8: Nit, it doesn't need to be in `haswell/`.
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 2: -- Copyright (C) 2015-2016 secunet Security Networks AG Why?
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 38: 100 PRM says 100us how about:
TOut_MS => 1); -- 100us
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 57: Mask_Set => 16#12# * 2 ** 24); At your convenience, I wonder if this would be easier to read as a table, e.g.
is use Sideband; procedure Upd (... begin Unset_And_Set_Mask (SBI_MPHY, ...); end Upd; begin Upd (SBI_MPHY_8008, 2#1111_1111# * 2 ** 24, 2#0001_0010# * 2 ** 24); Upd (SBI_MPHY_2008, 0, 1 * 2 ** 11); Upd (SBI_MPHY_2108, 0, 1 * 2 ** 11); Upd (SBI_MPHY_206C, 0, 1 * 2 ** 18 or 1 * 2 ** 21 or 1 * 2 ** 24); Upd (SBI_MPHY_216C, 0, 1 * 2 ** 18 or 1 * 2 ** 21 or 1 * 2 ** 24); Upd (SBI_MPHY_2080, 2#111# * 2 ** 13, 2#101# * 2 ** 13); ...
Probably not. Or maybe one line per value like in the PRM? Hmmm, and let Upd() do the shifting?
Upd (SBI_MPHY_8008, 31, 24, 2#0001_0010#); Upd (SBI_MPHY_2008, 11, 11, 2#0000_0001#); Upd (SBI_MPHY_2108, 11, 11, 2#0000_0001#);
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 158: end Unbend_Clkout_Dp; As I understand it, this should only be done with the CLKOUT_DP disabled. The PRM says "Optionally follow Sequence to disable", but I would be cautious and read it as "follow Sequence to disable" if it is enabled.
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... File common/haswell/hw-gfx-gma-pch-lynxpoint.ads:
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 2: -- Copyright (C) 2015-2016 secunet Security Networks AG ???
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 18: procedure Enable_Clkout_Dp; This is more like `Enable_Clkout_DP_For_FDI_And_Configure_mPHY`. Name doesn't have to be that long but should reflect what it's doing. Or maybe just split it into two procedures?
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... File common/haswell/hw-gfx-gma-power_and_clocks_haswell.adb:
https://review.coreboot.org/c/libgfxinit/+/41343/8/common/haswell/hw-gfx-gma... PS8, Line 296: PCH.Lynxpoint.Unbend_Clkout_Dp; This is something that isn't needed after reset, right?
Then it should be in Post_All_Off() (i.e. after disabling pipes that may rely on it). Sequence could be:
procedure Post_All_Off... begin PCH.Lynxpoint.Disable_Clkout_Dp; PCH.Lynxpoint.Unbend_Clkout_Dp; end Post_All_Off;
procedure Initialize... begin ... PCH.Lynxpoint.Enable_Clkout_Dp_SSC; PCH.Lynxpoint.Configure_FDI_mPHY; end Initialize;