[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