[PATCH] ps2port: adjust init routine to fix PS/2 keyboard issues

[Paul Menzel]

From: Matt DeVillier 
Date: Fri, 12 Aug 2016 14:21:58 -0500

PS/2 keyboards on Chromebooks with upstream coreboot + SeaBIOS often
fail to init properly / register keystrokes.  Modify ps2port init
to match that of TianoCore, which doesn't have said issues.

Signed-off-by: Matt DeVillier 
Signed-off-by: Paul Menzel 
---
Upstream from https://github.com/MrChromebox/SeaBIOS/

src/hw/ps2port.c | 18 +++++++++++++-----
  1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/hw/ps2port.c b/src/hw/ps2port.c
index 88b1059..2c334c0 100644
--- a/src/hw/ps2port.c
+++ b/src/hw/ps2port.c
@@ -465,6 +465,14 @@ ps2_keyboard_setup(void *data)
      if (ret)
          return;

+    // Disable KB/mouse interfaces
+    ret = i8042_command(I8042_CMD_KBD_DISABLE, NULL);
+    if (ret)
+        return;
+    ret = i8042_command(I8042_CMD_AUX_DISABLE, NULL);
+    if (ret)
+        return;
+
      // Controller self-test.
      u8 param[2];
      ret = i8042_command(I8042_CMD_CTL_TEST, param);
@@ -475,6 +483,11 @@ ps2_keyboard_setup(void *data)
          return;
      }

+    // KB write CMD
+    ret = i8042_command(I8042_CMD_CTL_WCTR, NULL);
+    if (ret)
+        return;
+
      // Controller keyboard test.
      ret = i8042_command(I8042_CMD_KBD_TEST, param);
      if (ret)
@@ -505,11 +518,6 @@ ps2_keyboard_setup(void *data)
          return;
      }

-    /* Disable keyboard */
-    ret = ps2_kbd_command(ATKBD_CMD_RESET_DIS, NULL);
-    if (ret)
-        return;
-
      // Set scancode command (mode 2)
      param[0] = 0x02;
      ret = ps2_kbd_command(ATKBD_CMD_SSCANSET, param);

[Gerd Hoffmann]

On Mon, Mar 16, 2020 at 10:38:25AM +0100, Paul Menzel wrote:
> From: Matt DeVillier 
> Date: Fri, 12 Aug 2016 14:21:58 -0500
>
> PS/2 keyboards on Chromebooks with upstream coreboot + SeaBIOS often
> fail to init properly / register keystrokes.  Modify ps2port init
> to match that of TianoCore, which doesn't have said issues.

Looks good to me.

cheers,
  Gerd

[Kevin O'Connor]

On Mon, Mar 16, 2020 at 10:38:25AM +0100, Paul Menzel wrote:
> From: Matt DeVillier 
> Date: Fri, 12 Aug 2016 14:21:58 -0500
>
> PS/2 keyboards on Chromebooks with upstream coreboot + SeaBIOS often
> fail to init properly / register keystrokes.  Modify ps2port init
> to match that of TianoCore, which doesn't have said issues.
>
> Signed-off-by: Matt DeVillier 
> Signed-off-by: Paul Menzel
>
> Upstream from https://github.com/MrChromebox/SeaBIOS/
>
> src/hw/ps2port.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/src/hw/ps2port.c b/src/hw/ps2port.c
> index 88b1059..2c334c0 100644
> --- a/src/hw/ps2port.c
> +++ b/src/hw/ps2port.c
> @@ -465,6 +465,14 @@ ps2_keyboard_setup(void *data)
>      if (ret)
>          return;
> // Disable KB/mouse interfaces
> ret = i8042_command(I8042_CMD_KBD_DISABLE, NULL);
> if (ret)
>    return;
> ret = i8042_command(I8042_CMD_AUX_DISABLE, NULL);
> if (ret)
>    return;

I do not think this change is correct.  My recollection is that the
underlying bug that led to non-functioning keyboards was fixed as of
dbf9dd27f.  Can someone confirm that the underlying problem is still
present after dbf9dd27f?

-Kevin
>  // Controller self-test.
>  u8 param[2];
>  ret = i8042_command(I8042_CMD_CTL_TEST, param);
>
> @@ -475,6 +483,11 @@ ps2_keyboard_setup(void *data)
>          return;
>      }
> // KB write CMD
> ret = i8042_command(I8042_CMD_CTL_WCTR, NULL);
> if (ret)
>    return;
> // Controller keyboard test.
>    ret = i8042_command(I8042_CMD_KBD_TEST, param);
>    if (ret)
>
> @@ -505,11 +518,6 @@ ps2_keyboard_setup(void *data)
>          return;
>      }
> /* Disable keyboard */
> ret = ps2_kbd_command(ATKBD_CMD_RESET_DIS, NULL);
> if (ret)
>    return;
> // Set scancode command (mode 2)
>    param[0] = 0x02;
>    ret = ps2_kbd_command(ATKBD_CMD_SSCANSET, param);

[Matt DeVillier]

Paul,

as requested I went ahead and retested a few different Chromebook
devices/platforms using upstream coreboot + SeaBIOS master, with the
stock EC firmware, to asses the current state of the PS2 keyboard
function:

google/link (IvyBridge): bad (good w/EC update)
google/wolf (Haswell): good
google/clapper (Baytrail): good (with HARDWARE_IRQ unset)
google/lulu (Broadwell): good
google/chell (Skylake): good (with HARDWARE_IRQ unset)
google/cyan (Braswell): good (with HARDWARE_IRQ unset)

so I think the patch is no longer needed, since LINK is the only board
which is problematic, and I'm not even sure the patch addresses that;
pretty sure the fix needs to be on the EC end.

regards,
Matt

On Tue, Mar 17, 2020 at 7:30 PM Kevin O'Connor  wrote:
> On Mon, Mar 16, 2020 at 10:38:25AM +0100, Paul Menzel wrote:
> > From: Matt DeVillier 
> > Date: Fri, 12 Aug 2016 14:21:58 -0500
> >
> > PS/2 keyboards on Chromebooks with upstream coreboot + SeaBIOS often
> > fail to init properly / register keystrokes.  Modify ps2port init
> > to match that of TianoCore, which doesn't have said issues.
> >
> > Signed-off-by: Matt DeVillier 
> > Signed-off-by: Paul Menzel
> >
> > Upstream from https://github.com/MrChromebox/SeaBIOS/
> >
> > src/hw/ps2port.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/hw/ps2port.c b/src/hw/ps2port.c
> > index 88b1059..2c334c0 100644
> > --- a/src/hw/ps2port.c
> > +++ b/src/hw/ps2port.c
> > @@ -465,6 +465,14 @@ ps2_keyboard_setup(void *data)
> >      if (ret)
> >          return;
> > // Disable KB/mouse interfaces
> > ret = i8042_command(I8042_CMD_KBD_DISABLE, NULL);
> > if (ret)
> >    return;
> > ret = i8042_command(I8042_CMD_AUX_DISABLE, NULL);
> > if (ret)
> >    return;
>
> I do not think this change is correct.  My recollection is that the
> underlying bug that led to non-functioning keyboards was fixed as of
> dbf9dd27f.  Can someone confirm that the underlying problem is still
> present after dbf9dd27f?
>
> -Kevin
> >  // Controller self-test.
> >  u8 param[2];
> >  ret = i8042_command(I8042_CMD_CTL_TEST, param);
> >
> > @@ -475,6 +483,11 @@ ps2_keyboard_setup(void *data)
> >          return;
> >      }
> > // KB write CMD
> > ret = i8042_command(I8042_CMD_CTL_WCTR, NULL);
> > if (ret)
> >    return;
> > // Controller keyboard test.
> >    ret = i8042_command(I8042_CMD_KBD_TEST, param);
> >    if (ret)
> >
> > @@ -505,11 +518,6 @@ ps2_keyboard_setup(void *data)
> >          return;
> >      }
> > /* Disable keyboard */
> > ret = ps2_kbd_command(ATKBD_CMD_RESET_DIS, NULL);
> > if (ret)
> >    return;
> > // Set scancode command (mode 2)
> >    param[0] = 0x02;
> >    ret = ps2_kbd_command(ATKBD_CMD_SSCANSET, param);
>
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Paul Menzel]

Dear Matt,

Am 26.03.20 um 20:29 schrieb Matt DeVillier:
> as requested I went ahead and retested a few different Chromebook
> devices/platforms using upstream coreboot + SeaBIOS master, with the
> stock EC firmware, to asses the current state of the PS2 keyboard
> function:
>
> google/link (IvyBridge): bad (good w/EC update)
> google/wolf (Haswell): good
> google/clapper (Baytrail): good (with HARDWARE_IRQ unset)
> google/lulu (Broadwell): good
> google/chell (Skylake): good (with HARDWARE_IRQ unset)
> google/cyan (Braswell): good (with HARDWARE_IRQ unset)
>
> so I think the patch is no longer needed, since LINK is the only board
> which is problematic, and I'm not even sure the patch addresses that;
> pretty sure the fix needs to be on the EC end.

Thank you so much going through this tedious QA work.

Just to be sure, by *master* you mean without your merged commit 
bfdb3f86 (ps2port: adjust init routine to fix PS/2 keyboard issues), 
that mean a commit before or with the commit reverted?

Kind regards,

Paul

[Matt DeVillier]

On Thu, Mar 26, 2020 at 2:33 PM Paul Menzel  wrote:
> Dear Matt,
>
> Am 26.03.20 um 20:29 schrieb Matt DeVillier:
> > as requested I went ahead and retested a few different Chromebook
> > devices/platforms using upstream coreboot + SeaBIOS master, with the
> > stock EC firmware, to asses the current state of the PS2 keyboard
> > function:
> >
> > google/link (IvyBridge): bad (good w/EC update)
> > google/wolf (Haswell): good
> > google/clapper (Baytrail): good (with HARDWARE_IRQ unset)
> > google/lulu (Broadwell): good
> > google/chell (Skylake): good (with HARDWARE_IRQ unset)
> > google/cyan (Braswell): good (with HARDWARE_IRQ unset)
> >
> > so I think the patch is no longer needed, since LINK is the only board
> > which is problematic, and I'm not even sure the patch addresses that;
> > pretty sure the fix needs to be on the EC end.
>
> Thank you so much going through this tedious QA work.
>
> Just to be sure, by *master* you mean without your merged commit
> bfdb3f86 (ps2port: adjust init routine to fix PS/2 keyboard issues),
> that mean a commit before or with the commit reverted?

I apologize, I didn't realize this commit had been merged already, so
was testing
against current master (de88a96) which includes this patch. I will
need to re-test
against 29ee1fb or earlier to see if it's actually needed
> Kind regards,
>
> Paul

[Kevin O'Connor]

On Thu, Mar 26, 2020 at 03:52:58PM -0500, Matt DeVillier wrote:
> On Thu, Mar 26, 2020 at 2:33 PM Paul Menzel  wrote:
> > Am 26.03.20 um 20:29 schrieb Matt DeVillier:
> > > as requested I went ahead and retested a few different Chromebook
> > > devices/platforms using upstream coreboot + SeaBIOS master, with the
> > > stock EC firmware, to asses the current state of the PS2 keyboard
> > > function:
> > >
> > > google/link (IvyBridge): bad (good w/EC update)
> > > google/wolf (Haswell): good
> > > google/clapper (Baytrail): good (with HARDWARE_IRQ unset)
> > > google/lulu (Broadwell): good
> > > google/chell (Skylake): good (with HARDWARE_IRQ unset)
> > > google/cyan (Braswell): good (with HARDWARE_IRQ unset)
> > >
> > > so I think the patch is no longer needed, since LINK is the only board
> > > which is problematic, and I'm not even sure the patch addresses that;
> > > pretty sure the fix needs to be on the EC end.
> >
> > Thank you so much going through this tedious QA work.
> >
> > Just to be sure, by *master* you mean without your merged commit
> > bfdb3f86 (ps2port: adjust init routine to fix PS/2 keyboard issues),
> > that mean a commit before or with the commit reverted?
>
> I apologize, I didn't realize this commit had been merged already, so
> was testing
> against current master (de88a96) which includes this patch. I will
> need to re-test
> against 29ee1fb or earlier to see if it's actually needed

Any further updates on this?

Thanks,
-Kevin

[Matt DeVillier]

unfortunately I still need to re-test with that commit reverted

On Sun, Apr 19, 2020 at 10:38 AM Kevin O'Connor  wrote:
> On Thu, Mar 26, 2020 at 03:52:58PM -0500, Matt DeVillier wrote:
> > On Thu, Mar 26, 2020 at 2:33 PM Paul Menzel
>
> wrote:
> > > Am 26.03.20 um 20:29 schrieb Matt DeVillier:
> > > > as requested I went ahead and retested a few different Chromebook
> > > > devices/platforms using upstream coreboot + SeaBIOS master, with the
> > > > stock EC firmware, to asses the current state of the PS2 keyboard
> > > > function:
> > > >
> > > > google/link (IvyBridge): bad (good w/EC update)
> > > > google/wolf (Haswell): good
> > > > google/clapper (Baytrail): good (with HARDWARE_IRQ unset)
> > > > google/lulu (Broadwell): good
> > > > google/chell (Skylake): good (with HARDWARE_IRQ unset)
> > > > google/cyan (Braswell): good (with HARDWARE_IRQ unset)
> > > >
> > > > so I think the patch is no longer needed, since LINK is the only
>
> board
> > > > which is problematic, and I'm not even sure the patch addresses that;
> > > > pretty sure the fix needs to be on the EC end.
> > >
> > > Thank you so much going through this tedious QA work.
> > >
> > > Just to be sure, by *master* you mean without your merged commit
> > > bfdb3f86 (ps2port: adjust init routine to fix PS/2 keyboard issues),
> > > that mean a commit before or with the commit reverted?
> >
> > I apologize, I didn't realize this commit had been merged already, so
> > was testing
> > against current master (de88a96) which includes this patch. I will
> > need to re-test
> > against 29ee1fb or earlier to see if it's actually needed
>
> Any further updates on this?
>
> Thanks,
> -Kevin

[Kevin O'Connor]

On Tue, Mar 17, 2020 at 08:30:32PM -0400, Kevin O'Connor wrote:
> On Mon, Mar 16, 2020 at 10:38:25AM +0100, Paul Menzel wrote:
> > From: Matt DeVillier 
> > Date: Fri, 12 Aug 2016 14:21:58 -0500
> >
> > PS/2 keyboards on Chromebooks with upstream coreboot + SeaBIOS often
> > fail to init properly / register keystrokes.  Modify ps2port init
> > to match that of TianoCore, which doesn't have said issues.
> >
> > Signed-off-by: Matt DeVillier 
> > Signed-off-by: Paul Menzel
> >
> > Upstream from https://github.com/MrChromebox/SeaBIOS/
> >
> > src/hw/ps2port.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/hw/ps2port.c b/src/hw/ps2port.c
> > index 88b1059..2c334c0 100644
> > --- a/src/hw/ps2port.c
> > +++ b/src/hw/ps2port.c
> > @@ -465,6 +465,14 @@ ps2_keyboard_setup(void *data)
> >      if (ret)
> >          return;
> > // Disable KB/mouse interfaces
> > ret = i8042_command(I8042_CMD_KBD_DISABLE, NULL);
> > if (ret)
> >    return;
> > ret = i8042_command(I8042_CMD_AUX_DISABLE, NULL);
> > if (ret)
> >    return;
>
> I do not think this change is correct.  My recollection is that the
> underlying bug that led to non-functioning keyboards was fixed as of
> dbf9dd27f.  Can someone confirm that the underlying problem is still
> present after dbf9dd27f?

FYI, I reverted this patch.

-Kevin