[PATCH] Janitor Tasks - coreboot cleanup

[Stefan Reinauer]

See patch

[ron minnich]

Acked-by: Ronald G. Minnich

[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.

[Stefan Reinauer]

On 2/22/10 4:45 PM, Uwe Hermann wrote:
> > --- 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?

No, it was intended as an orthogonal flag... the debugging is not
generally usable if you want to see "spew" but if you want to debug that
one file / subsystem ..
But maybe we have to go away from pure loglevel thinkin and need one
default loglevel per "subsystem" or "component"? not sure...
> > --- 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?

What the device tree does is actually define the values that get written
to the superio registers..
But we only have a few key words..
An "irq" is basically an 8 bit indexed io store;  io is a 16bit indexed
io store

So since stuff like the misc registers at f0/f1 may want to be set per
mainboard in devicetree.cb we need to have a keyword that copies 8 bit
to an indexed io address.
Hence the comment.

I don't really like the way the superio init is done like that, but I
don't have better suggestions either... It's better than putting all
those values in romstage.c or mainboard.c I guess...?

If you have ideas how to fix this up, we should do it..

Maybe have alias keywords idx8, idx16 so we at least don't have to call
it "IRQ"
> > --- 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.

I think it's the kind of debugging stuff that only gets touched while
you're actually working on that file, but not if you want to generally
increase verbosity.
> > /* 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?

This is mostly making some features (like disabling ethernet ports) more
"solid", and working around our still somewhat unflexible mptable
creation for different riser cards from other vendors I have here for
testing... The better way would be to create mptables completely on the
fly. It's definitely on the TODO list, but it needs some more massaging
of our device model first (getting all the APICs in place for all the
CPUs and bridges. Right now only K8 and Fam10 seem to have code to do
that, and it looks very adventurous, while it's probably also causing
the trouble with mp creation that hit the list a few days ago)
> > -#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.

Oh absolutely intentional.... Check the Makefile ;-) The complete file
is only compiled for CONFIG_GENERATE_PIRQ_TABLE==1

Thanks for the review, I'll try to add your findings in during the next
days...

Stefan