Re: [coreboot] [PATCH] Janitor Tasks - coreboot cleanup

[Uwe Hermann]

On Sun, Feb 21, 2010 at 08:11:51AM +0100, Stefan Reinauer wrote:
> This is a general cleanup patch
> drop include/part and move files to include/
> get rid lots of warnings
> make resource allocator happy with w83627thg
> trivial cbmem resume fix
> fix payload and log level settings in abuild
> fix kontron mptable for virtual wire mode
> drop some dead includes and dead code.
>
> Signed-off-by: Stefan Reinauer

Nice!
> --- src/southbridge/intel/i82801gx/i82801gx_pcie.c	(revision 5133)
> +++ src/southbridge/intel/i82801gx/i82801gx_pcie.c	(working copy)
> @@ -75,7 +75,7 @@
>    reg16 |= (1 << 6);
>    pci_write_config16(dev, 0x50, reg16);
>
> -#if EVEN_MORE_DEBUG
> +#ifdef EVEN_MORE_DEBUG

Maybe this should be merged into the general prink infrastructure
as SPEW messages etc?
> --- src/devices/pnp_device.c	(revision 5133)
> +++ src/devices/pnp_device.c	(working copy)
> @@ -240,7 +240,25 @@
>    	resource = new_resource(dev, PNP_IDX_DRQ1);
>    	resource->size = 1;
>    	resource->flags |= IORESOURCE_DRQ;
> }
> }
> /* These are not IRQs, but set the flag to have the
> * resource allocator do the right thing
> */

Can you elaborate here? What does "do the right thing" do exactly?
> if (info->flags & PNP_EN) {
> resource = new_resource(dev, PNP_IDX_EN);
> resource->size = 1;
> resource->flags |= IORESOURCE_IRQ;
> }
> if (info->flags & PNP_MSC0) {
> resource = new_resource(dev, PNP_IDX_MSC0);
> resource->size = 1;
> resource->flags |= IORESOURCE_IRQ;
> }
> if (info->flags & PNP_MSC1) {
> resource = new_resource(dev, PNP_IDX_MSC1);
> resource->size = 1;
> resource->flags |= IORESOURCE_IRQ;
> }
>
> }
> --- src/include/fallback.h	(revision 0)
> +++ src/include/fallback.h	(revision 0)
> @@ -0,0 +1,18 @@
> +#ifndef FALLBACK_H
> +#define FALLBACK_H

Please add the usual license header while we're at it.
> --- src/include/watchdog.h	(revision 0)
> +++ src/include/watchdog.h	(revision 0)
> @@ -0,0 +1,10 @@
> +#ifndef WATCHDOG_H
> +#define WATCHDOG_H

Ditto.
> --- src/include/reset.h	(revision 0)
> +++ src/include/reset.h	(revision 0)
> @@ -0,0 +1,11 @@
> +#ifndef RESET_H
> +#define RESET_H

Ditto.
> --- src/lib/cbmem.c	(revision 5133)
> +++ src/lib/cbmem.c	(working copy)
> @@ -184,7 +184,7 @@
>    if (acpi_slp_type == 3) {
>    	if (!cbmem_reinit(high_tables_base)) {
>    		/* Something went wrong, our high memory area got wiped */
> 	acpi_slp_type == 0;
> 	acpi_slp_type = 0;

Nice catch!
> --- src/cpu/intel/model_6ex/cache_as_ram_disable.c	(revision 5133)
> +++ src/cpu/intel/model_6ex/cache_as_ram_disable.c	(working copy)
> @@ -54,8 +54,7 @@
>    real_main(bist);
>
> /* No servicable parts below this line .. */
>
> -#if CAR_DEBUG
> +#ifdef CAR_DEBUG

This should also use the usualy printk infrastructure, or (if it should
remain independent of the loglevel) become a kconfig option in the Debug menu.
> /* Firewire 4:0.0 */
> /* Internal PCI bus (Firewire, PCI slot) */
> if (firewire) {
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, firewire_bus, 0x0, 0x2, 0x10);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, firewire_bus, 0x0, ioapic_id, 0x10);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, firewire_bus, 0x4, ioapic_id, 0x14);
>
> }
>
> if (riser) {
> /* Old riser card */
> // riser slot top 5:8.0
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x20, 0x2, 0x14);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x20, ioapic_id, 0x14);
> // riser slot middle 5:9.0
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x24, 0x2, 0x15);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x24, ioapic_id, 0x15);
> // riser slot bottom 5:a.0
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x28, 0x2, 0x16);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x28, ioapic_id, 0x16);
>
> /* New Riser Card */
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x30, 0x2, 0x14);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x34, 0x2, 0x15);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x38, 0x2, 0x16);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x30, ioapic_id, 0x14);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x34, ioapic_id, 0x15);
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, riser_bus, 0x38, ioapic_id, 0x16);
>
> }
>
> /* PCIe slot */
>
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, 0x1, 0x0, ioapic_id, 0x10);
>
> smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, 0x1, 0x1, ioapic_id, 0x11);

Some of the above looks like actual improved board support (not purely
cosmetic), can you please update the board wiki page accordingly?
> --- src/arch/i386/boot/pirq_routing.c	(revision 5133)
> +++ src/arch/i386/boot/pirq_routing.c	(working copy)
> @@ -3,7 +3,11 @@
>  #include <string.h>
>  #include <device/pci.h>
>
> -#if (CONFIG_DEBUG==1 && CONFIG_GENERATE_PIRQ_TABLE==1)
> +#ifndef CONFIG_IRQ_SLOT_COUNT
> +#warning "CONFIG_IRQ_SLOT_COUNT is not defined. PIRQ won't work correctly."
> +#endif

[...]
> -#if (CONFIG_PIRQ_ROUTE==1 && CONFIG_GENERATE_PIRQ_TABLE==1)
> +#if CONFIG_PIRQ_ROUTE

Is this intentional? That eliminates the CONFIG_GENERATE_PIRQ_TABLE check,
which is being used in the code base IIRC.
> --- src/arch/i386/include/arch/coreboot_tables.h	(revision 0)
> +++ src/arch/i386/include/arch/coreboot_tables.h	(revision 0)
> @@ -0,0 +1,25 @@
> +#ifndef COREBOOT_TABLE_H
> +#define COREBOOT_TABLE_H

[...]
> +#endif /* COREBOOT_TABLE_H */

You renamed coreboot_table.h to coreboot_tables.h here, so the define
name should also match.

Uwe.