Re: [coreboot] #if 0 crud in v3

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> 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

Sounds good.
> > 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?

I think that this #if 0 should be removed and the code included. 
Disabling VPCI headers may be important to some mainboards.
> > /* 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...

This code can be removed.
> > 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?

I think this code should be removed. I haven't ever seen anyone use HOI.
> > 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.

Remove the code. The GLIU registers are setup elsewhere.
> > /* 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
> > }

Marc