[coreboot] [PATCH] s289x ACPI fix

Myles Watson mylesgw at gmail.com
Fri Oct 16 16:41:11 CEST 2009


On Thu, Oct 15, 2009 at 6:24 PM, Peter Stuge <peter at stuge.se> 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 <mylesgw at gmail.com>
>
> Acked-by: Peter Stuge <peter at stuge.se>
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20091016/f9050e7e/attachment.html>


More information about the coreboot mailing list