[coreboot-gerrit] [L] Change in libgfxinit[cros]: gma tgl: Add support for allocating PLLs

[Angel Pons (Code Review)]

Attention is currently required from: Dinesh Gehlot, Tim Wawrzynczak.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/82141?usp=email )

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

Change subject: gma tgl: Add support for allocating PLLs
......................................................................

Patch Set 1: Code-Review+1

(17 comments)

File common/tigerlake/hw-gfx-gma-plls-combo_phy.adb:

https://review.coreboot.org/c/libgfxinit/+/82141/comment/fdf1d67d_3ad58003 :
PS1, Line 61: natural 
nit: `Natural`

https://review.coreboot.org/c/libgfxinit/+/82141/comment/ff334da9_7aaba4b3 :
PS1, Line 133:       subtype Candidate_Index is Positive range 1 .. 46;
You don't need this type at all. Declare the array type as follows:

```
type Candidate_Array is array (Positive range <>) of Div_Range;
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/5e03bdd8_b87a4d7b :
PS1, Line 136: 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 24, 28, 30, 32, 36, 40, 42, 44,
             :          48, 50, 52, 54, 56, 60, 64, 66, 68, 70, 72, 76, 78, 80, 84, 88, 90,
             :          92, 96, 98, 100, 102, 3, 5, 7, 9, 15, 21
That's a really odd table... Ah, it's in https://github.com/torvalds/linux/blob/f03359bca01bf4372cf2c118cd9a987a5951b...

https://review.coreboot.org/c/libgfxinit/+/82141/comment/79be9573_e41244f7 :
PS1, Line 158: for Index in Candidate_Index loop
```
for Div in Candidates loop
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/c65f16a4_6168cade :
PS1, Line 162: Candidates(Index)
```
Div
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/b1ddd50b_3ad37720 :
PS1, Line 170: Best_Div_Index := Index
```
Best_Div := Div;
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/d64fe367_5e9d14d6 :
PS1, Line 181:       if Best_Div mod 2 = 0 then
https://github.com/torvalds/linux/blob/f03359bca01bf4372cf2c118cd9a987a5951b...

https://review.coreboot.org/c/libgfxinit/+/82141/comment/81360f29_6058fbc8 :
PS1, Line 182:          pragma Assert (Best_Div > 1);
Already guaranteed by the type system:

```
subtype Div_Range is Pos64 range 2 .. 102;
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/bded7c6b_d8a2beea :
PS1, Line 215:       Params.KDiv := (case KDiv is
             :                       when      1 => 1,
             :                       when      2 => 2,
             :                       when      3 => 4,
             :                       when others => 1);
             :       Params.PDiv := (case PDiv is
             :                       when 2 => 1,
             :                       when 3 => 2,
             :                       when 5 => 4,
             :                       when 7 => 8,
             :                       when others => 1);
i915 warns if the values aren't sane:

https://github.com/torvalds/linux/blob/f03359bca01bf4372cf2c118cd9a987a5951b...

https://review.coreboot.org/c/libgfxinit/+/82141/comment/c78aaa9e_026068d7 :
PS1, Line 281: Shift_Right (Params.DCO_Fraction, 1)
Wouldn't `Params.DCO_Fraction / 2` work too?

File common/tigerlake/hw-gfx-gma-plls-dkl.adb:

PS1: 
Would it be possible to rename this to `dkl_phy` (or `dekel_phy`) for consistency with `combo_phy`? Or is there more non-PHY DKL (Dekel) stuff?

File common/tigerlake/hw-gfx-gma-plls.adb:

https://review.coreboot.org/c/libgfxinit/+/82141/comment/697001c6_41f9e42b :
PS1, Line 29: DIGI_A | DIGI_B | DIGI_C
`Combo_Port`

https://review.coreboot.org/c/libgfxinit/+/82141/comment/04adfe07_322b5a4b :
PS1, Line 30: DDI_TC1 | DDI_TC2 | DDI_TC3 | DDI_TC4 | DDI_TC5 | DDI_TC6
`USBC_Port`

https://review.coreboot.org/c/libgfxinit/+/82141/comment/99f3f42a_525ed884 :
PS1, Line 67: function Port_PLL_Match (Port : GPU_Port; PLL : T) return Boolean
            :       is
            :       begin
            :          if Port in Combo_Port and PLL in Combo_DPLLs then
            :             return True;
            :          elsif Port in USBC_Port and PLL in DKL_DPLLs then
            :             if (Port = DDI_TC1 and PLL = TCPLL1) or
            :                (Port = DDI_TC2 and PLL = TCPLL2) or
            :                (Port = DDI_TC3 and PLL = TCPLL3) or
            :                (Port = DDI_TC4 and PLL = TCPLL4) or
            :                (Port = DDI_TC5 and PLL = TCPLL5) or
            :                (Port = DDI_TC6 and PLL = TCPLL6)
            :             then
            :                return True;
            :             end if;
            :          end if;
            : 
            :          return False;
            :       end Port_PLL_Match;
```
      function Port_PLL_Match (Port : GPU_Port; PLL : T) return Boolean is
        (case Port in
         when Combo_Port => PLL in Combo_DPLLs,
         when DDI_TC1    => PLL = TCPLL1,
         when DDI_TC2    => PLL = TCPLL2,
         when DDI_TC3    => PLL = TCPLL3,
         when DDI_TC4    => PLL = TCPLL4,
         when DDI_TC5    => PLL = TCPLL5,
         when DDI_TC6    => PLL = TCPLL6,
         when others     => False);
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/31f3478c_873bd3c2 :
PS1, Line 113: if P in Combo_DPLLs then
             :                Combo_Phy.On (PLL, Port_Cfg, Success);
             :             else--if P in DKL_DPLLs then
             :                DKL.On (PLL, Port_Cfg, Success);
             :             end if;
```
case P is
   when Combo_DPLLs => Combo_Phy.On (PLL, Port_Cfg, Success);
   when DKL_DPLLs   => DKL.On (PLL, Port_Cfg, Success);
end case; 
```

https://review.coreboot.org/c/libgfxinit/+/82141/comment/8c122160_cc67ba16 :
PS1, Line 142:       if PLL in Combo_DPLLs then
             :          Combo_Phy.Free (PLL);
             :       elsif PLL in DKL_DPLLs then
             :          DKL.Free (PLL);
             :       end if;
```
case P is
   when Combo_DPLLs => Combo_Phy.Free (PLL);
   when DKL_DPLLs   => DKL.Free (PLL);
   when Invalid_PLL => null;
end case; 
```

File common/tigerlake/hw-gfx-gma-plls.ads:

https://review.coreboot.org/c/libgfxinit/+/82141/comment/cedee209_7981f368 :
PS1, Line 22: subtype Configurable_DPLLs is T range DPLL0 .. TCPLL6;
```
subtype Configurable_DPLLs is T range T'Succ (Invalid_PLL) .. T'Last;
```