Re: [PATCH v3] Resurrect IT8172 IDE controller driver

From: Sergei Shtylyov
Date: Sun Dec 28 2008 - 17:44:27 EST

Hello.

Shane McDonald wrote:

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 <mcdonald.shane@xxxxxxxxx>

Well, you're close. :-)

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-12-22 01:23:15.000000000 -0600
@@ -0,0 +1,188 @@
[...]
+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

+ { 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);
+ 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...

+ 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.

+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;
+
+ 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);


+ 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...

+ pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
+ else
+ pio = 2;

... and *else* branch is pointless.

+ 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.

+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


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