Attention is currently required from: Dinesh Gehlot, Jérémy Compostella, Tim Wawrzynczak.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/82140?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: gma tgl: Fill out power and clocks module ......................................................................
Patch Set 1: Code-Review+1
(23 comments)
File common/tigerlake/hw-gfx-gma-power_and_clocks.adb:
https://review.coreboot.org/c/libgfxinit/+/82140/comment/4f347914_111ca6e9 : PS1, Line 36: subtype AUX_Domain is Power_Domain range AUX_A .. AUX_USBC6; I end up using another subtype in a comment later on:
``` subtype Display_Domain is Power_Domain range AUX_A .. DDI_USBC6; ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/5ad22db5_e5b6af55 : PS1, Line 107: function Power_Domain_Type (PD : Power_Domain) return Power_Domain_Types is : (if PD in PW_Domain then Power_Well : elsif PD in DDI_Domain then Power_DDI : else Power_AUX); Ada is a powerful beast, so make use of its power! 😜
With this, the compiler will complain if a power domain is unhandled (very useful if this ever gets extended in the future):
``` function Power_Domain_Type (PD : Power_Domain) return Power_Domain_Types is (case PD is when PW_Domain'Range => Power_Well, when DDI_Domain'Range => Power_DDI, when AUX_Domain'Range => Power_AUX); ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/51ec5ede_da6fc0f2 : PS1, Line 124: PW1 `PW_Domain'First` (commenting to raise awareness, but marked as resolved because I suggested a different approach below)
https://review.coreboot.org/c/libgfxinit/+/82140/comment/bbce5270_f789e21a : PS1, Line 128: DDI_A `DDI_Domain'First`
https://review.coreboot.org/c/libgfxinit/+/82140/comment/27d613d4_90c15947 : PS1, Line 122: function PW_Index (PW : PW_Domain) return Natural : is : (Power_Domain'Pos (PW) - Power_Domain'Pos (PW1)); : : function DDI_Index (DDI : DDI_Domain) return Natural : is : (Power_Domain'Pos (DDI) - Power_Domain'Pos (DDI_A)); : : function AUX_Index (AUX : AUX_Domain) return Natural : is : (Power_Domain'Pos (AUX) - Power_Domain'Pos (AUX_A)); I'm not sure if this many functions are needed. I came up with something that should be equivalent, yet substantially less verbose:
``` function Domain_First (PD : Power_Domain) return Power_Domain is (case PD is when PW_Domain'Range => PW_Domain'First, when DDI_Domain'Range => DDI_Domain'First, when AUX_Domain'Range => AUX_Domain'First);
function Domain_Index (PD : Power_Domain) return Natural is (Power_Domain'Pos (PD) - Power_Domain'Pos (Domain_First (PD)));
function Power_Request_Mask (PD : Power_Domain) return Word32 is (2 * 2 ** (2 * Domain_Index (PD)));
function Power_State_Mask (PD : Power_Domain) return Word32 is (1 * 2 ** (2 * Domain_Index (PD))); ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/5335ef0f_08fa1f15 : PS1, Line 132: AUX_A `AUX_Domain'First`
https://review.coreboot.org/c/libgfxinit/+/82140/comment/488e8ef9_6899115f : PS1, Line 151: FUSE_STATUS_PG0_DIST_STATUS : constant := 1 * 2 ** 27; nit: No need to indent this one so much, it looks weird
https://review.coreboot.org/c/libgfxinit/+/82140/comment/274276da_4e3b51ed : PS1, Line 213: function Aux_To_Port (PD : AUX_USBC_Domain) return USBC_Port : is (case PD is : when AUX_USBC1 => DDI_TC1, : when AUX_USBC2 => DDI_TC2, : when AUX_USBC3 => DDI_TC3, : when AUX_USBC4 => DDI_TC4, : when AUX_USBC5 => DDI_TC5, : when AUX_USBC6 => DDI_TC6); nit: Format
``` function Aux_To_Port (PD : AUX_USBC_Domain) return USBC_Port is (case PD is when AUX_USBC1 => DDI_TC1, when AUX_USBC2 => DDI_TC2, when AUX_USBC3 => DDI_TC3, when AUX_USBC4 => DDI_TC4, when AUX_USBC5 => DDI_TC5, when AUX_USBC6 => DDI_TC6); ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/d7f86884_4a631811 : PS1, Line 231: nit: drop blank line?
https://review.coreboot.org/c/libgfxinit/+/82140/comment/18f65ddc_97d8b991 : PS1, Line 239: if PD in PW_Domain then : Registers.Wait_Set_Mask (Registers.FUSE_STATUS, : FUSE_STATUS_PGx_DIST_STATUS (PD)); : elsif PD in AUX_USBC_Domain then : Registers.Write (HIP_INDEX_REG (PD), HIP_INDEX_VAL (PD, 2)); : Registers.Wait_Set_Mask : (Register => DKL_CMN_UC_DW_27 (PD), : Mask => DKL_CMN_UC_DW_27_UC_HEALTH); : end if nit: perhaps be more explicit that nothing needs to be done for the other power domains? e.g. you can use a case statement, and `when others => null`
``` case PD is when PW_Domain => Registers.Wait_Set_Mask (Register => Registers.FUSE_STATUS, Mask => FUSE_STATUS_PGx_DIST_STATUS (PD)); when AUX_USBC_Domain => Registers.Write (HIP_INDEX_REG (PD), HIP_INDEX_VAL (PD, 2)); Registers.Wait_Set_Mask (Register => DKL_CMN_UC_DW_27 (PD), Mask => DKL_CMN_UC_DW_27_UC_HEALTH); when others => null; end case; ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/2a64e3dc_77ee455d : PS1, Line 263: (Register => PWR_CTL_DRIVER (PD_Type), Do we use the ``PWR_CTL_DRIVER`` register for any particular reason? If libgfxinit does the functions of a VBIOS/GOP driver, maybe we could use `PWR_CTL_BIOS`?
Not critical, just wanted to share some thoughts.
https://review.coreboot.org/c/libgfxinit/+/82140/comment/31b1ab49_1331a0b7 : PS1, Line 275: Debug.Put_Line ("Failed to enable power domain!"); Would be nice to print the power domain in question, but unfortunately `'Image` doesn't seem to work when building libgfxinit for coreboot. Maybe I should look into it.
https://review.coreboot.org/c/libgfxinit/+/82140/comment/87d65a37_8f6ef992 : PS1, Line 306: (DP1, DP2, DP3, HDMI1, HDMI2, HDMI3); Should `eDP` be in here, or does it get handled separately? I think Intel optimised the power domains back in Haswell times, so that machines (laptops) can save power when only the integrated LCD is active. So probably not.
https://review.coreboot.org/c/libgfxinit/+/82140/comment/90ab8021_2d793ff6 : PS1, Line 331: function Num_Active_Pipes return Natural is : Count : Natural := 0; : begin : for I in Pipe_Index loop : if Configs (I).Port /= Disabled then : Count := Count + 1; : end if; : end loop; : return Count; : end Num_Active_Pipes Is this used?
https://review.coreboot.org/c/libgfxinit/+/82140/comment/e5c7a9a7_e5dfff2d : PS1, Line 312: function Need_PD (PD : Power_Domain; Configs : Pipe_Configs) return Boolean : is : function Any_Port_Is (Port : Active_Port_Type) return Boolean is : (Configs (Primary).Port = Port or Configs (Secondary).Port = Port or : Configs (Tertiary).Port = Port); : : function Any_Port_In (Ports : Port_Array) return Boolean is : begin : for P of Ports loop : if (Configs (Primary).Port = P) or : (Configs (Secondary).Port = P) or : (Configs (Tertiary).Port = P) : then : return True; : end if; : end loop; : return False; : end Any_Port_In; : : function Num_Active_Pipes return Natural is : Count : Natural := 0; : begin : for I in Pipe_Index loop : if Configs (I).Port /= Disabled then : Count := Count + 1; : end if; : end loop; : return Count; : end Num_Active_Pipes; : begin : case PD is : when PW1 | PW2 => : return Configs (Primary).Port /= Disabled or Any_Port_In (Conventional_Ports); : when PW3 => : return Configs (Secondary).Port /= Disabled or Any_Port_In (TC_Ports); : when PW4 => : return Configs (Tertiary).Port /= Disabled; : when PW5 => : return False; : when DDI_A | AUX_A => : return Any_Port_Is (DP1) or Any_Port_Is (HDMI1) or Any_Port_Is (eDP); : when DDI_B | AUX_B => : return Any_Port_Is (DP2) or Any_Port_Is (HDMI2); : when DDI_C | AUX_C => : return Any_Port_Is (DP3) or Any_Port_Is (HDMI3); : when DDI_USBC1 | AUX_USBC1 => : return Any_Port_Is (USBC1_DP) or Any_Port_Is (USBC1_HDMI); : when DDI_USBC2 | AUX_USBC2 => : return Any_Port_Is (USBC2_DP) or Any_Port_Is (USBC2_HDMI); : when DDI_USBC3 | AUX_USBC3 => : return Any_Port_Is (USBC3_DP) or Any_Port_Is (USBC3_HDMI); : when DDI_USBC4 | AUX_USBC4 => : return Any_Port_Is (USBC4_DP) or Any_Port_Is (USBC4_HDMI); : when DDI_USBC5 | AUX_USBC5 => : return Any_Port_Is (USBC5_DP) or Any_Port_Is (USBC5_HDMI); : when DDI_USBC6 | AUX_USBC6 => : return Any_Port_Is (USBC6_DP) or Any_Port_Is (USBC6_HDMI); : end case; : end Need_PD; I felt this function had potential to be less verbose, and decided to reimplement it:
``` function Need_PD (PD : Power_Domain; Configs : Pipe_Configs) return Boolean is generic type T is new Port_Type; First : Port_Type := Port_Type (T'First); Last : Port_Type := Port_Type (T'Last); function Any_Port_Of return Boolean; function Any_Port_Of return Boolean is (Configs (Primary).Port in First .. Last or else Configs (Secondary).Port in First .. Last or else Configs (Tertiary).Port in First .. Last);
function Any_Port_Combo is new Any_Port_Of (T => Combo_Port_Type); function Any_Port_USBC is new Any_Port_Of (T => USBC_Port_Type);
function Is_Pipe_Active (Pipe : Pipe_Index) return Boolean is (Configs (Pipe).Port /= Disabled);
function PW_Need_PD (PD : PW_Domain) return Boolean is (case PD is when PW1 | PW2 => Is_Pipe_Active (Primary) or else Any_Port_Combo, when PW3 => Is_Pipe_Active (Secondary) or else Any_Port_USBC, when PW4 => Is_Pipe_Active (Tertiary), when PW5 => False);
function Port_Need_PD (PD : Display_Domain; Port : Port_Type) return Boolean is (case PD is when DDI_A | AUX_A => Port in DP1 | HDMI1 | eDP, when DDI_B | AUX_B => Port in DP2 | HDMI2, when DDI_C | AUX_C => Port in DP3 | HDMI3, when DDI_USBC1 | AUX_USBC1 => Port in USBC1_DP | USBC1_HDMI, when DDI_USBC2 | AUX_USBC2 => Port in USBC2_DP | USBC2_HDMI, when DDI_USBC3 | AUX_USBC3 => Port in USBC3_DP | USBC3_HDMI, when DDI_USBC4 | AUX_USBC4 => Port in USBC4_DP | USBC4_HDMI, when DDI_USBC5 | AUX_USBC5 => Port in USBC5_DP | USBC5_HDMI, when DDI_USBC6 | AUX_USBC6 => Port in USBC6_DP | USBC6_HDMI);
function Display_Need_PD (PD : Display_Domain) return Boolean is (Port_Need_PD (PD, Configs (Primary).Port) or else Port_Need_PD (PD, Configs (Secondary).Port) or else Port_Need_PD (PD, Configs (Tertiary).Port)); begin return (case PD is when PW_Domain => PW_Need_PD (PD), when Display_Domain => Display_Need_PD (PD)); end Need_PD; ```
This uses a new subtype (see comment towards the start of this file):
``` subtype Display_Domain is Power_Domain range AUX_A .. DDI_USBC6; ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/3bdd4572_dc956702 : PS1, Line 416: procedure Normalize_CDClk (CDClk : in Int64; Normalized: out Frequency_Type) : with : Post => Normalized >= 172_800_000 : is : Refclk_Freq : Frequency_Type; : begin : Get_Refclk (Refclk_Freq); : Normalized := : (case Refclk_Freq is : when 19_200_000 | 38_400_000 => : (if CDClk <= 172_800_000 then 172_800_000 : elsif CDClk <= 192_000_000 then 192_000_000 : elsif CDClk <= 307_200_000 then 307_200_000 : elsif CDClk <= 326_400_000 then 326_400_000 : elsif CDClk <= 556_800_000 then 556_800_000 : else 652_800_000), : when others => : (if CDClk <= 180_000_000 then 180_000_000 : elsif CDClk <= 192_000_000 then 192_000_000 : elsif CDClk <= 312_000_000 then 312_000_000 : elsif CDClk <= 324_000_000 then 324_000_000 : elsif CDClk <= 552_000_000 then 552_000_000 : else 648_000_000)); Where is this described?
https://review.coreboot.org/c/libgfxinit/+/82140/comment/8a52de50_6ec04954 : PS1, Line 454: natural Can this be a ``Word32``?
https://review.coreboot.org/c/libgfxinit/+/82140/comment/adc82d60_8a025ba9 : PS1, Line 455: function Ratio_For_19_2_MHz (CDCLk : Frequency_Type) return PLL_Ratio_Range is : begin : case CDClk is : when 172_800_000 => return 18; : when 192_000_000 => return 20; : when 307_200_000 => return 32; : when 326_400_000 | 652_800_000 => return 68; : when 556_800_000 => return 58; : when others => return 0; : end case; : end Ratio_For_19_2_MHz; nit: Use a case expression
``` function Ratio_For_19_2_MHz (CDCLk : Frequency_Type) return PLL_Ratio_Range is (case CDClk is when 172_800_000 => 18, when 192_000_000 => 20, when 307_200_000 => 32, when 326_400_000 | 652_800_000 => 68, when 556_800_000 => 58, when others => 0); ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/2ace25b7_cf887fd3 : PS1, Line 467: function Ratio_For_24_MHz (CDCLk : Frequency_Type) return PLL_Ratio_Range is : begin : case CDClk is : when 180_800_000 => return 15; : when 192_000_000 => return 16; : when 312_000_000 => return 26; : when 324_000_000 | 648_000_000 => return 54; : when 552_000_000 => return 46; : when others => return 0; : end case; : end Ratio_For_24_MHz; Same as above
https://review.coreboot.org/c/libgfxinit/+/82140/comment/da114c20_8e51cb30 : PS1, Line 557: 168_000_000 Normalized CDClk can't be that low.
https://review.coreboot.org/c/libgfxinit/+/82140/comment/5a3f7429_4c6695e9 : PS1, Line 554: Registers.Write : (Register => Registers.CDCLK_CTL, : Value => (case CDClk is : when 168_000_000 => 16#14e#, : when 172_800_000 => 16#158#, : when 179_200_000 => 16#164#, : when 180_000_000 => 16#166#, : when 192_000_000 => 16#17e#, : when 307_200_000 => 16#264#, : when 312_000_000 => 16#26e#, : when 324_000_000 => 16#286#, : when 326_400_000 => 16#28b#, : when 480_000_000 => 16#3be#, : when 552_000_000 => 16#44e#, : when 556_800_000 => 16#458#, : when 648_000_000 => 16#50e#, : when 652_800_000 => 16#518#, : when others => CDCLK_CTL_CD_FREQ_DECIMAL (CDClk)) or : CDCLK_CD2X_PIPE_NONE or : CD2X); This table of hardcoded values feels a bit unnecessary, `CDCLK_CTL_CD_FREQ_DECIMAL` returns the same values:
``` CDClk Table FREQ_DECIMAL 168000000 334 334 172800000 344 344 179200000 356 356 180000000 358 358 192000000 382 382 307200000 612 612 312000000 622 622 324000000 646 646 326400000 651 651 480000000 958 958 552000000 1102 1102 556800000 1112 1112 648000000 1294 1294 652800000 1304 1304 ```
For reference, this is how I checked:
``` with Ada.Text_IO; with Interfaces;
procedure Main is subtype Word32 is Interfaces.Unsigned_32; use type Word32;
subtype Int64 is Interfaces.Integer_64; use type Int64; function Div_Round_Closest (N, M : Int64) return Int64 is ((N + M / 2) / M);
function CDCLK_CTL_CD_FREQ_DECIMAL (Freq : Int64) return Word32 with Pre => Freq > 1_000_000 is begin -- Weirdest representation: CDClk - 1MHz in 10.1 (10 + 1 fractional bit) return Word32 (Div_Round_Closest (Freq - 1_000_000, 500_000)); end CDCLK_CTL_CD_FREQ_DECIMAL;
CDCLK_CD2X_PIPE_NONE : constant := 7 * 2 ** 19; CDCLK_CD2X_DIV_SEL_1 : constant := 0 * 2 ** 22; CDCLK_CD2X_DIV_SEL_2 : constant := 2 * 2 ** 22;
function Get_CDClk_Ctl (CDClk : Int64; CD2X : Word32) return Word32 is (case CDClk is when 168_000_000 => 16#14e#, when 172_800_000 => 16#158#, when 179_200_000 => 16#164#, when 180_000_000 => 16#166#, when 192_000_000 => 16#17e#, when 307_200_000 => 16#264#, when 312_000_000 => 16#26e#, when 324_000_000 => 16#286#, when 326_400_000 => 16#28b#, when 480_000_000 => 16#3be#, when 552_000_000 => 16#44e#, when 556_800_000 => 16#458#, when 648_000_000 => 16#50e#, when 652_800_000 => 16#518#, when others => CDCLK_CTL_CD_FREQ_DECIMAL (CDClk) or CDCLK_CD2X_PIPE_NONE or CD2X);
function Get_CDClk_Ctl_Alt (CDClk : Int64; CD2X : Word32) return Word32 is (CDCLK_CTL_CD_FREQ_DECIMAL (CDClk));
type CDClk_Array is array (Natural range <>) of Int64; CDClk_Table : constant CDClk_Array := (168_000_000, 172_800_000, 179_200_000, 180_000_000, 192_000_000, 307_200_000, 312_000_000, 324_000_000, 326_400_000, 480_000_000, 552_000_000, 556_800_000, 648_000_000, 652_800_000); begin Ada.Text_IO.Put(" CDClk"); Ada.Text_IO.Set_Col(12); Ada.Text_IO.Put(" Table"); Ada.Text_IO.Set_Col(20); Ada.Text_IO.Put(" FREQ_DECIMAL"); Ada.Text_IO.New_Line; for CDClk of CDClk_Table loop Ada.Text_IO.Put(CDClk'Image); Ada.Text_IO.Set_Col(12); Ada.Text_IO.Put(Get_CDClk_Ctl (CDClk, CDCLK_CD2X_DIV_SEL_1)'Image); Ada.Text_IO.Set_Col(20); Ada.Text_IO.Put(Get_CDClk_Ctl_Alt (CDClk, CDCLK_CD2X_DIV_SEL_1)'Image); Ada.Text_IO.New_Line; end loop; end Main; ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/50c6127c_4ce05089 : PS1, Line 657: Found := True; No need for the variable, just:
``` exit; ```
https://review.coreboot.org/c/libgfxinit/+/82140/comment/316ae369_f21fed15 : PS1, Line 718: PD_On (DDI_A); : PD_On (AUX_A); Why enable these in particular?