[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 <stepan at coresystems.de>
 
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.
-- 
http://www.hermann-uwe.de  | http://www.randomprojects.org
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org