Stefan Reinauer wrote: > See patches All are:
Acked-by: Peter Stuge <[email protected]> > +++ 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
pgp5TseSDlPXH.pgp
Description: PGP signature
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
