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

[Stefan Reinauer]

* 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