some more i945 patches

[Stefan Reinauer]

I'm pushing out a number of i945 patches in order to get more public
input and testing on the i945 chipset port and in order to keep some
structural changes (XSDT support et al) from bit-rotting.

See patches

Stefan

[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

[Stefan Reinauer]

Peter Stuge wrote:
> Stefan Reinauer wrote:
> > See patches
>
> All are:
>
> Acked-by: Peter Stuge

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

[Carl-Daniel Hailfinger]

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

14_config
13_flags
12_keyboard
11_simple_printk
08_usbdebug
07_timer
04_intel_945
03_intel_core

Regards,
Carl-Daniel