[PATCH] s289x ACPI fix

[Myles Watson]

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

Thanks,
Myles

[Peter Stuge]

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

But some comments:
> +++ 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. */
> {
> 	u32 dword;
> 	dword = 0x0120d218;
> 	pci_write_config32(dev, 0x7c, dword);

I don't think this is how Ron meant he likes CONFIG_ values in code.
More like:

if(!CONFIG_HAVE_MP_TABLE) {
}

Personally I favor letting the preprocessor do preprocessing, rather
than pushing this type of code removal to the compiler.
> @@ -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?

//Peter

[Stefan Reinauer]

Peter Stuge wrote:
> > @@ -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?

Most of that code is common code,... except calling the common function
with board specific parameters.. There might be other boards with
similar function calls, but it's still board specific code.

Stefan

[Peter Stuge]

Stefan Reinauer wrote:
> > > /* 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.
>
> Most of that code is common code,... except calling the common
> function with board specific parameters..

Right.
> There might be other boards with similar function calls, but it's
> still board specific code.

I was thinking of IRQ9 specifically. Is there a single board where it
should not be edge triggered to make Linux (and probably others)
happy?

//Peter

[ron minnich]

On Thu, Oct 15, 2009 at 5:24 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > #if !CONFIG_HAVE_MP_TABLE /* Initialize interrupt mapping. */
> > {
> > u32 dword;
> > dword = 0x0120d218;
> > pci_write_config32(dev, 0x7c, dword);
>
> I don't think this is how Ron meant he likes CONFIG_ values in code.
> More like:
>
> if(!CONFIG_HAVE_MP_TABLE) {
> }
>
> Personally I favor letting the preprocessor do preprocessing, rather
> than pushing this type of code removal to the compiler.

I'm not going to worry about this type of thing, but it is interesting
that even we let cpp do it, we try to indent and make
it look like C code, eh?

But #if value is fine if that is what you want to do. It's the #ifdef
that has caused trouble in the past. And, in the end, I do like to let
the C compiler do the work, true. But I'm in good company, the guys
who invented C are the guys who convinced me :-)

Thanks!

ron

[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

[ron minnich]

On Fri, Oct 16, 2009 at 7:41 AM, Myles Watson  wrote:
> 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.

Why would that break?

ron

[Myles Watson]

On Fri, Oct 16, 2009 at 8:48 AM, ron minnich  wrote:
> On Fri, Oct 16, 2009 at 7:41 AM, Myles Watson  wrote:
> > 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.
>
> Why would that break?

Before it gets optimized out it will look for the definition of the function
which wasn't compiled in and whose headers weren't included.  Won't it
break?  I could try it with some of the k8 raminit code that has conditional
includes all over the place.

Thanks,
Myles

[ron minnich]

On Fri, Oct 16, 2009 at 7:55 AM, Myles Watson  wrote:
> Before it gets optimized out it will look for the definition of the function
> which wasn't compiled in and whose headers weren't included.  Won't it
> break?  I could try it with some of the k8 raminit code that has conditional
> includes all over the place.

It depends to some extent on how that code is set up.

We do conditional function definition in many .h files a la Linux style:
#if CONFIG_THIS == 0
#define x()
#endif

So it's likely that
if (CONFIG_THIS) {
         x();
}

would work.

Anyway, I'm not going to worry about this if nobody else does :-)

Just FYI, the romcc code depends on romcc's intelligent optimization.

ron