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.
Signed-off-by: Shane McDonald <[email protected]>
---
drivers/ide/Kconfig | 7 +
drivers/ide/Makefile | 1
drivers/ide/it8172.c | 205 ++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 1
4 files changed, 214 insertions(+)
diff -uprN a/drivers/ide/Kconfig b/drivers/ide/Kconfig
--- a/drivers/ide/Kconfig 2008-11-23 00:53:23.000000000 -0600
+++ b/drivers/ide/Kconfig 2008-11-23 00:54:13.000000000 -0600
@@ -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
diff -uprN a/drivers/ide/Makefile b/drivers/ide/Makefile
--- a/drivers/ide/Makefile 2008-11-23 00:53:23.000000000 -0600
+++ b/drivers/ide/Makefile 2008-11-23 00:54:13.000000000 -0600
@@ -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
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.
+ * [email protected] or [email protected]
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN
+ * NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/ide.h>
+#include <linux/init.h>
+
+#define DRV_NAME "IT8172"
+
+/*
+ * Prototypes
+ */
+
+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);
+ unsigned long flags;
+ u16 drive_enables;
+ u32 drive_timing;
+
+ spin_lock_irqsave(&ide_lock, flags);
+ 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
+ * are being left at the default values of 8 PCI clocks (242 nsec
+ * for a 33 MHz clock). 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.
+ */
+
+ /*
+ * 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;
+
+ if (is_slave) {
+ drive_enables &= 0xc006;
+ if (pio > 1)
+ /* enable prefetch and IORDY sample-point */
+ drive_enables |= 0x0060;
+ } else {
+ drive_enables &= 0xc060;
+ if (pio > 1)
+ /* enable prefetch and IORDY sample-point */
+ drive_enables |= 0x0006;
+ }
+
+ pci_write_config_word(dev, 0x40, drive_enables);
+ spin_unlock_irqrestore(&ide_lock, flags);
+}
+
+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,
+ * 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.
+ */
+
+ u_speed = 0 << (drive->dn * 4);
+
+ pci_write_config_byte(dev, 0x48, reg48 | u_flag);
+ reg4a &= ~a_speed;
+ pci_write_config_byte(dev, 0x4a, reg4a | u_speed);
+ } else {
+ pci_write_config_byte(dev, 0x48, reg48 & ~u_flag);
+ pci_write_config_byte(dev, 0x4a, reg4a & ~a_speed);
+ }
+
+ if (speed >= XFER_MW_DMA_0)
+ pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
+ else
+ pio = 2;
+
+ it8172_set_pio_mode(drive, pio);
+}
+
+
+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;
+}
+
+
+static const struct ide_port_ops it8172_port_ops = {
+ .set_pio_mode = it8172_set_pio_mode,
+ .set_dma_mode = it8172_set_dma_mode,
+};
+
+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} },
+ .host_flags = IDE_HFLAG_SINGLE,
+ .pio_mask = ATA_PIO4,
+ .swdma_mask = ATA_SWDMA2_ONLY,
+ .mwdma_mask = ATA_MWDMA12_ONLY,
+ .udma_mask = ATA_UDMA2,
+};
+
+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))))
+ return -ENODEV; /* IT8172 is more than an IDE controller */
+ return ide_pci_init_one(dev, &it8172_chipset, NULL);
+}
+
+static struct pci_device_id it8172_pci_tbl[] = {
+ { PCI_VDEVICE(ITE, PCI_DEVICE_ID_ITE_8172), 0 },
+ { 0, },
+};
+MODULE_DEVICE_TABLE(pci, it8172_pci_tbl);
+
+static struct pci_driver it8172_pci_driver = {
+ .name = "IT8172_IDE",
+ .id_table = it8172_pci_tbl,
+ .probe = it8172_init_one,
+ .remove = ide_pci_remove,
+ .suspend = ide_pci_suspend,
+ .resume = ide_pci_resume,
+};
+
+static int __init it8172_ide_init(void)
+{
+ return ide_pci_register_driver(&it8172_pci_driver);
+}
+
+static void __exit it8172_ide_exit(void)
+{
+ pci_unregister_driver(&it8172_pci_driver);
+}
+
+module_init(it8172_ide_init);
+module_exit(it8172_ide_exit);
+
+MODULE_AUTHOR("[email protected]");
+MODULE_DESCRIPTION("PCI driver module for ITE 8172 IDE");
+MODULE_LICENSE("GPL");
diff -uprN a/include/linux/pci_ids.h b/include/linux/pci_ids.h
--- a/include/linux/pci_ids.h 2008-11-23 00:53:41.000000000 -0600
+++ b/include/linux/pci_ids.h 2008-11-23 01:03:59.000000000 -0600
@@ -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
> 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
This appears to be a PIIX clone, is it different enough to need its own
driver ?
> +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.
Alan Cox wrote:
>> 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
>>
>
> This 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.
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.
> + * [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.
> +/*
> + * 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("[email protected]");
>
I wonder why Steve didn't specify his full name... :-)
MBR, Sergei
Hello, I 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
[...]
>> + 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.
The maximum values give cycle time of 480 ns menaing that the
controller doesn't support PIO mode 0. Hm...
>> + * 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".
>> +
>> + 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.
>> +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,
No, PIO mode 0 isn't supported, hence the mask should be 0x1e.
MBR, Sergei
> > 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.
Alan Cox wrote:
>>> 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.
> Alan
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.
On Mon, Nov 24, 2008 at 4:33 AM, Sergei Shtylyov
<[email protected]> wrote:
> 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.
>> + * [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.
>> +/*
>> + * Prototypes
>> + */
>
> I see no prototypes here...
Ah yes, the prototypes were removed, but not the comment. I will remove this.
>> +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.
>> + 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.
I will remove it, and the associated spin_unlock_irqrestore.
>> + 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. :-)
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.
>> + * 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.
>> 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.
Hmmm, that's right, that is what the code is doing. I will update the
comment accordingly.
>> + /*
>> + * 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...
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?
>> + 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.
>> +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...
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.
>> + * 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.
>> + */
>> +
>> + 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.
> 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);
OK, I will only execute this code if it's a non-UDMA mode.
> 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.
>> +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.
I will remove this.
>> +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...
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.
>> .host_flags = IDE_HFLAG_SINGLE,
>> + .pio_mask = ATA_PIO4,
>> + .swdma_mask = ATA_SWDMA2_ONLY,
>> + .mwdma_mask = ATA_MWDMA12_ONLY,
>
> More modes are supportable...
I will update accordingly.
>> +static const struct ide_port_info it8172_chipset __devinitdata = {
>
> Please rename this variable to it8172_port_info.
I will update accordingly.
>> +};
>> +
>> +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.
I will clean up the parens and remove the first check.
>> +MODULE_AUTHOR("[email protected]");
>
> I wonder why Steve didn't specify his full name... :-)
I will change this to my name.
> MBR, Sergei
Thank you for your time!
Shane
On Mon, Nov 24, 2008 at 7:38 AM, Sergei Shtylyov
<[email protected]> wrote:
> Hello.
>
> Alan Cox wrote:
>
>>>> 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.
>> Alan
>
> MBR, Sergei
Shane
> 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.
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
On Thu, Dec 4, 2008 at 4:07 AM, Alan Cox <[email protected]> wrote:
>> 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.
>
> 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.
>
>
On Thu, 4 Dec 2008 08:01:49 -0600
"Shane McDonald" <[email protected]> wrote:
> 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
Hello.
Shane McDonald wrote:
>>Alan Cox wrote:
>>>>> 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?
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.
> Please excuse my dumb question -- I'm a little over my head here.
>>>Alan
> Shane
MBR, Sergei
Hello.
Alan Cox wrote:
>>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.
> 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
On Thursday 04 December 2008, Alan Cox wrote:
> On Thu, 4 Dec 2008 08:01:49 -0600
> "Shane McDonald" <[email protected]> wrote:
>
> > 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.
Shane McDonald wrote:
> I have made some of the required changes, but I have some questions.
> See in-line for my resolution / questions for each comment.
>
> On Mon, Nov 24, 2008 at 4:33 AM, Sergei Shtylyov
> <[email protected]> wrote:
>
>> 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.
>>> + * [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.
>
>
>>> +/*
>>> + * Prototypes
>>> + */
>>>
>> I see no prototypes here...
>>
>
> Ah yes, the prototypes were removed, but not the comment. I will remove this.
>
>
>>> +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.
>
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...
>>> + /*
>>> + * 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?
>> 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...
>>> + 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?
>
Yes, you're correct.
> I am not sure how to determine if IORDY sampling is supported by a
>
I think Bart has replied to thia already...
> 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.
>
That's because PIO modes above 2 necessiate IORDY, while for mode 2
the drive can require it optionally.
>>> +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...
>>
>
> 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...
> 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.
>>> + * 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. :-)
>>> +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.
>>
>
> 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.
>>> +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...
>>
Er, I was wrong here -- you actiually can't do that as the channel
will remain disabled.
> 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.
>>> .host_flags = IDE_HFLAG_SINGLE,
>>> + .pio_mask = ATA_PIO4,
>>>
I'm not seeing how PIO mode 0 is supportable.
>>> + .swdma_mask = ATA_SWDMA2_ONLY,
>>>
The SWDMA support could be dropped altogether -- these modes are slow
and long obsolete.
>>> + .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...
>>> +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...
> 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.
On Mon, Dec 8, 2008 at 6:02 AM, Sergei Shtylyov <[email protected]> wrote:
>
> Shane McDonald wrote:
>
>> On Mon, Nov 24, 2008 at 4:33 AM, Sergei Shtylyov
>> <[email protected]> wrote:
>>
>>>> +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.
>>
>
> 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...
Of course! I will drop the &. Also, because this variable is only
used once, I'll drop the
variable entirely.
>>>> + /*
>>>> + * 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?
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.
>>> 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.
>>>> +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...
>>>
>>
>> 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.
>>>> +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}},
>>>
>> 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.
>>>> .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.
>>>> + .swdma_mask = ATA_SWDMA2_ONLY,
>>>>
>
> The SWDMA support could be dropped altogether -- these modes are slow and long obsolete.
Dropped!
>>>> + .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...
Mode programming added, and mask now set to ATA_MWDMA2
>>>> +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