Hello. Shane McDonald wrote:
diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c --- a/drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +++ b/drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 @@ -0,0 +1,205 @@ +/* + * + * BRIEF MODULE DESCRIPTION + * IT8172 IDE controller support + * + * Copyright 2000 MontaVista Software Inc. + * Author: MontaVista Software, Inc. + * stevel@xxxxxxxxxx or source@xxxxxxxxxx
You can remove the former address, Steve Longerbeam is no longer with MV (quit long ago). And I think you may add your own copyright now.
+/* + * Prototypes + */
I see no prototypes here...
+ +static void it8172_set_pio_mode(ide_drive_t *drive, const u8 pio) +{ + ide_hwif_t *hwif = HWIF(drive); + struct pci_dev *dev = to_pci_dev(hwif->dev); + int is_slave = (&hwif->drives[1] == drive);
Use simpler "drive->dn & 1" (and drop the useless parens please) -- & can be dropped though as the controller has only a single channel...
+ spin_lock_irqsave(&ide_lock, flags);
Don't use ide_lock here please. Moreover, since the controller has only one channel, you don't need any spinlock here.
+ pci_read_config_word(dev, 0x40, &drive_enables); + pci_read_config_dword(dev, 0x44, &drive_timing); + + /* + * FIX! The DIOR/DIOW pulse width and recovery times in port 0x44
It's usually FIXME. :-)
+ * are being left at the default values of 8 PCI clocks (242 nsec + * for a 33 MHz clock).
It's 240, not 242 ns as 33 is actually 33.333.
These can be safely shortened at higher + * PIO modes. The DIOR/DIOW pulse width and recovery times only + * apply to PIO modes, not to the DMA modes. + */
Not true. They do apply to SW/MW DMA modes -- and if you look at your own code you'll see that.
+ + /* + * Enable port 0x44. The IT8172 spec is confused; it calls + * this register the "Slave IDE Timing Register", but in fact, + * it controls timing for both master and slave drives. + */ + drive_enables |= 0x4000;
This is strange because this IDE controller seems to be a clone of the one in the Intel PIIX chip. However, the spec. I have tellm it's not an exact clone.
I suggest that you take a close look at drivers/ide/piix.c...
+ + if (is_slave) { + drive_enables &= 0xc006; + if (pio > 1) + /* enable prefetch and IORDY sample-point */ + drive_enables |= 0x0060;
IORDY sampling shouldn't be enabled for PIO mode 2 always, only if the drive supports it.
Prefetch should only be enabled for ATA disks, not the ATAPI devices
+static void it8172_set_dma_mode(ide_drive_t *drive, const u8 speed) +{ + ide_hwif_t *hwif = HWIF(drive); + struct pci_dev *dev = to_pci_dev(hwif->dev); + int a_speed = 3 << (drive->dn * 4); + int u_flag = 1 << drive->dn; + int u_speed = 0; + u8 reg48, reg4a; + + const u8 mwdma_to_pio[] = { 0, 3, 4 }; + u8 pio; + + pci_read_config_byte(dev, 0x48, ®48); + pci_read_config_byte(dev, 0x4a, ®4a); + + if (speed >= XFER_UDMA_0) { + + /* Setting the DMA cycle time to 2 or 3 PCI clocks + * (60 and 91 nsec at 33 MHz PCI clock) seems to cause + * BadCRC errors during DMA transfers on some drives,
Sigh, such drives should have been blacklisted...
+ * even though both numbers meet the minimum ATAPI-4 spec + * of 73 and 54 nsec for UDMA 1 and 2 respectively. + * So the faster times are not implemented here. + * The good news is that the slower cycle time has + * very little affect on transfer performance.
This should only affect the write performance, reads.
+ */ + + u_speed = 0 << (drive->dn * 4);
I think you're actually setting UltraDMA mode 2 timing regardless of the mode selected.
The below code only applies to non-UltraDMA modes.
+ + if (speed >= XFER_MW_DMA_0) + pio = mwdma_to_pio[speed - XFER_MW_DMA_0]; + else + pio = 2; + + it8172_set_pio_mode(drive, pio);
Moreover, I'd suggest to reimplement it as the timing register layout is different from the original PIIX and additional DMA modes can be supported: MWDMA0 and SWDMA1.
+static unsigned int __devinit init_chipset_it8172(struct pci_dev *dev) +{ + unsigned char progif; + + /* + * Place both IDE interfaces into PCI "native" mode + */ + pci_read_config_byte(dev, PCI_CLASS_PROG, &progif); + pci_write_config_byte(dev, PCI_CLASS_PROG, progif | 0x05); + + return dev->irq; +}
As Alan have said, you can't do that here.
+static const struct ide_port_info it8172_chipset __devinitdata = { + .name = DRV_NAME, + .init_chipset = init_chipset_it8172, + .port_ops = &it8172_port_ops, + .enablebits = {{0x00, 0x00, 0x00}, {0x40, 0x00, 0x01} },
Wrong, should be: .enablebits = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00}}, If that doesn't work (firmware doesn't enable the channel), you can leave it all 0s...
.host_flags = IDE_HFLAG_SINGLE, + .pio_mask = ATA_PIO4, + .swdma_mask = ATA_SWDMA2_ONLY, + .mwdma_mask = ATA_MWDMA12_ONLY,
More modes are supportable...
+static const struct ide_port_info it8172_chipset __devinitdata = {
Please rename this variable to it8172_port_info.
+}; + +static int __devinit it8172_init_one(struct pci_dev *dev, + const struct pci_device_id *id) +{ + if ((!(PCI_FUNC(dev->devfn) & 1) || + (!((dev->class >> 8) == PCI_CLASS_STORAGE_IDE))))
Please, don't abuse parens, it should be just: if (!(PCI_FUNC(dev->devfn) & 1) || (dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
+ return -ENODEV; /* IT8172 is more than an IDE controller */
And I don't get it: why the first check is needed? Is there another IDE controller on function 1 that you want to ignore? I doubt it.
+MODULE_AUTHOR("SteveL@xxxxxxxxxx");
I wonder why Steve didn't specify his full name... :-) MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html