| Message ID | 1440141537-18737-5-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 21 August 2015 at 01:18, Bin Meng <bmeng.cn@gmail.com> wrote: > The existing i8042 keyboard controller driver has some issues. > First of all, it does not issue a self-test command (0xaa) to the > controller at the very beginning. Without this, the controller > does not respond any command at all. Secondly, it initializes a few nits "does not response to any command" > the configuration byte reigster to turn on keyboard's interrupt, turn on the keyboard's interrupt > which is not allowed as U-Boot we don't normally enable interrupt. as U-Boot does not normally allow interrupts to be processed. > Finally, at the end of the initialization routine, it wrongly > sets the controller to disable all interfaces including both > keyboard and mouse. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > Acked-by: Simon Glass <sjg@chromium.org> > > --- > > Changes in v2: > - Reorder this patch to follow the i8042 driver clean up patches > > drivers/input/i8042.c | 43 +++++++++++++++++++++---------------------- > 1 file changed, 21 insertions(+), 22 deletions(-) I am amazed this code actually works! But at least on chromebook_link, it does. Regards, Simon
Hi Simon, On Sat, Aug 22, 2015 at 7:27 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 21 August 2015 at 01:18, Bin Meng <bmeng.cn@gmail.com> wrote: >> The existing i8042 keyboard controller driver has some issues. >> First of all, it does not issue a self-test command (0xaa) to the >> controller at the very beginning. Without this, the controller >> does not respond any command at all. Secondly, it initializes > > a few nits > > "does not response to any command" OK > >> the configuration byte reigster to turn on keyboard's interrupt, > > turn on the keyboard's interrupt OK > >> which is not allowed as U-Boot we don't normally enable interrupt. > > as U-Boot does not normally allow interrupts to be processed. OK > >> Finally, at the end of the initialization routine, it wrongly >> sets the controller to disable all interfaces including both >> keyboard and mouse. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> Acked-by: Simon Glass <sjg@chromium.org> >> >> --- >> >> Changes in v2: >> - Reorder this patch to follow the i8042 driver clean up patches >> >> drivers/input/i8042.c | 43 +++++++++++++++++++++---------------------- >> 1 file changed, 21 insertions(+), 22 deletions(-) > > I am amazed this code actually works! But at least on chromebook_link, it does. > Me either. The old codes could work on QEMU too, but I believe that is because QEMU does not emulate hardware exactly (like does not have to issue self-test command before any command). As for chromebook_link, is i8042 implemented in the EC? I suspect some EC firmware may be quite of forgiveness :-) Regards, Bin
Hi Bin, On 23 August 2015 at 06:25, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sat, Aug 22, 2015 at 7:27 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 21 August 2015 at 01:18, Bin Meng <bmeng.cn@gmail.com> wrote: >>> The existing i8042 keyboard controller driver has some issues. >>> First of all, it does not issue a self-test command (0xaa) to the >>> controller at the very beginning. Without this, the controller >>> does not respond any command at all. Secondly, it initializes >> >> a few nits >> >> "does not response to any command" > > OK > >> >>> the configuration byte reigster to turn on keyboard's interrupt, >> >> turn on the keyboard's interrupt > > OK > >> >>> which is not allowed as U-Boot we don't normally enable interrupt. >> >> as U-Boot does not normally allow interrupts to be processed. > > OK > >> >>> Finally, at the end of the initialization routine, it wrongly >>> sets the controller to disable all interfaces including both >>> keyboard and mouse. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> Acked-by: Simon Glass <sjg@chromium.org> >>> >>> --- >>> >>> Changes in v2: >>> - Reorder this patch to follow the i8042 driver clean up patches >>> >>> drivers/input/i8042.c | 43 +++++++++++++++++++++---------------------- >>> 1 file changed, 21 insertions(+), 22 deletions(-) >> >> I am amazed this code actually works! But at least on chromebook_link, it does. >> > > Me either. The old codes could work on QEMU too, but I believe that is > because QEMU does not emulate hardware exactly (like does not have to > issue self-test command before any command). As for chromebook_link, > is i8042 implemented in the EC? I suspect some EC firmware may be > quite of forgiveness :-) Yes that explains it, I think. Anyway it's good to have it correct now! Regards, Simon
diff --git a/drivers/input/i8042.c b/drivers/input/i8042.c index a6d0d59..3812733 100644 --- a/drivers/input/i8042.c +++ b/drivers/input/i8042.c @@ -454,51 +454,50 @@ static void kbd_conv_char(unsigned char scan_code) static int kbd_reset(void) { - /* KB Reset */ + u8 config; + + /* controller self test */ if (kbd_input_empty() == 0) return -1; + out8(I8042_CMD_REG, CMD_SELF_TEST); + if (kbd_output_full() == 0) + return -1; + if (in8(I8042_DATA_REG) != KBC_TEST_OK) + return -1; + /* keyboard reset */ + if (kbd_input_empty() == 0) + return -1; out8(I8042_DATA_REG, CMD_RESET_KBD); - if (kbd_output_full() == 0) return -1; - if (in8(I8042_DATA_REG) != KBD_ACK) return -1; - if (kbd_output_full() == 0) return -1; - if (in8(I8042_DATA_REG) != KBD_POR) return -1; + /* set AT translation and disable irq */ if (kbd_input_empty() == 0) return -1; - - /* Set KBC mode */ - out8(I8042_CMD_REG, CMD_WR_CONFIG); - - if (kbd_input_empty() == 0) + out8(I8042_CMD_REG, CMD_RD_CONFIG); + if (kbd_output_full() == 0) return -1; - - out8(I8042_DATA_REG, - CONFIG_AT_TRANS | CONFIG_SET_BIST | CONFIG_KIRQ_EN); - + config = in8(I8042_DATA_REG); + config |= CONFIG_AT_TRANS; + config &= ~(CONFIG_KIRQ_EN | CONFIG_MIRQ_EN); if (kbd_input_empty() == 0) return -1; - - /* Enable Keyboard */ - out8(I8042_CMD_REG, CMD_KBD_EN); + out8(I8042_CMD_REG, CMD_WR_CONFIG); if (kbd_input_empty() == 0) return -1; + out8(I8042_DATA_REG, config); - out8(I8042_CMD_REG, CMD_WR_CONFIG); + /* enable keyboard */ if (kbd_input_empty() == 0) return -1; - - out8(I8042_DATA_REG, - CONFIG_AT_TRANS | CONFIG_MCLK_DIS | - CONFIG_KCLK_DIS | CONFIG_SET_BIST); + out8(I8042_CMD_REG, CMD_KBD_EN); if (kbd_input_empty() == 0) return -1;