Attention is currently required from: Dinesh Gehlot, Jérémy Compostella, Tim Wawrzynczak.

Patch set 1:Code-Review +1

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

View Change

23 comments:

• File common/tigerlake/hw-gfx-gma-power_and_clocks.adb:

  • Patch Set #1, 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;
    ```

  • Patch Set #1, 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);
    ```

  • Patch Set #1, Line 124: PW1

    `PW_Domain'First` (commenting to raise awareness, but marked as resolved because I suggested a different approach below)

  • Patch Set #1, Line 128: DDI_A

    `DDI_Domain'First`

  • Patch Set #1, 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)));
    ```

  • Patch Set #1, Line 132: AUX_A

    `AUX_Domain'First`

  • Patch Set #1, Line 151: FUSE_STATUS_PG0_DIST_STATUS : constant := 1 * 2 ** 27;

    nit: No need to indent this one so much, it looks weird

  • Patch Set #1, 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);
    ```

  • Patch Set #1, Line 231:

    nit: drop blank line?

  • Patch Set #1, 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;
    ```

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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?

  • Patch Set #1, 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;
    ```

  • Patch Set #1, 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?

  • Patch Set #1, Line 454: natural

    Can this be a ``Word32``?

  • Patch Set #1, 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);
    ```

  • Patch Set #1, 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

  • Patch Set #1, Line 557: 168_000_000

    Normalized CDClk can't be that low.

  • Patch Set #1, 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;
    ```

  • Patch Set #1, Line 657: Found := True;

    No need for the variable, just:

    ```
    exit;
    ```

  • Patch Set #1, Line 718:

          PD_On (DDI_A);
          PD_On (AUX_A);
    

    Why enable these in particular?

To view, visit change 82140. To unsubscribe, or for help writing mail filters, visit settings.