Change in ...coreboot[master]: Revert "libpayload: Reset PS/2 keyboard"

[Nico Huber (Code Review)]

Hello Patrick Rudolph, Paul Menzel, Patrick Georgi, Furquan Shaikh,

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

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

to review the following change.

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Revert "libpayload: Reset PS/2 keyboard"

Documentation is scarce on the matter, however the related coreboot
code suggests that after the ACK, the keyboard also sends the result
of the self test (passed/failed). It looks like this result is never
consumed here, probably resulting in further confusion for later com-
mands.

Let's revert this for now (if it's not too late for the 4.10 release)
and break things later again. IMHO, due to the fact that there are
dozens of different keyboard controller and keyboard implementations
and no accurate specification followed, such changes should be tested
on a lot of hardware before merge.

This reverts commit a99ed13e3397bc536012120aab8cadb827913863.
This reverts commit 7ae606f57f0b3d450ae748141b0e2367041b27d3.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33244/1

diff --git a/payloads/libpayload/drivers/i8042/i8042.h b/payloads/libpayload/drivers/i8042/i8042.h
index e864ac9..643167e 100644
--- a/payloads/libpayload/drivers/i8042/i8042.h
+++ b/payloads/libpayload/drivers/i8042/i8042.h
@@ -63,7 +63,6 @@
 #define I8042_KBCMD_EN			0xf4
 #define I8042_KBCMD_DEFAULT_DIS		0xf5
 #define I8042_KBCMD_SET_DEFAULT		0xf6
-#define I8042_KBCMD_ACK			0xfa
 #define I8042_KBCMD_RESEND		0xfe
 #define I8042_KBCMD_RESET		0xff

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index 240385c..3e5f988 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -172,7 +172,7 @@
 {
    i8042_write_data(cmd);

-	return i8042_wait_read_ps2() == I8042_KBCMD_ACK;
+	return i8042_wait_read_ps2() == 0xfa;
 }

int keyboard_havechar(void)
@@ -317,42 +317,27 @@
    /* Enable first PS/2 port */
    i8042_cmd(I8042_CMD_EN_KB);

-	/* Reset keyboard and self test (keyboard side) */
-	ret = keyboard_cmd(I8042_KBCMD_RESET);
-	if (!ret) {
-		printf("ERROR: Keyboard reset failed!\n");
-		return;
-	}
-
    /* Set scancode set 1 */
    ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard set scancode failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

ret = keyboard_cmd(I8042_SCANCODE_SET_1);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard scancode set#1 failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

/*
     * Set default parameters.
     * Fix for broken QEMU ps/2 make scancodes.
     */
    ret = keyboard_cmd(0xf6);
-	if (!ret) {
-		printf("ERROR: Keyboard set default params failed!\n");
+	if (!ret)
    	return;
-	}

/* Enable scanning */
    ret = keyboard_cmd(I8042_KBCMD_EN);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard enable scanning failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

console_add_input_driver(&cons);
 }

[Angel Pons (Code Review)]

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

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Patch Set 1: Code-Review+1

At least for the 4.10 release, I would like to see these patches reverted, since it is known they have caused issues.

I would cast a +2, but I would like to see others' opinion on the matter as well.

[Martin Kepplinger (Code Review)]

Martin Kepplinger has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33244 )

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Patch Set 1: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33244 )

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Patch Set 1: Code-Review+2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33244 )

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Patch Set 1: Code-Review+2

lets revert this for the release and then investigate afterwards how to fix the regression

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33244 )

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/33244/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33244/1//COMMIT_MSG@10 
PS1, Line 10: the keyboard also sends the result
            : of the self test (passed/failed)
That is what I found in the keyboard controller implementation of Chrome EC as well (i.e. it sends OK twice after the ACK). However, I could not trace the history of why it was done or any documentation to support it. I found this ancient document [1] and its abridged version [2] that indicate reset sends back ACK but does not talk about OK/FAIL response. If you run into any code comments/document that talk about the passed/failed response, I would love to add it to my references. Thanks!

[1] http://bitsavers.trailing-edge.com/pdf/ibm/pc/at/1502494_PC_AT_Technical_Ref...

[2] http://zet.aluzina.org/images/d/d4/8042.pdf

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33244 )

Change subject: Revert "libpayload: Reset PS/2 keyboard"
......................................................................

Revert "libpayload: Reset PS/2 keyboard"

Documentation is scarce on the matter, however the related coreboot
code suggests that after the ACK, the keyboard also sends the result
of the self test (passed/failed). It looks like this result is never
consumed here, probably resulting in further confusion for later com-
mands.

Let's revert this for now (if it's not too late for the 4.10 release)
and break things later again. IMHO, due to the fact that there are
dozens of different keyboard controller and keyboard implementations
and no accurate specification followed, such changes should be tested
on a lot of hardware before merge.

This reverts commit a99ed13e3397bc536012120aab8cadb827913863.
This reverts commit 7ae606f57f0b3d450ae748141b0e2367041b27d3.

Change-Id: I4d4304d5d8a01e013feac61016c59bcaeea81140
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33244
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Martin Kepplinger 
Reviewed-by: Patrick Georgi 
Reviewed-by: Felix Held 
Reviewed-by: Furquan Shaikh 
---
M payloads/libpayload/drivers/i8042/i8042.h
M payloads/libpayload/drivers/i8042/keyboard.c
2 files changed, 5 insertions(+), 21 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Felix Held: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Martin Kepplinger: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/payloads/libpayload/drivers/i8042/i8042.h b/payloads/libpayload/drivers/i8042/i8042.h
index e864ac9..643167e 100644
--- a/payloads/libpayload/drivers/i8042/i8042.h
+++ b/payloads/libpayload/drivers/i8042/i8042.h
@@ -63,7 +63,6 @@
 #define I8042_KBCMD_EN			0xf4
 #define I8042_KBCMD_DEFAULT_DIS		0xf5
 #define I8042_KBCMD_SET_DEFAULT		0xf6
-#define I8042_KBCMD_ACK			0xfa
 #define I8042_KBCMD_RESEND		0xfe
 #define I8042_KBCMD_RESET		0xff

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index 240385c..3e5f988 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -172,7 +172,7 @@
 {
    i8042_write_data(cmd);

-	return i8042_wait_read_ps2() == I8042_KBCMD_ACK;
+	return i8042_wait_read_ps2() == 0xfa;
 }

int keyboard_havechar(void)
@@ -317,42 +317,27 @@
    /* Enable first PS/2 port */
    i8042_cmd(I8042_CMD_EN_KB);

-	/* Reset keyboard and self test (keyboard side) */
-	ret = keyboard_cmd(I8042_KBCMD_RESET);
-	if (!ret) {
-		printf("ERROR: Keyboard reset failed!\n");
-		return;
-	}
-
    /* Set scancode set 1 */
    ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard set scancode failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

ret = keyboard_cmd(I8042_SCANCODE_SET_1);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard scancode set#1 failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

/*
     * Set default parameters.
     * Fix for broken QEMU ps/2 make scancodes.
     */
    ret = keyboard_cmd(0xf6);
-	if (!ret) {
-		printf("ERROR: Keyboard set default params failed!\n");
+	if (!ret)
    	return;
-	}

/* Enable scanning */
    ret = keyboard_cmd(I8042_KBCMD_EN);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
-		printf("ERROR: Keyboard enable scanning failed!\n");
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
    	return;
-	}

console_add_input_driver(&cons);
 }