Change in coreboot[master]: [RFC]libpayload/keyboard: Reset to scancode set #2 on exit

[Nico Huber (Code Review)]

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

Change subject: [RFC]libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

[RFC]libpayload/keyboard: Reset to scancode set #2 on exit

If we select scancode set #1 and keep that, it can confuse Linux
with keyboards that don't return to set #2 when asked to load the
defaults. This happens for instance with various integrated Think-
Pad keyboards but was also seen with an external PS/2 one.

The chosen configuration, scancode set #2 without translation, seems
to be the default for many systems. So we can expect other payloads
and kernels to work with it.

Change-Id: I28d74590e9f04d32bb2bbd461b67f15014f927ec
Signed-off-by: Nico Huber 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/47594/1

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index 09b35d3..1f8d88b 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -648,6 +648,11 @@
    keyboard_cmd(I8042_KBCMD_DEFAULT_DIS);
    keyboard_drain_input();

+	/* Nobody but us seems to still use scancode set #1.
+	   So try to hand over with more modern settings. */
+	set_scancode_set(2);
+	i8042_set_kbd_translation(false);
+
    /* Send keyboard disconnect command */
    i8042_cmd(I8042_CMD_DIS_KB);

[Angel Pons (Code Review)]

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

Change subject: [RFC]libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47594

to look at the new patch set (#3).

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

libpayload/keyboard: Reset to scancode set #2 on exit

If we select scancode set #1 and keep that, it can confuse Linux
with keyboards that don't return to set #2 when asked to load the
defaults. This happens for instance with various integrated Think-
Pad keyboards but was also seen with an external PS/2 one.

The chosen configuration, scancode set #2 without translation, seems
to be the default for many systems. So we can expect other payloads
and kernels to work with it.

Change-Id: I28d74590e9f04d32bb2bbd461b67f15014f927ec
Signed-off-by: Nico Huber 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/47594/3

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47594 )

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47594 )

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

libpayload/keyboard: Reset to scancode set #2 on exit

If we select scancode set #1 and keep that, it can confuse Linux
with keyboards that don't return to set #2 when asked to load the
defaults. This happens for instance with various integrated Think-
Pad keyboards but was also seen with an external PS/2 one.

The chosen configuration, scancode set #2 without translation, seems
to be the default for many systems. So we can expect other payloads
and kernels to work with it.

Change-Id: I28d74590e9f04d32bb2bbd461b67f15014f927ec
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47594
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 5 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index cc412ac..a695723 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -655,6 +655,11 @@
    keyboard_cmd(I8042_KBCMD_DEFAULT_DIS);
    keyboard_drain_input();

+	/* Nobody but us seems to still use scancode set #1.
+	   So try to hand over with more modern settings. */
+	set_scancode_set(2);
+	i8042_set_kbd_translation(false);
+
    /* Send keyboard disconnect command */
    i8042_cmd(I8042_CMD_DIS_KB);

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47594 )

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS4: 
This change breaks all recent chromebooks that pass the keymap via ACPI from coreboot when they run libpayload keyboard init (which only happens in developer mode).

The default power up state of the 8042 module in the chrome EC (and according to docs I can find online, like https://wiki.osdev.org/%228042%22_PS/2_Controller#Translation) is set2+translate (i.e. set1) which is the format that the keymap is provided to the kernel in (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...)

Since we don't always run the libpayload keyboard init because we don't always want keyboard input in firmware there isn't a way to consistently provide the keymap if it sometimes changes, so I don't think libpayload should be changing things from the default.  Or at least it should probably be guarded by a Kconfig.

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS4:
> This change breaks all recent chromebooks that pass the keymap via ACPI from coreboot when they run  […]

I think we can safely switch to set2 with translation. IIRC, Linux (w/o the
ACPI properties) wouldn't switch the translation off but should still work.
The only problem I encountered with Linux was that it expected set2 after
a keyboard reset.

I don't know what the actual benefits of set2 w/o translation for an average
keyboard are. If somebody misses something with translation enabled, we can
still revisit this.

NB. The ACPI story seems odd. I guess one can't use a random payload and
still expect Linux to work. Unless we would teach all payloads to return
to the settings found initially.

Change in coreboot[master]: libpayload/keyboard: Reset to scancode set #2 on exit

[Raul Rangel (Code Review)]

Raul Rangel has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/47594 )

Change subject: libpayload/keyboard: Reset to scancode set #2 on exit
......................................................................