Re: [LinuxBIOS] Remaining MCP55 bits (please comment!)

[Ward Vandewege]

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  [070321 17:06]:
> > Please check the comment below:
>
> dito
> > Ed Swierk  [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
>
> !DSPAM:4608f614212523072717219!