[LinuxBIOS] Remaining MCP55 bits (please comment!)

Ward Vandewege ward at gnu.org
Thu Apr 5 22:30:48 CEST 2007


Ping? What needs to happen to merge those final parts of the MCP55 patch?

Thanks,
Ward.

On Tue, Mar 27, 2007 at 12:08:35PM +0200, Stefan Reinauer wrote:
> * 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/
> 
> -- 
> linuxbios mailing list
> linuxbios at linuxbios.org
> http://www.linuxbios.org/mailman/listinfo/linuxbios
> 
> !DSPAM:4608f614212523072717219!
> 
-- 
Ward Vandewege <ward at fsf.org>
Free Software Foundation - Senior System Administrator




More information about the coreboot mailing list