Re: [coreboot] some more i945 patches

[Peter Stuge]

Stefan Reinauer wrote:
> See patches

All are:

Acked-by: Peter Stuge
> +++ 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