Change in coreboot[master]: libpayload/keyboard: Revise scancode set and translation config
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
libpayload/keyboard: Revise scancode set and translation config
Some background first: The original XT keyboards used what we call scancode set #1 today. The PC/AT keyboards introduced scancode set #2, but for compatibility, its controller translated scancodes back to set #1 by default. Newer keyboards (maybe all we have to deal with) also support switching the scancode set.
This means the translation option in the controller and the scancode set selection in the keyboard have to match. In libpayload, we only support set #1 scancodes. So we either need the controller's trans- lation on and set #2 selected in the keyboard, or the controller's translation off and set #1 selected in the keyboard.
Valid configurations: * SET #1 + XLATE off * SET #2 + XLATE on
The Linux kernel leaves the decision to enable or disable the con- troller's translation to the firmware. We follow that behaviour, because Linux can be a payload too, and ideally want any payload to succeed the same with any given coreboot image.
This way, coreboot (or the controller firmware) can decide how to start best. For instance, if the keyboard is integrated and one knows for sure that it can switch scancode sets, no translation should be necessary.
Currently, coreboot leaves the controller's translation at its default setting, unless DRIVERS_PS2_KEYBOARD is enabled. The latter enables the translation unconditionally. For QEMU this means that the option effectively toggles the translation, as QEMU's controller has it disabled by default. This probably made a lot of earlier testing inconsistent.
Fixes: commit a95a6bf646 (libpayload/drivers/i8402/kbd: Fix qemu) The reset introduced there effectively reverted the scancode selection made before. It's unclear if later changes to the code where only necessary to work around it.
Change-Id: Iad85af516a7b9f9c0269ff9652ed15ee81700057 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c 2 files changed, 30 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/46724/1
diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index b5dc9a3..298a7ad 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -367,10 +367,6 @@ default y if ARCH_X86 # uses IO default n
-config PC_KEYBOARD_AT_TRANSLATED - bool "AT Translation keyboard device" - default n - config PC_KEYBOARD_LAYOUT_US bool "English (US) keyboard layout" depends on PC_KEYBOARD diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 2dec3a3..aa21b9d 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -313,55 +313,36 @@ .input_type = CONSOLE_INPUT_TYPE_EC, };
-/* Enable keyboard translated */ -static bool enable_translated(void) +/** + * 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) { - if (!i8042_cmd(I8042_CMD_RD_CMD_BYTE)) { - int cmd = i8042_read_data_ps2(); - cmd |= I8042_CMD_BYTE_XLATE; - if (!i8042_cmd(I8042_CMD_WR_CMD_BYTE)) { - i8042_write_data(cmd); - } else { - printf("ERROR: i8042_cmd WR_CMD failed!\n"); - return false; - } - } else { - printf("ERROR: i8042_cmd RD_CMD failed!\n"); - return false; - } - return true; + const int ctrl_cfg = i8042_cmd_with_response(I8042_CMD_RD_CMD_BYTE); + return ctrl_cfg < 0 || (ctrl_cfg & I8042_CMD_BYTE_XLATE); }
/* Set scancode set 1 */ -static bool set_scancode_set(void) +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(I8042_SCANCODE_SET_1); + ret = keyboard_cmd(set); if (!ret) { - printf("ERROR: Keyboard scancode set#1 failed!\n"); - return ret; - } - - /* - * Set default parameters. - * Fix for broken QEMU PS/2 make scancodes. - */ - ret = keyboard_cmd(I8042_KBCMD_SET_DEFAULT); - if (!ret) { - printf("ERROR: Keyboard set default params failed!\n"); - return ret; - } - - /* Enable scanning */ - ret = keyboard_cmd(I8042_KBCMD_EN); - if (!ret) { - printf("ERROR: Keyboard enable scanning failed!\n"); + printf("ERROR: Keyboard scancode set#%u failed!\n", set); return ret; }
@@ -383,13 +364,19 @@ /* Enable first PS/2 port */ i8042_cmd(I8042_CMD_EN_KB);
- if (CONFIG(LP_PC_KEYBOARD_AT_TRANSLATED)) { - if (!enable_translated()) - return; - } else { - if (!set_scancode_set()) - return; - } + /* + * 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");
console_add_input_driver(&cons); }
Nico Huber has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
libpayload/keyboard: Revise scancode set and translation config
Some background first: The original XT keyboards used what we call scancode set #1 today. The PC/AT keyboards introduced scancode set #2, but for compatibility, its controller translated scancodes back to set #1 by default. Newer keyboards (maybe all we have to deal with) also support switching the scancode set.
This means the translation option in the controller and the scancode set selection in the keyboard have to match. In libpayload, we only support set #1 scancodes. So we either need the controller's trans- lation on and set #2 selected in the keyboard, or the controller's translation off and set #1 selected in the keyboard.
Valid configurations: * SET #1 + XLATE off * SET #2 + XLATE on
The Linux kernel leaves the decision to enable or disable the con- troller's translation to the firmware. We follow that behaviour, because Linux can be a payload too, and ideally want any payload to succeed the same with any given coreboot image.
This way, coreboot (or the controller firmware) can decide how to start best. For instance, if the keyboard is integrated and one knows for sure that it can switch scancode sets, no translation should be necessary.
Currently, coreboot leaves the controller's translation at its default setting, unless DRIVERS_PS2_KEYBOARD is enabled. The latter enables the translation unconditionally. For QEMU this means that the option effectively toggles the translation, as QEMU's controller has it disabled by default. This probably made a lot of earlier testing inconsistent.
Fixes: commit a95a6bf646 (libpayload/drivers/i8402/kbd: Fix qemu) The reset introduced there effectively reverted the scancode selection made before. It's unclear if later changes to the code where only necessary to work around it.
Change-Id: Iad85af516a7b9f9c0269ff9652ed15ee81700057 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/i8042.c M payloads/libpayload/drivers/i8042/i8042.h M payloads/libpayload/drivers/i8042/keyboard.c 4 files changed, 33 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/46724/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG@45 PS2, Line 45: where were
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG@43 PS2, Line 43: reverted the scancode : selection made before "because scancode set #2 is usually the default"
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 329: /* Set scancode set 1 */ Needs an update.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
Hey folks, I hope I figured things out now. Would be nice to get this tested as much as possible, especially on the Chromebooks (Drallion?) that needed the PC_KEYBOARD_AT_TRANSLATED Kconfig.
So far I tested only QEMU. Will check on some laptops soon.
There is also a follow up that I don't deemed necessary. In theory keyboards that don't allow switching the scancode set might exist. But as long as we don't have any or can set the correct translation mode in coreboot, I guess we can keep it unmerged.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
Patch Set 2:
Hey folks, I hope I figured things out now. Would be nice to get this tested as much as possible, especially on the Chromebooks (Drallion?) that needed the PC_KEYBOARD_AT_TRANSLATED Kconfig.
So far I tested only QEMU. Will check on some laptops soon.
There is also a follow up that I don't deemed necessary. In theory keyboards that don't allow switching the scancode set might exist. But as long as we don't have any or can set the correct translation mode in coreboot, I guess we can keep it unmerged.
Will give you feedback after we tested on Drallion/Sarien.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
This works on Drallion/Sarien.
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
(2 comments)
I'm not clear on the motivation for the change, e.g., to get rid of PC_KEYBOARD_AT_TRANSLATED or fix some other issue. I've worked on enough other OSes that "because Linux does it" isn't usually a very compelling argument, though this does seem like one of the few cases I've seen where Linux goes by what the firmware set (usually Linux goes off and does its own thing regardless). I'm not arguing against the change, but given the potential for breakage the phrase "if it ain't broke don't fix it" comes to mind. I'm sure something's being fixed, but it's not obvious to me and I'd generally expect the translation mode to be based on what code set(s) the keyboard can support, rather than setting the keyboard mode based on the current translation mode.
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 334: set < 1 || set > 3 Might be better as "set != I8042_SCANCODE_SET_1 && set != I8042_SCANCODE_SET_2", depending on whether these #defines still exist after this change.
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 374: 2 : 1 This should probably use I8042_SCANCODE_SET_2 : I8042_SCANCODE_SET_1 , or delete the seemingly now unused #defines from i8042.h.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
I'm not clear on the motivation for the change, e.g., to get rid of PC_KEYBOARD_AT_TRANSLATED or fix some other issue. I've worked on enough other OSes that "because Linux does it" isn't usually a very compelling argument, though this does seem like one of the few cases I've seen where Linux goes by what the firmware set (usually Linux goes off and does its own thing regardless). I'm not arguing against the change, but given the potential for breakage the phrase "if it ain't broke don't fix it" comes to mind. I'm sure something's being fixed, but it's not obvious to me and I'd generally expect the translation mode to be based on what code set(s) the keyboard can support, rather than setting the keyboard mode based on the current translation mode.
Hi Matt,
your wariness is absolutely warranted. I didn't mention all that's broken in the commit message simply because that would bloat it too much, IMHO. I think this should be tested with as much hardware as possible (in reasonable time).
The current state is that depending on hardware defaults and core- boot settings, it might work only with one setting of PC_KEYBOARD_ AT_TRANSLATED. And for that, you can pretty much roll a dice, it's not easy to predict.
What's broken: * The !PC_KEYBOARD_AT_TRANSLATED path resets the keyboard back to it's defaults after selecting set #1, reverting the latter for all keyboards that default to set #2. * Both paths ignore the state of the other setting. The PC_KEYBOARD_AT_TRANSLATED path is broken for any keyboard that doesn't default to set #2. The other path is broken for all keyboard controllers that have the translation enabled by default (in the original state of the path) or for either controller setting depending on the default state of the keyboard (in the current broken state). * If upfront knowledge about the hardware is needed, PC_KEYBOARD_AT_TRANSLATED would carry information that belongs into coreboot's devicetree.
Now that was considering integrated keyboards. If we consider plug- gable keyboards, we can take the above times 2.
Payloads are no onetime concept anymore. Considering that we might run after a different payload that made its own settings, we can take it times x additionally (I've stopped counting).
So long story short, changing one setting without considering the other is easily broken by flipping a switch in coreboot or a prior payload.
The usual upstream goal is to have coreboot work with any payload. And Linux is the one that has the least issues with PS/2 keyboards. Hence the thought to follow it. Their idea seems to be to leave the controller configuration to the firmware and to configure the key- board accordingly.
All the code I've read so far on the topic seems to assume that all keyboards can switch between scancode sets #1 and #2 at least.
Hello Felix Singer, Matt Delco, build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Julius Werner, Angel Pons, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46724
to look at the new patch set (#3).
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
libpayload/keyboard: Revise scancode set and translation config
Some background first: The original XT keyboards used what we call scancode set #1 today. The PC/AT keyboards introduced scancode set #2, but for compatibility, its controller translated scancodes back to set #1 by default. Newer keyboards (maybe all we have to deal with) also support switching the scancode set.
This means the translation option in the controller and the scancode set selection in the keyboard have to match. In libpayload, we only support set #1 scancodes. So we either need the controller's trans- lation on and set #2 selected in the keyboard, or the controller's translation off and set #1 selected in the keyboard.
Valid configurations: * SET #1 + XLATE off * SET #2 + XLATE on
The Linux kernel leaves the decision to enable or disable the con- troller's translation to the firmware. We follow that behaviour, because Linux can be a payload too, and ideally want any payload to succeed the same with any given coreboot image.
Currently, coreboot leaves the controller's translation at its default setting, unless DRIVERS_PS2_KEYBOARD is enabled. The latter enables the translation unconditionally. For QEMU this means that the option effectively toggles the translation, as QEMU's controller has it disabled by default. This probably made a lot of earlier testing inconsistent.
Fixes: commit a95a6bf646 (libpayload/drivers/i8402/kbd: Fix qemu) The reset introduced there effectively reverted the scancode selection made before (because 2 is the default). It's unclear if later changes to the code were only necessary to work around it.
Change-Id: Iad85af516a7b9f9c0269ff9652ed15ee81700057 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/i8042.c M payloads/libpayload/drivers/i8042/i8042.h M payloads/libpayload/drivers/i8042/keyboard.c 4 files changed, 33 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/46724/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 3:
(4 comments)
I've still a lot of testing ahead of me. Had to update a lot more after I tried one of these ThinkPad embedded keyboards. They have some peculiarities: They start a keyboard self-test automatically, and don't reset the scancode set when told to return to defaults...
Trying to find a reasonable sequence for these, I discovered lots of races in the current code :-/
The reset problem with this special keyboard is still not addressed. As Linux' atkbd driver assumes set #2 is the default, it doesn't work without special parameters. To work around this, we could either
* unconditionally enable translation and scancode set #2, or * always select scancode set #2 in keyboard_disconnect().
Read some more drivers in the meantime. SeaBIOS always uses scancode set #2 and translation and leaves the keyboard like that on exit. GRUB seems to use untranslated set #2 but then turns translation (back) on, on exit.
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG@43 PS2, Line 43: reverted the scancode : selection made before
"because scancode set #2 is usually the default"
Done
https://review.coreboot.org/c/coreboot/+/46724/2//COMMIT_MSG@45 PS2, Line 45: where
were
Done
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 329: /* Set scancode set 1 */
Needs an update.
Done
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 374: 2 : 1
This should probably use I8042_SCANCODE_SET_2 : I8042_SCANCODE_SET_1 , or delete the seemingly now u […]
I guess the parameter is clear enough. Removed the definitions.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 3: Code-Review+1
Hello Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Stefan Reinauer, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46724
to look at the new patch set (#4).
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
libpayload/keyboard: Revise scancode set and translation config
Some background first: The original XT keyboards used what we call scancode set #1 today. The PC/AT keyboards introduced scancode set #2, but for compatibility, its controller translated scancodes back to set #1 by default. Newer keyboards (maybe all we have to deal with) also support switching the scancode set.
This means the translation option in the controller and the scancode set selection in the keyboard have to match. In libpayload, we only support set #1 scancodes. So we either need the controller's trans- lation on and set #2 selected in the keyboard, or the controller's translation off and set #1 selected in the keyboard.
Valid configurations: * SET #1 + XLATE off * SET #2 + XLATE on
Both with and without the PC_KEYBOARD_AT_TRANSLATED option, we were only configuring one of the two settings, leaving room for invalid configurations. With this change, we try to select scancode set #2 first, which seems to be the most supported one, and configure the controller's translation accordingly. We try to fall back to set #1 on failure.
We also keep translation disabled during configuration steps to ensure that the controller doesn't accidentally translate confi- guration data.
On the coreboot side, we leave the controller's translation at its default setting, unless DRIVERS_PS2_KEYBOARD is enabled. The latter enables the translation unconditionally. For QEMU this means that the option effectively toggles the translation, as QEMU's controller has it disabled by default. This probably made a lot of earlier testing inconsistent.
Fixes: commit a95a6bf646 (libpayload/drivers/i8402/kbd: Fix qemu) The reset introduced there effectively reverted the scancode selection made before (because 2 is the default). It's unclear if later changes to the code were only necessary to work around it.
Change-Id: Iad85af516a7b9f9c0269ff9652ed15ee81700057 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/i8042.h M payloads/libpayload/drivers/i8042/keyboard.c 3 files changed, 18 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/46724/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 4: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 4: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
libpayload/keyboard: Revise scancode set and translation config
Some background first: The original XT keyboards used what we call scancode set #1 today. The PC/AT keyboards introduced scancode set #2, but for compatibility, its controller translated scancodes back to set #1 by default. Newer keyboards (maybe all we have to deal with) also support switching the scancode set.
This means the translation option in the controller and the scancode set selection in the keyboard have to match. In libpayload, we only support set #1 scancodes. So we either need the controller's trans- lation on and set #2 selected in the keyboard, or the controller's translation off and set #1 selected in the keyboard.
Valid configurations: * SET #1 + XLATE off * SET #2 + XLATE on
Both with and without the PC_KEYBOARD_AT_TRANSLATED option, we were only configuring one of the two settings, leaving room for invalid configurations. With this change, we try to select scancode set #2 first, which seems to be the most supported one, and configure the controller's translation accordingly. We try to fall back to set #1 on failure.
We also keep translation disabled during configuration steps to ensure that the controller doesn't accidentally translate confi- guration data.
On the coreboot side, we leave the controller's translation at its default setting, unless DRIVERS_PS2_KEYBOARD is enabled. The latter enables the translation unconditionally. For QEMU this means that the option effectively toggles the translation, as QEMU's controller has it disabled by default. This probably made a lot of earlier testing inconsistent.
Fixes: commit a95a6bf646 (libpayload/drivers/i8402/kbd: Fix qemu) The reset introduced there effectively reverted the scancode selection made before (because 2 is the default). It's unclear if later changes to the code were only necessary to work around it.
Change-Id: Iad85af516a7b9f9c0269ff9652ed15ee81700057 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/46724 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/i8042.h M payloads/libpayload/drivers/i8042/keyboard.c 3 files changed, 18 insertions(+), 54 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index f5b81a9..7a502b5 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -375,10 +375,6 @@ default y if ARCH_X86 # uses IO default n
-config PC_KEYBOARD_AT_TRANSLATED - bool "AT Translation keyboard device" - default n - config PC_KEYBOARD_LAYOUT_US bool "English (US) keyboard layout" depends on PC_KEYBOARD diff --git a/payloads/libpayload/drivers/i8042/i8042.h b/payloads/libpayload/drivers/i8042/i8042.h index 6d15d1e..bcb42fd 100644 --- a/payloads/libpayload/drivers/i8042/i8042.h +++ b/payloads/libpayload/drivers/i8042/i8042.h @@ -56,9 +56,6 @@ #define I8042_MODE_SCROLL_LOCK_ON (1 << 0) #define I8042_MODE_SCROLL_LOCK_OFF (0 << 0) #define I8042_KBCMD_SET_SCANCODE 0xf0 -#define I8042_SCANCODE_SET_1 (1) -#define I8042_SCANCODE_SET_2 (2) -#define I8042_SCANCODE_SET_3 (3) #define I8042_KBCMD_SET_TYPEMATIC 0xf3 #define I8042_KBCMD_EN 0xf4 #define I8042_KBCMD_DEFAULT_DIS 0xf5 diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 2dec3a3..22747ff 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -313,55 +313,22 @@ .input_type = CONSOLE_INPUT_TYPE_EC, };
-/* Enable keyboard translated */ -static bool enable_translated(void) -{ - if (!i8042_cmd(I8042_CMD_RD_CMD_BYTE)) { - int cmd = i8042_read_data_ps2(); - cmd |= I8042_CMD_BYTE_XLATE; - if (!i8042_cmd(I8042_CMD_WR_CMD_BYTE)) { - i8042_write_data(cmd); - } else { - printf("ERROR: i8042_cmd WR_CMD failed!\n"); - return false; - } - } else { - printf("ERROR: i8042_cmd RD_CMD failed!\n"); - return false; - } - return true; -} - -/* Set scancode set 1 */ -static bool set_scancode_set(void) +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(I8042_SCANCODE_SET_1); + ret = keyboard_cmd(set); if (!ret) { - printf("ERROR: Keyboard scancode set#1 failed!\n"); - return ret; - } - - /* - * Set default parameters. - * Fix for broken QEMU PS/2 make scancodes. - */ - ret = keyboard_cmd(I8042_KBCMD_SET_DEFAULT); - if (!ret) { - printf("ERROR: Keyboard set default params failed!\n"); - return ret; - } - - /* Enable scanning */ - ret = keyboard_cmd(I8042_KBCMD_EN); - if (!ret) { - printf("ERROR: Keyboard enable scanning failed!\n"); + printf("ERROR: Keyboard scancode set#%u failed!\n", set); return ret; }
@@ -383,13 +350,17 @@ /* Enable first PS/2 port */ i8042_cmd(I8042_CMD_EN_KB);
- if (CONFIG(LP_PC_KEYBOARD_AT_TRANSLATED)) { - if (!enable_translated()) - return; - } else { - if (!set_scancode_set()) - return; - } + 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");
console_add_input_driver(&cons); }
-
Angel Pons (Code Review) -
EricR Lai (Code Review)
-
Hung-Te Lin (Code Review)
-
Matt Delco (Code Review)
-
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)