Skip to contentSkip to conversation

Resurrect IT8172 IDE controller driver

PATCH Superseded Shane McDonald 2 participants 3 messages Dec 22, 2008 – Dec 31, 2008

Subsystems (2)
PCI libata

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Shane McDonald

author
#0 lore raw

PATCH v3 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-rc9 kernel, and I have had no problems with it in my configuration. The attached patch applies cleanly against 2.6.28-rc9.

Signed-off-by Shane McDonald <[email protected]>
---
 drivers/ide/Kconfig     |    7 +
 drivers/ide/Makefile    |    1 
 drivers/ide/it8172.c    |  188 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h |    1 
 4 files changed, 197 insertions(+)

Changes since version 2 of the patch:
- removed the temporary "is_slave" variable, as it was only used once and was set inefficiently
- implemented the PIO modes, rather than configuring to slowest mode supported by the chip
- remove SWDMA flags
- change module author to Steve Longerbeam (originally was Steve's obsolete email address.

drivers/ide/Kconfig +7 -0
diff -uprN a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -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
diff -uprN a/drivers/ide/Makefile b/drivers/ide/Makefile
@@ -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 +188 -0

188 lines not on this page · raw message

include/linux/pci_ids.h +1 -0
diff -uprN a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -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
#1 lore raw

Hello.

Quoted 10 lines · Shane McDonald

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.

Quoted 9 lines

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-rc9 kernel, and I have had no problems with it in my configuration. The attached patch applies cleanly against 2.6.28-rc9.

Signed-off-by Shane McDonald <[email protected]>

Well, you're close. :-)

Quoted 5 lines

diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c

drivers/ide/it8172.c +0 -0
@@ -0,0 +1,188 @@

[...]

drivers/ide/it8172.c line 50

Quoted 12 lines
drivers/ide/it8172.c +11 -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);
+	u16 drive_enables;
+	u32 drive_timing;
+
+				     /* RTx  PWx */
+	static const u8 timings[][2] = {
+					{ 7, 7 },
+					{ 3, 4 },

This is not enough for PIO mode 1, as this only gives 150 ns active +
120 ns recovery = 270 ns where minimum is 383 ns

Quoted 10 lines
drivers/ide/it8172.c +9 -0
+					{ 1, 2 }, };
+	/*
+	 * The highest value of DIOR/DIOW pulse width and recovery time
+	 * that can be set in the IT8172 is 8 PCI clock cycles.  As a result,
+	 * it cannot be configured for PIO mode 0.  This table sets these
+	 * parameters to the maximum supported by the IT8172.
+	 */
+
+	pci_read_config_word(dev, 0x40, &drive_enables);

drivers/ide/it8172.c line 71

Quoted 10 lines
drivers/ide/it8172.c +10 -0
+	pci_read_config_dword(dev, 0x44, &drive_timing);
+
+	/*
+	 * 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 (drive->dn) {

Would have been good to collapse both branches...

drivers/ide/it8172.c line 76

Quoted 6 lines
drivers/ide/it8172.c +5 -0
+		drive_enables &= 0xc006;
+		if (drive->media == ide_disk)
+			/* enable prefetch */
+			drive_enables |= 0x0040;
+		if (pio > 2 || ata_id_has_iordy(drive->id))

You can actually skip "pio > 2" test.

Quoted 6 lines
drivers/ide/it8172.c +5 -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);

drivers/ide/it8172.c line 113

Quoted 10 lines
drivers/ide/it8172.c +10 -0
+	int u_flag		= 1 << drive->dn;
+	int u_speed		= 0;
+	u8 reg48, reg4a;
+
+	pci_read_config_byte(dev, 0x48, &reg48);
+	pci_read_config_byte(dev, 0x4a, &reg4a);
+
+	if (speed >= XFER_UDMA_0) {
+		u8 udma = speed - XFER_UDMA_0;
+		u_speed = min_t(u8, 2 - (udma & 1), udma) << (drive->dn * 4);

There's no need to use such complex math when the chip only supports
UltraDMA modes 0 thru 2. Be simple:

		u_speed = udma << (drive->dn * 4);

drivers/ide/it8172.c line 125

Quoted 12 lines
drivers/ide/it8172.c +11 -0
+		pci_write_config_byte(dev, 0x48, reg48 | u_flag);
+		reg4a &= ~a_speed;
+		pci_write_config_byte(dev, 0x4a, reg4a | u_speed);
+	} else {
+		const u8 mwdma_to_pio[] = { 0, 3, 4 };
+		u8 pio;
+
+		pci_write_config_byte(dev, 0x48, reg48 & ~u_flag);
+		pci_write_config_byte(dev, 0x4a, reg4a & ~a_speed);
+
+		if (speed >= XFER_MW_DMA_0)

We dropped support for SWDMA, so the check should not be needed...

drivers/ide/it8172.c line 128

Quoted 4 lines
drivers/ide/it8172.c +3 -0
+			pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
+		else
+			pio = 2;

... and *else* branch is pointless.

drivers/ide/it8172.c line 130

Quoted 2 lines
drivers/ide/it8172.c +1 -0
+		it8172_set_pio_mode(drive, pio);

Well, not the best place in the PIIX driver -- DMA modes shouldn't
affect prefetch and IORDY settings (regardless of what the Intel's programming manuals say :-). Àlthough, it's probably OK to continue abusing the set_pio_mode() method.

drivers/ide/it8172.c line 143

Quoted 5 lines
drivers/ide/it8172.c +4 -0
+static const struct ide_port_info it8172_port_info __devinitdata = {
+	.name		= DRV_NAME,
+	.port_ops	= &it8172_port_ops,
+	.enablebits	= {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00} },

Inconsistent spacing, add space after first {...

MBR, Sergei

author
#2 lore raw

I have made the changes you have suggested. See resolution below. A v4 patch follows shortly.

Quoted 3 lines · Sergei Shtylyov

Shane McDonald wrote:

Well, you're close. :-)

Just 3 more iterations... :-)

Quoted 11 lines

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 line 4

Quoted 8 lines

diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +4 -0
@@ -0,0 +1,188 @@
+                                    /* RTx  PWx */
+       static const u8 timings[][2] = {
+                                       { 7, 7 },
+                                       { 3, 4 },

This is not enough for PIO mode 1, as this only gives 150 ns active + 120
ns recovery = 270 ns where minimum is 383 ns

OK, I think I understand the required timings now. I will update PIO mode 1 to set RTx to 7 and PWx to 4, giving 150 ns active + 240 ns recovery = 390 ns (> minimum 383 ns).

Quoted 3 lines

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 line 5

Quoted 1 line
drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +1 -0
+       if (drive->dn) {

Would have been good to collapse both branches...

Yes, the code is quite repetitive. I've collapsed it into a single chunk of code.

Quoted 3 lines

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 line 6

Quoted 1 line
drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +1 -0
+               if (pio > 2 || ata_id_has_iordy(drive->id))

You can actually skip "pio > 2" test.

Agreed.

Quoted 9 lines
Quoted 4 lines
drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3 -0
+       if (speed >= XFER_UDMA_0) {
+               u8 udma = speed - XFER_UDMA_0;
+               u_speed = min_t(u8, 2 - (udma & 1), udma) << (drive->dn *

4);

There's no need to use such complex math when the chip only supports
UltraDMA modes 0 thru 2. Be simple:

               u_speed = udma << (drive->dn * 4);

Suggested change has been implemented.

Quoted 3 lines
Quoted 1 line

+ if (speed >= XFER_MW_DMA_0)

We dropped support for SWDMA, so the check should not be needed...

Yes. Check will be removed.

Quoted 6 lines

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600

Quoted 4 lines
drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +3 -0
+                       pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
+               else
+                       pio = 2;

... and *else* branch is pointless.

and is now removed.

Quoted 6 lines
Quoted 1 line

+ it8172_set_pio_mode(drive, pio);

Well, not the best place in the PIIX driver -- DMA modes shouldn't affect
prefetch and IORDY settings (regardless of what the Intel's programming manuals say :-).
Àlthough, it's probably OK to continue abusing the set_pio_mode() method.

I will continue to abuse set_pio_mode().

Quoted 8 lines

drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600

Quoted 6 lines
drivers/ide/it8172.c 2008-12-22 01:23:15.000000000 -0600 renamed from drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600 +4 -0
+static const struct ide_port_info it8172_port_info __devinitdata = {
+       .name           = DRV_NAME,
+       .port_ops       = &it8172_port_ops,
+       .enablebits     = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00} },

Inconsistent spacing, add space after first {...

I will correct the spacing.

Quoted 1 line

MBR, Sergei

Shane