[coreboot] #if 0 crud in v3

Marc Jones Marc.Jones at AMD.com
Tue Feb 12 01:58:28 CET 2008


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

-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors 







More information about the coreboot mailing list