Hello Arthur Heymans, Matt DeVillier, Thomas Heijligen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/libgfxinit/+/35710
to review the following change.
Change subject: gma pcode: Move and revise mailbox handling ......................................................................
gma pcode: Move and revise mailbox handling
Unify the PCODE mailbox implementations (Linux' i915 uses the same implementation since Sandy Bridge, too) and add `Wait` and `Success` parameters so we can act correctly if something failed. This adds a lot of boilerplate. But we keep it contained in the new package `PCode` and the code outside it looks cleaner and handles errors more gracefully.
In GNATprove, we track state of the mailbox' readiness in a ghost variable `Mailbox_Ready`. This allows us to skip the initial wait loop if we know that we already waited at the end of a previous call. The first call to a mailbox procedure has to be made with `Wait_Ready => True`.
Also, start to experiment with a `use` clause for the `Register` package. It allows us to write a little more condensed code, with- out sacrificing much (in this program we can expect that `Read`/ `Write` means register access?) So far it looks good?
Change-Id: I5daa3effb7ab774e4a35bd8794b0f67f57e4caa4 Signed-off-by: Nico Huber nico.h@gmx.de --- M common/Makefile.inc M common/broxton/hw-gfx-gma-power_and_clocks.adb M common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.adb M common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.ads A common/hw-gfx-gma-pcode.adb A common/hw-gfx-gma-pcode.ads M common/hw-gfx-gma.adb M common/skylake/hw-gfx-gma-power_and_clocks_skylake.adb 8 files changed, 234 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/10/35710/1
diff --git a/common/Makefile.inc b/common/Makefile.inc index 7c5403b..67b6c17 100644 --- a/common/Makefile.inc +++ b/common/Makefile.inc @@ -31,6 +31,8 @@ gfxinit-y += hw-gfx-gma-pch-vga.adb gfxinit-y += hw-gfx-gma-pch-vga.ads gfxinit-y += hw-gfx-gma-pch.ads +gfxinit-y += hw-gfx-gma-pcode.adb +gfxinit-y += hw-gfx-gma-pcode.ads gfxinit-y += hw-gfx-gma-pipe_setup.adb gfxinit-y += hw-gfx-gma-pipe_setup.ads gfxinit-y += hw-gfx-gma-port_detect.ads diff --git a/common/broxton/hw-gfx-gma-power_and_clocks.adb b/common/broxton/hw-gfx-gma-power_and_clocks.adb index f6576d9..77f6b35 100644 --- a/common/broxton/hw-gfx-gma-power_and_clocks.adb +++ b/common/broxton/hw-gfx-gma-power_and_clocks.adb @@ -17,6 +17,7 @@ with HW.Debug; with HW.GFX.GMA.Config; with HW.GFX.GMA.Registers; +with HW.GFX.GMA.PCode; with HW.GFX.GMA.Power_And_Clocks_Haswell; with HW.GFX.GMA.DDI_Phy;
@@ -242,10 +243,19 @@ when others => CDCLK_CD2X_DIV_SEL_1); -- for CDClk = CDClk_Ref CDCLK_CD2X_SSA_Precharge : constant Word32 := (if Freq >= 500_000_000 then CDCLK_CD2X_SSA_PRECHARGE_ENABLE else 0); + + Success : Boolean; begin - Power_And_Clocks_Haswell.GT_Mailbox_Write + PCode.Mailbox_Write (MBox => BXT_PCODE_CDCLK_CONTROL, - Value => BXT_CDCLK_PREPARE_FOR_CHANGE); + Command => BXT_CDCLK_PREPARE_FOR_CHANGE, + Wait_Ready => True, + Success => Success); + if not Success then + pragma Debug (Debug.Put_Line + ("ERROR: PCODE didn't acknowledge frequency change.")); + return; + end if;
Write (Register => BXT_DE_PLL_ENABLE, @@ -274,9 +284,9 @@ CDCLK_CD2X_SSA_Precharge or CDCLK_CTL_CD_FREQ_DECIMAL (Freq));
- Power_And_Clocks_Haswell.GT_Mailbox_Write - (MBox => BXT_PCODE_CDCLK_CONTROL, - Value => Word32 ((Freq + (25_000_000 - 1)) / 25_000_000)); + PCode.Mailbox_Write + (MBox => BXT_PCODE_CDCLK_CONTROL, + Command => Word64 ((Freq + (25_000_000 - 1)) / 25_000_000)); end Set_CDClk;
---------------------------------------------------------------------------- diff --git a/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.adb b/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.adb index 1c23254..332c3df 100644 --- a/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.adb +++ b/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.adb @@ -17,6 +17,7 @@ with HW.Time; with HW.Debug; with HW.GFX.GMA.Config; +with HW.GFX.GMA.PCode; with HW.GFX.GMA.Registers;
package body HW.GFX.GMA.Power_And_Clocks_Haswell is @@ -55,8 +56,6 @@ IPS_CTL_ENABLE : constant := 1 * 2 ** 31; DISPLAY_IPS_CONTROL : constant := 16#19#;
- GT_MAILBOX_READY : constant := 1 * 2 ** 31; - ----------------------------------------------------------------------------
procedure PSR_Off @@ -88,18 +87,6 @@
----------------------------------------------------------------------------
- procedure GT_Mailbox_Write (MBox : Word32; Value : Word32) is - begin - pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity)); - - Registers.Wait_Unset_Mask (Registers.GT_MAILBOX, GT_MAILBOX_READY); - Registers.Write (Registers.GT_MAILBOX_DATA, Value); - Registers.Write (Registers.GT_MAILBOX, GT_MAILBOX_READY or MBox); - - Registers.Wait_Unset_Mask (Registers.GT_MAILBOX, GT_MAILBOX_READY); - Registers.Write (Registers.GT_MAILBOX_DATA, 0); - end GT_Mailbox_Write; - procedure IPS_Off is Enabled : Boolean; @@ -110,7 +97,7 @@ Registers.Is_Set_Mask (Registers.IPS_CTL, IPS_CTL_ENABLE, Enabled); if Enabled then if Config.Has_IPS_CTL_Mailbox then - GT_Mailbox_Write (DISPLAY_IPS_CONTROL, 0); + PCode.Mailbox_Write (DISPLAY_IPS_CONTROL, 0, Wait_Ready => True); Registers.Wait_Unset_Mask (Register => Registers.IPS_CTL, Mask => IPS_CTL_ENABLE, diff --git a/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.ads b/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.ads index 87f5d07..7c5a647 100644 --- a/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.ads +++ b/common/haswell_shared/hw-gfx-gma-power_and_clocks_haswell.ads @@ -15,7 +15,6 @@ private package HW.GFX.GMA.Power_And_Clocks_Haswell is
procedure PSR_Off; - procedure GT_Mailbox_Write (MBox : Word32; Value : Word32);
procedure Pre_All_Off; procedure Post_All_Off is null; diff --git a/common/hw-gfx-gma-pcode.adb b/common/hw-gfx-gma-pcode.adb new file mode 100644 index 0000000..dd0fc6e --- /dev/null +++ b/common/hw-gfx-gma-pcode.adb @@ -0,0 +1,138 @@ +-- +-- Copyright (C) 2014-2016, 2019 secunet Security Networks AG +-- +-- This program is free software; you can redistribute it and/or modify +-- it under the terms of the GNU General Public License as published by +-- the Free Software Foundation; either version 2 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU General Public License for more details. +-- + +with GNAT.Source_Info; + +with HW.Debug; +with HW.GFX.GMA.Registers; + +use HW.GFX.GMA.Registers; + +package body HW.GFX.GMA.PCode is + + GT_MAILBOX_READY : constant := 1 * 2 ** 31; + + -- Send a command and optionally wait for and return the reply. + procedure Mailbox_Write_Read + (MBox : in Word32; + Command : in Word64; + Reply : out Word64; + Wait_Ready : in Boolean := False; + Wait_Ack : in Boolean := True; + Success : out Boolean) + with + Pre => Mailbox_Ready or Wait_Ready, + Post => (if Wait_Ack and Success then Mailbox_Ready) + is + use type HW.Word64; + + Data : Word32; + begin + pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity)); + + Reply := 0; + Success := True; + + if Wait_Ready then + Wait_Unset_Mask (GT_MAILBOX, GT_MAILBOX_READY, Success => Success); + if not Success then + return; + end if; + end if; + + Write (GT_MAILBOX_DATA, Word32 (Command and 16#ffff_ffff#)); + Write (GT_MAILBOX_DATA_1, Word32 (Shift_Right (Command, 32))); + Write (GT_MAILBOX, GT_MAILBOX_READY or MBox); + Mailbox_Ready := False; + + if Wait_Ack then + Wait_Unset_Mask (GT_MAILBOX, GT_MAILBOX_READY, Success => Success); + Mailbox_Ready := Success; + + Read (GT_MAILBOX_DATA, Data); + Reply := Word64 (Data); + Read (GT_MAILBOX_DATA_1, Data); + Reply := Shift_Left (Word64 (Data), 32) or Reply; + + Write (GT_MAILBOX_DATA, 0); + Write (GT_MAILBOX_DATA_1, 0); + end if; + end Mailbox_Write_Read; + + procedure Mailbox_Write + (MBox : in Word32; + Command : in Word64; + Wait_Ready : in Boolean := False; + Wait_Ack : in Boolean := True; + Success : out Boolean) + is + pragma Warnings (GNATprove, Off, "unused assignment to ""Ignored_R"""); + Ignored_R : Word64; + begin + Mailbox_Write_Read + (MBox, Command, Ignored_R, Wait_Ready, Wait_Ack, Success); + end Mailbox_Write; + + procedure Mailbox_Request + (MBox : in Word32; + Command : in Word64; + Reply_Mask : in Word64; + Reply : in Word64 := 16#ffff_ffff_ffff_ffff#; + TOut_MS : in Natural := Registers.Default_Timeout_MS; + Wait_Ready : in Boolean := False; + Success : out Boolean) + is + use type HW.Word64; + + Timeout : constant Time.T := Time.MS_From_Now (TOut_MS); + Timed_Out : Boolean := False; + + Received_Reply : Word64; + begin + Success := False; + loop + pragma Loop_Invariant ((not Success and Wait_Ready) or Mailbox_Ready); + Mailbox_Write_Read + (MBox => MBox, + Command => Command, + Reply => Received_Reply, + Wait_Ready => not Success and Wait_Ready, + Success => Success); + exit when not Success; + + if (Received_Reply and Reply_Mask) = (Reply and Reply_Mask) then + -- Ignore timeout if we succeeded anyway. + Timed_Out := False; + exit; + end if; + exit when Timed_Out; + + Timed_Out := Time.Timed_Out (Timeout); + end loop; + + Success := Success and then not Timed_Out; + end Mailbox_Request; + + procedure Mailbox_Write + (MBox : Word32; + Command : Word64; + Wait_Ready : Boolean := False) + is + pragma Warnings (GNATprove, Off, "unused assignment to ""Ignored_S"""); + Ignored_S : Boolean; + begin + Mailbox_Write (MBox, Command, Wait_Ready, False, Ignored_S); + end Mailbox_Write; + +end HW.GFX.GMA.PCode; diff --git a/common/hw-gfx-gma-pcode.ads b/common/hw-gfx-gma-pcode.ads new file mode 100644 index 0000000..ef2564a --- /dev/null +++ b/common/hw-gfx-gma-pcode.ads @@ -0,0 +1,60 @@ +-- +-- Copyright (C) 2016, 2019 secunet Security Networks AG +-- +-- This program is free software; you can redistribute it and/or modify +-- it under the terms of the GNU General Public License as published by +-- the Free Software Foundation; either version 2 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU General Public License for more details. +-- + +with HW.GFX.GMA.Registers; + +private package HW.GFX.GMA.PCode is + + -- We have to ensure that previous usage of the mailbox finished + -- (Wait_Ready) or know that we already did so (Mailbox_Ready). + -- + -- If we wait for the other side to acknowledge (Wait_Ack), we + -- know that it's ready (=> Mailbox_Ready). + + -- XXX: Supposed to be a `Ghost` variable, but GNAT seems too broken? + Mailbox_Ready : Boolean with Part_Of => HW.GFX.GMA.State; + + -- Just send a command, discard the reply. + procedure Mailbox_Write + (MBox : in Word32; + Command : in Word64; + Wait_Ready : in Boolean := False; + Wait_Ack : in Boolean := True; + Success : out Boolean) + with + Pre => Mailbox_Ready or Wait_Ready, + Post => (if Wait_Ack and Success then Mailbox_Ready); + + -- Repeatedly send a request command the expected reply is received. + procedure Mailbox_Request + (MBox : in Word32; + Command : in Word64; + Reply_Mask : in Word64; + Reply : in Word64 := 16#ffff_ffff_ffff_ffff#; + TOut_MS : in Natural := Registers.Default_Timeout_MS; + Wait_Ready : in Boolean := False; + Success : out Boolean) + with + Pre => Mailbox_Ready or Wait_Ready, + Post => (if Success then Mailbox_Ready); + + -- For final mailbox commands that don't have to wait. + procedure Mailbox_Write + (MBox : Word32; + Command : Word64; + Wait_Ready : Boolean := False) + with + Pre => Mailbox_Ready or Wait_Ready; + +end HW.GFX.GMA.PCode; diff --git a/common/hw-gfx-gma.adb b/common/hw-gfx-gma.adb index 15d234a..dbdee5c 100644 --- a/common/hw-gfx-gma.adb +++ b/common/hw-gfx-gma.adb @@ -21,6 +21,7 @@ with HW.GFX.GMA.Config; with HW.GFX.GMA.Config_Helpers; with HW.GFX.GMA.Registers; +with HW.GFX.GMA.PCode; with HW.GFX.GMA.Power_And_Clocks; with HW.GFX.GMA.Panel; with HW.GFX.GMA.PLLs; @@ -39,6 +40,7 @@ (State => (Dev.Address_State, Registers.Address_State, + PCode.Mailbox_Ready, PLLs.State, Panel.Panel_State, Cur_Configs, Allocated_PLLs, HPD_Delay, Wait_For_HPD, @@ -376,6 +378,7 @@ (Config.Variable, Dev.Address_State, Registers.Address_State, + PCode.Mailbox_Ready, PLLs.State, Panel.Panel_State, Cur_Configs, Allocated_PLLs, HPD_Delay, Wait_For_HPD, @@ -435,6 +438,7 @@ pragma Debug (Debug.Set_Register_Write_Delay (Write_Delay));
Linear_FB_Base := 0; + PCode.Mailbox_Ready := False; Wait_For_HPD := HPD_Type'(others => False); HPD_Delay := HPD_Delay_Type'(others => Now); Allocated_PLLs := (others => PLLs.Invalid); diff --git a/common/skylake/hw-gfx-gma-power_and_clocks_skylake.adb b/common/skylake/hw-gfx-gma-power_and_clocks_skylake.adb index c6eb2cf..2ab7f21 100644 --- a/common/skylake/hw-gfx-gma-power_and_clocks_skylake.adb +++ b/common/skylake/hw-gfx-gma-power_and_clocks_skylake.adb @@ -18,6 +18,7 @@ with HW.Debug; with HW.GFX.GMA.Config; with HW.GFX.GMA.Registers; +with HW.GFX.GMA.PCode; with HW.GFX.GMA.Power_And_Clocks_Haswell;
use type HW.Word64; @@ -86,8 +87,6 @@ SKL_CDCLK_PREPARE_FOR_CHANGE : constant := 3; SKL_CDCLK_READY_FOR_CHANGE : constant := 1;
- GT_MAILBOX_READY : constant := 1 * 2 ** 31; - function CDCLK_CTL_CD_FREQ_DECIMAL (Freq : Pos16; Plus_Half : Boolean) @@ -98,22 +97,6 @@
----------------------------------------------------------------------------
- procedure GT_Mailbox_Write (MBox : Word32; Value : Word64) is - begin - pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity)); - - Registers.Wait_Unset_Mask (Registers.GT_MAILBOX, GT_MAILBOX_READY); - Registers.Write - (Registers.GT_MAILBOX_DATA, Word32 (Value and 16#ffff_ffff#)); - Registers.Write - (Registers.GT_MAILBOX_DATA_1, Word32 (Shift_Right (Value, 32))); - Registers.Write (Registers.GT_MAILBOX, GT_MAILBOX_READY or MBox); - - Registers.Wait_Unset_Mask (Registers.GT_MAILBOX, GT_MAILBOX_READY); - end GT_Mailbox_Write; - - ---------------------------------------------------------------------------- - procedure PD_Off (PD : Power_Domain) is Ctl1, Ctl2, Ctl3, Ctl4 : Word32; @@ -251,10 +234,7 @@
procedure Initialize is - CDClk_Change_Timeout : Time.T; - Timed_Out : Boolean; - - MBox_Data0 : Word32; + Success : Boolean; begin Registers.Set_Mask (Register => Registers.NDE_RSTWRN_OPT, @@ -282,29 +262,20 @@ (Register => Registers.LCPLL1_CTL, Mask => LCPLL1_CTL_PLL_LOCK);
- CDClk_Change_Timeout := Time.MS_From_Now (3); - Timed_Out := False; - loop - GT_Mailbox_Write - (MBox => SKL_PCODE_CDCLK_CONTROL, - Value => SKL_CDCLK_PREPARE_FOR_CHANGE); - Registers.Read (Registers.GT_MAILBOX_DATA, MBox_Data0); - if (MBox_Data0 and SKL_CDCLK_READY_FOR_CHANGE) /= 0 then - -- Ignore timeout if we succeeded anyway. - Timed_Out := False; - exit; - end if; - exit when Timed_Out; + PCode.Mailbox_Request + (MBox => SKL_PCODE_CDCLK_CONTROL, + Command => SKL_CDCLK_PREPARE_FOR_CHANGE, + Reply_Mask => SKL_CDCLK_READY_FOR_CHANGE, + Wait_Ready => True, + Success => Success);
- Timed_Out := Time.Timed_Out (CDClk_Change_Timeout); - end loop; - pragma Debug (Timed_Out, Debug.Put_Line - ("ERROR: PCODE not ready for frequency change after 3ms.")); + pragma Debug (not Success, Debug.Put_Line + ("ERROR: PCODE not ready for frequency change."));
- if not Timed_Out then - GT_Mailbox_Write + if Success then + PCode.Mailbox_Write (MBox => SKL_PCODE_CDCLK_CONTROL, - Value => 16#0000_0000#); -- 0 - 337.5MHz + Command => 16#0000_0000#); -- 0 - 337.5MHz -- 1 - 450.0MHz -- 2 - 540.0MHz -- 3 - 675.0MHz