Change in coreboot[master]: [RFC] libpayload/keyboard: Dynamically toggle controller translation

[Nico Huber (Code Review)]

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

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

[RFC] libpayload/keyboard: Dynamically toggle controller translation

This is what would be necessary to detect cases where we have to
toggle the keyboard controller's scancode translation.

After setting the matching scancode set for the current translation
setting, we read it back and try to adapt the translation setting
of necessary.

It adds a lot complexity, so we'd probably want to avoid it? Chances
that we find a keyboard that can't switch scancode sets and isn't
integrated (where we could set the correct translation setting in
coreboot) seem rather low.

Change-Id: I938184875e1c4c40cdb2762e1c5b6ed9c8f82b90
Signed-off-by: Nico Huber 
---
M payloads/libpayload/drivers/i8042/i8042.c
M payloads/libpayload/drivers/i8042/i8042.h
M payloads/libpayload/drivers/i8042/keyboard.c
3 files changed, 102 insertions(+), 21 deletions(-)

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

diff --git a/payloads/libpayload/drivers/i8042/i8042.c b/payloads/libpayload/drivers/i8042/i8042.c
index 317d04b..487863e 100644
--- a/payloads/libpayload/drivers/i8042/i8042.c
+++ b/payloads/libpayload/drivers/i8042/i8042.c
@@ -317,6 +317,24 @@
 }

/**
+ * Send command & data to keyboard controller.
+ *
+ * @param cmd: The command to be send.
+ * @param data: The data to be send.
+ * Returns 0 on success, -1 on failure.
+ */
+int i8042_cmd_with_data(const u8 cmd, const u8 data)
+{
+	const int ret = i8042_cmd(cmd);
+	if (ret != 0)
+		return ret;
+
+	i8042_write_data(data);
+
+	return ret;
+}
+
+/**
  * Probe for keyboard controller data and queue it.
  */
 static void i8042_data_poll(void)
diff --git a/payloads/libpayload/drivers/i8042/i8042.h b/payloads/libpayload/drivers/i8042/i8042.h
index 68fd149..f2dc0d0 100644
--- a/payloads/libpayload/drivers/i8042/i8042.h
+++ b/payloads/libpayload/drivers/i8042/i8042.h
@@ -29,6 +29,8 @@
 #ifndef __DRIVERS_I8042_I8042_H__
 #define __DRIVERS_I8042_I8042_H__

+#include <stdint.h>
+
 /* Port 0x64 commands */
 #define I8042_CMD_RD_CMD_BYTE		0x20
 #define I8042_CMD_WR_CMD_BYTE		0x60
@@ -67,5 +69,6 @@
 #define I8042_KBCMD_RESET		0xff

int i8042_cmd_with_response(u8 cmd);
+int i8042_cmd_with_data(u8 cmd, u8 data);

#endif /* __DRIVERS_I8042_I8042_H__ */
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index aa21b9d..c3cba9b 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -27,6 +27,7 @@
  * SUCH DAMAGE.
  */

+#include <assert.h>
 #include <stdbool.h>

#include <keycodes.h>
@@ -326,27 +327,90 @@
    return ctrl_cfg < 0 || (ctrl_cfg & I8042_CMD_BYTE_XLATE);
 }

-/* Set scancode set 1 */
-static bool set_scancode_set(const unsigned char set)
+static bool send_scancode_set(const unsigned char set)
 {
-	bool ret;
+	assert(set <= 3);

-	if (set < 1 || set > 3)
+	if (!keyboard_cmd(I8042_KBCMD_SET_SCANCODE) ||
+	    !keyboard_cmd(set)) {
+		printf("ERROR: Keyboard send scancode set#%u failed!\n", set);
+		return false;
+	}
+	return true;
+}
+
+static unsigned char select_scancode_set(const bool xlate)
+{
+	/*
+	 * We only support scancode set 1. The controller translation
+	 * would translate from set 2 to 1 for us, so we try to configure:
+	 *
+	 *   o set 1 if the controller doesn't translate, and
+	 *   o set 2 if the controller does.
+	 */
+	const unsigned char set = xlate ? 2 : 1;
+
+	if (!send_scancode_set(set))
+		return 2;
+
+	/*
+	 * Now try to confirm the current scancode set selection.
+	 */
+	if (!send_scancode_set(0))	/* retrieve current scancode set # */
+		return 2;
+
+	int actual_set = i8042_wait_read_ps2();
+	if (xlate) {
+		/* controllers translate even this answer */
+		switch (actual_set) {
+			case 0x43: actual_set = 1; break;
+			case 0x41: actual_set = 2; break;
+			case 0x3f: actual_set = 3; break;
+		}
+	}
+
+	if (actual_set < 1)	/* assume success on unexpected data */
+		return set;
+
+	return actual_set;
+}
+
+static bool prepare_scancode_set(void)
+{
+	const bool xlate = controller_translates();
+
+	const unsigned char set = select_scancode_set(xlate);
+
+	if (set > 2)	/* We can't do anything with scancode set #3. */
    	return false;

-	ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret) {
-		printf("ERROR: Keyboard set scancode failed!\n");
-		return ret;
+	if ((set != 2 && !xlate) || (set == 2 && xlate))
+		return true;
+
+	/*
+	 * In case we failed to set the correct scancode set, fall back
+	 * and try to change the controller's translation setting.
+	 */
+	printf("WARNING: Keyboard failed to select correct scancode set.\n");
+
+	int ctrl_cfg = i8042_cmd_with_response(I8042_CMD_RD_CMD_BYTE);
+	if (ctrl_cfg < 0) {
+		printf("ERROR: i8042_cmd RD_CMD failed!\n");
+		return false;
    }

-	ret = keyboard_cmd(set);
-	if (!ret) {
-		printf("ERROR: Keyboard scancode set#%u failed!\n", set);
-		return ret;
+	if (xlate)
+		ctrl_cfg &= ~I8042_CMD_BYTE_XLATE;
+	else
+		ctrl_cfg |= I8042_CMD_BYTE_XLATE;
+
+	const int ret = i8042_cmd_with_data(I8042_CMD_WR_CMD_BYTE, ctrl_cfg);
+	if (ret < 0) {
+		printf("ERROR: i8042_cmd WR_CMD failed!\n");
+		return false;
    }

-	return ret;
+	return true;
 }

void keyboard_init(void)
@@ -364,14 +428,10 @@
    /* Enable first PS/2 port */
    i8042_cmd(I8042_CMD_EN_KB);

-	/*
-	 * We only support scancode set 1. The controller translation
-	 * would translate from set 2 to 1 for us, so we try to configure
-	 *
-	 *   o set 1 if the controller doesn't translate, and
-	 *   o set 2 if the controller does.
-	 */
-	(void)set_scancode_set(controller_translates() ? 2 : 1);
+	if (!prepare_scancode_set()) {
+		printf("ERROR: Keyboard failed to configure scancode set, giving up.\n");
+		return;
+	}

/* Enable scanning */
    const bool ret = keyboard_cmd(I8042_KBCMD_EN);

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46730 )

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/46730/1/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/46730/1/payloads/libpayload/drivers... 
PS1, Line 365: 		switch (actual_set) {
switch and case should be at the same indent

https://review.coreboot.org/c/coreboot/+/46730/1/payloads/libpayload/drivers... 
PS1, Line 366: 			case 0x43: actual_set = 1; break;
trailing statements should be on next line

https://review.coreboot.org/c/coreboot/+/46730/1/payloads/libpayload/drivers... 
PS1, Line 367: 			case 0x41: actual_set = 2; break;
trailing statements should be on next line

https://review.coreboot.org/c/coreboot/+/46730/1/payloads/libpayload/drivers... 
PS1, Line 368: 			case 0x3f: actual_set = 3; break;
trailing statements should be on next line

[Paul Menzel (Code Review)]

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

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46730/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46730/1//COMMIT_MSG@14 
PS1, Line 14: of
if

[Nico Huber (Code Review)]

Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46730 )

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Abandoned

Looking at other drivers, they all assume that scancode-set selection should work.

[Nico Huber (Code Review)]

Nico Huber has restored this change. ( https://review.coreboot.org/c/coreboot/+/46730 )

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Restored

[Nico Huber (Code Review)]

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

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Patch Set 1:

Found some (newer, also USB capabale) PS/2 keyboards that probably
can't be switched to scancode set #1 :-/

[Nico Huber (Code Review)]

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

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Patch Set 1:

One alternative solution would be to follow SeaBIOS. They turn
the controller's translation off during init, select scancode
set #2 and then turn the translation on. Or we just teach lib-
payload set #2...

[M] Change in coreboot[master]: [RFC] libpayload/keyboard: Dynamically toggle controller translation

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46730?usp=email )

Change subject: [RFC] libpayload/keyboard: Dynamically toggle controller translation
......................................................................

Abandoned