Change in ...libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/libgfxinit/+/32730

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

gma i2c: Rework GMBUS reset procedure

Once we tried to use the GMBUS controller with an unconnected pair of
I2C pins, it got stuck and a reset via the SOFTWARE_CLEAR_INTERRUPT
bit didn't suffice to recover. Further tests have shown that we are
able to recover, if we switch to a valid pin pair first and issue an
I2C stop cycle before the reset.

Change-Id: If737ffb35afa309de7746f0c16025b9598f69460
Signed-off-by: Nico Huber 
---
M common/hw-gfx-gma-i2c.adb
1 file changed, 39 insertions(+), 31 deletions(-)

git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/30/32730/1

diff --git a/common/hw-gfx-gma-i2c.adb b/common/hw-gfx-gma-i2c.adb
index bc81734..3d41174 100644
--- a/common/hw-gfx-gma-i2c.adb
+++ b/common/hw-gfx-gma-i2c.adb
@@ -122,27 +122,39 @@

----------------------------------------------------------------------------

-   procedure GMBUS_Ready (Result : out Boolean)
+   function GMBUS_Ready (GMBUS2 : Word32) return Boolean is
+     ((GMBUS2 and (GMBUS2_HARDWARE_WAIT_PHASE or
+                   GMBUS2_SLAVE_STALL_TIMEOUT_ERROR or
+                   GMBUS2_GMBUS_INTERRUPT_STATUS or
+                   GMBUS2_NAK_INDICATOR or
+                   GMBUS2_GMBUS_ACTIVE)) = 0);
+
+   procedure Check_And_Reset (Success : out Boolean)
    is
       GMBUS2 : Word32;
    begin
-      Registers.Read (GMBUS_Regs (2), GMBUS2);
-      Result := (GMBUS2 and (GMBUS2_HARDWARE_WAIT_PHASE or
-                              GMBUS2_SLAVE_STALL_TIMEOUT_ERROR or
-                              GMBUS2_GMBUS_INTERRUPT_STATUS or
-                              GMBUS2_NAK_INDICATOR)) = 0;
-   end GMBUS_Ready;
-
-   procedure Reset_GMBUS (Success : out Boolean) is
-   begin
       pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity));

-      Registers.Write (GMBUS_Regs (1), GMBUS1_SOFTWARE_CLEAR_INTERRUPT);
-      Registers.Write (GMBUS_Regs (1), 0);
-      Registers.Write (GMBUS_Regs (0), GMBUS0_PIN_PAIR_SELECT_NONE);
+      Registers.Read (GMBUS_Regs (2), GMBUS2);
+      if (GMBUS2 and GMBUS2_GMBUS_ACTIVE) /= 0 then
+         Registers.Write
+           (Register => GMBUS_Regs (1),
+            Value    => GMBUS1_SOFTWARE_READY or GMBUS1_BUS_CYCLE_STOP);
+         Registers.Wait_Unset_Mask
+           (Register => GMBUS_Regs (2),
+            Mask     => GMBUS2_GMBUS_ACTIVE,
+            TOut_MS  => 1);
+         Registers.Read (GMBUS_Regs (2), GMBUS2);
+      end if;
+      Success := GMBUS_Ready (GMBUS2);

-      GMBUS_Ready (Success);
-   end Reset_GMBUS;
+      if not Success then
+         Registers.Write (GMBUS_Regs (1), GMBUS1_SOFTWARE_CLEAR_INTERRUPT);
+         Registers.Write (GMBUS_Regs (1), 0);
+         Registers.Read (GMBUS_Regs (2), GMBUS2);
+         Success := GMBUS_Ready (GMBUS2);
+      end if;
+   end Check_And_Reset;

procedure Init_GMBUS (Port : PCH_Port; Success : out Boolean) is
    begin
@@ -157,23 +169,19 @@
       -- TODO: Refactor + check for timeout.
       Registers.Wait_Unset_Mask (GMBUS_Regs (2), GMBUS2_INUSE);

-      GMBUS_Ready (Success);
-      if not Success then
-         Reset_GMBUS (Success);
-      end if;
+      Registers.Write (GMBUS_Regs (4), 0);
+      Registers.Write (GMBUS_Regs (5), 0);

-      if Success then
-         Registers.Write
-           (Register => GMBUS_Regs (0),
-            Value    => GMBUS0_GMBUS_RATE_SELECT_100KHZ or
-                        GMBUS0_PIN_PAIR_SELECT (Port));
-         Registers.Write
-           (Register => GMBUS_Regs (4),
-            Value    => 0);
-         Registers.Write
-           (Register => GMBUS_Regs (5),
-            Value    => 0);
-      end if;
+      -- Resetting the state machine only works if a valid port
+      -- is selected and we don't always know which ports are
+      -- valid. So do the cleanup before we use the GMBUS with
+      -- the current port. If the port is valid, the reset should
+      -- work, if not, it shouldn't matter.
+      Registers.Write
+        (Register => GMBUS_Regs (0),
+         Value    => GMBUS0_GMBUS_RATE_SELECT_100KHZ or
+                     GMBUS0_PIN_PAIR_SELECT (Port));
+      Check_And_Reset (Success);
    end Init_GMBUS;

procedure Release_GMBUS

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 1: Verified+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 1:

Arthur, it would be interesting to test this on the
troubled g45 board(s). That is, with a broken port
detection, so it would try to talk to unconnected
ports.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Arthur, it would be interesting to test this on the
> troubled g45 board(s). That is, with a broken port
> detection, so it would try to talk to unconnected
> ports.

I vaguely recall my x200 failing if unconnected ports are probed. I'll see what I can do.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 1:
> > Arthur, it would be interesting to test this on the
> > troubled g45 board(s). That is, with a broken port
> > detection, so it would try to talk to unconnected
> > ports.
>
> I vaguely recall my x200 failing if unconnected ports are probed. I'll see what I can do.

Oh, nvm then. I thought it was some desktop board. I've
tested on an X200s which seems to have roughly the same
schematics.

Change in libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 3: Code-Review+1

Not sure if testing was done, and just took a quick look at it.

Change in libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 3:

For the record, in #[e-mail] a Lenovo T400 user reported, that display init does not always work (maybe one out of five times). The cause was a random failure of reading the EDID information. Increasing the time-out to 1100 helped, and the user also tested this commit successfully.

cd 3rdparty/libgfxinit/ && git fetch "https://review.coreboot.org/libgfxinit" refs/changes/30/32730/3 && git cherry-pick FETCH_HEAD && cd - && git add 3rdparty/libgfxinit

Change in libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 3: Code-Review+2
> Patch Set 3:
>
> For the record, in #[e-mail] a Lenovo T400 user reported, that display init does not always work (maybe one out of five times). The cause was a random failure of reading the EDID information. Increasing the time-out to 1100 helped, and the user also tested this commit successfully.
> cd 3rdparty/libgfxinit/ && git fetch "https://review.coreboot.org/libgfxinit" refs/changes/30/32730/3 && git cherry-pick FETCH_HEAD && cd - && git add 3rdparty/libgfxinit

I guess it's good to go then

Change in libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

Patch Set 3:

Gave this a little more regression testing. G45, ILK, IVB, BXT
still working fine, it seems.

Change in libgfxinit[master]: gma i2c: Rework GMBUS reset procedure

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/libgfxinit/+/32730 )

Change subject: gma i2c: Rework GMBUS reset procedure
......................................................................

gma i2c: Rework GMBUS reset procedure

Once we tried to use the GMBUS controller with an unconnected pair of
I2C pins, it got stuck and a reset via the SOFTWARE_CLEAR_INTERRUPT
bit didn't suffice to recover. Further tests have shown that we are
able to recover, if we switch to a valid pin pair first and issue an
I2C stop cycle before the reset.

Change-Id: If737ffb35afa309de7746f0c16025b9598f69460
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/libgfxinit/+/32730
Reviewed-by: Angel Pons 
---
M common/hw-gfx-gma-i2c.adb
1 file changed, 39 insertions(+), 31 deletions(-)

Approvals:
  Nico Huber: Verified
  Angel Pons: Looks good to me, approved

diff --git a/common/hw-gfx-gma-i2c.adb b/common/hw-gfx-gma-i2c.adb
index bc81734..3d41174 100644
--- a/common/hw-gfx-gma-i2c.adb
+++ b/common/hw-gfx-gma-i2c.adb
@@ -122,27 +122,39 @@

----------------------------------------------------------------------------

-   procedure GMBUS_Ready (Result : out Boolean)
+   function GMBUS_Ready (GMBUS2 : Word32) return Boolean is
+     ((GMBUS2 and (GMBUS2_HARDWARE_WAIT_PHASE or
+                   GMBUS2_SLAVE_STALL_TIMEOUT_ERROR or
+                   GMBUS2_GMBUS_INTERRUPT_STATUS or
+                   GMBUS2_NAK_INDICATOR or
+                   GMBUS2_GMBUS_ACTIVE)) = 0);
+
+   procedure Check_And_Reset (Success : out Boolean)
    is
       GMBUS2 : Word32;
    begin
-      Registers.Read (GMBUS_Regs (2), GMBUS2);
-      Result := (GMBUS2 and (GMBUS2_HARDWARE_WAIT_PHASE or
-                              GMBUS2_SLAVE_STALL_TIMEOUT_ERROR or
-                              GMBUS2_GMBUS_INTERRUPT_STATUS or
-                              GMBUS2_NAK_INDICATOR)) = 0;
-   end GMBUS_Ready;
-
-   procedure Reset_GMBUS (Success : out Boolean) is
-   begin
       pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity));

-      Registers.Write (GMBUS_Regs (1), GMBUS1_SOFTWARE_CLEAR_INTERRUPT);
-      Registers.Write (GMBUS_Regs (1), 0);
-      Registers.Write (GMBUS_Regs (0), GMBUS0_PIN_PAIR_SELECT_NONE);
+      Registers.Read (GMBUS_Regs (2), GMBUS2);
+      if (GMBUS2 and GMBUS2_GMBUS_ACTIVE) /= 0 then
+         Registers.Write
+           (Register => GMBUS_Regs (1),
+            Value    => GMBUS1_SOFTWARE_READY or GMBUS1_BUS_CYCLE_STOP);
+         Registers.Wait_Unset_Mask
+           (Register => GMBUS_Regs (2),
+            Mask     => GMBUS2_GMBUS_ACTIVE,
+            TOut_MS  => 1);
+         Registers.Read (GMBUS_Regs (2), GMBUS2);
+      end if;
+      Success := GMBUS_Ready (GMBUS2);

-      GMBUS_Ready (Success);
-   end Reset_GMBUS;
+      if not Success then
+         Registers.Write (GMBUS_Regs (1), GMBUS1_SOFTWARE_CLEAR_INTERRUPT);
+         Registers.Write (GMBUS_Regs (1), 0);
+         Registers.Read (GMBUS_Regs (2), GMBUS2);
+         Success := GMBUS_Ready (GMBUS2);
+      end if;
+   end Check_And_Reset;

procedure Init_GMBUS (Port : PCH_Port; Success : out Boolean) is
    begin
@@ -157,23 +169,19 @@
       -- TODO: Refactor + check for timeout.
       Registers.Wait_Unset_Mask (GMBUS_Regs (2), GMBUS2_INUSE);

-      GMBUS_Ready (Success);
-      if not Success then
-         Reset_GMBUS (Success);
-      end if;
+      Registers.Write (GMBUS_Regs (4), 0);
+      Registers.Write (GMBUS_Regs (5), 0);

-      if Success then
-         Registers.Write
-           (Register => GMBUS_Regs (0),
-            Value    => GMBUS0_GMBUS_RATE_SELECT_100KHZ or
-                        GMBUS0_PIN_PAIR_SELECT (Port));
-         Registers.Write
-           (Register => GMBUS_Regs (4),
-            Value    => 0);
-         Registers.Write
-           (Register => GMBUS_Regs (5),
-            Value    => 0);
-      end if;
+      -- Resetting the state machine only works if a valid port
+      -- is selected and we don't always know which ports are
+      -- valid. So do the cleanup before we use the GMBUS with
+      -- the current port. If the port is valid, the reset should
+      -- work, if not, it shouldn't matter.
+      Registers.Write
+        (Register => GMBUS_Regs (0),
+         Value    => GMBUS0_GMBUS_RATE_SELECT_100KHZ or
+                     GMBUS0_PIN_PAIR_SELECT (Port));
+      Check_And_Reset (Success);
    end Init_GMBUS;

procedure Release_GMBUS