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 eswierk@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 + Crcpci_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 chainif (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 + Crcpci_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; }
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 donereg = pci_read_config32(NODE_HT(node), regpos);if ((reg & 0x10) == 0) break; // init completeudelay(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