[coreboot] [PATCH] Janitor Tasks - coreboot cleanup

[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