Re: PATCH: IDE PCI init cleanup

Andre Hedrick (andre@suse.com)
Wed, 1 Sep 1999 16:41:58 -0700 (PDT)


It will be late friday before I can catch up to the rest of you on this
issue. I am out of reach and no access to my hardware. If the init
process is identical in effect and the feel is different, that is
expected. What are the advantages/robustness in the changes? I have an
IDE chipset that is really dependent upon getting the device order
correct. One mistake will result in hda/b/c/d detection order of the
devices, becomes hdc/d/a/b. This is a real bad thing.

Andre Hedrick
The Linux IDE guy

On Tue, 31 Aug 1999, Martin Mares wrote:

> Hi Andre,
>
> When rewriting the PCI subsystem, I've also taken a look at initialization
> routines of the IDE drivers and there is a small patch trying to clean them
> up a bit (I didn't have a chance to test the patches, so please examine
> them carefully).
>
> Have a nice fortnight
> --
> Martin `MJ' Mares <mj@ucw.cz> http://atrey.karlin.mff.cuni.cz/~mj/
> Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
> "A mathematician is a machine for converting coffee into theorems."
>
>
> Changes:
>
> o Removed enabling of expansion ROMs. Given the principle PCI works,
> it cannot make any difference and the sole fact the ROM has assigned
> some address doesn't mean at all that the address won't collide with
> other devices.
> o Changed global cli() to local __cli() in alim15x3.c init. (I think
> the cli can be removed at all, but I'm not 100% sure.)
> o Replaced setting of PCI_LATENCY_TIMER by a call to pci_set_master().
> o Removed PCI_CACHE_LINE_SIZE setting on __sparc_v9__ as DaveM has
> said he'll implement these fixups in his new Sparc64 PCI code.
> o hpt34x: cleaned up resource handling.
> o Removed ide_special_settings() as it's not used.
> o Use pci_dev->name when printing messages.
> o pdc202xx: #if 0'd out the INTERRUPT_LINE check. INTERRUPT_LINE
> is not used at all (neither in the kernel nor by the hardware), so
> there is no need to touch it.
> o Base addresses in dev->resource[] don't contain the type bits,
> so it isn't needed to mask them out.
>
>
> diff -uN linux-2.3.16-pre1/drivers/block/aec6210.c rsrc/drivers/block/aec6210.c
> --- linux-2.3.16-pre1/drivers/block/aec6210.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/aec6210.c Tue Aug 31 10:27:33 1999
> @@ -52,9 +52,5 @@
>
> unsigned int __init pci_init_aec6210 (struct pci_dev *dev, const char *name)
> {
> - if (dev->resource[PCI_ROM_RESOURCE].start) {
> - pci_write_config_dword(dev, PCI_ROM_ADDRESS, dev->resource[PCI_ROM_RESOURCE].start | PCI_ROM_ADDRESS_ENABLE);
> - printk("%s: ROM enabled at 0x%08lx\n", name, dev->resource[PCI_ROM_RESOURCE].start);
> - }
> return dev->irq;
> }
> diff -uN linux-2.3.16-pre1/drivers/block/alim15x3.c rsrc/drivers/block/alim15x3.c
> --- linux-2.3.16-pre1/drivers/block/alim15x3.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/alim15x3.c Tue Aug 31 10:40:55 1999
> @@ -92,11 +92,11 @@
> if (r_clc >= 16)
> r_clc = 0;
> }
> - save_flags(flags);
> - cli();
> + __save_flags(flags);
> + __cli();
> pci_write_config_byte(dev, port, s_clc);
> pci_write_config_byte(dev, port+drive->select.b.unit+2, (a_clc << 4) | r_clc);
> - restore_flags(flags);
> + __restore_flags(flags);
>
> /*
> * setup active rec
> diff -uN linux-2.3.16-pre1/drivers/block/cmd646.c rsrc/drivers/block/cmd646.c
> --- linux-2.3.16-pre1/drivers/block/cmd646.c Sat Aug 14 12:27:53 1999
> +++ rsrc/drivers/block/cmd646.c Tue Aug 31 13:15:33 1999
> @@ -251,12 +251,6 @@
>
> hwif->chipset = ide_cmd646;
>
> - /* Set a good latency timer and cache line size value. */
> - (void) pci_write_config_byte(dev, PCI_LATENCY_TIMER, 64);
> -#ifdef __sparc_v9__
> - (void) pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, 0x10);
> -#endif
> -
> /* Setup interrupts. */
> (void) pci_read_config_byte(dev, 0x71, &mrdmode);
> mrdmode &= ~(0x30);
> diff -uN linux-2.3.16-pre1/drivers/block/hpt34x.c rsrc/drivers/block/hpt34x.c
> --- linux-2.3.16-pre1/drivers/block/hpt34x.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/hpt34x.c Tue Aug 31 13:15:41 1999
> @@ -343,37 +343,22 @@
> unsigned int __init pci_init_hpt34x (struct pci_dev *dev, const char *name)
> {
> unsigned short cmd;
> + int i;
> + unsigned long hpt34xIoBase = dev->resource[4].start;
>
> pci_write_config_byte(dev, HPT34X_PCI_INIT_REG, 0x00);
> pci_read_config_word(dev, PCI_COMMAND, &cmd);
> - if (cmd & PCI_COMMAND_MEMORY) {
> - if (dev->resource[PCI_ROM_RESOURCE].start) {
> - pci_write_config_byte(dev, PCI_ROM_ADDRESS, dev->resource[PCI_ROM_RESOURCE].start | PCI_ROM_ADDRESS_ENABLE);
> - printk(KERN_INFO "HPT345: ROM enabled at 0x%08lx\n", dev->resource[PCI_ROM_RESOURCE].start);
> - }
> - pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0xF0);
> - } else {
> - int i = 0;
> - unsigned long hpt34xIoBase = dev->resource[4].start;
> -
> - pci_write_config_word(dev, PCI_COMMAND, cmd & ~PCI_COMMAND_IO);
> - dev->resource[0].start = (hpt34xIoBase + 0x20);
> - dev->resource[1].start = (hpt34xIoBase + 0x34);
> - dev->resource[2].start = (hpt34xIoBase + 0x28);
> - dev->resource[3].start = (hpt34xIoBase + 0x3c);
> - for(i=0; i<4; i++)
> - dev->resource[i].flags |= PCI_BASE_ADDRESS_SPACE_IO;
> - /*
> - * Since 20-23 can be assigned and are R/W, we correct them.
> - */
> - pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, dev->resource[0].start);
> - pci_write_config_dword(dev, PCI_BASE_ADDRESS_1, dev->resource[1].start);
> - pci_write_config_dword(dev, PCI_BASE_ADDRESS_2, dev->resource[2].start);
> - pci_write_config_dword(dev, PCI_BASE_ADDRESS_3, dev->resource[3].start);
> -
> - pci_write_config_word(dev, PCI_COMMAND, cmd);
> - pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0x20);
> + pci_write_config_word(dev, PCI_COMMAND, cmd & ~PCI_COMMAND_IO);
> + for(i=0; i<4; i++) {
> + static int hpt34x_offsets[] = { 0x20, 0x34, 0x28, 0x3c };
> + static int hpt34x_sizes[] = { 8, 1, 8, 1 };
> + struct resource *r = &dev->resource[i];
> + r->start = hpt34xIoBase + hpt34x_offsets[i];
> + r->end = r->start + hpt34x_sizes[i] - 1;
> + r->flags |= PCI_BASE_ADDRESS_SPACE_IO | IORESOURCE_IO;
> + pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + 4*i, r->start);
> }
> + pci_write_config_word(dev, PCI_COMMAND, cmd);
> return dev->irq;
> }
>
> @@ -386,6 +371,7 @@
> pci_read_config_word(hwif->pci_dev, PCI_COMMAND, &pcicmd);
> #ifdef CONFIG_BLK_DEV_HPT34X_DMA
> #if 0
> + /* This check looks bogus --mj */
> hwif->autodma = (pcicmd & PCI_COMMAND_MEMORY) ? 1 : 0;
> #endif
> #endif /* CONFIG_BLK_DEV_HPT34X_DMA */
> diff -uN linux-2.3.16-pre1/drivers/block/hpt366.c rsrc/drivers/block/hpt366.c
> --- linux-2.3.16-pre1/drivers/block/hpt366.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/hpt366.c Tue Aug 31 10:22:17 1999
> @@ -466,8 +466,6 @@
> byte ata66 = 0;
>
> pci_read_config_byte(dev, 0x5a, &ata66);
> - if (dev->rom_address)
> - pci_write_config_byte(dev, PCI_ROM_ADDRESS, dev->rom_address | PCI_ROM_ADDRESS_ENABLE);
> printk("%s: reg5ah=0x%02x ATA-%s Cable Port%d\n", name, ata66, (ata66 & 0x02) ? "33" : "66", PCI_FUNC(dev->devfn));
> return dev->irq;
> }
> diff -uN linux-2.3.16-pre1/drivers/block/ide-pci.c rsrc/drivers/block/ide-pci.c
> --- linux-2.3.16-pre1/drivers/block/ide-pci.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/ide-pci.c Tue Aug 31 13:40:11 1999
> @@ -250,45 +250,6 @@
>
> static byte hpt363_shared_irq = 0;
>
> -/*
> - * This allows offboard ide-pci cards the enable a BIOS, verify interrupt
> - * settings of split-mirror pci-config space, place chipset into init-mode,
> - * and/or preserve an interrupt if the card is not native ide support.
> - */
> -static unsigned int __init ide_special_settings (struct pci_dev *dev, const char *name)
> -{
> - switch(dev->device) {
> - case PCI_DEVICE_ID_TTI_HPT343:
> - {
> - unsigned short pcicmd = 0;
> -
> - pci_write_config_byte(dev, 0x80, 0x00);
> - pci_read_config_word(dev, PCI_COMMAND, &pcicmd);
> - if (!(pcicmd & PCI_COMMAND_MEMORY)) {
> - int i;
> - unsigned long hpt34xIoBase = dev->resource[4].start;
> -
> - dev->resource[0].start = (hpt34xIoBase + 0x20);
> - dev->resource[1].start = (hpt34xIoBase + 0x34);
> - dev->resource[2].start = (hpt34xIoBase + 0x28);
> - dev->resource[3].start = (hpt34xIoBase + 0x3c);
> - for(i=0; i<4; i++)
> - dev->resource[i].flags |= PCI_BASE_ADDRESS_SPACE_IO;
> - pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0x20);
> - } else {
> - pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0xF0);
> - }
> - }
> - case PCI_DEVICE_ID_TTI_HPT366:
> - case PCI_DEVICE_ID_PROMISE_20246:
> - case PCI_DEVICE_ID_PROMISE_20262:
> - case PCI_DEVICE_ID_ARTOP_ATP850UF:
> - return dev->irq;
> - default:
> - break;
> - }
> - return 0;
> -}
>
> /*
> * Match a PCI IDE port against an entry in ide_hwifs[],
> @@ -446,7 +407,7 @@
> * space, place chipset into init-mode, and/or preserve
> * an interrupt if the card is not native ide support.
> */
> - pciirq = (d->init_chipset) ? d->init_chipset(dev, d->name) : ide_special_settings(dev, d->name);
> + pciirq = d->init_chipset ? d->init_chipset(dev, d->name) : 0;
> } else if (tried_config) {
> printk("%s: will probe irqs later\n", d->name);
> pciirq = 0;
> @@ -552,12 +513,8 @@
> /*
> * Set up BM-DMA capability (PnP BIOS should have done this)
> */
> + pci_set_master(dev);
> hwif->autodma = 0; /* default DMA off if we had to configure it here */
> - (void) pci_write_config_word(dev, PCI_COMMAND, pcicmd | PCI_COMMAND_MASTER);
> - if (pci_read_config_word(dev, PCI_COMMAND, &pcicmd) || !(pcicmd & PCI_COMMAND_MASTER)) {
> - printk("%s: %s error updating PCICMD\n", hwif->name, d->name);
> - dma_base = 0;
> - }
> }
> if (dma_base) {
> if (d->dma_init) {
> @@ -646,10 +603,10 @@
> hpt366_device_order_fixup(dev, d);
> else if (!IDE_PCI_DEVID_EQ(d->devid, IDE_PCI_DEVID_NULL) || (dev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
> if (IDE_PCI_DEVID_EQ(d->devid, IDE_PCI_DEVID_NULL))
> - printk("%s: unknown IDE controller on PCI bus %02x device %02x, VID=%04x, DID=%04x\n",
> - d->name, dev->bus->number, dev->devfn, devid.vid, devid.did);
> + printk("%s: unknown IDE controller on %s, VID=%04x, DID=%04x\n",
> + d->name, dev->name, devid.vid, devid.did);
> else
> - printk("%s: IDE controller on PCI bus %02x dev %02x\n", d->name, dev->bus->number, dev->devfn);
> + printk("%s: IDE controller on %s\n", d->name, dev->name);
> ide_setup_pci_device(dev, d);
> }
> }
> diff -uN linux-2.3.16-pre1/drivers/block/ns87415.c rsrc/drivers/block/ns87415.c
> --- linux-2.3.16-pre1/drivers/block/ns87415.c Sat Aug 14 12:27:56 1999
> +++ rsrc/drivers/block/ns87415.c Tue Aug 31 13:15:24 1999
> @@ -112,12 +112,6 @@
> byte stat;
> #endif
>
> - /* Set a good latency timer and cache line size value. */
> - (void) pci_write_config_byte(dev, PCI_LATENCY_TIMER, 64);
> -#ifdef __sparc_v9__
> - (void) pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, 0x10);
> -#endif
> -
> /*
> * We cannot probe for IRQ: both ports share common IRQ on INTA.
> * Also, leave IRQ masked during drive probing, to prevent infinite
> diff -uN linux-2.3.16-pre1/drivers/block/pdc202xx.c rsrc/drivers/block/pdc202xx.c
> --- linux-2.3.16-pre1/drivers/block/pdc202xx.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/pdc202xx.c Tue Aug 31 10:27:20 1999
> @@ -498,11 +498,7 @@
> byte primary_mode = inb(high_16 + 0x001a);
> byte secondary_mode = inb(high_16 + 0x001b);
>
> - if (dev->resource[PCI_ROM_RESOURCE].start) {
> - pci_write_config_dword(dev, PCI_ROM_ADDRESS, dev->resource[PCI_ROM_RESOURCE].start | PCI_ROM_ADDRESS_ENABLE);
> - printk("%s: ROM enabled at 0x%08lx\n", name, dev->resource[PCI_ROM_RESOURCE].start);
> - }
> -
> +#if 0 /* Should not have any effect. --mj */
> if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE) {
> byte irq = 0, irq2 = 0;
> pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
> @@ -512,6 +508,7 @@
> printk("%s: pci-config space interrupt mirror fixed.\n", name);
> }
> }
> +#endif
>
> printk("%s: (U)DMA Burst Bit %sABLED " \
> "Primary %s Mode " \
> diff -uN linux-2.3.16-pre1/drivers/block/sis5513.c rsrc/drivers/block/sis5513.c
> --- linux-2.3.16-pre1/drivers/block/sis5513.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/sis5513.c Tue Aug 31 13:16:08 1999
> @@ -333,16 +333,13 @@
> unsigned int __init pci_init_sis5513 (struct pci_dev *dev, const char *name)
> {
> struct pci_dev *host;
> - byte latency = 0, reg48h = 0;
> + byte reg48h = 0;
>
> - pci_read_config_byte(dev, PCI_LATENCY_TIMER, &latency);
> pci_read_config_byte(dev, 0x48, &reg48h);
>
> for (host = pci_devices; host; host=host->next) {
> if (host->vendor == PCI_VENDOR_ID_SI &&
> host->device == PCI_DEVICE_ID_SI_620) {
> - if (latency != 0x10)
> - pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0x10);
> host_dev = host;
> printk("%s: Chipset Core ATA-66, SiS620\n", name);
> printk("%s: Primary ATA-%s, Secondary ATA-%s Cable Detect\n",
> diff -uN linux-2.3.16-pre1/drivers/block/trm290.c rsrc/drivers/block/trm290.c
> --- linux-2.3.16-pre1/drivers/block/trm290.c Tue Aug 31 15:31:46 1999
> +++ rsrc/drivers/block/trm290.c Tue Aug 31 10:36:46 1999
> @@ -223,7 +223,7 @@
> cfgbase = dev->resource[4].start;
> if ((dev->class & 5) && cfgbase)
> {
> - hwif->config_data = cfgbase & PCI_BASE_ADDRESS_IO_MASK;
> + hwif->config_data = cfgbase;
> printk("TRM290: chip config base at 0x%04lx\n", hwif->config_data);
> } else {
> hwif->config_data = 0x3df0;
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.rutgers.edu
> Please read the FAQ at http://www.tux.org/lkml/
>

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/