Re: [coreboot] [PATCH] s289x ACPI fix

[Myles Watson]

On Thu, Oct 15, 2009 at 6:24 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > Initialize the interrupts even if you don't generate the MP_TABLE.  I
> > just copied the values from mptable.c.
> > Also set IRQ 9 to be edge-triggered to make Linux stop complaining.
> >
> > Signed-off-by: Myles Watson
>
> Acked-by: Peter Stuge

Rev 4787.
> But some comments:

Thanks for reviewing!
> > +++ cbv2/src/mainboard/tyan/s2891/acpi_tables.c
> > @@ -42,6 +42,19 @@ unsigned long acpi_fill_madt(unsigned lo
> >               apic_addr = pci_read_config32(dev, PCI_BASE_ADDRESS_1) &
>
> ~0xf;
> >           current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)
>
> current, 4,
> >                                              apic_addr, 0);
> >         #if !CONFIG_HAVE_MP_TABLE /* Initialize interrupt mapping.
>
> */
> >         {
>
> I don't think this is how Ron meant he likes CONFIG_ values in code.
> More like:
>
> if(!CONFIG_HAVE_MP_TABLE) {
> }

The problem with this style is that it doesn't support conditional
compilation.  In this case it doesn't matter, but if you wanted to put in:

if(CONFIG_HAVE_MP_TABLE) {
     use_some_MP_defines();
}

It would break, wouldn't it?  Since CONFIG_ variables are used to control
what gets included in the image, I think we should stick with #if.
> @@ -61,7 +74,7 @@ unsigned long acpi_fill_madt(unsigned lo
> >   /* IRQ9 ACPI active low. */
> >   current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)
> >         current, 0, 9, 9, MP_IRQ_TRIGGER_LEVEL |
>
> MP_IRQ_POLARITY_LOW);
> >         current, 0, 9, 9, MP_IRQ_TRIGGER_EDGE |
>
> MP_IRQ_POLARITY_LOW);
>
> This is definately something that should not be unique per board. I
> expect not just Tyan boards will have this code. Could it be moved
> out to a common place?

Actually, the factory BIOS and asus/m2v-mx_2e have it level triggered, which
is why I had it wrong.  It's a southbridge setting.  The other parts of
acpi_tables are fairly generic, but acpi_fill_madt has very little
duplication.

Thanks,
Myles