Change in coreboot[master]: libpayload/drivers/i8042: If configured ignore all i8042 init failures

[Mathew King (Code Review)]

Mathew King has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

libpayload/drivers/i8042: If configured ignore all i8042 init failures

Ensure that when LP_PC_KEYBOARD_IGNORE_INIT_FAILURE is set all init
failures are ignored.

BUG=b:145130110
TEST=Draillion keyboard is usable on every boot.

Change-Id: Ifb1c908b11afb8098a6cbbfa8a3d4391cabacd0f
Signed-off-by: Mathew King 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 14 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/37455/1

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index f9932ed..e4ed261 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -312,9 +312,18 @@
    .input_type = CONSOLE_INPUT_TYPE_EC,
 };

+static int keyboard_init_cmd(unsigned char cmd, const char *msg)
+{
+	if (!keyboard_cmd(cmd) && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
+		printf("ERROR: Keyboard %s failed!\n", msg);
+		return 0;
+	}
+
+	return 1;
+}
+
 void keyboard_init(void)
 {
-	unsigned int ret;
    map = &keyboard_layouts[0];

/* Initialized keyboard controller. */
@@ -329,34 +338,22 @@
    i8042_cmd(I8042_CMD_EN_KB);

/* 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 (!keyboard_init_cmd(I8042_KBCMD_SET_SCANCODE, "set scancode"))
    	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 (!keyboard_init_cmd(I8042_SCANCODE_SET_1, "scancode set#1"))
    	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 (!keyboard_init_cmd(I8042_KBCMD_SET_DEFAULT, "set default params"))
    	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 (!keyboard_init_cmd(I8042_KBCMD_EN, "enable scanning"))
    	return;
-	}

console_add_input_driver(&cons);
 }

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 1: Code-Review+2

wish we could figure out why this EC doesn't like keyboard init sequences...

[EricR Lai (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 1:

@Mat, could you cc me into this? b:145130110. I want to check this will impact factory or not.

[EricR Lai (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 1: Code-Review-1

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/37455/1//COMMIT_MSG@10 
PS1, Line 10: failures are ignored.
Please elaborate, that you factor out common code, and that in the end you ignore only the QEMU workaround.

https://review.coreboot.org/c/coreboot/+/37455/1//COMMIT_MSG@11 
PS1, Line 11: 
Not everyone has access to the bug tracker, and you should not expect people using git to have access to the Internet all the time. So please summarize the error you are seeing, as the comment states that the line is for QEMU.

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

https://review.coreboot.org/c/coreboot/+/37455/1/payloads/libpayload/drivers... 
PS1, Line 348: 0xf6
Please make that a separate commit, so each commit can be easily reverted in case of errors.

[Mathew King (Code Review)]

Hello EricR Lai, Simon Glass, Paul Menzel, Duncan Laurie, build bot (Jenkins),

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

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

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

libpayload/drivers/i8042: If configured ignore all i8042 init failures

There are keyboards that do not respond properly to init command but are
still usable. The flag LP_PC_KEYBOARD_IGNORE_INIT_FAILURE was added to
ignore init failures on boards where this issue happens. coreboot:23584
added another init command without checking that the flag was not set so
Drallion boards will sometimes return an error when this init command is
sent causing the keyboard driver to not be added and then the keyboard
does not work.

This change standardizes the keyboard init logic to ensure that:
1. The ignore init failure flag is always checked
2. Error messages are always printed if an command fails (and the
   init failure flag is not set) in a standard fashion
3. The proper named command is used

BUG=b:145130110
TEST=Draillion keyboard is usable on every boot.

Change-Id: Ifb1c908b11afb8098a6cbbfa8a3d4391cabacd0f
Signed-off-by: Mathew King 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 14 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/37455/2

[Mathew King (Code Review)]

Hello EricR Lai, Simon Glass, Paul Menzel, Duncan Laurie, build bot (Jenkins),

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

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

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

libpayload/drivers/i8042: If configured ignore all i8042 init failures

There are keyboards that do not respond properly to init command but are
still usable. The flag LP_PC_KEYBOARD_IGNORE_INIT_FAILURE was added to
ignore init failures on boards where this issue happens. CB:23584
added another init command without checking that the flag was not set so
Drallion boards will sometimes return an error when this init command is
sent causing the keyboard driver to not be added and then the keyboard
does not work.

This change standardizes the keyboard init logic to ensure that:
1. The ignore init failure flag is always checked
2. Error messages are always printed if an command fails (and the
   init failure flag is not set) in a standard fashion
3. The proper named command is used

BUG=b:145130110
TEST=Draillion keyboard is usable on every boot.

Change-Id: Ifb1c908b11afb8098a6cbbfa8a3d4391cabacd0f
Signed-off-by: Mathew King 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 14 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/37455/3

[Mathew King (Code Review)]

Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 3:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/37455/1//COMMIT_MSG@10 
PS1, Line 10: failures are ignored.
> Please elaborate, that you factor out common code, and that in the end you ignore only the QEMU work […]

Done

https://review.coreboot.org/c/coreboot/+/37455/1//COMMIT_MSG@11 
PS1, Line 11:
> Not everyone has access to the bug tracker, and you should not expect people using git to have acces […]

Done

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

https://review.coreboot.org/c/coreboot/+/37455/1/payloads/libpayload/drivers... 
PS1, Line 348: 0xf6
> Please make that a separate commit, so each commit can be easily reverted in case of errors.

I changed my commit message to make it clear that this is cleaning up what was added in CB:23584, I think that changing a from a hardcoded value to something more well defined is in scope here.

[Paul Menzel (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 3: -Code-Review

[Paul Menzel (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37455/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37455/3//COMMIT_MSG@19 
PS3, Line 19: an
a

[Mathew King (Code Review)]

Hello EricR Lai, Simon Glass, Paul Menzel, Duncan Laurie, build bot (Jenkins),

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

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

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

libpayload/drivers/i8042: If configured ignore all i8042 init failures

There are keyboards that do not respond properly to init command but are
still usable. The flag LP_PC_KEYBOARD_IGNORE_INIT_FAILURE was added to
ignore init failures on boards where this issue happens. CB:23584
added another init command without checking that the flag was not set so
Drallion boards will sometimes return an error when this init command is
sent causing the keyboard driver to not be added and then the keyboard
does not work.

This change standardizes the keyboard init logic to ensure that:
1. The ignore init failure flag is always checked
2. Error messages are always printed if a command fails (and the
   init failure flag is not set) in a standard fashion
3. The proper named command is used

BUG=b:145130110
TEST=Draillion keyboard is usable on every boot.

Change-Id: Ifb1c908b11afb8098a6cbbfa8a3d4391cabacd0f
Signed-off-by: Mathew King 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 14 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/37455/4

[Mathew King (Code Review)]

Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37455/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37455/3//COMMIT_MSG@19 
PS3, Line 19: an
> a

Done

[EricR Lai (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
PS4, Line 338: 	i8042_cmd(I8042_CMD_EN_KB);
@Mat, please check my comment in tracker. We can skip from here if EC don't want to change it.
Then we no longer need the flag LP_PC_KEYBOARD_IGNORE_INIT_FAILURE.

[Paul Menzel (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37455/1/payloads/libpayload/drivers... 
PS1, Line 348: 0xf6
> I changed my commit message to make it clear that this is cleaning up what was added in CB:23584, I  […]

For the future. Git makes it easy to commit small changes, so that should be used. First, it makes review easier as undisputed (trivial) changes can be accepted/submitted very quickly. Second, if your change would be reverted, this undisputed fix would also be reverted, which is unwanted.

[Mathew King (Code Review)]

Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
PS4, Line 338: 	i8042_cmd(I8042_CMD_EN_KB);
> @Mat, please check my comment in tracker. We can skip from here if EC don't want to change it. […]

I think your suggestion may have merit but I believe that we should move forward with this change and make sure we understand the full impact of your suggestion on the bug before making any further changes in this area.

[EricR Lai (Code Review)]

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

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/c/coreboot/+/37455/4/payloads/libpayload/drivers... 
PS4, Line 338: 	i8042_cmd(I8042_CMD_EN_KB);
> I think your suggestion may have merit but I believe that we should move forward with this change an […]

Please check this CL:https://review.coreboot.org/c/coreboot/+/37594, it can avoid the same issue again.

[Mathew King (Code Review)]

Mathew King has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37455 )

Change subject: libpayload/drivers/i8042: If configured ignore all i8042 init failures
......................................................................

Abandoned