| Submitter | Stefan Reinauer |
|---|---|
| Date | 2009-07-21 16:45:19 |
| Message ID | <4A65F09F.4000308@coresystems.de> |
| Download | mbox | patch |
| Permalink | /patch/58/ |
| State | Accepted |
| Headers | show |
Comments
Stefan Reinauer wrote: > See patches All are: Acked-by: Peter Stuge <peter@stuge.se> > +++ src/pc80/i8259.c (working copy) .. > +void i8259_configure_irq_trigger(int int_num, int is_level_triggered) .. > + /* Write new values */ > + printk_spew("%s: try to set interrupts 0x%x\n", __func__, int_bits); > + outb((u8)(int_bits & 0xff), ELCR1); > + outb((u8)(int_bits >> 8), ELCR2); > + > +#ifdef PARANOID_IRQ_TRIGGERS > + /* Try reading back the new values. This seems like an error but is not ... */ > + if (inb(ELCR1) != (int_bits & 0xff)) { > + printk_err("%s: lower order bits are wrong: want 0x%x, got 0x%x\n", > + __func__, (int_bits & 0xff), inb(ELCR1)); > + } > + > + if (inb(ELCR2) != (int_bits >> 8)) { > + printk_err("%s: lower order bits are wrong: want 0x%x, got 0x%x\n", > + __func__, (int_bits>>8), inb(ELCR2)); > + } > +#endif Copypaste right? Both of these are not the lower bits. > +++ src/boot/elfboot.c (working copy) .. > - * - After loading an ELF image copy coreboot to the upper half of the > + * - After loading an ELF image copy coreboot to the top of the > * buffer. Could fold buffer up onto the previous line. > - * - After loading an ELF image copy coreboot to the upper half of the > + * - After loading an ELF image copy coreboot to the top of the > * buffer. ..and again. Is this the same code? Could it be reused? > +++ src/cpu/x86/lapic/apic_timer.c (.../trunk/coreboot-v2) .. > +/* NOTE: We use the APIC TIMER register is to hold flags for AP init during > + * pre-memory init (ROMCC). Don't use init_timer() and udelay is redirected > + * to udelay_tsc(). > + */ What? "We use the .. is to hold flags for " ? This text is confusing. > +++ src/arch/i386/boot/acpi.c (working copy) .. > + memcpy(&ssdt->asl_compiler_id, "CORE", 4); .. > + memcpy(header->asl_compiler_id, ASLC, 4); Is this difference on purpose? Is one static and the other not? Please explain? > +#define RSDP_NAME "RSDP" > +#define RSDT_NAME "RSDT" > +#define HPET_NAME "HPET" > +#define MADT_NAME "APIC" > +#define MCFG_NAME "MCFG" > +#define SRAT_NAME "SRAT" > +#define SLIT_NAME "SLIT" > +#define SSDT_NAME "SSDT" > +#define FACS_NAME "FACS" > +#define FADT_NAME "FACP" > +#define XSDT_NAME "XSDT" Does it really make sense to use defines for these names? > +// Misnomer, the NAME above is the 4 byte signature, this (TABLE) is the > +// OEM_TABLE_ID. > +// > +#define ACPI_TABLE_CREATOR "COREBOOT" > +#define RSDT_TABLE ACPI_TABLE_CREATOR > +#define HPET_TABLE ACPI_TABLE_CREATOR > +#define MCFG_TABLE ACPI_TABLE_CREATOR > +#define MADT_TABLE ACPI_TABLE_CREATOR > +#define SRAT_TABLE ACPI_TABLE_CREATOR > +#define SLIT_TABLE ACPI_TABLE_CREATOR > +#define XSDT_TABLE ACPI_TABLE_CREATOR Seems even more wrong. Why use these defines? > +++ src/northbridge/intel/i945/early_init.c (working copy) .. > + reg32 = DMIBAR32(0x224); > + reg32 &= ~(7 << 0); > + reg32 |= (3 << 0); > + DMIBAR32(0x224) = reg32; How do you feel about making this a single function call? > -#if SETUP_PCIE_X16_LINK > + Is this a part of the Config.lb system? Should it be removed from there? Why was it there before and removed now? Is x16 never optional on 945? > +++ src/northbridge/intel/i945/raminit.c (working copy) .. > +#if C2_SELF_REFRESH_DISABLE Where is this set? > + * However, the Kontron 986LCD-M does not like unused clock > + * signals to be disabled. (Do you know why? Just curious.) > +++ src/pc80/keyboard.c (.../trunk/coreboot-v2) > @@ -75,7 +81,11 @@ > do { > if (!kbc_input_buffer_empty()) return 0; No error here? > outb(command, 0x60); > - if (!kbc_output_buffer_full()) return 0; > + if (!kbc_output_buffer_full()) { > + printk_err("Could not send keyboard command %02x\n", > + command); > + return 0; > + } > regval = inb(0x60); > --resend; > } while (regval == 0xFE && resend > 0); //Peter
I read through all patches, but for some, the reformatting makes it
really hard to see what changed. For others, I don't know the code well
enough to review except for coding style.
The following patches are
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
14_config
13_flags
12_keyboard
11_simple_printk
08_usbdebug
07_timer
04_intel_945
03_intel_core
Regards,
Carl-Daniel
Peter Stuge wrote: > Stefan Reinauer wrote: > >> See patches >> > > All are: > > Acked-by: Peter Stuge <peter@stuge.se> > > Thank you! >> +++ src/pc80/i8259.c (working copy) >> > .. > >> +void i8259_configure_irq_trigger(int int_num, int is_level_triggered) >> > .. > >> + /* Write new values */ >> + printk_spew("%s: try to set interrupts 0x%x\n", __func__, int_bits); >> + outb((u8)(int_bits & 0xff), ELCR1); >> + outb((u8)(int_bits >> 8), ELCR2); >> + >> +#ifdef PARANOID_IRQ_TRIGGERS >> + /* Try reading back the new values. This seems like an error but is not ... */ >> + if (inb(ELCR1) != (int_bits & 0xff)) { >> + printk_err("%s: lower order bits are wrong: want 0x%x, got 0x%x\n", >> + __func__, (int_bits & 0xff), inb(ELCR1)); >> + } >> + >> + if (inb(ELCR2) != (int_bits >> 8)) { >> + printk_err("%s: lower order bits are wrong: want 0x%x, got 0x%x\n", >> + __func__, (int_bits>>8), inb(ELCR2)); >> + } >> +#endif >> > > Copypaste right? Both of these are not the lower bits. > D'uh, yes, must be "higher" in the second case . I never got these messages, so I neglected them. >> +++ src/boot/elfboot.c (working copy) >> > .. > >> - * - After loading an ELF image copy coreboot to the upper half of the >> + * - After loading an ELF image copy coreboot to the top of the >> * buffer. >> > > Could fold buffer up onto the previous line. > Agreed. >> - * - After loading an ELF image copy coreboot to the upper half of the >> + * - After loading an ELF image copy coreboot to the top of the >> * buffer. >> > > ..and again. Is this the same code? Could it be reused? > > Agreed. Not sure if it's easy to reuse it.. I think Ron has still hopes that we drop elfboot completely... Maybe it is indeed time for that. >> +++ src/cpu/x86/lapic/apic_timer.c (.../trunk/coreboot-v2) >> > .. > >> +/* NOTE: We use the APIC TIMER register is to hold flags for AP init during >> + * pre-memory init (ROMCC). Don't use init_timer() and udelay is redirected >> + * to udelay_tsc(). >> + */ >> > > What? "We use the .. is to hold flags for " ? This text is confusing. > > This code was taken from AMD, but the lapic timer code never worked in ROMCC. (Nor does AMD pre-memory init) So I completely wiped the paragraph for now. >> +++ src/arch/i386/boot/acpi.c (working copy) >> > .. > >> + memcpy(&ssdt->asl_compiler_id, "CORE", 4); >> > .. > >> + memcpy(header->asl_compiler_id, ASLC, 4); >> > > Is this difference on purpose? Is one static and the other not? > Please explain? > No, it should be the same. > > >> +#define RSDP_NAME "RSDP" >> +#define RSDT_NAME "RSDT" >> +#define HPET_NAME "HPET" >> +#define MADT_NAME "APIC" >> +#define MCFG_NAME "MCFG" >> +#define SRAT_NAME "SRAT" >> +#define SLIT_NAME "SLIT" >> +#define SSDT_NAME "SSDT" >> +#define FACS_NAME "FACS" >> +#define FADT_NAME "FACP" >> +#define XSDT_NAME "XSDT" >> > > Does it really make sense to use defines for these names? > > No. But I didn't dare dropping them, in this run, either. Same goes for the next set (OEM_TABLE_ID) >> +// Misnomer, the NAME above is the 4 byte signature, this (TABLE) is the >> +// OEM_TABLE_ID. >> +// >> +#define ACPI_TABLE_CREATOR "COREBOOT" >> +#define RSDT_TABLE ACPI_TABLE_CREATOR >> +#define HPET_TABLE ACPI_TABLE_CREATOR >> +#define MCFG_TABLE ACPI_TABLE_CREATOR >> +#define MADT_TABLE ACPI_TABLE_CREATOR >> +#define SRAT_TABLE ACPI_TABLE_CREATOR >> +#define SLIT_TABLE ACPI_TABLE_CREATOR >> +#define XSDT_TABLE ACPI_TABLE_CREATOR >> > > Seems even more wrong. Why use these defines? > They were in use before, and when I started on the code, I wanted to somewhat clean it up, but not touch too many files. The best way would be to use ACPI_TABLE_CREATOR directly wherever those tables are created. We can give this a try, but I'm going to push these changes in first, or it'll become too confusing. >> +++ src/northbridge/intel/i945/early_init.c (working copy) >> > .. > >> + reg32 = DMIBAR32(0x224); >> + reg32 &= ~(7 << 0); >> + reg32 |= (3 << 0); >> + DMIBAR32(0x224) = reg32; >> > > How do you feel about making this a single function call? > For this part, or generally for this kind of construct? >> -#if SETUP_PCIE_X16_LINK >> + >> > > Is this a part of the Config.lb system? Should it be removed from > there? Why was it there before and removed now? Is x16 never optional > on 945? > The chipset can do it, so the code should look if something is there. There's a way to enable the links x16, then x1, then disable it, if none of the link widths bring up a connection. So, basically, always having this code there is safe. The downside is, the code does not work yet, it doesn't detect the one graphics card I have here. >> +++ src/northbridge/intel/i945/raminit.c (working copy) >> > .. > >> +#if C2_SELF_REFRESH_DISABLE >> > > Where is this set? > > Not at all yet. I think we're going to need this for S2R, but I'm not sure how to fit the decision in. >> + * However, the Kontron 986LCD-M does not like unused clock >> + * signals to be disabled. >> > > (Do you know why? Just curious.) > I guess it might be a bug in the mainboard. The Kontron is the only board I saw that behaves like that. Maybe they had some very smart reasons to do this. I don't know them, though. >> +++ src/pc80/keyboard.c (.../trunk/coreboot-v2) >> @@ -75,7 +81,11 @@ >> do { >> if (!kbc_input_buffer_empty()) return 0; >> > > No error here? > > Good question.. > >> outb(command, 0x60); >> - if (!kbc_output_buffer_full()) return 0; >> + if (!kbc_output_buffer_full()) { >> + printk_err("Could not send keyboard command %02x\n", >> + command); >> + return 0; >> + } >> regval = inb(0x60); >> --resend; >> } while (regval == 0xFE && resend > 0); >> > > > //Peter >
Patch
Rewrite keyboard driver to actually wait time in ms as specified in the specs, rather than doing inexact and slow idle loops. Also improve error reporting in case of problems Signed-off-by: Stefan Reinauer <stepan@coresystems.de> --- src/pc80/keyboard.c (.../branches/upstream/coreboot-v2) +++ src/pc80/keyboard.c (.../trunk/coreboot-v2) @@ -1,8 +1,9 @@ /* * This file is part of the coreboot project. * + * Copyright (C) 2009 coresystems GmbH * Copyright (C) 2008 Advanced Micro Devices, Inc. - * Copyright (C) ???? Ollie Lo <ollielo@hotmail.com> + * Copyright (C) 2003 Ollie Lo <ollielo@hotmail.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -23,16 +24,20 @@ #include <pc80/keyboard.h> #include <device/device.h> #include <arch/io.h> +#include <delay.h> +/* Wait 200ms for keyboard controller answers */ +#define KBC_TIMEOUT_IN_MS 200 + static int kbc_input_buffer_empty(void) { u32 timeout; - for(timeout = 1000000; timeout && (inb(0x64) & 0x02); timeout--) { - inb(0x80); + for(timeout = KBC_TIMEOUT_IN_MS; timeout && (inb(0x64) & 0x02); timeout--) { + mdelay(1); } if (!timeout) { - printk_err("Unexpected Keyboard controller input buffer full\n"); + printk_warning("Unexpected Keyboard controller input buffer full\n"); } return !!timeout; } @@ -41,12 +46,12 @@ static int kbc_output_buffer_full(void) { u32 timeout; - for(timeout = 1000000; timeout && ((inb(0x64) & 0x01) == 0); timeout--) { - inb(0x80); + for(timeout = KBC_TIMEOUT_IN_MS; timeout && ((inb(0x64) & 0x01) == 0); timeout--) { + mdelay(1); } if (!timeout) { - printk_err("Keyboard controller output buffer result timeout\n"); + printk_warning("Keyboard controller output buffer result timeout\n"); } return !!timeout; } @@ -55,7 +60,8 @@ static int kbc_cleanup_buffers(void) { u32 timeout; - for(timeout = 1000000; timeout && (inb(0x64) & 0x03); timeout--) { + for(timeout = KBC_TIMEOUT_IN_MS; timeout && (inb(0x64) & 0x03); timeout--) { + mdelay(1); inb(0x60); } @@ -75,7 +81,11 @@ do { if (!kbc_input_buffer_empty()) return 0; outb(command, 0x60); - if (!kbc_output_buffer_full()) return 0; + if (!kbc_output_buffer_full()) { + printk_err("Could not send keyboard command %02x\n", + command); + return 0; + } regval = inb(0x60); --resend; } while (regval == 0xFE && resend > 0); @@ -93,18 +103,21 @@ /* clean up any junk that might have been in the kbc */ if (!kbc_cleanup_buffers()) return; - /* reset/self test 8042 - send cmd 0xAA, */ + /* reset/self test 8042 - send cmd 0xAA */ if (!kbc_input_buffer_empty()) return; outb(0xAA, 0x64); - if (!kbc_output_buffer_full()) return; + if (!kbc_output_buffer_full()) { + printk_err("Could not reset keyboard controller.\n"); + return; + } /* read self-test result, 0x55 is returned in the output buffer (0x60) */ if ((regval = inb(0x60) != 0x55)) { - printk_err("Keyboard Controller selftest failed: 0x%x\n", regval); + printk_err("Keyboard Controller self-test failed: 0x%x\n", regval); return; } - /* Enable keyboard interface - No IRQ*/ + /* Enable keyboard interface - No IRQ */ resend = 10; regval = 0; do { @@ -112,8 +125,11 @@ outb(0x60, 0x64); if (!kbc_input_buffer_empty()) return; outb(0x20, 0x60); /* send cmd: enable keyboard */ - if ((inb(0x64) & 0x01)) { + if (kbc_output_buffer_full()) { regval = inb(0x60); + } else { + printk_info("Timeout while enabling keyboard. (No keyboard present?)\n"); + regval = inb(0x60); /* Better than 0 ? */ } --resend; } while (regval == 0xFE && resend > 0); @@ -127,7 +143,11 @@ printk_err("Keyboard selftest failed ACK: 0x%x\n", regval); return; } - if (!kbc_output_buffer_full()) return; + if (!kbc_output_buffer_full()) { + printk_err("Timeout waiting for keyboard after reset.\n"); + return; + } + regval = inb(0x60); if (regval != 0xAA) { printk_err("Keyboard selftest failed: 0x%x\n", regval); @@ -174,7 +194,7 @@ outb(0x60, 0x64); if (!kbc_input_buffer_empty()) return; outb(0x61, 0x60); /* send cmd: enable keyboard and IRQ 1 */ - if ((inb(0x64) & 0x01)) { + if (kbc_output_buffer_full()) { regval = inb(0x60); } --resend;