Resurrect IT8172 IDE controller driver
PATCH 1/1 Resurrect IT8172 IDE controller driver
Support for the IT8172 IDE controller was removed from the kernel sometime after 2.6.18. Support for the only boards that used the IT8172 was removed from the kernel after 2.6.18, as they had never compiled since 2.6.0. However, there are a couple of platforms that use this
chip: the PMC-Sierra Xiao Hu thin-client computer, which is no longer in production, and the Linksys NSS4000 Network Attached Storage box, which is based on the Xiao Hu board. I am attempting to add support for the Xiao Hu to the kernel, and this IT8172 IDE controller is the first bit of code in this effort.
This patch resurrects the IT8172 IDE controller code. I began with the 2.6.18 version of the it8172.c file, and have moved it forward so that it works with the latest version of the kernel. I have run this driver on a PMC-Sierra Xiao Hu board with the 2.6.28-rc6 kernel, and I have had no problems with it in my configuration. The attached patch applies cleanly against 2.6.28-rc6.
--- drivers/ide/Kconfig | 7 + drivers/ide/Makefile | 1 drivers/ide/it8172.c | 205 ++++++++++++++++++++++++++++++++++++++ include/linux/pci_ids.h | 1 4 files changed, 214 insertions(+)
drivers/ide/Kconfig +7 -0
@@ -524,6 +524,13 @@ config BLK_DEV_PIIX This allows the kernel to change PIO, DMA and UDMA speeds and to configure the chip to optimum performance. +config BLK_DEV_IT8172 + tristate "IT8172 IDE support" + select BLK_DEV_IDEDMA_PCI + help + This driver adds support for the IDE controller on the + IT8172 System Controller. + config BLK_DEV_IT8213 tristate "IT8213 IDE support" select BLK_DEV_IDEDMA_PCI
drivers/ide/Makefile +1 -0
@@ -46,6 +46,7 @@ obj-$(CONFIG_BLK_DEV_SC1200) += sc1200. obj-$(CONFIG_BLK_DEV_CY82C693) += cy82c693.o obj-$(CONFIG_BLK_DEV_DELKIN) += delkin_cb.o obj-$(CONFIG_BLK_DEV_HPT366) += hpt366.o +obj-$(CONFIG_BLK_DEV_IT8172) += it8172.o obj-$(CONFIG_BLK_DEV_IT8213) += it8213.o obj-$(CONFIG_BLK_DEV_IT821X) += it821x.o obj-$(CONFIG_BLK_DEV_JMICRON) += jmicron.o
drivers/ide/it8172.c +205 -0
205 lines not on this page · raw message
include/linux/pci_ids.h +1 -0
@@ -1658,6 +1658,7 @@#define PCI_VENDOR_ID_ROCKWELL 0x127A#define PCI_VENDOR_ID_ITE 0x1283 +#define PCI_DEVICE_ID_ITE_8172 0x8172#define PCI_DEVICE_ID_ITE_8211 0x8211#define PCI_DEVICE_ID_ITE_8212 0x8212#define PCI_DEVICE_ID_ITE_8213 0x8213
Quoted 2 lines
This patch resurrects the IT8172 IDE controller code. I began with the 2.6.18 version of the
it8172.cfile, and have moved it forward so
This appears to be a PIIX clone, is it different enough to need its own driver ?
drivers/ide/it8172.c line 146
Quoted 12 lines
drivers/ide/it8172.c +12
-0+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; +}
NAK. You can't do this here any more. The PCI code now understands 'legacy' PCI IDE header types and resources so you must fix up the resources as a PCI quirk early on in boot or you will leave bogus resource assignments around.
Alan
Hello.
Quoted 7 lines · Alan Cox
Quoted 3 lines
This patch resurrects the IT8172 IDE controller code. I began with the 2.6.18 version of the
it8172.cfile, and have moved it forward soThis appears to be a PIIX clone, is it different enough to need its own driver ?
Unfortunately, it's not an exact clone, so does need its own driver.
MBR, Sergei
Hello.
drivers/ide/it8172.c line 8
Quoted 13 lines · Shane McDonald
diff -uprN
a/drivers/ide/it8172.cb/drivers/ide/it8172.cdrivers/ide/it8172.c +8
-0@@ -0,0 +1,205 @@ +/* +* +* BRIEF MODULE DESCRIPTION +* IT8172 IDE controller support +* +* Copyright 2000 MontaVista Software Inc. +* Author: MontaVista Software, Inc. +* [email protected] or [email protected]
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.
drivers/ide/it8172.c line 43
Quoted 4 lines
drivers/ide/it8172.c +3
-0+/* +* Prototypes +*/
I see no prototypes here...
drivers/ide/it8172.c line 49
Quoted 7 lines
drivers/ide/it8172.c +6
-0+ +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...
drivers/ide/it8172.c line 54
Quoted 2 lines
drivers/ide/it8172.c +1
-0+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.
drivers/ide/it8172.c line 59
Quoted 6 lines
drivers/ide/it8172.c +5
-0+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. :-)
drivers/ide/it8172.c line 25
Quoted 2 lines
drivers/ide/it8172.c +2
-0+* 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.
drivers/ide/it8172.c line 64
Quoted 5 lines
drivers/ide/it8172.c +3
-0These 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.
drivers/ide/it8172.c line 71
Quoted 8 lines
drivers/ide/it8172.c +7
-0+ +/* +* 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...
drivers/ide/it8172.c line 77
Quoted 7 lines
drivers/ide/it8172.c +6
-0+ +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
Quoted 11 lines
drivers/ide/it8172.c +10
-0+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 };
drivers/ide/it8172.c line 108
Quoted 10 lines
drivers/ide/it8172.c +10
-0+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...
drivers/ide/it8172.c line 113
Quoted 6 lines
drivers/ide/it8172.c +5
-0+* 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.
drivers/ide/it8172.c line 116
Quoted 4 lines
drivers/ide/it8172.c +3
-0+*/ + + 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.
drivers/ide/it8172.c line 131
Quoted 8 lines
drivers/ide/it8172.c +7
-0+ +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.
drivers/ide/it8172.c line 146
Quoted 13 lines
drivers/ide/it8172.c +12
-0+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.
drivers/ide/it8172.c line 158
Quoted 6 lines
drivers/ide/it8172.c +5
-0+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...
drivers/ide/it8172.c line 162
Quoted 5 lines
.host_flags =
IDE_HFLAG_SINGLE,drivers/ide/it8172.c +3
-0+ .pio_mask = ATA_PIO4, + .swdma_mask = ATA_SWDMA2_ONLY, + .mwdma_mask = ATA_MWDMA12_ONLY,
More modes are supportable...
Quoted 1 line
+static const struct
ide_port_infoit8172_chipset__devinitdata= {
Please rename this variable to it8172_port_info.
drivers/ide/it8172.c line 170
Quoted 8 lines
drivers/ide/it8172.c +7
-0+}; + +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)
drivers/ide/it8172.c line 171
Quoted 2 lines
drivers/ide/it8172.c +1
-0+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.
Quoted 2 lines
+MODULE_AUTHOR("[email protected]");
I wonder why Steve didn't specify his full name... :-)
MBR, Sergei
Quoted 3 lines · Hello, I wrote:
Quoted 3 lines
diff -uprN
a/drivers/ide/it8172.cb/drivers/ide/it8172.cdrivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +0
-0
[...]
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ 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
Quoted 6 lines
It's usually FIXME. :-)
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 2 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +2
-0+ * 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.
The maximum values give cycle time of 480 ns menaing that the
controller doesn't support PIO mode 0. Hm...
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ * 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.
Don't know where the reset of the phrase go -- it should have been
"on reads the drive dicteates the UDMA timings".
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 3 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +2
-0+ + u_speed = 0 << (drive->dn * 4);+ */
I think you're actually setting UltraDMA mode 2 timing regardless of
the mode selected.
Oops, I thought I'd edited that out. That's mode 0 timing.
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+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} },
Quoted 9 lines
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...Quoted 2 lines
.host_flags =
IDE_HFLAG_SINGLE,
+ .pio_mask =ATA_PIO4,
No, PIO mode 0 isn't supported, hence the mask should be 0x1e.
MBR, Sergei
Quoted 4 lines
Quoted 1 line
It's 240, not 242 ns as 33 is actually 33.333.
The maximum values give cycle time of 480 ns menaing that the
controller doesn't support PIO mode 0. Hm...
Even if you clear the enable for the timing register ?
Alan
Hello.
Quoted 6 lines · Alan Cox
Quoted 4 lines
Quoted 1 line
It's 240, not 242 ns as 33 is actually 33.333.
The maximum values give cycle time of 480 ns menaing that the
controller doesn't support PIO mode 0. Hm...Even if you clear the enable for the timing register ?
These fast timing bits are documented as reserved.
Quoted 1 line
Alan
MBR, Sergei
Quoted 4 lines · Sergei Shtylyov
Hello.
Alan Cox wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
It's 240, not 242 ns as 33 is actually 33.333.
The maximum values give cycle time of 480 ns menaing that the controller
doesn't support PIO mode 0. Hm...Even if you clear the enable for the timing register ?
These fast timing bits are documented as reserved.
The spec says that PIO mode 0 is supported, but Sergei is correct -- the maximum values give a cycle time of 480 ns. How can this be? The old driver appeared to have tried to support PIO mode 0 by setting to the maximum.
Which fast timing bits are documented as reserved? My spec has the
IDE Drive 0/1 Recovery Time and IDE Drive 0/1 Pulse Width bits in it. Are there other timing bits that aren't documented in my spec?
Please excuse my dumb question -- I'm a little over my head here.
Quoted 3 lines
Quoted 1 line
Alan
MBR, Sergei
Shane
Hello.
Quoted 11 lines · Shane McDonald
Quoted 10 lines
Quoted 6 lines · Alan Cox
Quoted 4 lines
Quoted 1 line
It's 240, not 242 ns as 33 is actually 33.333.
The maximum values give cycle time of 480 ns menaing that the controller doesn't support PIO mode 0. Hm...
Even if you clear the enable for the timing register ?
These fast timing bits are documented as reserved.
Quoted 8 lines
The spec says that PIO mode 0 is supported, but Sergei is correct -- the maximum values give a cycle time of 480 ns. How can this be? The old driver appeared to have tried to support PIO mode 0 by setting to the maximum.
Which fast timing bits are documented as reserved? My spec has the
IDE Drive 0/1 Recovery Time and IDE Drive 0/1 Pulse Width bits in it. Are there other timing bits that aren't documented in my spec?
Off the top of my head: bits 0, 3, 4, and 7 at offset 0x40 are not
reserved in PIIX/ICH -- they enable the programmable timings for PIO and/or DMA. If they were left cleared, 600 ns cycle (PIO mode 0) was used.
Quoted 5 lines
Please excuse my dumb question -- I'm a little over my head here.
Quoted 1 line
Quoted 1 line
Alan
Shane
MBR, Sergei
Hello:
I have made some of the required changes, but I have some questions.
See in-line for my resolution / questions for each comment.
Quoted 4 lines · Sergei Shtylyov
Hello.
Shane McDonald wrote:
Quoted 16 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 8
Quoted 13 lines
diff -uprN
a/drivers/ide/it8172.cb/drivers/ide/it8172.cdrivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +8
-0@@ -0,0 +1,205 @@ +/* + * + * BRIEF MODULE DESCRIPTION + * IT8172 IDE controller support + * + * Copyright 2000 MontaVista Software Inc. + * Author: MontaVista Software, Inc. + * [email protected] or [email protected]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.
OK, I will update accordingly.
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 11
Quoted 3 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3
-0+/* + * Prototypes + */I see no prototypes here...
Ah yes, the prototypes were removed, but not the comment. I will remove this.
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 16
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+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...
I don't believe it can be dropped entirely -- although the
controller only has a single channel, it does support two drives on that channel. Unless I understand incorrectly, I will make the "drive->dn & 1" change.
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 17
Quoted 2 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +1
-0+ spin_lock_irqsave(&ide_lock, flags);Don't use
ide_lockhere please. Moreover, since the controller has only one
channel, you don't need any spinlock here.
I will remove it, and the associated spin_unlock_irqrestore.
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 22
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ 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 0x44It's usually FIXME. :-)
I will change. Although, the change to the pulse width of DIOR/DIOW doesn't seem that difficult to make. Maybe I should just go ahead and do that.
Quoted 4 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 24
Quoted 2 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +2
-0+ * 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.
I will update the comment to reflect reality, unless I implement the FIXME.
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 4 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3
-0These 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.
Hmmm, that's right, that is what the code is doing. I will update the comment accordingly.
Quoted 11 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +6
-0+ /* + * 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 atdrivers/ide/piix.c...
The specs I have indicate that it is not the same. For example, bits 13:12 in the IDETIM register at offset 0x40-0x41 of the PIIX give the IORDY Sample Point, but the same function is found in bits 19-17 in the SLVT register at offset 0x44-0x47 in the IT8172. Perhaps better comments are in order, or perhaps these bitfields should be defined in a .h file?
Quoted 9 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ 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
OK, I will update accordingly. Am I correct that prefetch should be enabled if "drive->media == ide_disk", and not otherwise?
I am not sure how to determine if IORDY sampling is supported by a drive. If I'm reading the code correctly, other drivers only check that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be the case for at least piix.c, siimage.c, and it8213.c.
Quoted 14 lines
Quoted 14 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +14
-0+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);
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +6
-0+ + 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...
Do you think I should keep this code in here? It kind of seems silly to run drivers at UDMA0 speeds just because of a few bad drives back in 2000 when the driver was originally written. Should this not be handled in userspace by hdparm? Other drivers don't seem to do something similar.
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 5 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ * 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.
Unless it is decided to eliminate the slower-cycle-time feature (in which case this comment would go away), I will update the comment accordingly as per your clarified email.
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 3 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3
-0+ */ + + u_speed = 0 << (drive->dn * 4);I think you're actually setting UltraDMA mode 2 timing regardless of the
mode selected.
Yes, as you said in the follow-on email, this is being set to UDMA mode 0 regardless of the mode selected, due to the preceeding slower-cycle-time comment. I will await your response before changing this.
Quoted 8 lines
The below code only applies to non-UltraDMA modes.
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +6
-0+ if (speed >= XFER_MW_DMA_0) + pio = mwdma_to_pio[speed - XFER_MW_DMA_0]; + else + pio = 2; + + it8172_set_pio_mode(drive, pio);
OK, I will only execute this code if it's a non-UDMA mode.
Quoted 3 lines
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.
I'm not quite sure what you mean. I'll implement something and get your feedback.
Quoted 7 lines
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +7
-0+static unsigned int __devinit init_chipset_it8172(struct pci_dev *dev) +{ + unsigned char progif; + + /* + * Place both IDE interfaces into PCI "native" mode + */
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ 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.
I will remove this.
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+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} },
Quoted 6 lines
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...
Well, mine is clearly wrong. Am I correct that {0x41, 0x80, 0x80} is checking that the IDE Decode Enable bit is set? This bit is in the same location in both the PIIX and the IT8172.
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 4 lines
.host_flags =
IDE_HFLAG_SINGLE,drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3
-0+ .pio_mask = ATA_PIO4, + .swdma_mask = ATA_SWDMA2_ONLY, + .mwdma_mask = ATA_MWDMA12_ONLY,More modes are supportable...
I will update accordingly.
Quoted 3 lines
Quoted 1 line
+static const struct
ide_port_infoit8172_chipset__devinitdata= {Please rename this variable to
it8172_port_info.
I will update accordingly.
Quoted 15 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +7
-0+}; + +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)
Quoted 6 lines
Quoted 3 lines
+ 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.
I will clean up the parens and remove the first check.
Quoted 3 lines
Quoted 1 line
+MODULE_AUTHOR("[email protected]");
I wonder why Steve didn't specify his full name... :-)
I will change this to my name.
Quoted 1 line
MBR, Sergei
Thank you for your time!
Shane
Quoted 4 lines
I am not sure how to determine if IORDY sampling is supported by a drive. If I'm reading the code correctly, other drivers only check that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be the case for at least
piix.c,siimage.c, andit8213.c.
Old IDE is wrong on this and in fact unless it has changed recently simply can't cope with and doesn't provide functions for it. A libata driver can use the libata helpers but those also rely on the core ata code doing certain things so if you cut and paste it over you might have to make other old IDE changes in parallel.
Would it be acceptable to claim that these changes are outside the scope of this patch, and code the IT8172 driver to behave in the same manner as the other wrong drivers, with a suitable comment indicating this fact?
Shane
Quoted 12 lines · Alan Cox
Quoted 4 lines
I am not sure how to determine if IORDY sampling is supported by a drive. If I'm reading the code correctly, other drivers only check that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be the case for at least
piix.c,siimage.c, andit8213.c.Old IDE is wrong on this and in fact unless it has changed recently simply can't cope with and doesn't provide functions for it. A libata driver can use the libata helpers but those also rely on the core ata code doing certain things so if you cut and paste it over you might have to make other old IDE changes in parallel.
Quoted 4 lines · On Thu, 4 Dec 2008 08:01:49 -0600 "Shane McDonald" <[email protected]>…
Would it be acceptable to claim that these changes are outside the scope of this patch, and code the IT8172 driver to behave in the same manner as the other wrong drivers, with a suitable comment indicating this fact?
I think so.. and if its moved over to libata it can just be fixed then
Alan
Quoted 9 lines · Alan Cox
Quoted 4 lines · On Thu, 4 Dec 2008 08:01:49 -0600 "Shane McDonald" <[email protected]>…
Would it be acceptable to claim that these changes are outside the scope of this patch, and code the IT8172 driver to behave in the same manner as the other wrong drivers, with a suitable comment indicating this fact?
I think so.. and if its moved over to libata it can just be fixed then
It shouldn't be necessary...
Shane, you can use helpers from <linux/ata.h> just fine in IDE host drivers so looking at what libata-core helper used by ata_piix.c:
if (ata_pio_need_iordy(adev))
control |= 2; /* IE enable */is actually doing:
unsigned int ata_pio_need_iordy(const struct ata_device *adev)
{
/* Controller doesn't support IORDY. Probably a pointless check
as the caller should know this */
if (adev->link->ap->flags & ATA_FLAG_NO_IORDY)
return 0;
/* PIO3 and higher it is mandatory */
if (adev->pio_mode > XFER_PIO_2)
return 1;
/* We turn it on when possible */
if (ata_id_has_iordy(adev->id))
return 1;
return 0;
}we see that all you need to add in your driver is an additional checking for ata_id_has_iordy().
Thanks,
Bart
PS when it comes to actually setting the transfer mode on the device (done in ide_config_drive_speed() core function) IORDY is also handled just fine (thanks to Sergei).
Hello.
Quoted 7 lines · Alan Cox
Quoted 4 lines
I am not sure how to determine if IORDY sampling is supported by a drive. If I'm reading the code correctly, other drivers only check that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be the case for at least
piix.c,siimage.c, andit8213.c.Old IDE is wrong on this and in fact unless it has changed recently simply can't cope with and doesn't provide functions for it. A libata
It's not as wrong as you're trying to paint it: ide_get_best_pio_mode()
has been here for ages and it have been returning the IORDY setting -- although it actually asks the drive only when auto-tuning the mode, and when being given the explicit mode only tells to use IORDY on modes > 2.
WBR, Sergei
Hello.
Quoted 52 lines · Shane McDonald
I have made some of the required changes, but I have some questions. See in-line for my resolution / questions for each comment.
Quoted 5 lines · Sergei Shtylyov
Hello.
Shane McDonald wrote:
Quoted 17 lines
Quoted 6 lines
diff -uprN
a/drivers/ide/it8172.cb/drivers/ide/it8172.cdrivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 8
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +8
-0@@ -0,0 +1,205 @@ +/* + * + * BRIEF MODULE DESCRIPTION + * IT8172 IDE controller support + * + * Copyright 2000 MontaVista Software Inc. + * Author: MontaVista Software, Inc. + * [email protected] or [email protected]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.OK, I will update accordingly.
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 11
Quoted 4 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3
-0+/* + * Prototypes + */I see no prototypes here...
Ah yes, the prototypes were removed, but not the comment. I will remove this.
Quoted 9 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 16
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+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...
Quoted 5 lines
I don't believe it can be dropped entirely -- although the
controller only has a single channel, it does support two drives on that channel. Unless I understand incorrectly, I will make the "drive->dn& 1" change.
Believe it or not, drive->dn will be 0 or 1 in this case, so & is
superfluous. :-)
I don't insist on dropping it though...
Quoted 11 lines
Quoted 11 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 22
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +6
-0+ /* + * 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 just don't undestand what this bit is good for in IT8172G...
Perhaps by clearing it once could achieve PIO0 timings, just like by clearing the fast timing bits on PIIX?
Quoted 10 lines
Quoted 2 lines
I suggest that you take a close look at
drivers/ide/piix.c...The specs I have indicate that it is not the same. For example, bits 13:12 in the IDETIM register at offset 0x40-0x41 of the PIIX give the IORDY Sample Point, but the same function is found in bits 19-17 in the SLVT register at offset 0x44-0x47 in the IT8172. Perhaps better comments are in order, or perhaps these bitfields should be defined in a .h file?
No, IT8172G just implements them differently. But you can still find
in this driver a good example of how things should be done...
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ if (is_slave) { + drive_enables &= 0xc006; + if (pio > 1) + /* enable prefetch and IORDY sample-point */ + drive_enables |= 0x0060;
Quoted 8 lines
Quoted 4 lines
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 devicesOK, I will update accordingly. Am I correct that prefetch should be enabled if "
drive->media==ide_disk", and not otherwise?
Yes, you're correct.
Quoted 2 lines
I am not sure how to determine if IORDY sampling is supported by a
I think Bart has replied to thia already...
Quoted 4 lines
drive. If I'm reading the code correctly, other drivers only check that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be the case for at least
piix.c,siimage.c, andit8213.c.
That's because PIO modes above 2 necessiate IORDY, while for mode 2
the drive can require it optionally.
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +14
-0+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);Quoted 9 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 47
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +6
-0+ + 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...
Quoted 3 lines
Do you think I should keep this code in here? It kind of seems silly to run drivers at UDMA0 speeds just because of a few bad drives back in 2000 when the driver was originally written.
If was only happening for some drives, you probably shouldn't keep it...
Quoted 4 lines
Should this not be
handled in userspace by hdparm? Other drivers don't seem to do something similar.
This is usually handled by blacklisting the drives for the user's
convenience.
Quoted 8 lines
Quoted 8 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 52
Quoted 6 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ * 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.
"On reads the drive dictatees the timings" I was going to write. :-)
Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +7
-0+static unsigned int __devinit init_chipset_it8172(struct pci_dev *dev) +{ + unsigned char progif; + + /* + * Place both IDE interfaces into PCI "native" mode + */Quoted 9 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 line 64
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+ 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.
Quoted 2 lines
I will remove this.
You'll probably need to add this as a quirk to drivers/pci/quirks.c
-- unless the bootloader sets the controller to native mode itself.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +5
-0+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} },
Quoted 7 lines
Quoted 7 lines
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...
Er, I was wrong here -- you actiually can't do that as the channel
will remain disabled.
Quoted 4 lines
Well, mine is clearly wrong. Am I correct that {
0x41,0x80,0x80} is checking that the IDE Decode Enable bit is set? This bit is in the same location in both the PIIX and the IT8172.
Yes, you're correct. You should add the code to force it enabled if
the bootloader doesn't do that.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.host_flags =
IDE_HFLAG_SINGLE,
+ .pio_mask =ATA_PIO4,
I'm not seeing how PIO mode 0 is supportable.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ .swdma_mask =
ATA_SWDMA2_ONLY,
The SWDMA support could be dropped altogether -- these modes are slow
and long obsolete.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+ .mwdma_mask =
ATA_MWDMA12_ONLY,More modes are supportable...
I will update accordingly.
These masks were stolen from the piix.c driver and are determined by
the limitation of the timing register fileds being only 2-bit wide. IT8172G has these bits implemented differently, hence the limitation shouldn't apply. You shouldn't just update the masks without doing anythiung about programming the modes...
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+MODULE_AUTHOR("[email protected]");
I wonder why Steve didn't specify his full name... :-)
I will change this to my name.
I don't think it would be a legitimate change...
Quoted 2 lines
Thank you for your time!
If you could give me more free time instead... :-)
MBR, Sergei
I have updated this driver with a version 3 of the patch, taking into account all of these comments.
See resolutions below. The updated patch will follow shortly.
Quoted 17 lines · Sergei Shtylyov
Quoted 14 lines · Shane McDonald
Quoted 10 lines · Sergei Shtylyov
drivers/ide/it8172.c line 49
Quoted 6 lines
drivers/ide/it8172.c +5
-0+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...
Quoted 8 lines
Quoted 5 lines
I don't believe it can be dropped entirely -- although the
controller only has a single channel, it does support two drives on that channel. Unless I understand incorrectly, I will make the "drive->dn& 1" change.Believe it or not,
drive->dnwill be 0 or 1 in this case, so & is superfluous. :-) I don't insist on dropping it though...
Of course! I will drop the &. Also, because this variable is only used once, I'll drop the
variable entirely.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/ide/it8172.c +6
-0+/* +* 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 ;
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
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 just don't undestand what this bit is good for in IT8172G... Perhaps by clearing it once could achieve PIO0 timings, just like by clearing the fast timing bits on PIIX?
That would make sense, but there is nothing in the spec that states that. The bit does need
to be set, so I'll leave this in.
Quoted 11 lines
Quoted 9 lines
Quoted 1 line
I suggest that you take a close look at
drivers/ide/piix.c...The specs I have indicate that it is not the same. For example, bits 13:12 in the IDETIM register at offset 0x40-0x41 of the PIIX give the IORDY Sample Point, but the same function is found in bits 19-17 in the SLVT register at offset 0x44-0x47 in the IT8172. Perhaps better comments are in order, or perhaps these bitfields should be defined in a .h file?
No, IT8172G just implements them differently. But you can still find in this driver a good example of how things should be done...
I have updated the driver to add in the proper handling of the various PIO modes,
in a similar manner to what the PIIX driver does.
Quoted 25 lines
Quoted 25 lines
Quoted 24 lines
Quoted 11 lines
drivers/ide/it8172.c +10
-0+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 };drivers/ide/it8172.c line 108
Quoted 10 lines
drivers/ide/it8172.c +10
-0+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...
Quoted 5 lines
Quoted 3 lines
Do you think I should keep this code in here? It kind of seems silly to run drivers at UDMA0 speeds just because of a few bad drives back in 2000 when the driver was originally written.
If was only happening for some drives, you probably shouldn't keep it...
Code removed.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/ide/it8172.c line 158
Quoted 5 lines
drivers/ide/it8172.c +5
-0+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:
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
.enablebits = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00}},Well, mine is clearly wrong. Am I correct that {
0x41,0x80,0x80} is checking that the IDE Decode Enable bit is set? This bit is in the same location in both the PIIX and the IT8172.Yes, you're correct. You should add the code to force it enabled if the bootloader doesn't do that.
Updated accordingly.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.host_flags =
IDE_HFLAG_SINGLE,
+ .pio_mask =ATA_PIO4,I'm not seeing how PIO mode 0 is supportable.
I'll indicate PIO mode 0 is not supported.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ .swdma_mask =
ATA_SWDMA2_ONLY,The SWDMA support could be dropped altogether -- these modes are slow and long obsolete.
Dropped!
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+ .mwdma_mask =
ATA_MWDMA12_ONLY,More modes are supportable...
I will update accordingly.
These masks were stolen from the
piix.cdriver and are determined by the limitation of the timing register fileds being only 2-bit wide. IT8172G has these bits implemented differently, hence the limitation shouldn't apply. You shouldn't just update the masks without doing anythiung about programming the modes...
Mode programming added, and mask now set to ATA_MWDMA2
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
+MODULE_AUTHOR("[email protected]");
I wonder why Steve didn't specify his full name... :-)
I've changed it to "Steve Longerbeam", and I removed the obsolete email address.
Shane