Re: PATCH: fix keyboard init on Dell Latitude E6400 on GRUB_MACHINE_COREBOOT

From: Daniel Kiper <dkiper@net-space.pl>
To: Leah Rowe <leah@libreboot.org>
Cc: grub-devel@gnu.org, pmenzel@molgen.mpg.de, phcoder@gmail.com
Subject: Re: PATCH: fix keyboard init on Dell Latitude E6400 on GRUB_MACHINE_COREBOOT
Date: Tue, 31 Oct 2023 14:05:41 +0100	[thread overview]
Message-ID: <20231031130541.7shcur7o5to25a5x@tomti.i.net-space.pl> (raw)
In-Reply-To: <20231031085454.5f2ea784d8b31655a79809e0@libreboot.org>

Hi,

Adding Paul and Vladimir...

Next time please send patches using "git send-email".

Daniel

On Tue, Oct 31, 2023 at 08:54:54AM +0000, Leah Rowe via Grub-devel wrote:
> This patch concerns GRUB as a coreboot payload, on x86. It does this:
> retain current behaviour for non-coreboot targets, but force scancode
> set 2 (AT) with translation (treat as XT) in GRUB, whereas GRUB
> presently tries doing set 2 untranslated, before trying set 1.
>
> On Dell Latitude E6400 this is problematic because the EC which
> emulates the keyboard controller, reports scan set 2 with translation
> so all seems well in code, but it only actually outputs set 1; however,
> GRUB detects otherwise and then does set 2, translated. This resulted
> in messed up key input.
>
> It fixes keyboards in GRUB on Dell Latitude E6400/E6430. Nicholas Chin
> told me the EC which emulates a keyboard controller, reports scancode
> set 2 (AT, not XT) with translation (treat as XT), but only outputs set
> 1 even when changing it; 2+translate is standard. On other boards,
> changing it works. It's an EC bug on Dell Latitude E6400.
>
> GRUB does untranslated set 2 so keys were screwed. The fix forces
> 2+translation on coreboot.
>
> Tested and works perfectly. This diff is taken from my work on top of
> GRUB rev e58b870ff926415e23fc386af41ff81b2f588763 but it should apply
> in latest commits from GRUB master branch.
>
> PS: This also fixes Dell Latitude E6430.
>
> --
> Leah Rowe <leah@libreboot.org>

> >From 96c0bbe5d406b616360a7fce7cee67d7692c0d6d Mon Sep 17 00:00:00 2001
> From: Leah Rowe <leah@libreboot.org>
> Date: Mon, 30 Oct 2023 22:19:21 +0000
> Subject: [PATCH 1/1] at_keyboard coreboot: force scancodes2+translate
>
> Scan code set 2 with translation should be assumed in
> every case, as the default starting position.
>
> However, GRUB is trying to detect and use other modes
> such as set 2 without translation, or set 1 without
> translation from set 2; it also detects no-mode and
> assumes mode 1, on really old keyboards.
>
> The current behaviour has been retained, for everything
> except GRUB_MACHINE_COREBOOT; for the latter, scan code
> set 2 with translation is hardcoded, and forced in code.
>
> This is required to make keyboard initialisation work on
> the MEC5035 EC used by the Dell Latitude E6400, when
> running GRUB as a coreboot payload on that laptop. The
> EC reports scancode set 2 with translation when probed,
> but actually only outputs scancode set 1.
>
> Since GRUB is attempting to use it without translation,
> and since the machine reports set 2 with translation,
> but only ever outputs set 1 scancodes, this results in
> wrong keypresses for every key.
>
> This fix fixed that, by forcing set 2 with translation,
> treating it as set 1, but only on coreboot. This is the
> same behaviour used in GNU+Linux systems and SeaBIOS.
> With this change, GRUB keyboard initialisation now works
> just fine on those machines.
>
> This has *also* been tested on other coreboot machines
> running GRUB; several HP EliteBooks, ThinkPads and
> Dell Precision T1650. All seems to work just fine.
>
> Signed-off-by: Leah Rowe <leah@libreboot.org>
> ---
>  grub-core/term/at_keyboard.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
> index f8a129eb7..8207225c2 100644
> --- a/grub-core/term/at_keyboard.c
> +++ b/grub-core/term/at_keyboard.c
> @@ -138,6 +138,7 @@ write_mode (int mode)
>    return (i != GRUB_AT_TRIES);
>  }
>
> +#if !defined (GRUB_MACHINE_COREBOOT)
>  static int
>  query_mode (void)
>  {
> @@ -161,10 +162,12 @@ query_mode (void)
>      return 3;
>    return 0;
>  }
> +#endif
>
>  static void
>  set_scancodes (void)
>  {
> +#if !defined (GRUB_MACHINE_COREBOOT)
>    /* You must have visited computer museum. Keyboard without scancode set
>       knowledge. Assume XT. */
>    if (!grub_keyboard_orig_set)
> @@ -173,20 +176,33 @@ set_scancodes (void)
>        ps2_state.current_set = 1;
>        return;
>      }
> +#endif
>
>  #if !USE_SCANCODE_SET
>    ps2_state.current_set = 1;
>    return;
> -#else
> +#endif
>
> +#if defined (GRUB_MACHINE_COREBOOT)
> +  /* enable translation */
> +  grub_keyboard_controller_write (grub_keyboard_controller_orig
> +				  & ~KEYBOARD_AT_DISABLE);
> +#else
> +  /* if not coreboot, disable translation and try mode 2 first, before 1 */
>    grub_keyboard_controller_write (grub_keyboard_controller_orig
>  				  & ~KEYBOARD_AT_TRANSLATE
>  				  & ~KEYBOARD_AT_DISABLE);
> +#endif
>
>    keyboard_controller_wait_until_ready ();
>    grub_outb (KEYBOARD_COMMAND_ENABLE, KEYBOARD_REG_DATA);
> -
>    write_mode (2);
> +
> +#if defined (GRUB_MACHINE_COREBOOT)
> +  /* mode 2 with translation, so make grub treat as set 1 */
> +  ps2_state.current_set = 1;
> +#else
> +  /* if not coreboot, translation isn't set; test 2 and fall back to 1 */
>    ps2_state.current_set = query_mode ();
>    grub_dprintf ("atkeyb", "returned set %d\n", ps2_state.current_set);
>    if (ps2_state.current_set == 2)
> --
> 2.39.2

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel