Change in coreboot[master]: libpayload/keyboard: Turn init sequence into a state machine

[Nico Huber (Code Review)]

Hello  Felix Singer, Matt Delco, Patrick Georgi, Furquan Shaikh, Julius Werner, Angel Pons, Patrick Rudolph, EricR Lai,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

libpayload/keyboard: Turn init sequence into a state machine

We'll process the init sequence as part of the polling loop. This
should have several advantages:

* It eases error handling, i.e. we can return to an earlier state.
* We don't have to stall initialization when a keyboard takes a
  little longer.
* Generally, these keyboards can be hot-plugged (albeit not by
  design).

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47086/1

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index efe6bcd..27abd95 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -207,9 +207,96 @@
    return false;
 }

+/**
+ * Get translation state from the controller.
+ *
+ * We want to act on the state left by the firmware, same as
+ * Linux does. If an error occurs, we assume it's enabled by
+ * default, as the OSDev wiki suggests.
+ */
+static bool controller_translates(void)
+{
+	const int ctrl_cfg = i8042_cmd_with_response(I8042_CMD_RD_CMD_BYTE);
+	return ctrl_cfg < 0 || (ctrl_cfg & I8042_CMD_BYTE_XLATE);
+}
+
+static bool set_scancode_set(const unsigned char set)
+{
+	bool ret;
+
+	if (set < 1 || set > 3)
+		return false;
+
+	ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
+	if (!ret) {
+		printf("ERROR: Keyboard set scancode failed!\n");
+		return ret;
+	}
+
+	ret = keyboard_cmd(set);
+	if (!ret) {
+		printf("ERROR: Keyboard scancode set#%u failed!\n", set);
+		return ret;
+	}
+
+	return ret;
+}
+
+static enum keyboard_state {
+	STATE_INIT = 0,
+	STATE_CONFIGURE,
+	STATE_ENABLE_SCAN,
+	STATE_RUNNING,
+} keyboard_state;
+
+static void keyboard_poll(void)
+{
+	enum keyboard_state next_state = keyboard_state;
+
+	switch (keyboard_state) {
+
+	case STATE_INIT:
+		/* Wait until keyboard_init() has been called. */
+		break;
+
+	case STATE_CONFIGURE:
+		/*
+		 * 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.
+		 */
+		if (!set_scancode_set(controller_translates() ? 2 : 1))
+			break;
+
+		next_state = STATE_ENABLE_SCAN;
+		break;
+
+	case STATE_ENABLE_SCAN:
+		/* Enable scanning */
+		if (!keyboard_cmd(I8042_KBCMD_EN)) {
+			printf("ERROR: Keyboard enable scanning failed!\n");
+			keyboard_state = STATE_CONFIGURE;
+			break;
+		}
+
+		next_state = STATE_RUNNING;
+		break;
+
+	case STATE_RUNNING:
+		/* TODO: Use echo command to detect detach. */
+		break;
+	}
+
+	if (keyboard_state != next_state)
+		keyboard_state = next_state;
+}
+
 bool keyboard_havechar(void)
 {
-	return i8042_data_ready_ps2();
+	keyboard_poll();
+	return keyboard_state == STATE_RUNNING && i8042_data_ready_ps2();
 }

unsigned char keyboard_get_scancode(void)
@@ -342,43 +429,11 @@
    .input_type = CONSOLE_INPUT_TYPE_EC,
 };

-/**
- * Get translation state from the controller.
- *
- * We want to act on the state left by the firmware, same as
- * Linux does. If an error occurs, we assume it's enabled by
- * default, as the OSDev wiki suggests.
- */
-static bool controller_translates(void)
-{
-	const int ctrl_cfg = i8042_cmd_with_response(I8042_CMD_RD_CMD_BYTE);
-	return ctrl_cfg < 0 || (ctrl_cfg & I8042_CMD_BYTE_XLATE);
-}
-
-static bool set_scancode_set(const unsigned char set)
-{
-	bool ret;
-
-	if (set < 1 || set > 3)
-		return false;
-
-	ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret) {
-		printf("ERROR: Keyboard set scancode failed!\n");
-		return ret;
-	}
-
-	ret = keyboard_cmd(set);
-	if (!ret) {
-		printf("ERROR: Keyboard scancode set#%u failed!\n", set);
-		return ret;
-	}
-
-	return ret;
-}
-
 void keyboard_init(void)
 {
+	if (keyboard_state != STATE_INIT)
+		return;
+
    map = &keyboard_layouts[0];

/* Initialized keyboard controller. */
@@ -392,19 +447,7 @@
    keyboard_cmd(I8042_KBCMD_DEFAULT_DIS);
    keyboard_drain_input();

-	/*
-	 * 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);
-
-	/* Enable scanning */
-	const bool ret = keyboard_cmd(I8042_KBCMD_EN);
-	if (!ret)
-		printf("ERROR: Keyboard enable scanning failed!\n");
+	keyboard_state = STATE_CONFIGURE;

console_add_input_driver(&cons);
 }

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 1: Code-Review+1

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47086 )

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47086/1/payloads/libpayload/drivers... 
PS1, Line 455: void keyboard_disconnect(void)
Do we need to clear state if disconnect?

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47086/1/payloads/libpayload/drivers... 
PS1, Line 455: void keyboard_disconnect(void)
> Do we need to clear state if disconnect?

Yep, thanks! I'm not used to returning to the payload, but console_init()
can be run again at some point of course.

[Nico Huber (Code Review)]

Hello  Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Paul Menzel, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

libpayload/keyboard: Turn init sequence into a state machine

We'll process the init sequence as part of the polling loop. This
should have several advantages:

* It eases error handling, i.e. we can return to an earlier state.
* We don't have to stall initialization when a keyboard takes a
  little longer.
* Generally, these keyboards can be hot-plugged (albeit not by
  design).

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47086/2

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47086/1/payloads/libpayload/drivers... 
PS1, Line 455: void keyboard_disconnect(void)
> Yep, thanks! I'm not used to returning to the payload, but console_init() […]

Done (still have to teach payloads to call this function, though).

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Hello  Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Paul Menzel, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

libpayload/keyboard: Turn init sequence into a state machine

We'll process the init sequence as part of the polling loop. This
should have several advantages:

* It eases error handling, i.e. we can return to an earlier state.
* We don't have to stall initialization when a keyboard takes a
  little longer.
* Generally, these keyboards can be hot-plugged (albeit not by
  design).

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47086/3

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

Patch Set 3: Code-Review+2

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47086 )

Change subject: libpayload/keyboard: Turn init sequence into a state machine
......................................................................

libpayload/keyboard: Turn init sequence into a state machine

We'll process the init sequence as part of the polling loop. This
should have several advantages:

* It eases error handling, i.e. we can return to an earlier state.
* We don't have to stall initialization when a keyboard takes a
  little longer.
* Generally, these keyboards can be hot-plugged (albeit not by
  design).

Change-Id: I9cf5cf31eb420b3994bec20e56a72d37f3d2996e
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47086
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 138 insertions(+), 38 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 9da3902..dcba031 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -209,9 +209,139 @@
    return false;
 }

+static bool set_scancode_set(const unsigned char set)
+{
+	bool ret;
+
+	if (set < 1 || set > 3)
+		return false;
+
+	ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
+	if (!ret) {
+		printf("ERROR: Keyboard set scancode failed!\n");
+		return ret;
+	}
+
+	ret = keyboard_cmd(set);
+	if (!ret) {
+		printf("ERROR: Keyboard scancode set#%u failed!\n", set);
+		return ret;
+	}
+
+	return ret;
+}
+
+static enum keyboard_state {
+	STATE_INIT = 0,
+	STATE_DISABLE_SCAN,
+	STATE_DRAIN_INPUT,
+	STATE_DISABLE_TRANSLATION,
+	STATE_CONFIGURE,
+	STATE_CONFIGURE_SET1,
+	STATE_ENABLE_TRANSLATION,
+	STATE_ENABLE_SCAN,
+	STATE_RUNNING,
+	STATE_IGNORE,
+} keyboard_state;
+
+static uint64_t keyboard_time;
+
+static void keyboard_poll(void)
+{
+	enum keyboard_state next_state = keyboard_state;
+	unsigned int i;
+
+	switch (keyboard_state) {
+
+	case STATE_INIT:
+		/* Wait until keyboard_init() has been called. */
+		break;
+
+	case STATE_DISABLE_SCAN:
+		(void)keyboard_cmd(I8042_KBCMD_DEFAULT_DIS);
+		next_state = STATE_DRAIN_INPUT;
+		break;
+
+	case STATE_DRAIN_INPUT:
+		/* Limit number of bytes drained per poll. */
+		for (i = 0; i < 50 && i8042_data_ready_ps2(); ++i)
+			(void)i8042_read_data_ps2();
+		if (i == 0)
+			next_state = STATE_DISABLE_TRANSLATION;
+		break;
+
+	case STATE_DISABLE_TRANSLATION:
+		/* Be opportunistic and assume it's disabled on failure. */
+		(void)i8042_set_kbd_translation(false);
+		next_state = STATE_CONFIGURE;
+		break;
+
+	case STATE_CONFIGURE:
+		if (set_scancode_set(2))
+			next_state = STATE_ENABLE_TRANSLATION;
+		else
+			next_state = STATE_CONFIGURE_SET1;
+		break;
+
+	case STATE_CONFIGURE_SET1:
+		if (!set_scancode_set(1)) {
+			printf("ERROR: Keyboard failed to set any scancode set.\n");
+			next_state = STATE_DISABLE_SCAN;
+			break;
+		}
+
+		next_state = STATE_ENABLE_SCAN;
+		break;
+
+	case STATE_ENABLE_TRANSLATION:
+		if (i8042_set_kbd_translation(true) != 0) {
+			printf("ERROR: Keyboard controller set translation failed!\n");
+			next_state = STATE_DISABLE_SCAN;
+			break;
+		}
+
+		next_state = STATE_ENABLE_SCAN;
+		break;
+
+	case STATE_ENABLE_SCAN:
+		if (!keyboard_cmd(I8042_KBCMD_EN)) {
+			printf("ERROR: Keyboard enable scanning failed!\n");
+			next_state = STATE_DISABLE_SCAN;
+			break;
+		}
+
+		next_state = STATE_RUNNING;
+		break;
+
+	case STATE_RUNNING:
+		/* TODO: Use echo command to detect detach. */
+		break;
+
+	case STATE_IGNORE:
+		/* TODO: Try again after timeout if it ever seems useful. */
+		break;
+
+	}
+
+	switch (next_state) {
+	case STATE_INIT:
+	case STATE_RUNNING:
+	case STATE_IGNORE:
+		break;
+	default:
+		if (timer_us(keyboard_time) > 30*1000*1000)
+			next_state = STATE_IGNORE;
+		break;
+	}
+
+	if (keyboard_state != next_state)
+		keyboard_state = next_state;
+}
+
 bool keyboard_havechar(void)
 {
-	return i8042_data_ready_ps2();
+	keyboard_poll();
+	return keyboard_state == STATE_RUNNING && i8042_data_ready_ps2();
 }

unsigned char keyboard_get_scancode(void)
@@ -344,30 +474,11 @@
    .input_type = CONSOLE_INPUT_TYPE_EC,
 };

-static bool set_scancode_set(const unsigned char set)
-{
-	bool ret;
-
-	if (set < 1 || set > 3)
-		return false;
-
-	ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret) {
-		printf("ERROR: Keyboard set scancode failed!\n");
-		return ret;
-	}
-
-	ret = keyboard_cmd(set);
-	if (!ret) {
-		printf("ERROR: Keyboard scancode set#%u failed!\n", set);
-		return ret;
-	}
-
-	return ret;
-}
-
 void keyboard_init(void)
 {
+	if (keyboard_state != STATE_INIT)
+		return;
+
    map = &keyboard_layouts[0];

/* Initialized keyboard controller. */
@@ -377,21 +488,8 @@
    /* Enable first PS/2 port */
    i8042_cmd(I8042_CMD_EN_KB);

-	/* Disable scanning */
-	keyboard_cmd(I8042_KBCMD_DEFAULT_DIS);
-	keyboard_drain_input();
-
-	i8042_set_kbd_translation(false);
-
-	if (set_scancode_set(2))
-		i8042_set_kbd_translation(true);
-	else if (!set_scancode_set(1))
-		return; /* give up, leave keyboard input disabled */
-
-	/* Enable scanning */
-	const bool ret = keyboard_cmd(I8042_KBCMD_EN);
-	if (!ret)
-		printf("ERROR: Keyboard enable scanning failed!\n");
+	keyboard_state = STATE_DISABLE_SCAN;
+	keyboard_time = timer_us(0);

console_add_input_driver(&cons);
 }
@@ -418,4 +516,6 @@

/* Release keyboard controller driver */
    i8042_close();
+
+	keyboard_state = STATE_INIT;
 }