[coreboot] some more i945 patches

[Peter Stuge]

Stefan Reinauer wrote:
> See patches

All are:

Acked-by: Peter Stuge <peter at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090721/77e83b3a/attachment.sig>