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:[EMAIL PROTECTED]
http://www.amd.com/embeddedprocessors
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
