[commit] r6116 - trunk/src/arch/i386/include/arch

[repository service]

Author: uwe
Date: Mon Nov 22 17:23:54 2010
New Revision: 6116
URL: https://tracker.coreboot.org/trac/coreboot/changeset/6116

Log:
Drop unused ACPI_WRITE_MADT_IOAPIC #define.

This should probably be C code in some .c file anyway.

Signed-off-by: Uwe Hermann 
Acked-by: Uwe Hermann

Modified:
   trunk/src/arch/i386/include/arch/acpi.h

Modified: trunk/src/arch/i386/include/arch/acpi.h
==============================================================================
--- trunk/src/arch/i386/include/arch/acpi.h	Mon Nov 22 16:57:57 2010	(r6115)
+++ trunk/src/arch/i386/include/arch/acpi.h	Mon Nov 22 17:23:54 2010	(r6116)
@@ -431,17 +431,6 @@
 /* cpu/intel/speedstep/acpi.c */
 void generate_cpu_entries(void);

-#define ACPI_WRITE_MADT_IOAPIC(dev,id)			\
-do {							\
-	struct resource *res;				\
-	res = find_resource(dev, PCI_BASE_ADDRESS_0);	\
-	if (!res) break;				\
-	current += acpi_create_madt_ioapic(		\
-		(acpi_madt_ioapic_t *)current,		\
-		id, res->base, gsi_base);		\
-	gsi_base += 4;					\
-} while(0);
-
 #else // CONFIG_GENERATE_ACPI_TABLES

#define write_acpi_tables(start) (start)

[Stefan Reinauer]

* repository service  [101122 17:23]:
> Author: uwe
> Date: Mon Nov 22 17:23:54 2010
> New Revision: 6116
> URL: https://tracker.coreboot.org/trac/coreboot/changeset/6116
>
> Log:
> Drop unused ACPI_WRITE_MADT_IOAPIC #define.
>
> This should probably be C code in some .c file anyway.

As a macro I think it belongs in a .h file.

Should we not rather use it than drop it? Sounds kind of useful.
We don't really have IOAPICs in our device tree, and using
PCI_BASE_ADDRESS_0 sounds wrong. How do we get IOAPICs into the tree? I
think the southbridges should add it where appropriate and I think we
need a new device type, as we have one for local APICs too.

Stefan.
> Signed-off-by: Uwe Hermann 
> Acked-by: Uwe Hermann
>
> Modified:
>    trunk/src/arch/i386/include/arch/acpi.h
> Modified: trunk/src/arch/i386/include/arch/acpi.h
>
> --- trunk/src/arch/i386/include/arch/acpi.h	Mon Nov 22 16:57:57 2010	(r6115)
> +++ trunk/src/arch/i386/include/arch/acpi.h	Mon Nov 22 17:23:54 2010	(r6116)
> @@ -431,17 +431,6 @@
>  /* cpu/intel/speedstep/acpi.c */
>  void generate_cpu_entries(void);
>
> -#define ACPI_WRITE_MADT_IOAPIC(dev,id)			\
> -do {							\
> struct resource *res;				\
> res = find_resource(dev, PCI_BASE_ADDRESS_0);	\
> if (!res) break;				\
> current += acpi_create_madt_ioapic(		\
> (acpi_madt_ioapic_t *)current,		\
> id, res->base, gsi_base);		\
> gsi_base += 4;					\
>
> -} while(0);
>
> #else // CONFIG_GENERATE_ACPI_TABLES
>
> #define write_acpi_tables(start) (start)

[Uwe Hermann]

On Mon, Nov 22, 2010 at 07:30:15PM +0100, Stefan Reinauer wrote:
> repository service  [101122 17:23]:
> > Author: uwe
> > Date: Mon Nov 22 17:23:54 2010
> > New Revision: 6116
> > URL: https://tracker.coreboot.org/trac/coreboot/changeset/6116
> >
> > Log:
> > Drop unused ACPI_WRITE_MADT_IOAPIC #define.
> >
> > This should probably be C code in some .c file anyway.
>
> As a macro I think it belongs in a .h file.

If it shall remain a macro then yes. In general I think having code in
.h files as macro is not desirable, unless it cannot be avoided for some
reason. I'd prefer some function in C code instead of a macro.

Or, as you suggested, we do neither and add a devicetree.cb facility which
auto-generates the resp. entries/code then (in mptable, in MADT, etc.).
> We don't really have IOAPICs in our device tree, and using
> PCI_BASE_ADDRESS_0 sounds wrong.

Not necessarily wrong, but at the very least it's very specific to some
chipset. For example the 6700PXH 64bit PCI Hub has two integrated
IOAPICs, one at PCI device 0.1 and one at 0.3. Two BARs of those
two devices (PCI_BASE_ADDRESS_0 each) contain/determine to IOAPIC's
base address:

Offset 10h: MBAR--Memory Base Register (D0: F1, F3):

31:12 RW 0 Address (ADDR): These bits determine the base address
  of the I/OxAPIC.

I guess a chipset like this lead to the PCI_BASE_ADDRESS_0 stuff in
that macro.
> How do we get IOAPICs into the tree? I
> think the southbridges should add it where appropriate and I think we
> need a new device type, as we have one for local APICs too.

Yep, sounds good. I'll start another thread about this.

Uwe.