Hello Arthur Heymans, Matt DeVillier, Thomas Heijligen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/libgfxinit/+/35714
to review the following change.
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
gma config: Allow to cache CDClk in variable config state
Add `CDClk` and `Max_CDClk` fields to the variable config state. They will be used to cache the current hardware state, so that we don't have to poke registers in every call to Update_Outputs().
Also introduce `CDClk_Range`, which is chosen such that common dot-clock limits (90% of CDClk) are in range of `Frequency_Type`.
Change-Id: I3d9c956eabcedb2f8299a1642ff0172cd7f54e45 Signed-off-by: Nico Huber nico.h@gmx.de --- M common/hw-gfx-gma-config.ads.template M common/hw-gfx-gma.ads 2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/14/35714/1
diff --git a/common/hw-gfx-gma-config.ads.template b/common/hw-gfx-gma-config.ads.template index d5bd2ba..bbfdc05 100644 --- a/common/hw-gfx-gma-config.ads.template +++ b/common/hw-gfx-gma-config.ads.template @@ -60,6 +60,8 @@ type Valid_Port_Array is array (Port_Type) of Boolean; type Variable_Config is record Valid_Port : Valid_Port_Array; + CDClk : CDClk_Range; + Max_CDClk : CDClk_Range; Raw_Clock : Frequency_Type; Dyn_CPU : Gen_CPU_Type; Dyn_CPU_Var : Gen_CPU_Variant; @@ -67,6 +69,8 @@
Initial_Settings : constant Variable_Config := (Valid_Port => (others => False), + CDClk => CDClk_Range'First, + Max_CDClk => CDClk_Range'First, Raw_Clock => Frequency_Type'First, Dyn_CPU => Gen_CPU_Type'First, Dyn_CPU_Var => Gen_CPU_Variant'First); @@ -74,6 +78,8 @@ Variable : Variable_Config with Part_Of => GMA.State;
Valid_Port : Valid_Port_Array renames Variable.Valid_Port; + CDClk : CDClk_Range renames Variable.CDClk; + Max_CDClk : CDClk_Range renames Variable.Max_CDClk; Raw_Clock : Frequency_Type renames Variable.Raw_Clock; CPU : Gen_CPU_Type renames Variable.Dyn_CPU; CPU_Var : Gen_CPU_Variant renames Variable.Dyn_CPU_Var; @@ -271,7 +277,7 @@
----------------------------------------------------------------------------
- Default_CDClk_Freq : <ilkhswvar> Frequency_Type := + Default_CDClk_Freq : <ilkhswvar> CDClk_Range := (if Gen_G45 then 320_000_000 -- unused elsif CPU_Ironlake then 450_000_000 elsif CPU_Sandybridge or CPU_Ivybridge then 400_000_000 @@ -279,7 +285,7 @@ elsif Gen_Haswell then 450_000_000 elsif Gen_Broxton then 288_000_000 elsif Gen_Skylake then 337_500_000 - else Frequency_Type'First); + else CDClk_Range'First);
Default_RawClk_Freq : <hswvar> Frequency_Type := (if Gen_G45 then 100_000_000 -- unused, depends on FSB diff --git a/common/hw-gfx-gma.ads b/common/hw-gfx-gma.ads index bdc4704..69df07d 100644 --- a/common/hw-gfx-gma.ads +++ b/common/hw-gfx-gma.ads @@ -206,6 +206,13 @@
private
+ -- Older Generations only allow dot clocks <= 90% of the CDClk. + -- Hence, to calculate the limit, CDClk must start higher. + subtype CDClk_Range is Frequency_Type + range Frequency_Type'First * 10 / 9 + 1 .. Frequency_Type'Last; + + ---------------------------------------------------------------------------- + PCI_Usable : Boolean with Part_Of => State; use type HW.PCI.Index; procedure PCI_Read16 (Value : out Word16; Offset : HW.PCI.Index)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
Patch Set 1: Verified+1
Hello Arthur Heymans, Matt DeVillier, Thomas Heijligen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/libgfxinit/+/35714
to look at the new patch set (#2).
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
gma config: Allow to cache CDClk in variable config state
Add `CDClk` and `Max_CDClk` fields to the variable config state. They will be used to cache the current hardware state, so that we don't have to poke registers in every call to Update_Outputs().
Also introduce `CDClk_Range`, which is chosen such that common dot-clock limits (90% of CDClk) are in range of `Frequency_Type`.
Change-Id: I3d9c956eabcedb2f8299a1642ff0172cd7f54e45 Signed-off-by: Nico Huber nico.h@gmx.de --- M common/hw-gfx-gma-config.ads.template 1 file changed, 18 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/14/35714/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
Patch Set 2: Verified+1
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
Patch Set 3: Code-Review+1
tested on Purism Librem 15v4 w/4k display
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/libgfxinit/+/35714 )
Change subject: gma config: Allow to cache CDClk in variable config state ......................................................................
gma config: Allow to cache CDClk in variable config state
Add `CDClk` and `Max_CDClk` fields to the variable config state. They will be used to cache the current hardware state, so that we don't have to poke registers in every call to Update_Outputs().
Also introduce `CDClk_Range`, which is chosen such that common dot-clock limits (90% of CDClk) are in range of `Frequency_Type`.
Change-Id: I3d9c956eabcedb2f8299a1642ff0172cd7f54e45 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/libgfxinit/+/35714 Reviewed-by: Matt DeVillier matt.devillier@gmail.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Angel Pons th3fanbus@gmail.com --- M common/hw-gfx-gma-config.ads.template 1 file changed, 18 insertions(+), 2 deletions(-)
Approvals: Nico Huber: Verified Matt DeVillier: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/common/hw-gfx-gma-config.ads.template b/common/hw-gfx-gma-config.ads.template index e4e557a..f7312fe 100644 --- a/common/hw-gfx-gma-config.ads.template +++ b/common/hw-gfx-gma-config.ads.template @@ -57,9 +57,21 @@
----------------------------------------------------------------------------
+ -- On older generations dot clocks are limited to 90% of + -- the CDClk rate. To ease proofs, we limit CDClk's range. + CDClk_Min : constant Frequency_Type := + (case Gen is + when G45 .. Ironlake => Frequency_Type'First * 100 / 90 + 1, + when others => Frequency_Type'First); + subtype CDClk_Range is Frequency_Type range CDClk_Min .. Frequency_Type'Last; + + ---------------------------------------------------------------------------- + type Valid_Port_Array is array (Port_Type) of Boolean; type Variable_Config is record Valid_Port : Valid_Port_Array; + CDClk : CDClk_Range; + Max_CDClk : CDClk_Range; Raw_Clock : Frequency_Type; Dyn_CPU : Gen_CPU_Type; Dyn_CPU_Var : Gen_CPU_Variant; @@ -67,6 +79,8 @@
Initial_Settings : constant Variable_Config := (Valid_Port => (others => False), + CDClk => CDClk_Range'First, + Max_CDClk => CDClk_Range'First, Raw_Clock => Frequency_Type'First, Dyn_CPU => Gen_CPU_Type'First, Dyn_CPU_Var => Gen_CPU_Variant'First); @@ -74,6 +88,8 @@ Variable : Variable_Config with Part_Of => GMA.State;
Valid_Port : Valid_Port_Array renames Variable.Valid_Port; + CDClk : CDClk_Range renames Variable.CDClk; + Max_CDClk : CDClk_Range renames Variable.Max_CDClk; Raw_Clock : Frequency_Type renames Variable.Raw_Clock; CPU : Gen_CPU_Type renames Variable.Dyn_CPU; CPU_Var : Gen_CPU_Variant renames Variable.Dyn_CPU_Var; @@ -271,7 +287,7 @@
----------------------------------------------------------------------------
- Default_CDClk_Freq : <ilkhswvar> Frequency_Type := + Default_CDClk_Freq : <ilkhswvar> CDClk_Range := (if Gen_G45 then 320_000_000 -- unused elsif CPU_Ironlake then 450_000_000 elsif CPU_Sandybridge or CPU_Ivybridge then 400_000_000 @@ -279,7 +295,7 @@ elsif Gen_Haswell then 450_000_000 elsif Gen_Broxton then 288_000_000 elsif Gen_Skylake then 337_500_000 - else Frequency_Type'First); + else CDClk_Range'First);
Default_RawClk_Freq : <hswvar> Frequency_Type := (if Gen_G45 then 100_000_000 -- unused, depends on FSB
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)