Peter Stuge wrote: > Stefan Reinauer wrote: > >> See patches >> > > All are: > > Acked-by: Peter Stuge <[email protected]> > > 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: [email protected] • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
signature.asc
Description: OpenPGP digital signature
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
