[LinuxBIOS] Remaining MCP55 bits (please comment!)

Stefan Reinauer stepan at coresystems.de
Tue Mar 27 12:08:35 CEST 2007


* Lu, Yinghai <yinghai.lu at amd.com> [070321 17:06]:
> Please check the comment below:
 
dito

> * Ed Swierk <eswierk at arastra.com> [070319 20:01]:
> > Index: src/devices/hypertransport.c
> > ===================================================================
> > --- src/devices/hypertransport.c.orig
> > +++ src/devices/hypertransport.c
> > @@ -279,20 +279,7 @@ static void ht_collapse_early_enumeratio
> >  		}
> >  		/* Has the link failed? */
> >  		if (ctrl & (1 << 4)) {
> > -			/*
> > -			 * Either the link has failed, or we have
> > -			 * a CRC error.
> > -			 * Sometimes this can happen due to link
> > -			 * retrain, so lets knock it down and see
> > -			 * if its transient
> > -			 */
> > -			ctrl |= ((1 << 4) | (1 <<8)); // Link fail + Crc
> > -			pci_write_config16(prev.dev, prev.pos +
> prev.ctrl_off, ctrl);
> > -			ctrl = pci_read_config16(prev.dev, prev.pos +
> prev.ctrl_off);
> > -			if (ctrl & ((1 << 4) | (1 << 8))) {
> > -				printk_alert("Detected error on
> Hypertransport Link\n");
> > -				return;
> > -			}
> > +			return;
>  
> This needs to be dropped. It backs out a very useful error detection and
> is there due to Yinghai using a very old tree for the port.
> 
> YH: Actually hyptertransport.c is used for linuxbios_ram stage code, so
> We already reset that in CAR stage code. So...
 
So, can we completely drop hypertransport.c? This would make things
easier.
 
> > @@ -449,39 +425,53 @@ unsigned int hypertransport_scan_chain(s
> >  		if (flags & 0x1f) {
> >  			break;
> >  		}
> > -
> >  		flags &= ~0x1f; /* mask out base Unit ID */
> > -                flags |= next_unitid & 0x1f;
> > +
> > +		count = (flags >> 5) & 0x1f; /* get unit count */
> > +		not_use_count = 0;
> > +		temp_unitid = next_unitid;
> > +#if HT_CHAIN_END_UNITID_BASE < HT_CHAIN_UNITID_BASE
> 
> 
> Yinghai: Could you please comment this a bit? What does it do? 
> There are so many magic variables doing so much magic stuff.
> 
> Why is this ifdefed? Don't we want this on all systems? Why not?
> 
> YH: I have sent another patch, after that big tar ball, that one is more
> clear and useful for strange HT device.
 
ah, i remember. That's fine. Let's get this in then and look at the
other patch after this

> > Index: src/cpu/x86/mtrr/mtrr.c
> > ===================================================================
> > --- src/cpu/x86/mtrr/mtrr.c.orig
> > +++ src/cpu/x86/mtrr/mtrr.c
> > @@ -282,10 +282,16 @@ static void set_fixed_mtrr_resource(void
> >  	
> >  }
> >  
> > +#ifndef CONFIG_VAR_MTRR_HOLE
> > +#define CONFIG_VAR_MTRR_HOLE 1
> > +#endif
> 
> This needs some documentation as well. Why would one disable this?
> 
> Why would one leave it enabled? There are other HOLE variables as well?
> Is this one not the same or directly depending?
> 
> YH: some IB or other device want the kernel to change mtrr from uncached
> for WC, and some kernel can not handle subtract case. It seems latest
> kernel already could change subtract to add schema and change some range
> to WC...
 
So this is required for compatibility to older kernels?

> > Index: src/northbridge/amd/amdk8/coherent_ht.c
> > ===================================================================
> > --- src/northbridge/amd/amdk8/coherent_ht.c.orig
> > +++ src/northbridge/amd/amdk8/coherent_ht.c
> > @@ -1,10 +1,3 @@
> > -#if USE_DCACHE_RAM
> > -
> > -#include "coherent_ht_car.c"
> > -
> > -#else
> > -
> 
> 
> Does this mean we only need one version of coherent_ht.c again?
> Can we then please also drop coherent_ht_car.c? (!!!!)
> 
> YH: depends if you like to use ROMCC with it.
 
Will coherent_ht.c work with both now?

> > +#if ENABLE_APIC_EXT_ID==1
> > +#warning "FIXME Is the right place to enable apic ext id here?"
> > +
> > +      u32 val;
> > +
> > +        val = pci_read_config32(NODE_HT(node), 0x68);
> > +        val |= (HTTC_APIC_EXT_SPUR | HTTC_APIC_EXT_ID |
> HTTC_APIC_EXT_BRD_CST);
> > +        pci_write_config32(NODE_HT(node), 0x68, val);
> > +#endif
> > +}
> > +
> >  static void enable_routing(u8 node)
> >  {
> >  	u32 val;
> > @@ -199,17 +208,6 @@ static void enable_routing(u8 node)
> >  	print_spew(" done.\r\n");
> >  }
> >  
> > -static void enable_apic_ext_id(u8 node)
> > -{
> > -
> > -	u32 val;
> > -
> > -        val = pci_read_config32(NODE_HT(node), 0x68);
> > -        val |= (HTTC_APIC_EXT_SPUR | HTTC_APIC_EXT_ID |
> HTTC_APIC_EXT_BRD_CST);
> > -        pci_write_config32(NODE_HT(node), 0x68, val);
> > -}
> 
> Personally I think such function moves are nasty. I think it should be
> dropped if there is no reason.
> 
> YH: MACRO?
 
d'uh. of course. fine.
 
> >  #if CONFIG_MAX_PHYSICAL_CPUS > 1
> > -static struct setup_smp_result setup_smp2(void)
> > +static unsigned setup_smp2(void)
> 
> Above you start dropping "unsigned" (which i think is very cool) but
> here you use it again for new stuff.  Unsigned _what_?
> 
> YH: I want to gcc to decide the size sometime if we don't care about the
> size.
 
I see. Is there a real benefit?

> >  	print_spew("coherent_ht_finalize\r\n");
> > +#if K8_REV_F_SUPPORT == 0
> >  	rev_a0 = is_cpu_rev_a0();
> > +#endif
> 
> I think we should completely drop all K8 checks for rev a and rev b and
> instead put in one early check that prints
> 
> "Revision Ax and Bx systems are not supported by LinuxBIOS. Halted"
> 
> Because it is true. A rev B3 system will not be initialized correctly.
> 
> YH: some big cluster in LANL still use B3?
 
Ron, have a word? Is this still updated? Does the cluster work with the
latest image?

Last time I tried on a Solo with B3 it would not initialize. Which was
the main reason I suggested dropping this.
We don't want to drop code required for an existing userbase of course.

We could clear out all Rev Ax  references though. Opinions?

> > -#if HAVE_MP_TABLE==1
> > +#if HAVE_MP_TABLE
> 
> this looks wrong ^^^^^^

What if someone says option HAVE_MP_TABLE=0
 
> > Index: src/pc80/serial.c
> > ===================================================================
> > --- src/pc80/serial.c.orig
> > +++ src/pc80/serial.c
> > @@ -92,6 +92,7 @@ static void uart_init(void)
> >  	outb(UART_LCS, TTYS0_BASE + UART_LCR);
> >  }
> >  #else
> > +#include "../lib/uart8250.c"
> >  extern void uart8250_init(unsigned base_port, unsigned divisor,
> unsigned lcs);
> >  static void uart_init(void)
> >  {
> 
> Why this include? Looks wrong. Include stinks (if used like that).
> 
> YH: make the CONFIG_PRINTK_IN_CAR happy

Then we want it to stay.
 

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.dehttp://www.coresystems.de/




More information about the coreboot mailing list