[LinuxBIOS] Remaining MCP55 bits

[Ed Swierk]

Is there a patch with the remaining bits of Yinghai's MCP55 patch? I'm
bringing up an MCP55 board and this would be a good opportunity to try
to get the code working.

--Ed

[Ward Vandewege]

On Thu, Mar 15, 2007 at 10:31:59AM -0700, Ed Swierk wrote:
> Is there a patch with the remaining bits of Yinghai's MCP55 patch? I'm
> bringing up an MCP55 board and this would be a good opportunity to try
> to get the code working.

Same here - I'd like to start working from the mainline tree. Uwe or Stefan,
any thoughts on what's still missing? I know it's the USB debug stuff, but there
seem to be some other problems too - I just tried disabling the USB debug
functionality for the m57sli, and got an error related to do_printk:

gcc -m32 -nostdlib -nostartfiles -static -o linuxbios -T ldscript.ld crt0.o
crt0.o: In function `ram_check':crt0.S:(.rom.text+0x12a): undefined reference
to  `do_printk'
:crt0.S:(.rom.text+0x137): undefined reference to `do_printk'
:crt0.S:(.rom.text+0x148): undefined reference to `do_printk'
:crt0.S:(.rom.text+0x15a): undefined reference to `do_printk'
:crt0.S:(.rom.text+0x16b): undefined reference to `do_printk'
crt0.o:crt0.S:(.rom.text+0x17c): more undefined references to `do_printk'
follow
collect2: ld returned 1 exit status
make[1]: *** [linuxbios] Error 1
make[1]: Leaving directory
`/home/ward/lb/LinuxBIOSv2/targets/gigabyte/m57sli/m5 7sli/normal'
make: *** [normal/linuxbios.rom] Error 1

Thanks,
Ward.

[Carl-Daniel Hailfinger]

On 19.03.2007 19:34, Ward Vandewege wrote:
> On Thu, Mar 15, 2007 at 10:31:59AM -0700, Ed Swierk wrote:
> > Is there a patch with the remaining bits of Yinghai's MCP55 patch? I'm
> > bringing up an MCP55 board and this would be a good opportunity to try
> > to get the code working.
>
> Same here - I'd like to start working from the mainline tree. Uwe or Stefan,
> any thoughts on what's still missing? I know it's the USB debug stuff, but there
> seem to be some other problems too - I just tried disabling the USB debug
> functionality for the m57sli, and got an error related to do_printk:

Yes, there's still some work to do. I'll look at it later this week.

Regards,
Carl-Daniel

[Ed Swierk]

Here's a patch with most of the remaining changes from the Yinghai
monster patch, except for USB debug support.

I had to increase ROM_IMAGE_SIZE in abuild to get the MCP55 boards to
build; not sure if this is the correct solution.

I have not yet had a chance to test these changes, so use at your own risk!

--Ed

[Ward Vandewege]

Hi Ed,

Awesome - that works just fine for the M57SLI-S4. Still a lot of warnings,
but the code builds, and it boots the system just fine!

Thanks,
Ward.

On Mon, Mar 19, 2007 at 12:01:53PM -0700, Ed Swierk wrote:
> Here's a patch with most of the remaining changes from the Yinghai
> monster patch, except for USB debug support.
>
> I had to increase ROM_IMAGE_SIZE in abuild to get the MCP55 boards to
> build; not sure if this is the correct solution.
>
> I have not yet had a chance to test these changes, so use at your own risk!

[Ed Swierk]

On 3/19/07, Ward Vandewege  wrote:
> Awesome - that works just fine for the M57SLI-S4. Still a lot of warnings,
> but the code builds, and it boots the system just fine!

That's good enough for me!

Signed-off-by: Ed Swierk

--Ed

[Ward Vandewege]

On Mon, Mar 19, 2007 at 01:09:49PM -0700, Ed Swierk wrote:
> On 3/19/07, Ward Vandewege  wrote:
> > Awesome - that works just fine for the M57SLI-S4. Still a lot of warnings,
> > but the code builds, and it boots the system just fine!
>
> That's good enough for me!
>
> Signed-off-by: Ed Swierk

Acked-by: Ward Vandewege

I have tested the patch - it works for the gigabyte m57sli-s4, but someone
should probably review the actual code and ack this, so it can be commited.

Thanks,
Ward.

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Stefan Reinauer]

Small review. Most of the patch is fine, but some things do need to be
changed before this can go in. Can someone else comment on this? Yinghai?
And, more important, can someone generate a new patch incorporating the
comments below?

All files that I did not mention in this review are fine for commit
under the condition that they do not break abuild, which I did not
check.

Overall comment, not explicitly to YhLu, is we are using far too many
config options (Or rather: preprocessor madness) for stuff that should
either be dropped, always enabled, or done in the code.

Often the reason seems to be lazyness to fix things in the rest of the
code, so a magic variable is added and enabled on some boards, so that 
we don't risk being easily able to use new features on all K8 boards.

Please, help getting this straightened out. It is sitting on the TODO
list far too long now.

* 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.
> @@ -396,25 +385,12 @@ unsigned int hypertransport_scan_chain(s
>    	/* Wait until the link initialization is complete */
>    	do {
>    		ctrl = pci_read_config16(prev.dev, prev.pos + prev.ctrl_off);
> 	if (ctrl & (1 << 6))
> 		goto end_of_chain;	// End of chain
> 	if (ctrl & ((1 << 4) | (1 << 8))) {
> 		/*
> 		 * 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");
> 			goto end_of_chain;
> 		}
> 	/* Is this the end of the hypertransport chain?
> 	 * Has the link failed?
> 	 * If so further scanning is pointless.
> 	 */
> 	if (ctrl & ((1 << 6) | (1 << 4))) {
> 		goto end_of_chain;
> }
> } while((ctrl & (1 << 5)) == 0);

dito.
> @@ -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?
> 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?
> Index: src/cpu/x86/lapic/lapic_cpu_init.c
>
> --- src/cpu/x86/lapic/lapic_cpu_init.c.orig
> +++ src/cpu/x86/lapic/lapic_cpu_init.c
> @@ -13,6 +13,13 @@
>  #include <smp/atomic.h>
>  #include <smp/spinlock.h>
>  #include <cpu/cpu.h>
> +#include <cpu/x86/tsc.h>
>
> +static inline void print_debug_tsc_x(const char *str, unsigned i, unsigned val, unsigned val2)
> +{
>    printk_debug("%s[%02x]=%08x%08x\n", str, i, val, val2);
>
> +}
>
> #if CONFIG_SMP == 1
>
> @@ -374,6 +381,7 @@ void initialize_cpus(struct bus *cpu_bus
>  {
>    struct device_path cpu_path;
>    struct cpu_info *info;
>
> tsc_t tsc;
>
> /* Find the info struct for this cpu */
> info = cpu_info();
>
> @@ -400,6 +408,9 @@ void initialize_cpus(struct bus *cpu_bus
>      cpus_ready_for_init(); 
>
>    tsc = rdtsc();
>    print_debug_tsc_x("\nFinal              : tsc ", 10,  tsc.hi, tsc.lo);
>
> #if CONFIG_SMP == 1
>    #if SERIAL_CPU_INIT == 0
>    /* start all aps at first, so we can init ECC all together */
> @@ -419,6 +430,8 @@ void initialize_cpus(struct bus *cpu_bus
>    /* Now wait the rest of the cpus stop*/
>    wait_other_cpus_stop(cpu_bus);
>  #endif
>    tsc = rdtsc();
>    print_debug_tsc_x("\nFinal              : tsc ", 11,  tsc.hi, tsc.lo);
>
> }

All changes to the whole file are just debugging. And not exactly nice. I suggest dropping it.
> 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? (!!!!)
> -#if CONFIG_USE_INIT
> +#if CONFIG_USE_PRINTK_IN_CAR

Is this rename consistent with the rest of the tree?

If not, can someone do a sed s/CONFIG_USE_INIT/CONFIG_USE_PRINTK_IN_CAR/
on all x86 boards in the tree?
> +static void enable_apic_ext_id(u8 node) 
> +{
> +#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.
> @@ -259,23 +257,6 @@ static void rename_temp_node(u8 node)
>
> print_spew(" done.\r\n");
>  }
> -#if K8_HT_CHECK_PENDING_LINK == 1
> -static void wait_ht_stable(uint8_t node)
> -{
>    uint8_t linkn;
>    for(linkn = 0; linkn<3; linkn++) {
>            uint8_t regpos;
>            uint16_t i;
>            uint32_t reg;
>            regpos = 0x98 + 0x20 * linkn;
>            for(i = 0; i < 0xff; i++) { //wait to make sure it is done
>                    reg = pci_read_config32(NODE_HT(node), regpos);
>                    if ((reg & 0x10) == 0) break; // init complete
>                    udelay(10);
>            } 
>    }
>
> -}
> -#endif

Why is this dropped?
> if (!is_cpu_pre_e0()) {
>
> +#if K8_HT_FREQ_1G_SUPPORT == 1
> #if K8_REV_F_SUPPORT == 0
> if (!is_cpu_pre_e0())
> #endif
> {
> return freq_cap;
> }
>
> +#endif

This nesting of ifdef statements looks wrong. Why would supporting more
than before drop code. This means "K8_NO_REV_E_AND_EARLIER_SUPPORT"
instead of "K8_REV_F_SUPPORT".  I believe it should be dropped (and many
places like that in the code) It just makes the code ugly to read for no
good reason. (Rev F are all using CAR anyways, so code size impact is
minimal)

Yinghai? What is it good for?
> @@ -676,11 +665,6 @@ static void setup_uniprocessor(void)
>    disable_probes();
>  }
>
> -struct setup_smp_result {
> int nodes;
> int needs_reset;
>
> -};
>
> #if CONFIG_MAX_PHYSICAL_CPUS > 2
>  static int optimize_connection_group(const u8 *opt_conn, int num) {
>    int needs_reset = 0;
> @@ -695,21 +679,20 @@ static int optimize_connection_group(con
>  #endif
>
> #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_?
> 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.
> Index: src/arch/i386/boot/tables.c
>
> --- src/arch/i386/boot/tables.c.orig
> +++ src/arch/i386/boot/tables.c
> @@ -10,38 +10,12 @@
>  #include <arch/acpi.h>
>  #include "linuxbios_table.h"
>
> -// Global Descriptor Table, defined in c_start.S
> -extern uint8_t gdt;
> -extern uint8_t gdt_end;
>
> -/* i386 lgdt argument */
> -struct gdtarg {
> unsigned short limit;
> unsigned int base;
>
> -} __attribute__((packed));
>
> -// Copy GDT to new location and reload it
> -// 2003-07 by SONE Takeshi
> -// Ported from Etherboot to LinuxBIOS 2005-08 by Steve Magnani
> -void move_gdt(unsigned long newgdt)
> -{
> uint16_t num_gdt_bytes = &gdt_end - &gdt;
> struct gdtarg gdtarg;
> printk_debug("Moving GDT to %#lx...", newgdt);
> memcpy((void*)newgdt, &gdt, num_gdt_bytes);
> gdtarg.base = newgdt;
> gdtarg.limit = num_gdt_bytes - 1;
> __asm__ __volatile__ ("lgdt %0\n\t" : : "m" (gdtarg));
> printk_debug("ok\n");
>
> -}
>
> struct lb_memory *write_tables(void)

I think dropping move_gdt _here_ (in tables) is good. BUT: It is a
regression caused by Yinghai working on an older tree.

It should be added again elsewhere. Where?

We have this nasty bugger in v3 as well.. it must die ;-)
> -#if HAVE_MP_TABLE==1
> +#if HAVE_MP_TABLE

this looks wrong ^^^^^^
> Index: src/arch/i386/lib/cpu.c
>
> --- src/arch/i386/lib/cpu.c.orig
> +++ src/arch/i386/lib/cpu.c
> @@ -207,7 +207,6 @@ static void set_cpu_ops(struct device *c
>    		}
>    	}
>    }
> die("Unknown cpu");

This should be a printk at least?
> return;
>   found:
>    cpu->ops = driver->ops;
> @@ -223,7 +222,7 @@ void cpu_initialize(void)
>    struct device *cpu;
>    struct cpu_info *info;
>    struct cpuinfo_x86 c;
> info = cpu_info();

This should be dropped.
> 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).
> Index: src/mainboard/gigabyte/m57sli/Options.lb
>
> --- src/mainboard/gigabyte/m57sli/Options.lb.orig
> +++ src/mainboard/gigabyte/m57sli/Options.lb
> @@ -213,7 +213,7 @@ default K8_HT_FREQ_1G_SUPPORT=1
>  default CONFIG_CONSOLE_VGA=1
>  default CONFIG_PCI_ROM_RUN=1
>
> -default CONFIG_USBDEBUG_DIRECT=1
> +#default CONFIG_USBDEBUG_DIRECT=1
>
> #HT Unit ID offset, default is 1, the typical one, 0 mean only one HT device
>  default HT_CHAIN_UNITID_BASE=0

No USB debug console? What other console is there on the m57sli?
> Index: util/abuild/abuild
>
> --- util/abuild/abuild.orig
> +++ util/abuild/abuild
> @@ -142,14 +142,14 @@ EOF
>    		cat <<EOF
>  romimage "normal"
>    option USE_FALLBACK_IMAGE=0
> option ROM_IMAGE_SIZE=0x16000
> option ROM_IMAGE_SIZE=0x17000
> option LINUXBIOS_EXTRA_VERSION=".0-normal"
> payload PAYLOAD
>
> end
>
> romimage "fallback" 
>    option USE_FALLBACK_IMAGE=1
> option ROM_IMAGE_SIZE=0x16000
> option ROM_IMAGE_SIZE=0x17000
> option LINUXBIOS_EXTRA_VERSION=".0-fallback"
> payload PAYLOAD
>
> end

Which boards are failing without this option? They should get their 
specific Config.abuild instead. This option will break building images
with filo payload on qa.linuxbios.org for many small systems.

Stefan

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Lu, Yinghai]

Please check the comment below:

-----Original Message-----
From: [e-mail]
[mailto:[e-mail]] On Behalf Of Stefan Reinauer
Sent: Wednesday, March 21, 2007 5:40 AM
To: Ed Swierk
Cc: Carl-Daniel Hailfinger; Ward Vandewege; LinuxBIOS
Subject: Re: [LinuxBIOS] Remaining MCP55 bits (please comment!)

* 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...
> @@ -396,25 +385,12 @@ unsigned int hypertransport_scan_chain(s
>    	/* Wait until the link initialization is complete */
>    	do {
>    		ctrl = pci_read_config16(prev.dev, prev.pos +

prev.ctrl_off);
> 	if (ctrl & (1 << 6))
> 		goto end_of_chain;	// End of chain
> 	if (ctrl & ((1 << 4) | (1 << 8))) {
> 		/*
> 		 * 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");
> 			goto end_of_chain;
> 		}
> 	/* Is this the end of the hypertransport chain?
> 	 * Has the link failed?
> 	 * If so further scanning is pointless.
> 	 */
> 	if (ctrl & ((1 << 6) | (1 << 4))) {
> 		goto end_of_chain;
> }
> } while((ctrl & (1 << 5)) == 0);

dito. 
YH: ....
> @@ -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.
> 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...
> Index: src/cpu/x86/lapic/lapic_cpu_init.c
>
> --- src/cpu/x86/lapic/lapic_cpu_init.c.orig
> +++ src/cpu/x86/lapic/lapic_cpu_init.c
> @@ -13,6 +13,13 @@
>  #include <smp/atomic.h>
>  #include <smp/spinlock.h>
>  #include <cpu/cpu.h>
> +#include <cpu/x86/tsc.h>
>
> +static inline void print_debug_tsc_x(const char *str, unsigned i,

unsigned val, unsigned val2)
> +{
>    printk_debug("%s[%02x]=%08x%08x\n", str, i, val, val2);
>
> +}
>
> #if CONFIG_SMP == 1
>
> @@ -374,6 +381,7 @@ void initialize_cpus(struct bus *cpu_bus
>  {
>    struct device_path cpu_path;
>    struct cpu_info *info;
>
> tsc_t tsc;
>
> /* Find the info struct for this cpu */
> info = cpu_info();
>
> @@ -400,6 +408,9 @@ void initialize_cpus(struct bus *cpu_bus
>      cpus_ready_for_init(); 
>
>    tsc = rdtsc();
>    print_debug_tsc_x("\nFinal              : tsc ", 10,  tsc.hi,

tsc.lo);
> #if CONFIG_SMP == 1
>    #if SERIAL_CPU_INIT == 0
>    /* start all aps at first, so we can init ECC all together */
> @@ -419,6 +430,8 @@ void initialize_cpus(struct bus *cpu_bus
>    /* Now wait the rest of the cpus stop*/
>    wait_other_cpus_stop(cpu_bus);
>  #endif
>    tsc = rdtsc();
>    print_debug_tsc_x("\nFinal              : tsc ", 11,  tsc.hi,

tsc.lo);
> }

All changes to the whole file are just debugging. And not exactly nice.
I suggest dropping it.
> 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.
> -#if CONFIG_USE_INIT
> +#if CONFIG_USE_PRINTK_IN_CAR

Is this rename consistent with the rest of the tree?

If not, can someone do a sed s/CONFIG_USE_INIT/CONFIG_USE_PRINTK_IN_CAR/
on all x86 boards in the tree?

YH: It make PRINTK can be used even you are not use initobjects.
> +static void enable_apic_ext_id(u8 node) 
> +{
> +#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?
> @@ -259,23 +257,6 @@ static void rename_temp_node(u8 node)
>
> print_spew(" done.\r\n");
>  }
> -#if K8_HT_CHECK_PENDING_LINK == 1
> -static void wait_ht_stable(uint8_t node)
> -{
>    uint8_t linkn;
>    for(linkn = 0; linkn<3; linkn++) {
>            uint8_t regpos;
>            uint16_t i;
>            uint32_t reg;
>            regpos = 0x98 + 0x20 * linkn;
>            for(i = 0; i < 0xff; i++) { //wait to make sure it is

done
>                    reg = pci_read_config32(NODE_HT(node),

regpos);
>                    if ((reg & 0x10) == 0) break; // init

complete
>                    udelay(10);
>            } 
>    }
>
> -}
> -#endif

Why is this dropped?
> if (!is_cpu_pre_e0()) {
>
> +#if K8_HT_FREQ_1G_SUPPORT == 1
> #if K8_REV_F_SUPPORT == 0
> if (!is_cpu_pre_e0())
> #endif
> {
> return freq_cap;
> }
>
> +#endif

This nesting of ifdef statements looks wrong. Why would supporting more
than before drop code. This means "K8_NO_REV_E_AND_EARLIER_SUPPORT"
instead of "K8_REV_F_SUPPORT".  I believe it should be dropped (and many
places like that in the code) It just makes the code ugly to read for no
good reason. (Rev F are all using CAR anyways, so code size impact is
minimal)

Yinghai? What is it good for?
> @@ -676,11 +665,6 @@ static void setup_uniprocessor(void)
>    disable_probes();
>  }
>
> -struct setup_smp_result {
> int nodes;
> int needs_reset;
>
> -};
>
> #if CONFIG_MAX_PHYSICAL_CPUS > 2
>  static int optimize_connection_group(const u8 *opt_conn, int num) {
>    int needs_reset = 0;
> @@ -695,21 +679,20 @@ static int optimize_connection_group(con
>  #endif
>
> #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.
> 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?
> Index: src/arch/i386/boot/tables.c
>
> --- src/arch/i386/boot/tables.c.orig
> +++ src/arch/i386/boot/tables.c
> @@ -10,38 +10,12 @@
>  #include <arch/acpi.h>
>  #include "linuxbios_table.h"
>
> -// Global Descriptor Table, defined in c_start.S
> -extern uint8_t gdt;
> -extern uint8_t gdt_end;
>
> -/* i386 lgdt argument */
> -struct gdtarg {
> unsigned short limit;
> unsigned int base;
>
> -} __attribute__((packed));
>
> -// Copy GDT to new location and reload it
> -// 2003-07 by SONE Takeshi
> -// Ported from Etherboot to LinuxBIOS 2005-08 by Steve Magnani
> -void move_gdt(unsigned long newgdt)
> -{
> uint16_t num_gdt_bytes = &gdt_end - &gdt;
> struct gdtarg gdtarg;
> printk_debug("Moving GDT to %#lx...", newgdt);
> memcpy((void*)newgdt, &gdt, num_gdt_bytes);
> gdtarg.base = newgdt;
> gdtarg.limit = num_gdt_bytes - 1;
> __asm__ __volatile__ ("lgdt %0\n\t" : : "m" (gdtarg));
> printk_debug("ok\n");
>
> -}
>
> struct lb_memory *write_tables(void)

I think dropping move_gdt _here_ (in tables) is good. BUT: It is a
regression caused by Yinghai working on an older tree.

It should be added again elsewhere. Where?

We have this nasty bugger in v3 as well.. it must die ;-)
> -#if HAVE_MP_TABLE==1
> +#if HAVE_MP_TABLE

this looks wrong ^^^^^^
> Index: src/arch/i386/lib/cpu.c
>
> --- src/arch/i386/lib/cpu.c.orig
> +++ src/arch/i386/lib/cpu.c
> @@ -207,7 +207,6 @@ static void set_cpu_ops(struct device *c
>    		}
>    	}
>    }
> die("Unknown cpu");

This should be a printk at least?
> return;
>   found:
>    cpu->ops = driver->ops;
> @@ -223,7 +222,7 @@ void cpu_initialize(void)
>    struct device *cpu;
>    struct cpu_info *info;
>    struct cpuinfo_x86 c;
> info = cpu_info();

This should be dropped.
> 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
> Index: src/mainboard/gigabyte/m57sli/Options.lb
>
> --- src/mainboard/gigabyte/m57sli/Options.lb.orig
> +++ src/mainboard/gigabyte/m57sli/Options.lb
> @@ -213,7 +213,7 @@ default K8_HT_FREQ_1G_SUPPORT=1
>  default CONFIG_CONSOLE_VGA=1
>  default CONFIG_PCI_ROM_RUN=1
>
> -default CONFIG_USBDEBUG_DIRECT=1
> +#default CONFIG_USBDEBUG_DIRECT=1
>
> #HT Unit ID offset, default is 1, the typical one, 0 mean only one HT

device
> default HT_CHAIN_UNITID_BASE=0

No USB debug console? What other console is there on the m57sli?

YH: serial console
> Index: util/abuild/abuild
>
> --- util/abuild/abuild.orig
> +++ util/abuild/abuild
> @@ -142,14 +142,14 @@ EOF
>    		cat <<EOF
>  romimage "normal"
>    option USE_FALLBACK_IMAGE=0
> option ROM_IMAGE_SIZE=0x16000
> option ROM_IMAGE_SIZE=0x17000
> option LINUXBIOS_EXTRA_VERSION=".0-normal"
> payload PAYLOAD
>
> end
>
> romimage "fallback" 
>    option USE_FALLBACK_IMAGE=1
> option ROM_IMAGE_SIZE=0x16000
> option ROM_IMAGE_SIZE=0x17000
> option LINUXBIOS_EXTRA_VERSION=".0-fallback"
> payload PAYLOAD
>
> end

Which boards are failing without this option? They should get their 
specific Config.abuild instead. This option will break building images
with filo payload on qa.linuxbios.org for many small systems.

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Ward Vandewege]

Hi all,

What's the status on this? It would be really great to have the MCP55 code
merged into the v2 tree.

Thanks,
Ward.

On Wed, Mar 21, 2007 at 09:06:51AM -0700, Lu, Yinghai wrote:
> Please check the comment below:
>
> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]] On Behalf Of Stefan Reinauer
> Sent: Wednesday, March 21, 2007 5:40 AM
> To: Ed Swierk
> Cc: Carl-Daniel Hailfinger; Ward Vandewege; LinuxBIOS
> Subject: Re: [LinuxBIOS] Remaining MCP55 bits (please comment!)
> 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;

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Stefan Reinauer]

* Ward Vandewege  [070327 11:43]:
> Hi all,
>
> What's the status on this? It would be really great to have the MCP55 code
> merged into the v2 tree.
>
> Thanks,
> Ward.

I think we're mostly through.

[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

[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!

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Stefan Reinauer]

* Ward Vandewege  [070405 22:30]:
> Ping? What needs to happen to merge those final parts of the MCP55 patch?

Ok, most of the code is in, and it is fatal, as usual with megapatches
on old tree versions.

I don't want to be Mr. Forefinger, but the delivery quality and interest
in getting this officially supported was obviously below the standards
that we usually require.

Stefan

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Peter Stuge]

On Sat, Apr 07, 2007 at 07:54:16PM +0200, Stefan Reinauer wrote:
> Ward Vandewege  [070405 22:30]:
> > Ping? What needs to happen to merge those final parts of the
> > MCP55 patch?
>
> Ok, most of the code is in, and it is fatal, as usual with
> megapatches on old tree versions.

I guess the problem is that it wasn't actually a patch.
> I don't want to be Mr. Forefinger, but the delivery quality and
> interest in getting this officially supported was obviously below
> the standards that we usually require.

Maybe we should agree on requiring rebasing of patches against old
revisions before they have a chance of being committed.

This will of course mean more work for the submitter, but I think it
is a very reasonable requirement.

//Peter

[LinuxBIOS] Remaining MCP55 bits (please comment!)

[Corey Osgood]

Peter Stuge wrote:
> > I don't want to be Mr. Forefinger, but the delivery quality and
> > interest in getting this officially supported was obviously below
> > the standards that we usually require.
>
> Maybe we should agree on requiring rebasing of patches against old
> revisions before they have a chance of being committed.
>
> This will of course mean more work for the submitter, but I think it
> is a very reasonable requirement.

Full ack. When I get a chance, I'll lend a hand with fixing targets, if
it isn't done by then. If anyone else is working on this, can we get a
rundown on who's working on what?

-Corey

[Ed Swierk]

On 3/19/07, Ward Vandewege  wrote:
> Same here - I'd like to start working from the mainline tree. Uwe or Stefan,
> any thoughts on what's still missing? I know it's the USB debug stuff, but there
> seem to be some other problems too - I just tried disabling the USB debug
> functionality for the m57sli, and got an error related to do_printk:
>
> gcc -m32 -nostdlib -nostartfiles -static -o linuxbios -T ldscript.ld crt0.o
> crt0.o: In function `ram_check':crt0.S:(.rom.text+0x12a): undefined reference
> to  `do_printk'
> :crt0.S:(.rom.text+0x137): undefined reference to `do_printk'
> :crt0.S:(.rom.text+0x148): undefined reference to `do_printk'
> :crt0.S:(.rom.text+0x15a): undefined reference to `do_printk'
> :crt0.S:(.rom.text+0x16b): undefined reference to `do_printk'
> crt0.o:crt0.S:(.rom.text+0x17c): more undefined references to `do_printk'
> follow
> collect2: ld returned 1 exit status
> make[1]: *** [linuxbios] Error 1
> make[1]: Leaving directory
> `/home/ward/lb/LinuxBIOSv2/targets/gigabyte/m57sli/m5 7sli/normal'
> make: *** [normal/linuxbios.rom] Error 1

I'm testing a pach that includes the remaining bits except for USB
debug. I'll post it in about an hour.

--Ed