Change in coreboot[master]: libpayload/keyboard: Add debug output to all state transitions

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

libpayload/keyboard: Add debug output to all state transitions

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47591/1

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index ab516b4..dad8a44 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -250,9 +250,28 @@
    STATE_ENABLE_TRANSLATION,
    STATE_ENABLE_SCAN,
    STATE_RUNNING,
-	STATE_IGNORE,
+	STATE_IGNORE
 } keyboard_state;

+#define STATE_NAMES_ENTRY(name) [STATE_##name] = #name
+static const char *const state_names[] = {
+	STATE_NAMES_ENTRY(INIT),
+	STATE_NAMES_ENTRY(SIMPLIFIED_INIT),
+	STATE_NAMES_ENTRY(DISABLE_SCAN),
+	STATE_NAMES_ENTRY(DRAIN_INPUT),
+	STATE_NAMES_ENTRY(DISABLE_TRANSLATION),
+	STATE_NAMES_ENTRY(START_SELF_TEST),
+	STATE_NAMES_ENTRY(SELF_TEST),
+	STATE_NAMES_ENTRY(CONFIGURE),
+	STATE_NAMES_ENTRY(CONFIGURE_SET1),
+	STATE_NAMES_ENTRY(ENABLE_TRANSLATION),
+	STATE_NAMES_ENTRY(ENABLE_SCAN),
+	STATE_NAMES_ENTRY(RUNNING),
+	STATE_NAMES_ENTRY(IGNORE)
+};
+_Static_assert(ARRAY_SIZE(state_names) == STATE_IGNORE + 1,
+		"`state_names` need to be kept in sync.");
+
 static uint64_t keyboard_time;
 static uint64_t state_time;

@@ -305,20 +324,23 @@

case STATE_SELF_TEST:
    	if (!i8042_data_ready_ps2()) {
-			if (timer_us(state_time) > 5*1000*1000)
+			if (timer_us(state_time) > 5*1000*1000) {
+				debug("WARNING: Keyboard self-test timed out.\n");
    			next_state = STATE_DISABLE_SCAN;
+			}
    		break;
    	}

const uint8_t self_test_result = i8042_read_data_ps2();
    	switch (self_test_result) {
    	case 0xaa:
-			/* Success! */
+			debug("INFO: Keyboard self-test succeeded.\n");
    		next_state = STATE_CONFIGURE;
    		break;
    	case 0xfc:
    	case 0xfd:
    		/* Failure. Try again. */
+			debug("WARNING: Keyboard self-test failed.\n");
    		next_state = STATE_START_SELF_TEST;
    		break;
    	default:
@@ -387,6 +409,7 @@
    }

if (keyboard_state != next_state) {
+		debug("INFO: Keyboard advancing state to '%s'.\n", state_names[next_state]);
    	keyboard_state = next_state;
    	state_time = timer_us(0);
    }

[build bot (Jenkins) (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47591/1/payloads/libpayload/drivers... 
PS1, Line 256: #define STATE_NAMES_ENTRY(name) [STATE_##name] = #name
space prohibited before open square bracket '['

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47591/1/payloads/libpayload/drivers... 
PS1, Line 253: 	STATE_IGNORE
Where did the comma go?

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47591/1/payloads/libpayload/drivers... 
PS1, Line 253: 	STATE_IGNORE
> Where did the comma go?

Removed on purpose because this is supposed to be the last
state (as used in the assertion below). I know it's crude,
couldn't come up with something better (I tried to add a
STATE_LAST, or STATE_COUNT for that, but then GCC got sad
and warned I should handle it in the switch()).

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47591/1/payloads/libpayload/drivers... 
PS1, Line 253: 	STATE_IGNORE
> Removed on purpose because this is supposed to be the last […]

NB. Maybe we should get rid of the IGNORE state. As we start the
initialization automatically (and I wouldn't want to change that
to keep the regression potential low), we end up in the IGNORE
state when no keyboard is attached, hence breaking hot-plugging
after 30s :-/ it would need more accurate timeout handling to
fix that and keep an IGNORE state for incompatible keyboards
(which might break, again, ThinkPads on a cold boot).

[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/+/47591

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

libpayload/keyboard: Add debug output to all state transitions

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47591/2

[Nico Huber (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/47591/1/payloads/libpayload/drivers... 
PS1, Line 253: 	STATE_IGNORE
> NB. Maybe we should get rid of the IGNORE state. As we start the […]

All done. Got rid of the oddities and check `state_names` at runtime now.

Late hot-plugging should also work now.

[build bot (Jenkins) (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47591/2/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/47591/2/payloads/libpayload/drivers... 
PS2, Line 256: #define STATE_NAMES_ENTRY(name) [STATE_##name] = #name
space prohibited before open square bracket '['

[Angel Pons (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/47591/2/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/47591/2/payloads/libpayload/drivers... 
PS2, Line 256: #name
Note to forgetful myself: this preprocessor magic means "stringify"

[Hung-Te Lin (Code Review)]

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

Change subject: libpayload/keyboard: Add debug output to all state transitions
......................................................................

libpayload/keyboard: Add debug output to all state transitions

Change-Id: I643a821d4c41fc068f2bab0bd571b0a4a359f59a
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47591
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 31 insertions(+), 2 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 cb386e9..91a51bb 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -253,6 +253,31 @@
    STATE_IGNORE,
 } keyboard_state;

+#define STATE_NAMES_ENTRY(name) [STATE_##name] = #name
+static const char *const state_names[] = {
+	STATE_NAMES_ENTRY(INIT),
+	STATE_NAMES_ENTRY(SIMPLIFIED_INIT),
+	STATE_NAMES_ENTRY(DISABLE_SCAN),
+	STATE_NAMES_ENTRY(DRAIN_INPUT),
+	STATE_NAMES_ENTRY(DISABLE_TRANSLATION),
+	STATE_NAMES_ENTRY(START_SELF_TEST),
+	STATE_NAMES_ENTRY(SELF_TEST),
+	STATE_NAMES_ENTRY(CONFIGURE),
+	STATE_NAMES_ENTRY(CONFIGURE_SET1),
+	STATE_NAMES_ENTRY(ENABLE_TRANSLATION),
+	STATE_NAMES_ENTRY(ENABLE_SCAN),
+	STATE_NAMES_ENTRY(RUNNING),
+	STATE_NAMES_ENTRY(IGNORE),
+};
+
+__attribute__((unused))
+static const char *state_name(enum keyboard_state state)
+{
+	if (state >= ARRAY_SIZE(state_names) || !state_names[state])
+		return "<unknown>";
+	return state_names[state];
+}
+
 static uint64_t keyboard_time;
 static uint64_t state_time;

@@ -305,20 +330,23 @@

case STATE_SELF_TEST:
    	if (!i8042_data_ready_ps2()) {
-			if (timer_us(state_time) > 5*1000*1000)
+			if (timer_us(state_time) > 5*1000*1000) {
+				debug("WARNING: Keyboard self-test timed out.\n");
    			next_state = STATE_DISABLE_SCAN;
+			}
    		break;
    	}

const uint8_t self_test_result = i8042_read_data_ps2();
    	switch (self_test_result) {
    	case 0xaa:
-			/* Success! */
+			debug("INFO: Keyboard self-test succeeded.\n");
    		next_state = STATE_CONFIGURE;
    		break;
    	case 0xfc:
    	case 0xfd:
    		/* Failure. Try again. */
+			debug("WARNING: Keyboard self-test failed.\n");
    		next_state = STATE_START_SELF_TEST;
    		break;
    	default:
@@ -387,6 +415,7 @@
    }

if (keyboard_state != next_state) {
+		debug("INFO: Keyboard advancing state to '%s'.\n", state_name(next_state));
    	keyboard_state = next_state;
    	state_time = timer_us(0);
    }