[coreboot] #if 0 crud in v3

[Carl-Daniel Hailfinger]

Hi,

Ron's latest patch triggered me to look at how much dead code we already
have in v3. The situation is not bad, but we could use more commits
which add comments before #if 0 and #if 1, just to make sure the code is
not left in forever without known purpose.

Analysis of the current state follows:

./southbridge/amd/cs5536/cs5536.c
> void chipsetinit(void)
> {
>    struct device *dev;
>    struct msr msr;
>    struct southbridge_amd_cs5536_dts_config *sb;
>    const struct msrinit *csi;
>
> post_code(P80_CHIPSET_INIT);
>    dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
>    		      PCI_DEVICE_ID_AMD_CS5536_ISA, 0);
>    if (!dev) {
>    	printk(BIOS_ERR, "%s: Could not find the south bridge!\n",
>    	       __FUNCTION__);
>    	return;
>    }
>    sb = (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;
>
> #if 0

How about
#ifdef CONFIG_SUSPEND_TO_RAM
> if (!IsS3Resume())
>    {
>    	struct acpi_init *aci = acpi_init_table;
>    	for (/* Nothing */; aci->ioreg; aci++) {
>    		outl(aci->regdata, aci->ioreg);
>    		inl(aci->ioreg);
>    	}
>    	pm_chipset_init();
>    }
> #endif
>
> /* Set HD IRQ. */
>    outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);
>    outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_IN_AUX1_SELECT);
> static void southbridge_init(struct device *dev)
> {
>    struct southbridge_amd_cs5536_dts_config *sb =
>        (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;
>
> /*
>     * struct device *gpiodev;
>     * unsigned short gpiobase = MDD_GPIO;
>     */
>
> printk(BIOS_ERR, "cs5536: %s\n", __FUNCTION__);
>
> setup_i8259();
>    lpc_init(sb);
>    uarts_init(sb);
>
> if (sb->enable_gpio_int_route) {
>    	vr_write((VRC_MISCELLANEOUS << 8) + PCI_INT_AB,
>    		 (sb->enable_gpio_int_route & 0xFFFF));
>    	vr_write((VRC_MISCELLANEOUS << 8) + PCI_INT_CD,
>    		 (sb->enable_gpio_int_route >> 16));
>    }
>
> printk(BIOS_ERR, "cs5536: %s: enable_ide_nand_flash is %d\n",
>           __FUNCTION__, sb->enable_ide_nand_flash);
>    if (sb->enable_ide_nand_flash == 1)
>    	enable_ide_nand_flash_header();
>
> enable_USB_port4(sb);
>
> if (sb->enable_ide)
>    	ide_init(dev);
>
> #warning Add back in unwanted VPCI support
> #if 0

Any reason not to remove this #if 0?
> /* Disable unwanted virtual PCI devices. */
>    for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) {
>    	printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n",
>    	       sb->unwanted_vpci[i]);
>    	outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8);
>    	outl(0xDEADBEEF, 0xCFC);
>    }
> #endif
>    printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__);
> }
>
> /**

./northbridge/amd/geodelx/raminit.c
> static void check_ddr_max(u8 dimm0, u8 dimm1)
> {
>    u8 spd_byte0, spd_byte1;
>    u16 speed;
>
> /* PC133 identifier */
>    spd_byte0 = spd_read_byte(dimm0, SPD_MIN_CYCLE_TIME_AT_CAS_MAX);
>    if (spd_byte0 == 0xFF)
>    	spd_byte0 = 0;
>    spd_byte1 = spd_read_byte(dimm1, SPD_MIN_CYCLE_TIME_AT_CAS_MAX);
>    if (spd_byte1 == 0xFF)
>    	spd_byte1 = 0;
>
> /* I don't think you need this check. */
> #if 0

We may need an opinion from someone familiar with the hardware details...
> if (spd_byte0 < 0xA0 || spd_byte0 < 0xA0) {
>    	printk(BIOS_EMERG, "DIMM overclocked. Check GeodeLink speed\n");
>    	post_code(POST_PLL_MEM_FAIL);
>    	hlt();
>    }
> #endif
>
> /* Use the slowest DIMM. */
>    if (spd_byte0 < spd_byte1)
>    	spd_byte0 = spd_byte1;
>
> /* Turn SPD ns time into MHz. Check what the asm does to this math. */
>    speed = 2 * ((10000 / (((spd_byte0 >> 4) * 10) + (spd_byte0 & 0x0F))));
>
> /* Current speed > max speed? */
>    if (geode_link_speed() > speed) {
>    	printk(BIOS_EMERG, "DIMM overclocked. Check GeodeLink speed\n");
>    	post_code(POST_PLL_MEM_FAIL);
>    	hlt();
>    }
> }
> void sdram_set_registers(void)
> {
>    struct msr msr;
>
> /* Set Timing Control */
>    msr = rdmsr(MC_CF1017_DATA);
>    msr.lo &= ~(7 << CF1017_LOWER_RD_TMG_CTL_SHIFT);
>    if (geode_link_speed() < 334)
>    	msr.lo |= (3 << CF1017_LOWER_RD_TMG_CTL_SHIFT);
>    else
>    	msr.lo |= (4 << CF1017_LOWER_RD_TMG_CTL_SHIFT);
>    wrmsr(MC_CF1017_DATA, msr);
>
> /* Set Refresh Staggering */
>    msr = rdmsr(MC_CF07_DATA);
>    msr.lo &= ~0xF0;
>    msr.lo |= 0x40;		/* Set refresh to 4 SDRAM clocks. */
>    wrmsr(MC_CF07_DATA, msr);
>
> /* Memory Interleave: Set HOI here otherwise default is LOI. */
> #if 0

Switch to config option or NVRAM option or kill the code altogether?
> msr = rdmsr(MC_CF8F_DATA);
>    msr.hi |= CF8F_UPPER_HOI_LOI_SET;
>    wrmsr(MC_CF8F_DATA, msr);
> #endif
> }

./northbridge/amd/geodelx/geodelx.c
> static void geodelx_northbridge_init(struct device *dev)
> {
>    /* struct msr msr; */
>
> printk(BIOS_SPEW, ">> Entering northbridge.c: %s\n", __FUNCTION__);
>
> enable_shadow(dev);
>
> #if 0

No idea about this one.
> /* Swiss cheese */
>    msr = rdmsr(MSR_GLIU0_SHADOW);
>
> msr.hi |= 0x3;
>    msr.lo |= 0x30000;
>
> printk(BIOS_DEBUG, "MSR 0x%08X is now 0x%08X:0x%08X\n",
>           MSR_GLIU0_SHADOW, msr.hi, msr.lo);
>    printk(BIOS_DEBUG, "MSR 0x%08X is now 0x%08X:0x%08X\n",
>           MSR_GLIU1_SHADOW, msr.hi, msr.lo);
>    /* TODO: Is the respective wrmsr() missing? */
> #endif
> }

./device/device.c
> void dev_phase4(void)
> {
>    struct resource *io, *mem;
>    struct device *root;
>
> printk(BIOS_INFO, "Phase 4: Allocating resources...\n");
>
> root = &dev_root;
>    if (!root->ops) {
>    	printk(BIOS_ERR,
>    	       "Phase 4: dev_root missing ops initialization\nPhase 4: Failed.\n");
>    	return;
>    }
>    if (!root->ops->phase4_read_resources) {
>    	printk(BIOS_ERR,
>    	       "dev_root ops missing read_resources\nPhase 4: Failed.\n");
>    	return;
>    }
>
> if (!root->ops->phase4_set_resources) {
>    	printk(BIOS_ERR,
>    	       "dev_root ops missing set_resources\nPhase 4: Failed.\n");
>    	return;
>    }
>
> printk(BIOS_INFO, "Phase 4: Reading resources...\n");
>    root->ops->phase4_read_resources(root);
>    printk(BIOS_INFO, "Phase 4: Done reading resources.\n");
>
> /* Get the resources. */
>    io = &root->resource[0];
>    mem = &root->resource[1];
>
> /* Make certain the I/O devices are allocated somewhere safe. */
>    io->base = DEVICE_IO_START;
>    io->flags |= IORESOURCE_ASSIGNED;
>    io->flags &= ~IORESOURCE_STORED;
>
> /* Now reallocate the PCI resources memory with the
>     * highest addresses I can manage.
>     */
>    mem->base = resource_max(&root->resource[1]);
>    mem->flags |= IORESOURCE_ASSIGNED;
>    mem->flags &= ~IORESOURCE_STORED;
>
> #if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1
>    /* Allocate the VGA I/O resource. */
>    allocate_vga_resource();
> #endif
>
> /* Store the computed resource allocations into device registers. */
>    printk(BIOS_INFO, "Phase 4: Setting resources...\n");
>    root->ops->phase4_set_resources(root);
>    printk(BIOS_INFO, "Phase 4: Done setting resources.\n");
> #if 0

Blindly copied from v2 in v3:r69. Was never enabled in v2 (committed
as dead code by Eric Biederman in v2:r1664 with log message
"Updates for 64bit resource support, handling missing devices and cpus
 in the config file").
Kill?
> mem->flags |= IORESOURCE_STORED;
>    report_resource_stored(root, mem, "");
> #endif
>
> printk(BIOS_INFO, "Phase 4: Done allocating resources.\n");
> }

./device/pci_device.c
> struct resource *pci_get_resource(struct device *dev, unsigned long
> index) { struct resource *resource; unsigned long value, attr;
> resource_t moving, limit; /* Initialize the resources to nothing. */
> resource = new_resource(dev, index); /* Get the initial value. */
> value = pci_read_config32(dev, index); /* See which bits move. */
> moving = pci_moving_config32(dev, index); /* Initialize attr to the
> bits that do not move. */ attr = value & ~moving; /* If it is a 64bit
> resource look at the high half as well. */ if (((attr &
> PCI_BASE_ADDRESS_SPACE_IO) == 0) && ((attr &
> PCI_BASE_ADDRESS_MEM_LIMIT_MASK) == PCI_BASE_ADDRESS_MEM_LIMIT_64)) {
> /* Find the high bits that move. */ moving |= ((resource_t)
> pci_moving_config32(dev, index + 4)) << 32; } /* Find the resource
> constraints. * Start by finding the bits that move. From there: * -
> Size is the least significant bit of the bits that move. * - Limit is
> all of the bits that move plus all of the lower bits. * See PCI Spec
> 6.2.5.1. */ limit = 0; if (moving) { resource->size = 1;
> resource->align = resource->gran = 0; while (!(moving &
> resource->size)) { resource->size <<= 1; resource->align += 1;
> resource->gran += 1; } resource->limit = limit = moving |
> (resource->size - 1); } /* Some broken hardware has read-only
> registers that do not * really size correctly. * Example: the Acer
> M7229 has BARs 1-4 normally read-only. * so BAR1 at offset 0x10 reads
> 0x1f1. If you size that register * by writing 0xffffffff to it, it
> will read back as 0x1f1 -- a * violation of the spec. * We catch this
> case and ignore it by observing which bits move, * This also catches
> the common case unimplemented registers * that always read back as 0.
> */ if (moving == 0) { if (value != 0) { printk(BIOS_DEBUG, "%s
> register %02lx(%08lx), read-only ignoring it\n", dev_path(dev), index,
> value); } resource->flags = 0; } else if (attr &
> PCI_BASE_ADDRESS_SPACE_IO) { /* An I/O mapped base address. */ attr &=
> PCI_BASE_ADDRESS_IO_ATTR_MASK; resource->flags |= IORESOURCE_IO; /* I
> don't want to deal with 32bit I/O resources. */ resource->limit =
> 0xffff; } else { /* A Memory mapped base address. */ attr &=
> PCI_BASE_ADDRESS_MEM_ATTR_MASK; resource->flags |= IORESOURCE_MEM; if
> (attr & PCI_BASE_ADDRESS_MEM_PREFETCH) { resource->flags |=
> IORESOURCE_PREFETCH; } attr &= PCI_BASE_ADDRESS_MEM_LIMIT_MASK; if
> (attr == PCI_BASE_ADDRESS_MEM_LIMIT_32) { /* 32bit limit. */
> resource->limit = 0xffffffffUL; } else if (attr ==
> PCI_BASE_ADDRESS_MEM_LIMIT_1M) { /* 1MB limit. */ resource->limit =
> 0x000fffffUL; } else if (attr == PCI_BASE_ADDRESS_MEM_LIMIT_64) { /*
> 64bit limit. */ resource->limit = 0xffffffffffffffffULL;
> resource->flags |= IORESOURCE_PCI64; } else { /* Invalid value. */
> resource->flags = 0; } } /* Don't let the limit exceed which bits can
> move. */ if (resource->limit > limit) { resource->limit = limit; } #if 0

This just disables some debug printing. Enable it on the most verbose level?
> if (resource->flags) { printk(BIOS_DEBUG, "%s %02x ->", dev_path(dev),
> resource->index); printk(BIOS_DEBUG, " value: 0x%08llx zeroes:
> 0x%08llx ones: 0x%08llx attr: %08lx\n", value, zeroes, ones, attr);
> printk(BIOS_DEBUG, "%s %02x -> size: 0x%08llx max: 0x%08llx %s\n ",
> dev_path(dev), resource->index, resource->size, resource->limit,
> resource_type(resource)); } #endif return resource; }
> void pci_level_irq(unsigned char intNum) { unsigned short intBits =
> inb(0x4d0) | (((unsigned)inb(0x4d1)) << 8); printk(BIOS_SPEW, "%s:
> current ints are 0x%x\n", __func__, intBits); intBits |= (1 <<
> intNum); printk(BIOS_SPEW, "%s: try to set ints 0x%x\n", __func__,
> intBits); /* Write new values. */ outb((unsigned char)intBits, 0x4d0);
> outb((unsigned char)(intBits >> 8), 0x4d1); /* This seems like an
> error but is not. */ #if 1

Just remove the #if 1?
> if (inb(0x4d0) != (intBits & 0xff)) { printk(BIOS_ERR, "%s: lower
> order bits are wrong: want 0x%x, got 0x%x\n", __func__, intBits &
> 0xff, inb(0x4d0)); } if (inb(0x4d1) != ((intBits >> 8) & 0xff)) {
> printk(BIOS_ERR, "%s: lower order bits are wrong: want 0x%x, got
> 0x%x\n", __func__, (intBits >> 8) & 0xff, inb(0x4d1)); } #endif }

./arch/x86/serial.c
> void uart_init(void) { unsigned ttysx_div; #if 0

Config option?
> unsigned ttysx_index; static const unsigned char divisor[] = { 1, 2,
> 3, 6, 12, 24, 48, 96 }; // read CMOS settings? ttysx_index =
> read_option(CMOS_VSTART_baud_rate, CMOS_VLEN_baud_rate, 0);
> ttysx_index &= 7; ttysx_div = divisor[ttysx_index]; #else ttysx_div =
> TTYSx_DIV; #endif uart8250_init(TTYSx_BASE, ttysx_div, UART_LCS); }

./arch/x86/archtables.c
Now that file is king of #if 0.
> #warning enable disabled code in archtables.c #if 0 // Copy GDT to new
> location and reload it // 2003-07 by SONE Takeshi // Ported from
> Etherboot to coreboot 2005-08 by Steve Magnani void move_gdt(unsigned
> long newgdt) { u16 num_gdt_bytes = &gdt_end - &gdt; struct gdtarg
> gdtarg; printk(BIOS_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(BIOS_DEBUG,"OK\n"); } #endif struct
> lb_memory *arch_write_tables(void) { #if 0 #if HAVE_MP_TABLE==1
> unsigned long new_low_table_end; #endif #endif unsigned long
> low_table_start, low_table_end; unsigned long rom_table_start,
> rom_table_end; rom_table_start = 0xf0000; rom_table_end = 0xf0000; /*
> Start low addr at 16 bytes instead of 0 because of a buglet * in the
> generic linux unzip code, as it tests for the a20 line. */
> low_table_start = 0; low_table_end = 16;
> post_code(POST_STAGE2_ARCH_WRITE_TABLES_ENTER); /* This table must be
> betweeen 0xf0000 & 0x100000 */ /* we need to make a decision: create
> empty functions * in .h files if the cpp variable is undefined, or
> #ifdef? */ #ifdef CONFIG_PIRQ_TABLE rom_table_end =
> write_pirq_routing_table(rom_table_end); rom_table_end =
> (rom_table_end + 1023) & ~1023; #endif /* Write ACPI tables */ /*
> write them in the rom area because DSDT can be large (8K on epia-m)
> which * pushes coreboot table out of first 4K if set up in low table
> area */ // rom_table_end = write_acpi_tables(rom_table_end); //
> rom_table_end = (rom_table_end+1023) & ~1023; /* copy the smp block to
> address 0 */ post_code(POST_STAGE2_ARCH_WRITE_TABLES_MIDDLE); /* The
> smp table must be in 0-1K, 639K-640K, or 960K-1M */ //
> new_low_table_end = write_smp_table(low_table_end); #if 0 #if
> HAVE_MP_TABLE==1 /* Don't write anything in the traditional x86 BIOS
> data segment, * for example the linux kernel smp need to use 0x467 to
> pass reset vector */ if(new_low_table_end>0x467){ unsigned
> mptable_size = new_low_table_end - low_table_end -
> SMP_FLOATING_TABLE_LEN; /* We can not put mptable here, we need to
> copy them to somewhere else*/
> if((rom_table_end+mptable_size)<0x100000) { /* We can copy mptable on
> rom_table, and leave low space for lbtable */ printk(BIOS_DEBUG,"Move
> mptable to 0x%0x\n", rom_table_end); memcpy((unsigned char
> *)rom_table_end, (unsigned char
> *)(low_table_end+SMP_FLOATING_TABLE_LEN), mptable_size);
> memset((unsigned char *)low_table_end, '\0', mptable_size +
> SMP_FLOATING_TABLE_LEN);
> smp_write_floating_table_physaddr(low_table_end, rom_table_end);
> low_table_end += SMP_FLOATING_TABLE_LEN; rom_table_end +=
> mptable_size; rom_table_end = (rom_table_end+1023) & ~1023; } else {
> /* We can need to put mptable low and from 0x500 */
> printk(BIOS_DEBUG,"Move mptable to 0x%0x\n", 0x500); memcpy((unsigned
> char *)0x500, (unsigned char *)(low_table_end+SMP_FLOATING_TABLE_LEN),
> mptable_size); memset((unsigned char *)low_table_end, '\0',
> 0x500-low_table_end); smp_write_floating_table_physaddr(low_table_end,
> 0x500); low_table_end = 0x500 + mptable_size; } } #endif #endif /*
> Don't write anything in the traditional x86 BIOS data segment */ if
> (low_table_end < 0x500) { low_table_end = 0x500; } #warning GDT should
> be placed in a reserved position from the beginning on. #if 0 //
> Relocate the GDT to reserved memory, so it won't get clobbered
> move_gdt(low_table_end); low_table_end += &gdt_end - &gdt; #endif /*
> The coreboot table must be in 0-4K or 960K-1M */ write_coreboot_table(
> low_table_start, low_table_end, rom_table_start, rom_table_end);
> return get_lb_mem(); }

Sorry, but I couldn't stomach the code above.

./arch/x86/mc146818rtc.c
> void rtc_init(int invalid) { #if defined(CONFIG_OPTION_TABLE) &&
> (CONFIG_OPTION_TABLE == 1) unsigned char x; int cmos_invalid,
> checksum_invalid; #endif printk(BIOS_DEBUG, "Initializing realtime
> clock.\n"); #if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE
> == 1) /* See if there has been a CMOS power problem. */ x =
> CMOS_READ(RTC_VALID); cmos_invalid = !(x & RTC_VRT); /* See if there
> is a CMOS checksum error */ checksum_invalid =
> !rtc_checksum_valid(PC_CKS_RANGE_START, PC_CKS_RANGE_END,PC_CKS_LOC);
> if (invalid || cmos_invalid || checksum_invalid) {
> printk(BIOS_WARNING, "RTC:%s%s%s zeroing cmos\n", invalid?" Clear
> requested":"", cmos_invalid?" Power Problem":"", checksum_invalid?"
> Checksum invalid":""); #if 0

Is this dependent on whether we want to use NVRAM?
> CMOS_WRITE(0, 0x01); CMOS_WRITE(0, 0x03); CMOS_WRITE(0, 0x05); for(i =
> 10; i < 48; i++) { CMOS_WRITE(0, i); } if (cmos_invalid) { /* Now
> setup a default date of Sat 1 January 2000 */ CMOS_WRITE(0, 0x00); /*
> seconds */ CMOS_WRITE(0, 0x02); /* minutes */ CMOS_WRITE(1, 0x04); /*
> hours */ CMOS_WRITE(7, 0x06); /* day of week */ CMOS_WRITE(1, 0x07);
> /* day of month */ CMOS_WRITE(1, 0x08); /* month */ CMOS_WRITE(0,
> 0x09); /* year */ } #endif } #endif /* Setup the real time clock */
> CMOS_WRITE(RTC_CONTROL_DEFAULT, RTC_CONTROL); /* Setup the frequency
> it operates at */ CMOS_WRITE(RTC_FREQ_SELECT_DEFAULT,
> RTC_FREQ_SELECT); #if defined(CONFIG_OPTION_TABLE) &&
> (CONFIG_OPTION_TABLE == 1) /* See if there is a coreboot CMOS checksum
> error */ checksum_invalid = !rtc_checksum_valid(CB_CKS_RANGE_START,
> CB_CKS_RANGE_END,CB_CKS_LOC); if(checksum_invalid)
> printk(BIOS_WARNING, "Invalid coreboot CMOS checksum.\n"); /* Make
> certain we have a valid checksum */
> rtc_set_checksum(PC_CKS_RANGE_START, PC_CKS_RANGE_END,PC_CKS_LOC);
> #endif /* Clear any pending interrupts */ (void)
> CMOS_READ(RTC_INTR_FLAGS); }

Regards,
Carl-Daniel