[coreboot] some more i945 patches

[Stefan Reinauer]

Peter Stuge wrote:
> Stefan Reinauer wrote:
>   
>> See patches
>>     
>
> All are:
>
> Acked-by: Peter Stuge <peter at 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
>   


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 250 bytes
Desc: OpenPGP digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090721/3332b5bf/attachment.sig>