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/+/47088
to review the following change.
Change subject: libpayload/keyboard: Implement self-test ......................................................................
libpayload/keyboard: Implement self-test
The keyboard self-test is required for some devices. At least one device (integrated keyboard in a ThinkPad X201) actually starts the test automatically leading to spurious output and no response for the first seconds.
We wait up to 5s for the self-test result. On failure or timeout, the command will be repeated indefinitely. This happens all in the background of the UI polling loop.
TODO: Figure out if we should make this optional.
Change-Id: Ie07b31e74d06e116ac81e76309621eed39a19b49 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 46 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/47088/1
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index a30bdaf..a9b2e61 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -244,6 +244,7 @@
static enum keyboard_state { STATE_INIT = 0, + STATE_SELF_TEST, STATE_CONFIGURE, STATE_ENABLE_SCAN, STATE_RUNNING, @@ -251,6 +252,16 @@
uint64_t state_time;
+static void start_self_test(void) +{ + if (!keyboard_cmd(I8042_KBCMD_RESET)) + printf("ERROR: Keyboard self-test couldn't be started.\n"); + /* We ignore errors and always move to the self-test state + which will simply try again if necessary. */ + keyboard_state = STATE_SELF_TEST; + state_time = timer_us(0); +} + static void keyboard_poll(void) { enum keyboard_state next_state = keyboard_state; @@ -261,6 +272,34 @@ /* Wait until keyboard_init() has been called. */ break;
+ case STATE_SELF_TEST: + if (!i8042_data_ready_ps2()) { + if (timer_us(state_time) > 5*1000*1000) { + start_self_test(); + return; + } + break; + } + + const uint8_t self_test_result = i8042_read_data_ps2(); + switch (self_test_result) { + case 0xaa: + /* Success! */ + break; + case 0xfc: + case 0xfd: + /* Failure. Try again. */ + start_self_test(); + return; + default: + printf("WARNING: Keyboard sent spurious 0x%02x\n", + self_test_result); + return; + } + + next_state = STATE_CONFIGURE; + break; + case STATE_CONFIGURE: /* * We only support scancode set 1. The controller translation @@ -269,8 +308,10 @@ * 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; + if (!set_scancode_set(controller_translates() ? 2 : 1)) { + start_self_test(); + return; + }
next_state = STATE_ENABLE_SCAN; break; @@ -279,8 +320,8 @@ /* Enable scanning */ if (!keyboard_cmd(I8042_KBCMD_EN)) { printf("ERROR: Keyboard enable scanning failed!\n"); - keyboard_state = STATE_CONFIGURE; - break; + start_self_test(); + return; }
next_state = STATE_RUNNING; @@ -451,8 +492,7 @@ keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); keyboard_drain_input();
- keyboard_state = STATE_CONFIGURE; - state_time = timer_us(0); + start_self_test();
console_add_input_driver(&cons); }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... PS1, Line 455: state_time = timer_us(0); We could also keep starting in STATE_CONFIGURE and let it fall back to the self-test on error.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... PS1, Line 295: printf("WARNING: Keyboard sent spurious 0x%02x\n", Add some more context, as we have several spurious messages now, so it wouldn’t be clear where it comes from?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... PS1, Line 295: printf("WARNING: Keyboard sent spurious 0x%02x\n",
Add some more context, as we have several spurious messages now, so it wouldn’t be clear where it co […]
Ack. This also needs some more work, as we wouldn't want to print anything by default here. In the worst case it would clutter the screen while you'd still be able to control things with a USB keyboard, for instance, if the screen was still readable.
Hello Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47088
to look at the new patch set (#2).
Change subject: libpayload/keyboard: Implement self-test ......................................................................
libpayload/keyboard: Implement self-test
The keyboard self-test is required for some devices. At least one device (integrated keyboard in a ThinkPad X201) actually starts the test automatically leading to spurious output and no response for the first seconds.
We wait up to 5s for the self-test result. On failure or timeout, the command will be repeated indefinitely. This happens all in the background of the UI polling loop.
To not unnecessarily delay the boot process, we first try an oppor- tunistic initialization which skips the self-test.
Change-Id: Ie07b31e74d06e116ac81e76309621eed39a19b49 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 47 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/47088/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... PS1, Line 295: printf("WARNING: Keyboard sent spurious 0x%02x\n",
Ack. This also needs some more work, as we wouldn't want to print anything […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 2: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47088/1/payloads/libpayload/drivers... PS1, Line 455: state_time = timer_us(0);
We could also keep starting in STATE_CONFIGURE and let it fall back to […]
Done
Hello Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47088
to look at the new patch set (#3).
Change subject: libpayload/keyboard: Implement self-test ......................................................................
libpayload/keyboard: Implement self-test
The keyboard self-test is required for some devices. At least one device (integrated keyboard in a ThinkPad X201) actually starts the test automatically leading to spurious output and no response for the first seconds.
We wait up to 5s for the self-test result. On failure or timeout, the command will be repeated until the 30s init timer runs out. This happens all in the background of the UI polling loop.
To not unnecessarily delay the boot process, we first try an oppor- tunistic initialization which skips the self-test.
Change-Id: Ie07b31e74d06e116ac81e76309621eed39a19b49 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 47 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/47088/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
Patch Set 3: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47088 )
Change subject: libpayload/keyboard: Implement self-test ......................................................................
libpayload/keyboard: Implement self-test
The keyboard self-test is required for some devices. At least one device (integrated keyboard in a ThinkPad X201) actually starts the test automatically leading to spurious output and no response for the first seconds.
We wait up to 5s for the self-test result. On failure or timeout, the command will be repeated until the 30s init timer runs out. This happens all in the background of the UI polling loop.
To not unnecessarily delay the boot process, we first try an oppor- tunistic initialization which skips the self-test.
Change-Id: Ie07b31e74d06e116ac81e76309621eed39a19b49 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/47088 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 47 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 114e18b..14089bd 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -233,9 +233,12 @@
static enum keyboard_state { STATE_INIT = 0, + STATE_SIMPLIFIED_INIT, STATE_DISABLE_SCAN, STATE_DRAIN_INPUT, STATE_DISABLE_TRANSLATION, + STATE_START_SELF_TEST, + STATE_SELF_TEST, STATE_CONFIGURE, STATE_CONFIGURE_SET1, STATE_ENABLE_TRANSLATION, @@ -258,6 +261,15 @@ /* Wait until keyboard_init() has been called. */ break;
+ case STATE_SIMPLIFIED_INIT: + /* On the first try, start opportunistically, do + the first steps at once and skip the self-test. */ + (void)keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); + keyboard_drain_input(); + (void)i8042_set_kbd_translation(false); + next_state = STATE_CONFIGURE; + break; + case STATE_DISABLE_SCAN: (void)keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); next_state = STATE_DRAIN_INPUT; @@ -274,7 +286,40 @@ case STATE_DISABLE_TRANSLATION: /* Be opportunistic and assume it's disabled on failure. */ (void)i8042_set_kbd_translation(false); - next_state = STATE_CONFIGURE; + next_state = STATE_START_SELF_TEST; + break; + + case STATE_START_SELF_TEST: + if (!keyboard_cmd(I8042_KBCMD_RESET)) + printf("ERROR: Keyboard self-test couldn't be started.\n"); + /* We ignore errors and always move to the self-test state + which will simply try again if necessary. */ + next_state = STATE_SELF_TEST; + break; + + case STATE_SELF_TEST: + if (!i8042_data_ready_ps2()) { + if (timer_us(state_time) > 5*1000*1000) + next_state = STATE_DISABLE_SCAN; + break; + } + + const uint8_t self_test_result = i8042_read_data_ps2(); + switch (self_test_result) { + case 0xaa: + /* Success! */ + next_state = STATE_CONFIGURE; + break; + case 0xfc: + case 0xfd: + /* Failure. Try again. */ + next_state = STATE_START_SELF_TEST; + break; + default: + printf("WARNING: Keyboard self-test received spurious 0x%02x\n", + self_test_result); + break; + } break;
case STATE_CONFIGURE: @@ -491,7 +536,7 @@ /* Enable first PS/2 port */ i8042_cmd(I8042_CMD_EN_KB);
- keyboard_state = STATE_DISABLE_SCAN; + keyboard_state = STATE_SIMPLIFIED_INIT; keyboard_time = state_time = timer_us(0);
console_add_input_driver(&cons);
-
Angel Pons (Code Review) -
Hung-Te Lin (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)